Skip to content

fix(parser): parse the leading negated-turn scope on static cost modifiers (Geyser Drake, Naiad of Hidden Coves)#6198

Merged
matthewevans merged 4 commits into
phase-rs:mainfrom
minion1227:minion_negated_turn_costmod
Jul 19, 2026
Merged

fix(parser): parse the leading negated-turn scope on static cost modifiers (Geyser Drake, Naiad of Hidden Coves)#6198
matthewevans merged 4 commits into
phase-rs:mainfrom
minion1227:minion_negated_turn_costmod

Conversation

@minion1227

Copy link
Copy Markdown
Contributor

Summary

"During turns other than yours, spells you cast cost {1} less to cast" (Geyser
Drake
, Naiad of Hidden Coves) parsed with condition: None — the leading
negated-turn timing clause was not recognized by the cost-modifier scope stripper,
so the reducer applied on every turn, including the controller's own.

Root cause: strip_cost_mod_during_your_turn_scope (static_helpers.rs) only
handled a trailing " during your turn" suffix (Paladin Class). A leading
"During turns other than yours," / "During your turn," clause — the form these
reducers print — fell through and dropped the timing restriction.

Fix: add parse_leading_turn_scope, mapping the leading clause to the same
StaticCondition the CDA / dispatch / type-change static parsers already use for
this vocabulary — Not(DuringYourTurn) for "turns other than yours",
DuringYourTurn for the affirmative form — and route the single shared
leading-clause fallback in try_parse_cost_modification through it, so every
cost-mod branch (self-spell, first-qualified, generic) gates the negated form
symmetrically with the affirmative one (previously only the affirmative
"During your turn," was recognized). Parse-only: the cost-mod condition
resolver already evaluates StaticCondition::Not and DuringYourTurn
(evaluate_cost_mod_static_condition), gating against the source controller
(CR 102.1).

Files changed

  • crates/engine/src/parser/oracle_static/static_helpers.rs — new parse_leading_turn_scope combinator; the shared leading-clause fallback now recognizes both the affirmative and negated forms (single authority for all branches).
  • crates/engine/src/parser/oracle_static/tests.rs — parser unit test over the leading negated + affirmative forms, a plain-unconditioned regression, and a self-spell branch-symmetry case.
  • crates/engine/tests/integration/negated_turn_spell_cost_reduction.rs (new, registered) — runtime cast-pipeline regression.

CR references

  • CR 604.1 — continuous-effect duration/condition (the timing gate).
  • CR 601.2f — total cost determination (where the reducer applies).
  • CR 102.1 — the active player is the player whose turn it is (the gate is evaluated against the source controller).

Implementation method (required)

Found in the same fresh cost-modifier live-parse audit that surfaced the ordinal
fix; live-verified against HEAD (condition: None). The Not(DuringYourTurn)
mapping reuses the negated-turn vocabulary already present in cda.rs / dispatch.rs
/ type_change.rs — a building-block reuse, not a new condition.

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 warnings — 0 warnings.
  • cargo test -p engine --lib parser::oracle_static — green (incl. the new leading-scope unit test).
  • cargo test -p engine --test integration negated_turn_spell_cost_reduction3/3 pass: a {2} spell stays {2} on the controller's own turn and is reduced to {1} on other turns (cost read at WaitingFor::TargetSelection; reverting the parser fix reduces on the controller's turn too and fails the test).

Gate A

Tilt was down; ran checks directly. clippy 0 warnings; parser lib suite green;
integration regression 3/3.

Anchored on

upstream/main @ 11cc01581.

Final review-impl

Independent fresh-context review (diff + CLAUDE.md + review-impl skill only, no
prior conversation) ran clean on all lenses — correct seam, nom-mandate (pure
alt/value/tag), regression safety (trailing "during your turn" suffix and
plain cost mods unchanged; the comma in tag("during your turn, ") can't mis-fire
on the comma-less trailing form), realizability (evaluate_cost_mod_static_condition
handles Not + DuringYourTurn, gated against the source controller), and a
discriminating runtime test (flips on revert). It raised one LOW finding: the
negated form was initially handled only in the generic branch, leaving the
self-spell / first-qualified branches asymmetric. Addressed in this PR by
routing the negated form through the single shared leading-clause authority so all
three branches are symmetric (covered by the added self-spell unit-test case). No
real card exercised the asymmetric subclass, but the fix is now correct for it.

Claimed parse impact

2 cards move from an unconditioned every-turn reducer to a correctly negated-turn-
gated reducer: Geyser Drake, Naiad of Hidden Coves. No card regresses — the
trailing " during your turn" suffix path (Paladin Class) and plain unconditioned
cost mods are unchanged (covered by the parser unit test's regression asserts).

Validation Failures

None.

CI Failures

None expected.

…fiers (Geyser Drake, Naiad of Hidden Coves)

"During turns other than yours, spells you cast cost {1} less to cast" (Geyser
Drake, Naiad of Hidden Coves) parsed with condition: None -- the leading
negated-turn timing clause was not recognized by the cost-modifier parser, so
the reducer applied on EVERY turn, including the controller's own.

Root cause: the cost-modifier leading-condition handling recognized only the
affirmative "During your turn," prefix (Tithe Taker). A leading "During turns
other than yours," clause -- the form these reducers print -- fell through and
dropped the timing restriction.

Fix: add `parse_leading_turn_scope`, mapping the leading clause to the same
`StaticCondition` the CDA / dispatch / type-change static parsers already use for
this vocabulary -- `Not(DuringYourTurn)` for "turns other than yours",
`DuringYourTurn` for the affirmative form -- and route the shared leading-clause
fallback in `try_parse_cost_modification` through it. A single authority for every
cost-mod branch (self-spell, first-qualified, generic), so the negated form is
handled symmetrically with the affirmative one. Parse-only: the cost-mod
condition resolver already evaluates `StaticCondition::Not` and `DuringYourTurn`
(`evaluate_cost_mod_static_condition`), gating against the source controller
(CR 102.1).

CR 604.1 (continuous-effect duration/condition) / CR 601.2f (total cost
determination) / CR 102.1 (active player).

Tests: parser unit test over the leading negated + affirmative forms, a
plain-unconditioned regression, and a self-spell branch-symmetry case; registered
runtime cast-pipeline regression (negated_turn_spell_cost_reduction) proving a {2}
spell stays {2} on the controller's own turn and is reduced to {1} on other turns
-- the cost is read at `WaitingFor::TargetSelection`, and reverting the parser fix
reduces it on the controller's turn too, failing the test.

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 12:47
@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

@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 parsing leading turn-scope clauses, specifically handling both affirmative ('during your turn, ') and negated ('during turns other than yours, ') conditions for cost-modification static abilities. It extracts this logic into a reusable helper parse_leading_turn_scope using nom combinators, updates try_parse_cost_modification to utilize it, and adds comprehensive integration tests to verify correct parsing and runtime behavior. The implementation adheres perfectly to the repository's architectural guidelines, including proper nom combinator usage and Magic Comprehensive Rules (CR) annotations. No review comments were provided, and there is no additional feedback to address.

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.

@matthewevans matthewevans self-assigned this Jul 19, 2026
@matthewevans

Copy link
Copy Markdown
Member

Maintainer update for ca18fc9bd5e935e9758432dfb2ee3724fa183e3b:

  • Fixed the leading-scope dispatch gap: During … is peeled before both self-spell and first-qualified-spell parsing, and is composed with the first-spell gate rather than replacing it.
  • Added the missing runtime regression using Scryfall-verified Hurkyl's Final Meditation from hand: the locked pending-cast cost is 7 on its controller's turn and 10 on another player's turn. The parser regression also covers the leading-scope first-spell branch.
  • Independent adversarial re-review found no remaining blocker. Local security/diff scan and full review/comment/thread pagination were clean.

Held pending current-head CI, a fresh parse-diff artifact, and the now-DIRTY merge state. I did not run builds or tests directly; cargo fmt --all and the repository pre-commit parser gates passed.

@matthewevans matthewevans added the bug Bug fix label Jul 19, 2026
@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 4 card(s), 4 signature(s) (baseline: main b008e772d4c3)

🟡 Modified fields (4 signatures)

  • 2 cards · 🔄 static/ReduceCost · changed field conditional: not (during your turn)
    • Affected (first 3): Geyser Drake, Naiad of Hidden Coves
  • 1 card · 🔄 static/RaiseCost · changed field affects: cardself
    • Affected (first 3): Hurkyl's Final Meditation
  • 1 card · 🔄 static/RaiseCost · changed field conditional: not (during your turn)
    • Affected (first 3): Hurkyl's Final Meditation
  • 1 card · 🔄 static/ReduceCost · changed field affects: cardself
    • Affected (first 3): Discontinuity

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

# Conflicts:
#	crates/engine/src/parser/oracle_static/static_helpers.rs
@matthewevans

Copy link
Copy Markdown
Member

Maintainer branch update for d25b12d8458fb55428b2225298d8b4eb847599df:

  • Merged current main and resolved the shared static_helpers.rs conflict without dropping this PR's leading-turn scope behavior.
  • The resolution uses main's generalized ordinal spell gate and composes it with the leading scope; the Hurkyl's hand/pending-cast regression remains intact.
  • Post-merge adversarial review found no blocker. cargo fmt --all, diff checks, and repository pre-commit parser gates passed; no builds or tests were run directly.

Held pending the new current-head CI, parse-diff artifact, and GitHub's transient mergeStateStatus=BLOCKED while checks queue.

@matthewevans matthewevans removed their assignment Jul 19, 2026
The hurkyls_final_meditation_self_cost_tracks_controller_turn regression
cast Hurkyl's Final Meditation with CastPaymentMode::Auto but seeds no
mana source, so Auto payment failed with "Cannot pay mana cost" before the
helper could read the locked pending-cast cost. Hurkyl's takes no target,
so the cast never pauses at TargetSelection the way the Geyser helper does.
Manual mode surfaces WaitingFor::ManaPayment and pauses at the locked-cost
boundary without auto-tapping, matching the helper's own
assert!(matches!(waiting_for, ManaPayment { .. })).

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

Copy link
Copy Markdown
Contributor Author

Pushed 354c0b03 on top of your merge to fix the CI failure introduced by the hurkyls_final_meditation_self_cost_tracks_controller_turn regression.

The helper cast Hurkyl's Final Meditation with CastPaymentMode::Auto but the scenario seeds no mana source, so Auto-mode payment failed with ActionNotAllowed("Cannot pay mana cost") before the pending-cast cost could be read. Hurkyl's takes no target, so unlike the Geyser helper the cast never pauses at TargetSelection. Switched to CastPaymentMode::Manual, which surfaces WaitingFor::ManaPayment and pauses at the locked-cost boundary without auto-tapping — matching the helper's own assert!(matches!(waiting_for, ManaPayment { .. })).

Verified locally (Tilt down): all 4 tests in negated_turn_spell_cost_reduction pass, including the Hurkyl's P0=7 / P1=10 assertions. No production code touched — test-only.

@matthewevans

Copy link
Copy Markdown
Member

Acknowledged the 354c0b03 test-fixture correction: switching the Hurkyl regression to manual payment reaches the intended ManaPayment boundary without changing the parser behavior under review. The remaining gate is refreshed CI and current-head parse-diff evidence.

@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: the leading turn-scope parser uses the shared static-condition vocabulary, composes with the ordinal gate, and the registered runtime regressions distinguish both controller-turn outcomes.

@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 5b4a648 Jul 19, 2026
13 checks passed
@minion1227
minion1227 deleted the minion_negated_turn_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