Skip to content

fix(parser): parse external entry tap-state grammar - #6853

Merged
matthewevans merged 3 commits into
mainfrom
maintainer/rescue-4991-vigorous-farming
Aug 2, 2026
Merged

fix(parser): parse external entry tap-state grammar#6853
matthewevans merged 3 commits into
mainfrom
maintainer/rescue-4991-vigorous-farming

Conversation

@matthewevans

@matthewevans matthewevans commented Aug 2, 2026

Copy link
Copy Markdown
Member

Rescues closed contributor PR #4991 for Vigorous Farming.

Parses the general external-entry tap-state grammar through the replacement pipeline and adds parser plus runtime coverage. Rebased onto current main and hardened after maintainer review.

Summary by CodeRabbit

  • Bug Fixes

    • Improved recognition of effects that cause permanents to enter the battlefield tapped or untapped.
    • Supports both singular and plural wording, optional “the battlefield” phrasing, and more varied subjects.
    • Correctly handles replacement-effect ordering when multiple land-entry effects apply, ensuring the selected order produces the expected result.
  • Tests

    • Added coverage for long-form untapped-entry text and real gameplay scenarios involving competing replacement effects.

jaso0n0818 and others added 3 commits August 1, 2026 17:43
…external entry

Vigorous Farming ("Lands you control enter the battlefield untapped.")
fell through to Unimplemented for the same reason Frozen Aether did before
the previous fix: CR 614.1c external-entry replacement effects are
templated by WotC in both a short form ("enter untapped") and the
fully-spelled long form ("enter the battlefield untapped"), and only the
short form was recognized.

Mirrors the tapped-side fix in the same two gates:
- `oracle_classifier::is_replacement_pattern` — added the long-form
  untapped suffix alongside the existing short-form and tapped-long-form
  checks.
- `oracle_replacement::parse_external_entry_suffix` — added long-form
  "enter(s) the battlefield untapped" arms alongside the existing
  short-form and tapped-long-form arms.

Added parser tests for both the real card (Vigorous Farming, single-type
subject) and the controller-scoped Or-filter subject shape.

clippy -D warnings clean; full engine test green (14714 lib + 1721
integration, 0 failures).

Model: claude-sonnet-5 (Claude Code)
@matthewevans matthewevans added the bug Bug fix label Aug 2, 2026
@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The parser now supports long-form singular and plural untapped battlefield-entry text through one grammar helper. Parser tests and a Vigorous Farming integration test validate classification, replacement ordering, and the final untapped land entry.

Changes

External-entry replacement parsing

Layer / File(s) Summary
External-entry grammar and classification
crates/engine/src/parser/oracle_classifier.rs, crates/engine/src/parser/oracle_replacement.rs
The classifier recognizes singular and plural “enter the battlefield untapped” text. A shared helper parses subjects, verb number, optional battlefield wording, and tap state. Parser tests cover typed land and opponent-controlled Or-filter subjects.
Replacement pipeline validation
crates/engine/src/parser/oracle_tests.rs, crates/engine/tests/integration/issue_4991_vigorous_farming.rs, crates/engine/tests/integration/main.rs
Tests verify the self-reference untap replacement, replacement ordering, and final untapped land entry. The integration module is registered in the test suite.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • phase-rs/phase#6780: Both changes modify replacement-effect parsing in oracle_replacement.rs, but they cover different grammar cases.

Suggested reviewers: jacobwoodson

Sequence Diagram(s)

sequenceDiagram
  participant OracleClassifier
  participant ExternalEntryParser
  participant ReplacementPipeline
  participant ReplacementChoice
  OracleClassifier->>ExternalEntryParser: parse untapped battlefield-entry text
  ExternalEntryParser->>ReplacementPipeline: produce replacement definition
  ReplacementPipeline->>ReplacementChoice: present replacement ordering
  ReplacementChoice-->>ReplacementPipeline: apply self-tap replacement first
  ReplacementPipeline-->>ReplacementChoice: land enters untapped
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the parser fix for external-entry tap-state grammar.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch maintainer/rescue-4991-vigorous-farming

Comment @coderabbitai help to get the list of available commands.

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/engine/src/parser/oracle_classifier.rs`:
- Around line 736-737: Update is_replacement_pattern to return false before
scanning the long-form patterns when has_trigger_prefix(lower) is true,
preventing triggered abilities from matching these replacement phrases. Add
regression coverage for both “enter” and “enters” forms while preserving
replacement classification for non-triggered text.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b1be679d-2218-4133-956c-84117048aba6

📥 Commits

Reviewing files that changed from the base of the PR and between b902333 and 968d571.

📒 Files selected for processing (5)
  • crates/engine/src/parser/oracle_classifier.rs
  • crates/engine/src/parser/oracle_replacement.rs
  • crates/engine/src/parser/oracle_tests.rs
  • crates/engine/tests/integration/issue_4991_vigorous_farming.rs
  • crates/engine/tests/integration/main.rs

Comment on lines +736 to +737
"enter the battlefield untapped",
"enters the battlefield untapped",

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Exclude triggered abilities from the new long-form classifier patterns.

is_replacement_pattern returns true for Whenever a land enters the battlefield untapped, .... The external-entry suffix parser requires end-of-input, so it cannot parse the trigger body after the comma. The classifier can send this triggered ability to replacement parsing instead of trigger parsing.

If has_trigger_prefix(lower) is true for either long-form phrase, return false before this pattern scan. Add regression coverage for both enter and enters forms.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/parser/oracle_classifier.rs` around lines 736 - 737, Update
is_replacement_pattern to return false before scanning the long-form patterns
when has_trigger_prefix(lower) is true, preventing triggered abilities from
matching these replacement phrases. Add regression coverage for both “enter” and
“enters” forms while preserving replacement classification for non-triggered
text.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

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

🟢 Added (1 signature)

  • 1 card · ➕ replacement/ChangeZone · added: ChangeZone (scope=you control land, to zone=battlefield)
    • Affected (first 3): Vigorous Farming

🔴 Removed (1 signature)

  • 1 card · ➖ ability/unknown · removed: unknown
    • Affected (first 3): Vigorous Farming

@matthewevans
matthewevans added this pull request to the merge queue Aug 2, 2026
Merged via the queue into main with commit d376f99 Aug 2, 2026
15 checks passed
@matthewevans
matthewevans deleted the maintainer/rescue-4991-vigorous-farming branch August 2, 2026 08:40
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