fix(parser): parse Faerie Miscreant singleton named condition - #7297
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe parser now accepts a single named creature condition without a connector while rejecting empty names and multiple unconnected conditions. Parser and integration tests cover Faerie Miscreant’s trigger, draw behavior, and intervening state. ChangesFaerie Miscreant parser fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Maintainer HOLD (current head |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_nom/condition.rs`:
- Around line 965-976: Add a verified CR annotation directly above the
connector-less singleton handling in the named-card parsing function,
documenting the named-card rule basis and that a trimmed non-empty name is
accepted as one condition. Use the applicable Comprehensive Rules number and
description, preserving the existing empty-name error and singleton return
behavior.
In `@crates/engine/tests/integration/issue_7154_faerie_miscreant.rs`:
- Around line 118-120: Update the test flow around move_to_zone so the
companion’s move to Zone::Graveyard uses the replacement-aware production
pipeline via ProposedEvent::ZoneChange, rather than calling zones::move_to_zone
directly. Process the resulting events through the runner so replacement effects
and normal zone-change handling occur before the resolution-time recheck.
- Around line 69-85: Add a runtime integration scenario alongside
does_not_draw_without_another_faerie_miscreant that places another creature with
a different name under P0’s control, then cast and resolve Faerie Miscreant and
assert zero cards are drawn. Ensure the setup includes the required “another”
creature while differing from “Faerie Miscreant,” so evaluation exercises the
named companion condition rather than only FilterProp::Another.
🪄 Autofix
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: ec630ae3-ec46-46f4-aa14-86324d42fdac
📒 Files selected for processing (5)
crates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/tests/integration/issue_7154_faerie_miscreant.rscrates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.md
💤 Files with no reviewable changes (1)
- docs/parser-misparse-backlog.md
| ) -> Result<(Vec<&'a str>, Option<ControlNamedConnector>), nom::Err<OracleError<'a>>> { | ||
| let Some((connector_index, connector_len, connector, serial_comma)) = | ||
| find_shared_control_named_final_connector(names_text) | ||
| else { | ||
| return Err(nom::Err::Error(nom::error::Error::new( | ||
| error_input, | ||
| nom::error::ErrorKind::Fail, | ||
| ))); | ||
| let name = names_text.trim(); | ||
| if name.is_empty() { | ||
| return Err(nom::Err::Error(nom::error::Error::new( | ||
| error_input, | ||
| nom::error::ErrorKind::Fail, | ||
| ))); | ||
| } | ||
| return Ok((vec![name], None)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a verified CR annotation for singleton named-card parsing.
Lines 965-976 make a connector-less named card valid as one condition. This rules-facing behavior has no verified CR <number>: <description> annotation. Add an annotation that describes the named-card rule basis and the singleton condition semantics.
As per coding guidelines: “Implement MTG behavior according to the Comprehensive Rules; verify the relevant CR section before completion, and annotate rules-related code with a verified CR number and description.”
🤖 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_nom/condition.rs` around lines 965 - 976, Add
a verified CR annotation directly above the connector-less singleton handling in
the named-card parsing function, documenting the named-card rule basis and that
a trimmed non-empty name is accepted as one condition. Use the applicable
Comprehensive Rules number and description, preserving the existing empty-name
error and singleton return behavior.
Sources: Coding guidelines, Path instructions
|
Generated for head Parse changes introduced by this PR · 2 card(s), 5 signature(s) (baseline: main
|
|
Maintainer HOLD (current head |
|
Maintainer HOLD (current head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current-head implementation review is clean.
Reviewed cda8ae096f6793abe66aa95d332b5ed356845dc9 against the full diff, current CI, and the exact-head parse-diff artifact. The singleton branch at crates/engine/src/parser/oracle_nom/condition.rs:818-829 returns exactly one typed presence condition while preserving empty-name rejection and the trigger effect boundary; the existing CR 201.2 function documentation at :784-800 covers the named-card rule. Runtime regressions drive cast, ETB trigger collection, and resolution-time recheck through the production Destroy pipeline (crates/engine/tests/integration/issue_7154_faerie_miscreant.rs:47-133).
The prior CodeRabbit review is attached to 8a79a7e, not this head; its two functional findings are resolved by 3f0a333. Its annotation request is already satisfied by the adjacent named-card parser documentation. The current exact-head CI and parse artifact are green/available.
2 participants
Summary
Fixes Faerie Miscreant’s singleton named-card intervening-if condition so it lowers to a live battlefield presence check and preserves its draw effect. Adds parser and runtime coverage, including the resolution-time recheck required by an intervening “if” clause.
Closes #7154.
Files changed
crates/engine/src/parser/oracle_nom/condition.rs— accept singletonyou control … named …conditions without inventing a connector.crates/engine/src/parser/oracle_trigger_tests.rs— assert the full Faerie Miscreant parse shape and draw effect.crates/engine/tests/integration/issue_7154_faerie_miscreant.rs— add positive, negative, and resolution-time recheck regressions.crates/engine/tests/integration/main.rs— register the integration module.docs/parser-misparse-backlog.md— clear the resolved Faerie Miscreant entry.Track
Developer
LLM
Model: gpt-5.6-sol
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 109.1; CR 201.2; CR 603.4.
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
/Users/lucacola/.rustup/toolchains/nightly-2026-04-19-aarch64-apple-darwin/bin/cargo fmt --all -- --check— passed./Users/lucacola/.rustup/toolchains/nightly-2026-04-19-aarch64-apple-darwin/bin/cargo test -p phase-engine --test integration issue_7154_faerie_miscreant -- --nocapture— passed: 3 passed, 0 failed./Users/lucacola/.rustup/toolchains/nightly-2026-04-19-aarch64-apple-darwin/bin/cargo test -p phase-engine you_control_named_rejects_empty_singleton_name -- --nocapture— passed: 1 passed, 0 failed.cargo clippy-strict— completed before the final test-only assertion correction; no errors reported.PHASE_REFRESH_MTGJSON=1 ./scripts/gen-card-data.sh— completed; no tracked card-data change.cargo coverage— completed before the final test-only assertion correction; Faerie Miscreant remained supported.cargo semantic-audit— completed: 32,749 cards audited; Faerie Miscreant has zero findings.cargo test -p phase-engine— the post-correction full run was intentionally interrupted at the user’s request to reclaim disk space; GitHub Actions is the CI-owned full-suite verification.Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=8a79a7e78b572ac59ecb6412d268e0e58c99fdd5 base=8a95d233016077c852d46cf8b5fc9c65aa20bebb
Anchored on
Final review-impl
Final review-impl PASS head=8a79a7e78
Claimed parse impact
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Bug Fixes
Tests