Skip to content

fix(parser): parse the ordinal gate on "the Nth spell you cast each turn costs less" - #6196

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
minion1227:minion_ordinal_spell_costmod
Jul 19, 2026
Merged

fix(parser): parse the ordinal gate on "the Nth spell you cast each turn costs less"#6196
matthewevans merged 1 commit into
phase-rs:mainfrom
minion1227:minion_ordinal_spell_costmod

Conversation

@minion1227

Copy link
Copy Markdown
Contributor

Summary

The cost-mod ordinal gate was dropped for every ordinal past "first". The parser's
FirstQualifiedSpell machinery hard-coded the literal prefix "the first " and a
SpellsCastThisTurn == 0 gate, so "The second spell you cast each turn costs {N}
less to cast"
returned NotApplicable and fell through to the generic
cost-modifier path — emitting a filterless, conditionless reducer that
cheapened every spell the controller cast, not just the second.

This parameterizes the existing seam by ordinal (FirstQualifiedSpell
NthQualifiedSpell { filter, timing, ordinal }; "first" → 1, "second" → 2, …)
so the gate becomes SpellsCastThisTurn(you) == ordinal - 1 — exactly N - 1
qualifying spells already cast this turn ⇒ the spell now being cast is the Nth.
"first" is the unchanged N = 1 (== 0) case, so the merged first-spell
behavior is byte-for-byte preserved. This is a parameterize-don't-proliferate
refactor of one combinator seam, not a new sibling; new parse_spell_ordinal_prefix

  • parse_ordinal_word combinators cover first..tenth.

Class (5 cards): Highspire Bell-Ringer, Uthros Psionicist, Raging Battle Mouse,
Monk Class, Alisaie Leveilleur — all print "the second spell you cast each turn
costs {N} less" and all previously cheapened every spell.

Files changed

  • crates/engine/src/parser/oracle_static/grammar.rs — new parse_spell_ordinal_prefix + parse_ordinal_word nom combinators; FirstQualifiedSpellNthQualifiedSpell { filter, timing, ordinal }; parse_nth_qualified_spell_filter / nth_qualified_spell_condition / nth_qualified_spell_subject_fully_consumed parameterized by ordinal (gate == ordinal - 1).
  • crates/engine/src/parser/oracle_static/static_helpers.rs — cost-mod consumer threads the ordinal into the condition builder.
  • crates/engine/src/parser/oracle_static/keyword_grant.rs — keyword-grant consumer threads the ordinal (renamed call sites).
  • crates/engine/src/parser/oracle_static/tests.rs — renamed seam unit tests; new nth_qualified_spell_filter_second_spell_gates_on_one_prior_cast asserting ordinal → SpellsCastThisTurn == 1.
  • crates/engine/tests/integration/nth_spell_ordinal_cost_reduction.rs (new, registered) — cast-pipeline regression.
  • docs/parser-misparse-backlog.md — backlog hygiene (see Claimed parse impact).

CR references

  • CR 601.2f — determining a spell's total cost (cost modifiers apply at this step); the ordinal gate is evaluated here.
  • CR 107.3 — numeric/ordinal values in rules text.

Both were carried over from the existing seam (already verified against docs/MagicCompRules.txt); no new CR number introduced.

Implementation method (required)

Found via a fresh live-parse semantic-misparse audit of cost-modifier statics (the
07-09 card-data snapshot re-parsed against Oracle text; three independent auditor
passes converged on this pattern). Live-verified against HEAD before writing code
(parse was ModifyCost { spell_filter: None, condition: None }). Designed as a
leaf-level parameterization of the existing FirstQualifiedSpell combinator seam
per "parameterize, don't proliferate".

Track

Developer / misparse fix.

LLM

Claude Opus 4.8 (1M context).

Verification

  • cargo fmt --all — clean.
  • cargo clippy -p engine --all-targets --features proptest -- -D warnings0 warnings.
  • cargo test -p engine --lib parser::oracle_static1204 passed, 0 failed (incl. the 3 seam tests).
  • cargo test -p engine --test integration nth_spell_ordinal_cost_reductionpass; first spell pays full {2}, second discounted to {1}, third full {2} again (revert-failing — pre-fix discounts the first cast).
  • AST live-verify: first → SpellsCastThisTurn == 0, second → == 1, third → == 2, filter → Typed[Card].

Gate A

Tilt was down; ran the checks directly. clippy (all-targets, proptest, -D warnings)
0 warnings; parser lib suite 1204/0; integration regression green; fmt clean.

Anchored on

upstream/main @ 44c59a6f4 (release: v0.30.0).

Final review-impl

Independent fresh-context review (diff + CLAUDE.md + review-impl skill only, no
prior conversation). Verdict: clean on all checked lenses — correct seam
(single cost-mod ordinal authority; two production consumers both threaded),
nom-mandate (pure combinators; word-boundary safe against "firstborn"/"seconds"),
new-field threading (all 4 sites thread ordinal), off-by-one (confirmed against
game_object.rs — cost mods evaluated in finalize_cast before
record_spell_cast_from_zone increments the counter, so the Nth cast sees exactly
N-1 recorded), discriminating runtime test (flips on revert), CR citations
resolve and describe the code.

One LOW / pre-existing / out-of-scope note: nth_qualified_spell_condition
hardcodes CountScope::Controller, so a hypothetical "the second spell your
opponents
cast each turn costs {N} more" would count the controller's spells.
This is pre-existing (the opponent-cast phrases + Controller scope both predate
this PR and were already reachable for "first"), and zero real cards use an
opponent-scoped ordinal cost/keyword form (verified against card data), so nothing
ships incorrect. Left out of scope to keep this a focused parameterization; a
future fix would thread the caster scope or decline the opponent branch once a real
card exists.

Claimed parse impact

5 cards move from a filterless/conditionless (every-spell) reducer to a correct
ordinal-gated reducer: Highspire Bell-Ringer, Uthros Psionicist, Raging Battle
Mouse, Monk Class, Alisaie Leveilleur. No card regresses (the N = 1 path is
unchanged). Backlog hygiene: removed Uthros Psionicist + Raging Battle Mouse from
root cause #2 and Highspire Bell-Ringer from #28 (all now parse fully clean, live-
verified); decremented the associated counts. Monk Class (§2) and Alisaie
Leveilleur (§5) retain unrelated misparses and stay listed.

Validation Failures

None.

CI Failures

None expected.

…urn costs less"

"The second spell you cast each turn costs {N} less to cast" (Highspire
Bell-Ringer, Uthros Psionicist, Raging Battle Mouse, Monk Class, Alisaie
Leveilleur) parsed with spell_filter: None and condition: None — the ordinal
"second ... each turn" gate was silently dropped, so the reducer cheapened EVERY
spell the controller cast instead of only the second.

Root cause: `parse_first_qualified_spell_filter` (grammar.rs) hard-coded the
literal prefix "the first " and a `SpellsCastThisTurn == 0` gate, so any ordinal
past "first" returned NotApplicable and fell through to the generic, filterless,
conditionless cost-modifier path.

Fix: parameterize the existing seam by ordinal (FirstQualifiedSpell ->
NthQualifiedSpell { filter, timing, ordinal }; "first" -> 1, "second" -> 2, ...)
so the gate becomes `SpellsCastThisTurn(you) == ordinal - 1` -- exactly
ordinal-1 qualifying spells already cast this turn => the spell now being cast is
the Nth. "first" is the unchanged ordinal=1 (== 0) case, so merged first-spell
behavior is byte-for-byte preserved. A "parameterize, don't proliferate" refactor
of one combinator seam, not a new sibling. New `parse_spell_ordinal_prefix` +
`parse_ordinal_word` combinators cover first..tenth.

Parse-only: the runtime already evaluates the SpellsCastThisTurn static condition
at cost determination (collect_battlefield_cost_modifiers ->
evaluate_cost_mod_static_condition).

CR 601.2f (total cost determination) / CR 107.3 (numeric/ordinal values).

Tests: parser unit test asserting "second" -> SpellsCastThisTurn == 1; registered
runtime cast-pipeline regression (nth_spell_ordinal_cost_reduction) proving the
first spell pays full {2}, the second is discounted to {1}, and the third pays
full {2} again (fails on revert -- the pre-fix reducer discounts the first cast).

Backlog: removed Uthros Psionicist and Raging Battle Mouse from root cause phase-rs#2 and
Highspire Bell-Ringer from root cause phase-rs#28 (all now parse fully clean); decremented
the associated counts. Monk Class and Alisaie Leveilleur retain other misparses
and stay listed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@minion1227
minion1227 requested a review from matthewevans as a code owner July 19, 2026 11:41
@github-actions github-actions Bot added the needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) label Jul 19, 2026
@matthewevans matthewevans self-assigned this Jul 19, 2026
@matthewevans

Copy link
Copy Markdown
Member

Reviewed current head 3434097720d81135d2a99e1a61ccc3fe02bed369: no implementation finding from the independent review.

This PR remains held, not approved or enqueued, while its current Rust/parser CI jobs finish and the required parse-diff sticky artifact is published for this head. The artifact is required review evidence for this parser change.

@matthewevans matthewevans removed their assignment Jul 19, 2026

@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 generalizes the "first qualified spell" cost-reduction and keyword-grant parser to support arbitrary ordinals (e.g., "second", "third"), addressing CR 601.2f. It replaces FirstQualifiedSpell with NthQualifiedSpell, introduces helpers to parse English ordinal words, and updates the static condition logic to gate on SpellsCastThisTurn == N - 1. Integration tests have been added to verify that second-spell discounts apply correctly, and the parser backlog documentation has been updated accordingly. The reviewer feedback suggests adding a CR 107.3 annotation to the parse_ordinal_word function to comply with the repository's style guide (Rule R6).

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 +2008 to +2013
/// Map an English ordinal word to its 1-based value. Only "first"/"second" occur
/// on printed once-per-turn spell-cost modifiers today; the higher ordinals are
/// included so the whole ordinal class stays covered without a follow-up edit.
/// The trailing `" "` guard in [`parse_spell_ordinal_prefix`] enforces a word
/// boundary, so "firstborn" / "seconds" never partial-match.
fn parse_ordinal_word(i: &str) -> OracleResult<'_, u32> {

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.

medium

According to the repository style guide (Rule R6), every rules-touching line of engine code must carry a comment of the form CR <number>: <description>. Since parse_ordinal_word parses ordinal numbers from rules text, it should be annotated with a reference to CR 107.3 (which governs numeric and ordinal values in rules text).

Suggested change
/// Map an English ordinal word to its 1-based value. Only "first"/"second" occur
/// on printed once-per-turn spell-cost modifiers today; the higher ordinals are
/// included so the whole ordinal class stays covered without a follow-up edit.
/// The trailing `" "` guard in [`parse_spell_ordinal_prefix`] enforces a word
/// boundary, so "firstborn" / "seconds" never partial-match.
fn parse_ordinal_word(i: &str) -> OracleResult<'_, u32> {
/// CR 107.3: Map an English ordinal word to its 1-based value. Only "first"/"second" occur
/// on printed once-per-turn spell-cost modifiers today; the higher ordinals are
/// included so the whole ordinal class stays covered without a follow-up edit.
/// The trailing " " guard in [parse_spell_ordinal_prefix] enforces a word
/// boundary, so "firstborn" / "seconds" never partial-match.
fn parse_ordinal_word(i: &str) -> OracleResult<'_, u32> {
References
  1. Rule R6: Every rules-touching line of engine code must carry a comment of the form CR : . (link)

@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR · 5 card(s), 1 signature(s) (baseline: main 44c59a6f45f7)

🟡 Modified fields (1 signature)

  • 5 cards · 🔄 static/ReduceCost · changed field conditional: card spells cast this turn (your) = 1
    • Affected (first 3): Alisaie Leveilleur, Highspire Bell-Ringer, Monk Class (+2 more)

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

@matthewevans matthewevans self-assigned this Jul 19, 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 current-head review.

Evidence: the parser parameterizes the existing ordinal authority and threads it through both cost-modifier and keyword-grant consumers; the registered cast-pipeline regression distinguishes first/second/third casts (2/1/2 mana); current required CI is green; and the current parse-diff shows exactly the claimed five ReduceCost condition changes. The Gemini CR 107.3 suggestion was reviewed and not applied because that rule governs X, not English ordinal words.

@matthewevans matthewevans added the bug Bug fix label Jul 19, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 19, 2026
@matthewevans matthewevans removed their assignment Jul 19, 2026
Merged via the queue into phase-rs:main with commit 727474f Jul 19, 2026
13 checks passed
@minion1227
minion1227 deleted the minion_ordinal_spell_costmod branch July 20, 2026 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants