Partial: Dragon Man, Reformed Robot - #7030
Conversation
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughThe change adds shared gerund-cost parsing, preserves modeled additional costs in graveyard-cast permissions, rejects unsupported riders, and validates affordability for 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
| if let Some(extra) = static_extra { | ||
| if let Some(amount) = find_pay_life_cost(&extra.cost, state, player, prepared.object_id) | ||
| { | ||
| if !super::life_costs::can_pay_life_cast_or_activation_cost(state, player, amount) { | ||
| return false; | ||
| } | ||
| if !extra.cost.is_payable(state, player, prepared.object_id) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect AbilityCost::is_payable for sentinel-count and composite handling.
set -euo pipefail
rg -nP --type=rust -C2 'fn is_payable' | head -40
fd -t f -e rs --exec rg -ln 'fn is_payable' {} \; | while IFS= read -r f; do
echo "=== $f ==="
ast-grep outline "$f" --match 'is_payable' --view expanded
rg -n -C6 'EXILE_COST_X|REMOVE_COUNTER_COST_X|REMOVE_COUNTER_COST_ANY_NUMBER|u32::MAX' "$f"
rg -n -A12 'AbilityCost::(OneOf|Composite)' "$f"
doneRepository: phase-rs/phase
Length of output: 319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Changed file context:"
sed -n '13828,13865p' crates/engine/src/game/casting.rs
echo
echo "Find is_payable occurrences:"
rg -n -C4 'fn is_payable|use .*is_payable|\.is_payable\(' crates/engine/src crates -g '*.rs' | head -200
echo
echo "Find AbilityCost definitions:"
rg -n -C4 'enum AbilityCost|struct AbilityCost|type AbilityCost' crates/engine/src crates -g '*.rs' | head -200
echo
echo "Specific sentinel identifiers:"
rg -n 'EXILE_COST_X|REMOVE_COUNTER_COST_X|REMOVE_COUNTER_COST_ANY_NUMBER|u32::MAX|CostSentinel|Sentinel' crates/engine/src crates -g '*.rs' | head -200Repository: phase-rs/phase
Length of output: 39835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "AbilityCost definition and payable helper body:"
sed -n '8730,9050p' crates/engine/src/types/ability.rs
sed -n '250,720p' crates/engine/src/game/cost_payability.rs
echo
echo "Targeted is_payable tests:"
sed -n '1300,1450p' crates/engine/src/game/cost_payability.rs
sed -n '33920,34480p' crates/engine/src/game/casting_tests.rsRepository: phase-rs/phase
Length of output: 50370
Add a direct test for variable-count exile affordability.
is_payable handles Composite and OneOf correctly, and variable remove-counter counts are already payable. The remaining untested shape on this cast-permission path is variable exile costs; make sure Exile { EXILE_COST_X, .. } accepts any eligible exile card choice.
🤖 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/casting.rs` around lines 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.
| let cost = parse_gerund_cost(cost_text); | ||
| if matches!(cost, AbilityCost::Unimplemented { .. }) { | ||
| return None; | ||
| } | ||
| option = option.cost(cost); | ||
| return Some(option); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which "in addition to ... other costs" closer variants appear alongside a flash rider.
set -euo pipefail
fd -t f 'card-data' | head -20
fd -t f -e json 'card-data' --exec rg -o -i 'as though it had flash by [^."]*' {} \; | sort -u | head -50
fd -t f -e json 'card-data' --exec rg -o -i 'in addition to (paying )?(its|their) other costs' {} \; | sort | uniq -cRepository: phase-rs/phase
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching oracle_casting/restriction nearby\n'
fd -t f 'oracle_casting|restriction' crates/engine/src/parser | sed -n '1,120p'
printf '\nRelevant section of oracle_casting.rs\n'
sed -n '200,290p' crates/engine/src/parser/oracle_casting.rs
printf '\nRelevant section of restriction.rs\n'
sed -n '2070,2110p' crates/engine/src/parser/oracle_static/restriction.rs
printf '\nSearch nearby definitions and call sites\n'
rg -n "parse.*self.*flash|self.flash|self-fla|In addition to|paying their other costs|its other costs|other costs|AbilityCost::Unimplemented" crates/engine/src/parser crates/engine -g '*.rs' | sed -n '1,220p'Repository: phase-rs/phase
Length of output: 37167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe: extract the exact parser condition from oracle_casting.rs and test the
# candidate self-flash rider variants against the same strip_suffix branch.
python3 - <<'PY'
from pathlib import Path
import re
path = Path('crates/engine/src/parser/oracle_casting.rs')
text = path.read_text()
m = re.search(r'if let Some\(cost_text\) = text\.strip_suffix\(" in addition to paying its other costs"\)', text)
print('self-flash strip_suffix target:', m.group(0) if m else 'not found')
target = ' in addition to paying its other costs'
cases = [
"If you cast this spell, you may cast it as though it had flash in addition to its other costs.",
"If you cast this spell, you may cast it as though it had flash in addition to their other costs.",
"If you cast this spell, you may cast it as though it had flash in addition to paying its other costs.",
"If you cast this spell, you may cast it as though it had flash in addition to paying their other costs.",
]
for case in cases:
print(repr(case), 'strips=', case.endswith(target), 'slices=', case[:-len(target)] if case.endswith(target) else None)
PYRepository: phase-rs/phase
Length of output: 708
🌐 Web query:
Magic: the Gathering collector search oracle cards as though it had flash by in addition to its other costs
💡 Result:
In Magic: The Gathering, "as though it had flash" is not a keyword ability itself, but a specific phrasing used in card text to create a static, permissive effect [1][2]. This effect allows a player to cast a spell at a time they would normally be unable to, specifically treating the spell as if it possessed the Flash keyword [3][1]. Under the Comprehensive Rules, rule 609.4 defines these "as though" effects as those that allow a player to do something as if a specific condition were true [4][1]. When an effect states, for example, "You may cast creature spells as though they had flash," it modifies the timing restrictions for casting those spells, enabling the player to cast them anytime they could cast an instant [5][3]. Key technical aspects include: 1. Scope of Effect: These effects apply strictly to the action of casting the spell [6][7][8]. They do not change the timing for activating abilities that can normally only be activated "as a sorcery" [6][7][8]. 2. Bestow and Other Qualities: If a player is granted the ability to cast certain spells (e.g., Auras) as though they had flash, they may initiate casting a spell with variable qualities (such as a creature with Bestow) as though it had flash if they choose to cast it in a mode that satisfies those qualities [3][9]. 3. Cumulative Effects: If multiple "as though" effects apply, they function cumulatively [4][1]. For instance, if you are allowed to cast spells from a graveyard as though they were in your hand and also as though they had flash, both permissions apply [4][1]. Common examples of cards featuring this effect include Vedalken Orrery ("You may cast spells as though they had flash") [4][1], Winding Canyons [1], Prophet of Kruphix [5], and Borne Upon a Wind [10][8].
Citations:
- 1: https://mtg.wiki/page/As_though
- 2: https://mtg.wiki/page/Flash
- 3: https://mtg.fandom.com/wiki/Casting_spells
- 4: https://magic-rulebook.vercel.app/rules/609
- 5: https://scryfall.com/card/ths/199/prophet-of-kruphix
- 6: https://gatherer.wizards.com/MOM/en-us/52/complete-the-circuit
- 7: https://scryfall.com/card/ltr/470/gandalf-the-white
- 8: https://gatherer.wizards.com/Pages/Card/Details.aspx?name=Borne%20Upon%20a%20wind
- 9: https://apps.magicjudges.org/forum/topic/28674/
- 10: https://scryfall.com/card/ltr/761/borne-upon-a-wind
🌐 Web query:
Magic the Gathering Oracle "You may cast this spell as though it had flash by in addition to paying its other costs"
💡 Result:
In Magic: The Gathering, the phrase "You may cast this spell as though it had flash by [doing X] in addition to paying its other costs" is a template used to grant a spell a timing-based ability (flash) conditional on paying an additional cost. 1. Timing: The phrase "as though it had flash" allows you to cast the spell at any time you could cast an instant, regardless of its original card type (e.g., a sorcery) [1][2][3]. 2. Cost: The phrase "by [doing X] in addition to paying its other costs" defines an additional cost you must pay to utilize this timing benefit [4]. "Other costs" refer to the spell's mana cost and any other applicable costs (such as kicker, commander tax, or previous alternative costs) [4][5]. 3. Interaction: This construction allows a player to pay the specified additional cost to change the spell's timing restriction [4][5]. Because this is an additional cost, it is added to the spell's total cost calculation [4][5]. If the spell is cast using this effect, the card is treated as having flash for that specific casting event [6][4]. An example of this phrasing can be found on cards like Tegwyll's Scouring, which states: "You may cast Tegwyll's Scouring as though it had flash by tapping three untapped creatures you control with flying in addition to paying its other costs" [7].
Citations:
- 1: https://gatherer.wizards.com/AVR/en-us/225/alchemists-refuge
- 2: https://gatherer.wizards.com/CNS/en-us/206/vedalken-orrery
- 3: https://yawgatog.com/resources/oracle-changes/soi-ema/
- 4: https://mtg-rules.vercel.app/chapters/118
- 5: https://draftsim.com/mtg-alternative-costs/
- 6: https://scryfall.com/card/vis/14/parapet
- 7: https://www.mtgsalvation.com/cards/wilds-of-eldraine-commander/56272-tegwylls-scouring
🌐 Web query:
Tegwyll's Scouring Oracle text "You may cast ... as though it had flash by tapping three untapped creatures ... in addition to paying its other costs"
💡 Result:
The official Oracle text for the Magic: The Gathering card Tegwyll's Scouring is: "You may cast this spell as though it had flash by tapping three untapped creatures you control with flying in addition to paying its other costs. Destroy all creatures. Create three 1/1 black Faerie Rogue creature tokens with flying." [1][2] This card is a Sorcery from the Wilds of Eldraine Commander set [1][2]. The ability allows you to cast it at instant speed (as though it had flash) by tapping three untapped creatures with flying you control, provided you also pay the standard mana cost of {4}{B}{B} [1][2].
Citations:
- 1: https://gatherer.wizards.com/WOC/en-us/16/tegwylls-scouring
- 2: https://scryfall.com/card/woc/16/tegwylls-scouring
🌐 Web query:
site:api.scryfall.com/cards/named name:"Tegwyll's Scouring" oracle_text:"You may cast"
💡 Result:
No exact search results were found for that Scryfall API query.
The card’s Oracle text is:
You may cast Tegwyll’s Scouring as though it had flash by tapping three untapped creatures you control with flying in addition to paying its other costs. Destroy all creatures. Create three 1/1 black Faerie Rogue creature tokens with flying. (mtgstocks.com)
Citations:
Handle self-flash additional-cost closers the same shape as the graveyard parser.
The self-flash arm currently matches only by <cost> in addition to paying its other costs. A canonical variant such as You may cast ... as though it had flash by tapping X in addition to paying their other costs would fail this strip_suffix, fall through the other guarded arms, and emit Some(option) with no cost. Use the shared additional-cost-combinator pattern from parse_cast_permission_additional_cost_rider: optional paying , then its other costs or their other costs, before de-gerunding the cost.
🤖 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_casting.rs` around lines 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.
Source: Coding guidelines
| pub(crate) fn parse_gerund_cost(phrase: &str) -> AbilityCost { | ||
| type E<'a> = super::oracle_nom::error::OracleError<'a>; | ||
| let lower = phrase.trim().to_lowercase(); | ||
| // Compose one `value(stem, tag(gerund))` arm per cost verb — each maps a | ||
| // gerund onto the imperative stem `parse_oracle_cost` already recognizes. | ||
| let deconjugated = alt(( | ||
| value("pay", tag::<_, _, E<'_>>("paying ")), | ||
| value("discard", tag("discarding ")), | ||
| value("sacrifice", tag("sacrificing ")), | ||
| value("tap", tag("tapping ")), | ||
| value("remove", tag("removing ")), | ||
| value("exile", tag("exiling ")), | ||
| )) | ||
| .parse(lower.as_str()); | ||
| let Ok((rest, stem)) = deconjugated else { | ||
| return AbilityCost::Unimplemented { | ||
| description: phrase.trim().to_string(), | ||
| }; | ||
| }; | ||
| parse_oracle_cost(&format!("{stem} {rest}")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the original casing when de-conjugating the gerund.
parse_gerund_cost lowercases the whole phrase and then feeds the lowercased remainder into parse_oracle_cost. The downstream filter parsers keep the casing of their input for subtypes: cost_exile_self_and_count_other_you_control_recovers_count_and_filter (Line 2128) asserts TypeFilter::Subtype("Vehicle".to_string()) from a mixed-case input. A subtype-bearing gerund rider such as "sacrificing a Vehicle" therefore lowers to Subtype("vehicle") instead of Subtype("Vehicle"), which will not match the printed subtype at runtime.
No currently parsed card reaches that arm (the shipped riders are discard, pay-life, and exile), so this is latent. It becomes a wrong-filter bug as soon as a subtype rider is added. Match on the lowered text but slice the original, as the rest of this module does with nom_on_lower / TextPair.
♻️ Suggested shape
- let lower = phrase.trim().to_lowercase();
- let deconjugated = alt((
- value("pay", tag::<_, _, E<'_>>("paying ")),
- value("discard", tag("discarding ")),
- value("sacrifice", tag("sacrificing ")),
- value("tap", tag("tapping ")),
- value("remove", tag("removing ")),
- value("exile", tag("exiling ")),
- ))
- .parse(lower.as_str());
- let Ok((rest, stem)) = deconjugated else {
+ let original = phrase.trim();
+ let lower = original.to_lowercase();
+ let deconjugated = nom_on_lower(original, &lower, |i| {
+ alt((
+ value("pay", tag::<_, _, E<'_>>("paying ")),
+ value("discard", tag("discarding ")),
+ value("sacrifice", tag("sacrificing ")),
+ value("tap", tag("tapping ")),
+ value("remove", tag("removing ")),
+ value("exile", tag("exiling ")),
+ ))
+ .parse(i)
+ });
+ let Some((stem, rest)) = deconjugated else {
return AbilityCost::Unimplemented {
- description: phrase.trim().to_string(),
+ description: original.to_string(),
};
};As per coding guidelines: "Use TextPair for case-insensitive matching that preserves original casing".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) fn parse_gerund_cost(phrase: &str) -> AbilityCost { | |
| type E<'a> = super::oracle_nom::error::OracleError<'a>; | |
| let lower = phrase.trim().to_lowercase(); | |
| // Compose one `value(stem, tag(gerund))` arm per cost verb — each maps a | |
| // gerund onto the imperative stem `parse_oracle_cost` already recognizes. | |
| let deconjugated = alt(( | |
| value("pay", tag::<_, _, E<'_>>("paying ")), | |
| value("discard", tag("discarding ")), | |
| value("sacrifice", tag("sacrificing ")), | |
| value("tap", tag("tapping ")), | |
| value("remove", tag("removing ")), | |
| value("exile", tag("exiling ")), | |
| )) | |
| .parse(lower.as_str()); | |
| let Ok((rest, stem)) = deconjugated else { | |
| return AbilityCost::Unimplemented { | |
| description: phrase.trim().to_string(), | |
| }; | |
| }; | |
| parse_oracle_cost(&format!("{stem} {rest}")) | |
| } | |
| pub(crate) fn parse_gerund_cost(phrase: &str) -> AbilityCost { | |
| type E<'a> = super::oracle_nom::error::OracleError<'a>; | |
| let original = phrase.trim(); | |
| let lower = original.to_lowercase(); | |
| // Compose one `value(stem, tag(gerund))` arm per cost verb — each maps a | |
| // gerund onto the imperative stem `parse_oracle_cost` already recognizes. | |
| let deconjugated = nom_on_lower(original, &lower, |i| { | |
| alt(( | |
| value("pay", tag::<_, _, E<'_>>("paying ")), | |
| value("discard", tag("discarding ")), | |
| value("sacrifice", tag("sacrificing ")), | |
| value("tap", tag("tapping ")), | |
| value("remove", tag("removing ")), | |
| value("exile", tag("exiling ")), | |
| )) | |
| .parse(i) | |
| }); | |
| let Some((stem, rest)) = deconjugated else { | |
| return AbilityCost::Unimplemented { | |
| description: original.to_string(), | |
| }; | |
| }; | |
| parse_oracle_cost(&format!("{stem} {rest}")) | |
| } |
🤖 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_cost.rs` around lines 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.
Source: Coding guidelines
| 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. |
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_runScope Expansion
None.
Validation Failures
See review/cross-check notes.
CI Failures
Summary by CodeRabbit
Bug Fixes
Tests