Skip to content

[codex] fix(parser): split repeated named spell conditions#5123

Open
ntindle wants to merge 1 commit into
phase-rs:mainfrom
ntindle:codex/root30-sift-named-spells
Open

[codex] fix(parser): split repeated named spell conditions#5123
ntindle wants to merge 1 commit into
phase-rs:mainfrom
ntindle:codex/root30-sift-named-spells

Conversation

@ntindle

@ntindle ntindle commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes the Root #30 subcluster for Sift Through Sands: repeated named-spell cast conditions of the form you've cast a spell named A and a spell named B this turn.

Root cause: the generic single-spell-history parser treated the whole phrase after the first spell named as one literal Named value, producing a filter named peer through depths and a spell named reach through mists. The condition therefore checked for one impossible card name instead of requiring both spells to have been cast.

This PR adds a nom combinator before the generic single-spell parser that recognizes repeated a spell named ... items and lowers them to StaticCondition::And / Or over existing SpellsCastThisTurn quantity checks. The effect-condition bridge already maps that to AbilityCondition::And for the optional search clause.

Backlog update: removes Sift Through Sands from docs/parser-misparse-backlog.md Root #30, changing the branch-local count from 12 to 11.

Parsed-card diff audit

Targeted baseline/after exports:

  • Before: /tmp/phase-root30-sift-baseline.json
  • After: /tmp/phase-root30-sift-after.json
  • Filter: Sift Through Sands

Raw parsed card keys in both exports:

  • sift through sands

Expected parse movement:

  • Before: SpellsCastThisTurn had one Named filter: peer through depths and a spell named reach through mists.
  • After: the optional search clause is gated by an And of two SpellsCastThisTurn >= 1 checks, named peer through depths and reach through mists.

Sibling scan against current MTGJSON found exactly one repeated named-spell condition text:

jq -r '.data | to_entries[] | .key as $name | .value[] | select(.text? and (.text | test("spell named .* (and|or|and/or) a spell named"; "i"))) | $name + " :: " + (.text | gsub("\\n"; " / "))' data/mtgjson/AtomicCards.json

coverage-parse-diff reported no support-signature clusters (oracle_changed: 0), so the expected raw AST repair is confined to the audited card.

Validation

  • grep -n "^201.2" docs/MagicCompRules.txt
  • grep -n "^603.4" docs/MagicCompRules.txt
  • cargo fmt --all
  • ./scripts/check-parser-combinators.sh
  • git diff --check
  • cargo test -p engine --features cli --lib youve_cast_repeated_named_spells_this_turn_is_compound -- --nocapture
  • cargo test -p engine --features cli --lib sift_through_sands_condition_requires_both_named_spells -- --nocapture
  • cargo test -p engine --features cli --lib spell_this_turn -- --nocapture
  • cargo run -p engine --features cli --bin oracle-gen -- data --filter "Sift Through Sands" --output /tmp/phase-root30-sift-after.json
  • cargo run -p engine --features cli --bin card-data-validate -- /tmp/phase-root30-sift-after.json
  • cargo run -p engine --features cli --bin coverage-parse-diff -- /tmp/phase-root30-sift-baseline.json /tmp/phase-root30-sift-after.json --markdown /tmp/phase-root30-sift-parse-diff.md --json /tmp/phase-root30-sift-parse-diff.json

Tilt was not running in this checkout (No tilt apiserver found: tilt-default), so I used the direct cargo fallback allowed by the repo instructions.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@ntindle ntindle marked this pull request as ready for review July 5, 2026 04:24
@ntindle ntindle requested a review from matthewevans as a code owner July 5, 2026 04:24
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main 623d82ab3d22)

1 card(s) · ability/SearchLibrary · field conditional: named "peer through depths and a spell named reach through mists" card spells cast this turn (your) ≥ 1named "peer through depths" card spells cast this turn (your) ≥ 1 and named "reach through mists" card spells cast this…

Examples: Sift Through Sands

1 card(s) · ability/Shuffle · field conditional: named "peer through depths and a spell named reach through mists" card spells cast this turn (your) ≥ 1named "peer through depths" card spells cast this turn (your) ≥ 1 and named "reach through mists" card spells cast this…

Examples: Sift Through Sands

@matthewevans matthewevans self-assigned this Jul 5, 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.

[MED] The new repeated named-spell helper cites the wrong Comprehensive Rule for this spell-resolution condition. Evidence: crates/engine/src/parser/oracle_nom/condition.rs adds parse_repeated_named_spells_this_turn with CR 201.2 + CR 603.4, but docs/MagicCompRules.txt defines CR 603.4 as the intervening-if triggered-ability rule, and the parse-diff shows this helper is lowering Sift Through Sands' spell-resolution "If you've cast..." condition. Why it matters: this repo treats verified CR comments as review evidence, and a wrong CR citation is worse than no citation around rules-sensitive parser code. Suggested fix: keep the CR 201.2 named-object grounding here and either remove CR 603.4 from this helper comment or replace/split it with a rule that actually applies to conditional spell resolution.

@matthewevans matthewevans added the bug Bug fix label Jul 5, 2026
@matthewevans matthewevans removed their assignment Jul 5, 2026
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