fix(parser): preserve discard-this-way conditional outcomes - #6855
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (6)
📝 WalkthroughWalkthroughThe parser now recognizes affirmative and negated “discard a card this way” conditions. Regression tests cover Great Desert Hellion and Chains of Mephistopheles. Integration tests verify draw and mill outcomes after successful or failed discard attempts. ChangesDiscard condition parsing
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 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…f Mephistopheles The replacement's trailing "if the player discards a card this way, they draw a card. If the player doesn't discard a card this way, they mill a card." was silently dropped — Discard, Draw, and Mill all ran unconditionally instead of Draw/Mill being mutually exclusive on whether the discard actually removed a card. The reflexive-connector recognizer only handled the bare "if you do,"/"if the player does," forms, not the echoed-verb "discards a card this way" phrasing this card uses. Extends parse_affirmative_reflexive_connector / parse_negated_reflexive_connector with the untyped echoed-discard form, mapping to the same effect_performed() condition the bare connector already uses (and that the engine's mandatory- rider seeding already resolves correctly for a failed discard). Closes #5653
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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_nom/condition.rs`:
- Around line 9425-9463: Refactor parse_discard_this_way_affirmative_connector
and parse_discard_this_way_negated_connector to compose the existing subject
alternatives from parse_affirmative_reflexive_connector and
parse_negated_reflexive_connector with the shared discard-this-way verb suffix,
preserving each subject’s correct verb form. Remove the duplicated full tag()
permutations so both bare and discard-specific connectors derive their
subject/anaphor coverage from one source.
🪄 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: 9840f88d-7eed-4a0f-96dd-8ec0cd25ec08
📒 Files selected for processing (6)
crates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/chains_of_mephistopheles_discard_draw_or_mill.rscrates/engine/tests/integration/main.rs
| parse_discard_this_way_affirmative_connector, | ||
| )) | ||
| .parse(input) | ||
| } | ||
|
|
||
| /// CR 608.2c + CR 701.9a: the echoed-verb affirmative form — "if [subject] | ||
| /// discard(s) a card this way, " (Chains of Mephistopheles / Magus of the | ||
| /// Chains: "If the player discards a card this way, they draw a card."). This | ||
| /// is the untyped-object sibling of `parse_you_discard_this_way_clause` (which | ||
| /// requires a typed filter and lowers to `ZoneChangedThisWay`): an unqualified | ||
| /// "a card" carries no type information to check, so the condition collapses | ||
| /// to the same "did the preceding discard occur" gate as the bare "if you | ||
| /// do, " connector. The controller-specific "if you discard ..." form shares | ||
| /// that gate: its preceding optional discard publishes `OptionalEffectPerformed`, | ||
| /// so the follow-up stays attached to the discard outcome rather than executing | ||
| /// after the sacrifice alternative. | ||
| fn parse_discard_this_way_affirmative_connector(input: &str) -> OracleResult<'_, AbilityCondition> { | ||
| alt(( | ||
| value( | ||
| AbilityCondition::effect_performed(), | ||
| tag("if you discard a card this way, "), | ||
| ), | ||
| value( | ||
| AbilityCondition::effect_performed(), | ||
| tag("if a player discards a card this way, "), | ||
| ), | ||
| value( | ||
| AbilityCondition::effect_performed(), | ||
| tag("if they discard a card this way, "), | ||
| ), | ||
| value( | ||
| AbilityCondition::effect_performed(), | ||
| tag("if that player discards a card this way, "), | ||
| ), | ||
| value( | ||
| AbilityCondition::effect_performed(), | ||
| tag("if the player discards a card this way, "), | ||
| ), | ||
| )) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Compose the subject dimension instead of repeating full tag() strings per subject.
parse_discard_this_way_affirmative_connector and parse_discard_this_way_negated_connector each hard-code one tag() per subject ("you", "a player", "they", "that player", "the player"), duplicating the exact subject list already enumerated in parse_affirmative_reflexive_connector and parse_negated_reflexive_connector. This repeats the full compound phrase ("if a player discards a card this way, ") instead of composing an existing subject-alternative with a shared verb-phrase suffix.
If a new subject/anaphor form is added later, it must be updated in two independent places (the bare "does"/"doesn't" list and the "discard(s) a card this way" list), risking silent drift between them.
Compose the subject alternative (with its matching verb form) once, then append the fixed " discard(s)/discard(s)n't a card this way, " suffix, so both lists share one subject-to-verb-form source.
♻️ Suggested composition approach
-fn parse_discard_this_way_affirmative_connector(input: &str) -> OracleResult<'_, AbilityCondition> {
- alt((
- value(
- AbilityCondition::effect_performed(),
- tag("if you discard a card this way, "),
- ),
- value(
- AbilityCondition::effect_performed(),
- tag("if a player discards a card this way, "),
- ),
- value(
- AbilityCondition::effect_performed(),
- tag("if they discard a card this way, "),
- ),
- value(
- AbilityCondition::effect_performed(),
- tag("if that player discards a card this way, "),
- ),
- value(
- AbilityCondition::effect_performed(),
- tag("if the player discards a card this way, "),
- ),
- ))
- .parse(input)
-}
+fn parse_discard_this_way_subject(input: &str) -> OracleResult<'_, &str> {
+ alt((
+ tag("you discard"),
+ tag("a player discards"),
+ tag("they discard"),
+ tag("that player discards"),
+ tag("the player discards"),
+ ))
+ .parse(input)
+}
+
+fn parse_discard_this_way_affirmative_connector(input: &str) -> OracleResult<'_, AbilityCondition> {
+ value(
+ AbilityCondition::effect_performed(),
+ (tag("if "), parse_discard_this_way_subject, tag(" a card this way, ")),
+ )
+ .parse(input)
+}Based on learnings, the referenced documentation for this file states: "The discard-this-way condition additions should therefore be generalized across supported subject/anaphor forms and wired into the existing condition dispatcher, not implemented as card-name or full-string special cases." As per coding guidelines, crates/engine/src/parser/**/*.rs requires composing nom combinators across independent dimensions "instead of enumerating full-string permutations."
Also applies to: 9493-9523
🤖 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 9425 - 9463,
Refactor parse_discard_this_way_affirmative_connector and
parse_discard_this_way_negated_connector to compose the existing subject
alternatives from parse_affirmative_reflexive_connector and
parse_negated_reflexive_connector with the shared discard-this-way verb suffix,
preserving each subject’s correct verb form. Remove the duplicated full tag()
permutations so both bare and discard-specific connectors derive their
subject/anaphor coverage from one source.
Sources: Coding guidelines, Path instructions
3cbf437 to
454d0cc
Compare
Parse changes introduced by this PR · 4 card(s), 5 signature(s) (baseline: main
|
2 participants
Rescues closed contributor PR #5808 for echoed discard-this-way conditionals. Handles Chains of Mephistopheles and Magus forms while retaining the Great Desert Hellion optional-discard outcome gate.
Summary by CodeRabbit
Bug Fixes
Tests