Skip to content

fix(parser): parse compound activation conditions#5115

Merged
matthewevans merged 3 commits into
phase-rs:mainfrom
ntindle:codex/root28-activation-condition-or
Jul 8, 2026
Merged

fix(parser): parse compound activation conditions#5115
matthewevans merged 3 commits into
phase-rs:mainfrom
ntindle:codex/root28-activation-condition-or

Conversation

@ntindle

@ntindle ntindle commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Parse compound activation restriction conditions whose later branches repeat if, such as Activate only if this land entered this turn or if you control a basic land.
  • Let source-zone shorthand fragments after or parse as source-zone conditions, such as on the battlefield or in your graveyard.
  • Use the shared type-phrase parser for qualified you control a ... restriction conditions so basic land becomes ObjectCount(Land + HasSupertype(Basic), controller You) >= 1 rather than a literal subtype-like string.
  • Remove Hidden Lair and Skyblade's Boon from root chore: update coverage stats and badges #28 in docs/parser-misparse-backlog.md.

Root Cause

parse_connector_split split restriction conditions on or, then parsed each fragment as atomic condition text. That failed when Oracle repeated the condition marker on the second branch (or if you control ...) or used source-zone shorthand (or in your graveyard). After the compound parse failed, the fallback atomic parser kept only the first recognized leaf it could scan.

Parse Audit

Focused before/after export covered all 17 root #28 backlog cards:

  • Fixer, Techno Terror
  • Garenbrig Squire
  • Goremand
  • Grizzled Wolverine
  • Hermit of the Natterknolls
  • Hidden Lair
  • Highspire Bell-Ringer
  • Hurkyl's Final Meditation
  • Ichneumon Druid
  • MACH-1, Swooping Scoundrel
  • Shadowheart, Sharran Cleric
  • Skarrgan Hellkite
  • Skyblade's Boon
  • Tomb Tyrant
  • Trade Caravan
  • Uthros Research Craft
  • Uthros, Titanic Godcore

Raw changed card keys:

  • hidden lair
  • skyblade's boon

Intentional deltas:

  • Hidden Lair: second mana ability restriction changes from SourceEnteredThisTurn only to Or(SourceEnteredThisTurn, ObjectCount(basic land you control) >= 1).
  • Skyblade's Boon: activated ability restriction changes from SourceInZone(Graveyard) only to Or(SourceInZone(Battlefield), SourceInZone(Graveyard)).

coverage-parse-diff on the focused exports reported no signature-level card-parse changes, which is expected for this internal activation-condition shape.

Verification

  • cargo fmt --all
  • ./scripts/check-parser-combinators.sh
  • cargo test -p engine --features cli --lib parses_compound_or_if_restriction_fragments -- --nocapture
  • cargo test -p engine --features cli --lib parses_compound_source_zone_shorthand -- --nocapture
  • cargo test -p engine --features cli --lib restriction_condition -- --nocapture
  • target/debug/card-data-validate /tmp/phase-root28-after.json
  • target/debug/coverage-parse-diff /tmp/phase-root28-base.json /tmp/phase-root28-after.json --markdown /tmp/phase-root28-diff.md --json /tmp/phase-root28-diff.json
  • cargo clippy -p engine --all-targets -- -D warnings
  • git diff --check

@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 01:21
@ntindle ntindle requested a review from matthewevans as a code owner July 5, 2026 01:21
@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

✓ No card-parse changes detected.

@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] Current head is not queueable because CI is red in parser-condition coverage. Evidence: the failed Rust tests jobs in run 28725528734 show parser::oracle_condition::tests::parses_you_control_conditions no longer returns ParsedCondition::YouControlLegendaryCreature, parser::oracle_condition::tests::parses_controls_creature_with_keyword_both_scopes now returns a generic QuantityComparison instead of ControlsCreatureWithKeyword, and the deadly_rollick_ir snapshot changed from YouControlSubtypeCountAtLeast to a generic object-count comparison.

Why it matters: this PR touches the shared condition parser, and those canonical condition variants are consumed downstream by the parser/IR pipeline. Please update the grammar so the existing specific conditions still win for those sibling cases, or update the architecture intentionally with discriminating coverage and snapshot changes if the generic representation is deliberate.

I did not push a maintainer fix because this cross-repository PR has maintainerCanModify=false.

@matthewevans matthewevans added the bug Bug fix label Jul 6, 2026
# Conflicts:
#	crates/engine/src/parser/oracle_condition.rs
@ntindle

ntindle commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Updated this branch with current origin/main (4827c91) and resolved the parser-condition merge conflict by keeping upstream's later generic you control ordering. That removes the early qualified-presence shadowing that made the specific YouControlLegendaryCreature, ControlsCreatureWithKeyword, and Deadly Rollick commander condition paths regress. Local checks: cargo fmt --all; WSL cargo test -p engine --lib parses_you_control_conditions; WSL cargo test -p engine --lib parses_controls_creature_with_keyword_both_scopes; WSL cargo test -p engine --lib deadly_rollick (all passed).

@matthewevans matthewevans self-assigned this Jul 8, 2026
@ntindle

ntindle commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up: merged latest origin/main again after #5123/#5109 landed; head is now f635f6e and current with main. Local checks for this follow-up: cargo fmt --all; git diff --check.

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

Maintainer sweep approval for current head f635f6efa039424f3fe61b320150817017c3bf50.

I rechecked the stale requested-change item against the current diff. The branch is current with origin/main, hard-stop classification is clean, the previous parser-condition regressions are addressed by keeping the specific you control condition paths ahead of the generic quantity fallback, and the parse-diff sticky reports no card-parse changes. Required Rust/card-data checks are still pending, so auto-merge is enabled for the merge queue to wait on them.

@matthewevans matthewevans enabled auto-merge July 8, 2026 07:44
@matthewevans matthewevans removed their assignment Jul 8, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 8, 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.

Maintainer re-review: current head is clean; prior parser-condition blocker is resolved and CI is green.

Merged via the queue into phase-rs:main with commit c10be1f Jul 8, 2026
12 checks passed
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