Fix Dragon Man, Reformed Robot - #7030
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Warning Review limit reached
Next review available in: 2 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 (1)
📝 WalkthroughWalkthroughThe parser now models gerund additional costs, preserves them in graveyard-cast permissions, rejects unsupported riders, and validates all static extra costs. Regression and integration tests cover discard and exile costs. ChangesGraveyard cast cost handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant GraveyardPermissionParser
participant CastValidation
participant GameState
OracleText->>GraveyardPermissionParser: parse gerund additional cost
GraveyardPermissionParser-->>GraveyardPermissionParser: create CastExtraCost or decline
GraveyardPermissionParser->>CastValidation: provide cast permission
CastValidation->>CastValidation: check AbilityCost::is_payable
CastValidation->>GameState: apply mana and additional costs
GameState-->>GameState: move cards and resolve permanent
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/engine/src/parser/oracle_static/tests.rs (2)
14268-14286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe dispatch assertion does not check what the comment claims.
The comment states the dispatch must leave "no Unimplemented node behind for it", but the assertion only proves that some static with the same
modeexists. A leftoverEffect::Unimplementedfor the same line would still pass.hogaak_full_card_records_restriction_and_drops_no_unimplemented_lineincrates/engine/src/parser/oracle_casting.rs(Line 873) shows the pattern that actually enforces the claim.Add the absence check, or narrow the comment.
💚 Suggested addition
assert!( parsed .statics .iter() .any(|parsed_def| parsed_def.mode == def.mode), "full Oracle dispatch must route Dragon Man's line to the discard-cost \ permission, got {:?}", parsed.statics ); + assert!( + !parsed.abilities.iter().any(|ability| matches!( + ability.effect.as_ref(), + crate::types::ability::Effect::Unimplemented { .. } + )), + "the graveyard line must not also leave an Unimplemented effect behind: {:?}", + parsed.abilities + );🤖 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_static/tests.rs` around lines 14268 - 14286, Strengthen the assertion in the full Oracle dispatch test around parse_oracle_text so it verifies that no Unimplemented node remains for the tested line, following the absence-check pattern in hogaak_full_card_records_restriction_and_drops_no_unimplemented_line. Keep the existing mode-dispatch assertion and ensure the comment accurately reflects both requirements.Source: Path instructions
14339-14342: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exile filter contents, not just
is_some().
filter.is_some()passes for any filter, including a bareTargetFilter::Any. The rules-relevant part of Demilich's rider is that only instant and sorcery cards can pay it. A regression that widened the filter to "any card" would keep this test green.Assert the disjunction legs carry
TypeFilter::InstantandTypeFilter::Sorcery, ascost_exile_self_and_count_other_you_control_recovers_count_and_filterdoes incrates/engine/src/parser/oracle_cost.rs(Line 2120).🤖 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_static/tests.rs` around lines 14339 - 14342, Strengthen the Demilich exile-cost assertion around filter by matching its contents rather than only checking filter.is_some(). Verify the filter’s disjunction includes TypeFilter::Instant and TypeFilter::Sorcery, following the assertion pattern used by cost_exile_self_and_count_other_you_control_recovers_count_and_filter, while rejecting a bare TargetFilter::Any.Source: Path instructions
crates/engine/tests/integration/demilich_helbrute_graveyard_exile_cost.rs (1)
131-159: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe blocked case does not prove the instant/sorcery filter is enforced.
The blocked scenario has three eligible cards and the allowed scenario has four. Both outcomes are explained by raw count alone. If the exile filter regressed to "any card", the blocked scenario would still contain only three non-Demilich cards, so the test stays green while the rules fidelity is lost.
Add a third scenario: three instant/sorcery cards plus one ineligible graveyard card (for example a creature card). The cast must still be blocked. That isolates the filter from the count.
🤖 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/tests/integration/demilich_helbrute_graveyard_exile_cost.rs` around lines 131 - 159, Extend demilich_graveyard_cast_blocked_without_four_exilable_cards with a third scenario containing exactly three instant/sorcery cards and one ineligible card such as a creature. Give it the same phase, life, and mana setup, then assert can_cast_object_now is false to verify the instant/sorcery filter rather than raw graveyard count.Source: Path instructions
🤖 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/game/casting.rs`:
- Around line 13848-13851: Add a focused test covering the cast-permission path
around static_extra and is_payable for an Exile cost with a variable count such
as EXILE_COST_X. Verify that the cost is payable for every eligible exile-card
choice, while preserving the existing behavior for ineligible choices.
In `@crates/engine/src/parser/oracle_casting.rs`:
- Around line 236-241: Update the self-flash parsing arm around
parse_gerund_cost to recognize both “its other costs” and “their other costs”
closers, with an optional “paying ” prefix, matching the additional-cost
combinator used by parse_cast_permission_additional_cost_rider. De-gerund the
captured cost before calling parse_gerund_cost, and preserve the existing
Unimplemented handling and option.cost flow.
In `@crates/engine/src/parser/oracle_cost.rs`:
- Around line 87-107: Update parse_gerund_cost to use TextPair (or the module’s
existing nom_on_lower pattern) so gerund matching remains case-insensitive while
the matched remainder retains its original casing. Pass that original-cased
remainder to parse_oracle_cost, preserving subtype values such as “Vehicle” for
downstream filters.
In `@crates/engine/src/parser/oracle_static/tests.rs`:
- Around line 14339-14342: The tests do not verify that the exile-cost filters
restrict cards correctly. In
crates/engine/src/parser/oracle_static/tests.rs#L14339-L14342, replace the
presence check with assertions that the disjunction legs use TypeFilter::Instant
and TypeFilter::Sorcery; in
crates/engine/src/parser/oracle_static/tests.rs#L14391-L14400, bind the filter
instead of discarding it and assert TypeFilter::Creature with
FilterProp::Another; in
crates/engine/tests/integration/demilich_helbrute_graveyard_exile_cost.rs#L131-L159,
add three eligible instant/sorcery cards plus one ineligible graveyard card and
assert casting remains blocked.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_static/tests.rs`:
- Around line 14268-14286: Strengthen the assertion in the full Oracle dispatch
test around parse_oracle_text so it verifies that no Unimplemented node remains
for the tested line, following the absence-check pattern in
hogaak_full_card_records_restriction_and_drops_no_unimplemented_line. Keep the
existing mode-dispatch assertion and ensure the comment accurately reflects both
requirements.
- Around line 14339-14342: Strengthen the Demilich exile-cost assertion around
filter by matching its contents rather than only checking filter.is_some().
Verify the filter’s disjunction includes TypeFilter::Instant and
TypeFilter::Sorcery, following the assertion pattern used by
cost_exile_self_and_count_other_you_control_recovers_count_and_filter, while
rejecting a bare TargetFilter::Any.
In `@crates/engine/tests/integration/demilich_helbrute_graveyard_exile_cost.rs`:
- Around line 131-159: Extend
demilich_graveyard_cast_blocked_without_four_exilable_cards with a third
scenario containing exactly three instant/sorcery cards and one ineligible card
such as a creature. Give it the same phase, life, and mana setup, then assert
can_cast_object_now is false to verify the instant/sorcery filter rather than
raw graveyard count.
🪄 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: b0da4f2d-c633-4a71-8c1a-1378ea488b02
📒 Files selected for processing (9)
crates/engine/src/game/casting.rscrates/engine/src/parser/oracle_casting.rscrates/engine/src/parser/oracle_cost.rscrates/engine/src/parser/oracle_static/mod.rscrates/engine/src/parser/oracle_static/restriction.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/tests/integration/demilich_helbrute_graveyard_exile_cost.rscrates/engine/tests/integration/dragon_man_reformed_robot_graveyard_discard_cost.rscrates/engine/tests/integration/main.rs
| assert!( | ||
| filter.is_some(), | ||
| "the exile cost must carry the instant/sorcery card filter, got {filter:?}" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No test asserts the exile cost's filter, at any layer. The parser tests check count, zone, and mode, and the integration test varies only the number of eligible cards. A regression that widened the exile filter to "any card" — or that dropped FilterProp::Another — keeps every assertion in this PR green, while letting an ineligible card, or Helbrute itself, pay the cost.
crates/engine/src/parser/oracle_static/tests.rs#L14339-L14342: replacefilter.is_some()with assertions that the disjunction legs carryTypeFilter::InstantandTypeFilter::Sorcery.crates/engine/src/parser/oracle_static/tests.rs#L14391-L14400: bind the filter instead of discarding it with.., then assertTypeFilter::CreatureandFilterProp::Another.crates/engine/tests/integration/demilich_helbrute_graveyard_exile_cost.rs#L131-L159: add a scenario with three instant/sorcery cards plus one ineligible graveyard card, and assert the cast stays blocked.
📍 Affects 2 files
crates/engine/src/parser/oracle_static/tests.rs#L14339-L14342(this comment)crates/engine/src/parser/oracle_static/tests.rs#L14391-L14400crates/engine/tests/integration/demilich_helbrute_graveyard_exile_cost.rs#L131-L159
🤖 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_static/tests.rs` around lines 14339 - 14342,
The tests do not verify that the exile-cost filters restrict cards correctly. In
crates/engine/src/parser/oracle_static/tests.rs#L14339-L14342, replace the
presence check with assertions that the disjunction legs use TypeFilter::Instant
and TypeFilter::Sorcery; in
crates/engine/src/parser/oracle_static/tests.rs#L14391-L14400, bind the filter
instead of discarding it and assert TypeFilter::Creature with
FilterProp::Another; in
crates/engine/tests/integration/demilich_helbrute_graveyard_exile_cost.rs#L131-L159,
add three eligible instant/sorcery cards plus one ineligible graveyard card and
assert casting remains blocked.
Source: Path instructions
|
Generated for head Parse changes introduced by this PR · 14 card(s), 9 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — compound gerund additional costs are not safe to mark supported.
🔴 Blocker
[HIGH] Compound gerund additional costs remain falsely payable. Evidence: crates/engine/src/parser/oracle_static/restriction.rs:2105-2110 rejects only a top-level AbilityCost::Unimplemented; the cost parser can retain an unimplemented discard/sacrifice conjunct inside Composite, crates/engine/src/game/cost_payability.rs:641-650,706-708 recursively treats that node as payable, and payment then fails at crates/engine/src/game/costs.rs:1341-1344. The current parse-diff names Demonic Embrace and Wickerfolk Indomitable, while its 14 cards / 9 signatures also exceeds the PR narrative. Why it matters: CR 601.2f and CR 601.2h (verified in docs/MagicCompRules.txt:2468,2472) require additional costs to be locked and fully payable; this path advertises a cast that cannot make a partial/unpayable payment. Suggested fix: recursively reject the permission until every composite conjunct is concrete, with runtime tests that exercise both compound components.
[MED] Gerund parsing lowercases subtype identity. Evidence: crates/engine/src/parser/oracle_cost.rs passes a lowercased remainder through the cost parsing path. Why it matters: subtype filters require the original spelling/canonical identity. Suggested fix: match case-insensitively with TextPair (or the existing equivalent) while passing the original-cased remainder downstream.
[MED] The added tests do not prove the claimed filters or dispatch completeness. Evidence: the new parser tests do not assert the actual filters/Another, and do not directly assert no Effect::Unimplemented remains for the dispatched line. Why it matters: a widened filter or partial parser fallback can remain green. Suggested fix: assert precise filter structure plus ineligible cases and a direct no-Unimplemented dispatch assertion.
Recommendation: request changes.
|
Correction to my current-head changes-requested review: I retract only the MED claim that gerund lowercasing breaks subtype identity. |
JacobWoodson
changed the title
matthewevans
removed
the
needs-maintainer
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/game/cost_payability.rs`:
- Around line 1256-1277: The current unit test only checks
AbilityCost::is_payable and bypasses the real cast/payment flow. Add an
integration test under crates/engine/tests/integration/ that casts the ability
with X=0 when no eligible cards exist, then covers positive-X announcement and
selection with an eligible graveyard card, verifying payment and card movement;
register the test module in integration/main.rs.
🪄 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: 1b5cc059-5987-481d-ab6c-420c7b315493
📒 Files selected for processing (10)
crates/engine/src/game/casting.rscrates/engine/src/game/cost_payability.rscrates/engine/src/parser/oracle_casting.rscrates/engine/src/parser/oracle_cost.rscrates/engine/src/parser/oracle_static/mod.rscrates/engine/src/parser/oracle_static/restriction.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/tests/integration/demilich_helbrute_graveyard_exile_cost.rscrates/engine/tests/integration/dragon_man_reformed_robot_graveyard_discard_cost.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/engine/tests/integration/main.rs
- crates/engine/src/parser/oracle_static/mod.rs
- crates/engine/tests/integration/dragon_man_reformed_robot_graveyard_discard_cost.rs
- crates/engine/src/game/casting.rs
- crates/engine/tests/integration/demilich_helbrute_graveyard_exile_cost.rs
- crates/engine/src/parser/oracle_cost.rs
- crates/engine/src/parser/oracle_static/restriction.rs
- crates/engine/src/parser/oracle_static/tests.rs
| #[test] | ||
| fn variable_exile_cost_is_payable_at_x_zero() { | ||
| let mut scenario = GameScenario::new(); | ||
| let source = scenario.add_creature(P0, "Harvest Pyre", 0, 1).id(); | ||
| let cost = AbilityCost::Exile { | ||
| count: EXILE_COST_X, | ||
| zone: Some(Zone::Graveyard), | ||
| filter: Some(TargetFilter::Typed(TypedFilter::new(TypeFilter::Instant))), | ||
| }; | ||
|
|
||
| assert!( | ||
| cost.is_payable(&scenario.state, P0, source), | ||
| "X exile costs are payable at X=0 before any eligible card is selected" | ||
| ); | ||
|
|
||
| scenario.add_spell_to_graveyard(P0, "Lightning Bolt", true); | ||
| assert!( | ||
| cost.is_payable(&scenario.state, P0, source), | ||
| "X exile costs stay payable when eligible cards can set X above zero" | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Exercise the real cast and payment path.
Both assertions call AbilityCost::is_payable, so both pass through the new unconditional sentinel branch. The test does not verify X announcement, positive-X selection, actual exile payment, or card movement.
Add an integration test under crates/engine/tests/integration/ and register it in crates/engine/tests/integration/main.rs. Cover X=0 with no eligible cards and positive X through the real casting pipeline.
As per path instructions, “Tests belong under crates/engine/tests/integration/ and must be registered in integration/main.rs.”
🤖 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/game/cost_payability.rs` around lines 1256 - 1277, The
current unit test only checks AbilityCost::is_payable and bypasses the real
cast/payment flow. Add an integration test under
crates/engine/tests/integration/ that casts the ability with X=0 when no
eligible cards exist, then covers positive-X announcement and selection with an
eligible graveyard card, verifying payment and card movement; register the test
module in integration/main.rs.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head still accepts compound additional-cost riders that contain an unimplemented mandatory sub-cost, lacks end-to-end X-exile coverage, and fails required Rust lint.
[HIGH] The present-but-unmodeled rider guard is only shallow. Evidence: crates/engine/src/parser/oracle_cost.rs:174-185 constructs AbilityCost::Composite for comma/and costs, but crates/engine/src/parser/oracle_static/restriction.rs:2105-2110 and crates/engine/src/parser/oracle_casting.rs:253-258 reject only a top-level AbilityCost::Unimplemented. A compound rider with one unsupported child is therefore accepted; crates/engine/src/game/cost_payability.rs:644-658 recursively visits composite legs and :710-715 treats Unimplemented as payable. Why it matters: a required additional cost can be represented as supported while an unsupported component is silently admitted. Suggested fix: use a recursive supported-cost guard before accepting either rider and add a compound-gerund regression that proves the whole permission is declined.
[MED] The X-exile coverage stops at the pure payability helper rather than the spell-cast pipeline. Evidence: crates/engine/src/game/cost_payability.rs:1257-1276 calls AbilityCost::is_payable, while real casting derives the X upper bound at crates/engine/src/game/casting_costs.rs:7801-7852, concretizes it at :5370-5407, and produces the graveyard selection prompt at :14137-14213. Why it matters: a disconnect in announcement, concretization, or payment can ship undetected. Suggested fix: add production cast-flow coverage for X=0 with no eligible card and positive-X selection/payment/movement.
[MED] Required Rust lint is red on this head. Evidence: GitHub run 31292692855, job 93192500218, reports clippy::question_mark for crates/engine/src/parser/oracle_casting.rs:232-248. Why it matters: CI cannot pass. Suggested fix: apply Clippy's ?-based Option propagation in that parser branch.
The parse-diff receipt is current for 2de7357c007faf2653489e65826f74a5e1011426 (14 cards / 9 signatures); the named surface is otherwise explained.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the parser now advertises 12 graveyard-cast cards with an additional cost, but unsupported nested cost riders can still pass the legality checks and be silently ignored at payment.
[HIGH] parse_additional_cost can return AbilityCost::Unimplemented inside a composite additional-cost rider (crates/engine/src/parser/oracle_cost.rs:160-185), while the acceptance guards only reject a top-level unimplemented cost (crates/engine/src/parser/oracle_static/restriction.rs:2105-2110, crates/engine/src/parser/oracle_casting.rs:251-255, and crates/engine/src/game/cost_payability.rs:644-658,710-715). A conjunction such as a supported mana payment plus an unsupported rider is therefore considered castable and the unsupported rider can be skipped. That makes the added GraveyardCastPermission(... extra_cost=additional) surface rules-incorrect. Make the nested-cost walk authoritative for parser acceptance, cast legality, and payment so any unsupported child fails the entire additional cost; add a regression with a supported child plus an unsupported sibling.
[MED] The X-valued graveyard-exile additional-cost coverage stops at is_payable. It does not drive the actual announcement, X concretization, selection, and payment pipeline. Add a cast-pipeline regression that proves the selected number of graveyard cards is exiled and that the resulting X value/payment state is carried through resolution.
The current parse-diff artifact is correctly bound to 2ae0461bcfab6498c5f74038eb9a811806a31eaa and exposes the affected card class; please retain it on the next head and request re-review after the strict-failure and pipeline regressions are in place.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verified current head: additional-cost parsing/payability fixes and the Clippy follow-up are covered by green CI.
2 participants
Summary
Fixes a parse-fidelity defect on Dragon Man, Reformed Robot.
Issue: Graveyard-cast permission drops the "by discarding a card in addition to paying its other costs" additional cost (extra_cost is None instead of a CastExtraCost with mode Additional / discard-a-card), so it parses as castable from the graveyard for its normal cost with no discard required.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— passed./scripts/check-parser-combinators.sh (Gate A)— passedcargo clippy-strict— passedcargo test -p phase-engine— failedcargo export-cards data --stats --output data/card-data.json --sidecar-dir client/public + mirror to client/public (card-data regen)— passedcargo coverage— incompletecargo semantic-audit— not_runRe-verified at chunk-2 checkpoint with fresh card-data: supported:true gap:0, semantic-audit clean. The run-time 'partial' was a Windows census-test artifact (cargo test path-separator bug), not a code defect.
Scope Expansion
None.
Validation Failures
See review/cross-check notes.
CI Failures
Summary by CodeRabbit
Bug Fixes
Tests