Feat/demon of fates design alt cost#5856
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces parsing support and integration tests for alternative-cost grants that allow casting spells by paying life equal to their mana value, specifically supporting Demon of Fate's Design. The feedback highlights two key improvements: verifying that the remainder of parse_type_phrase is empty to prevent over-broad alternative cost matching, and replacing a multiline string literal in the tests with concat! to avoid fragile trailing whitespaces that could be stripped by IDEs or formatters.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let base_filter = if filter_for_parse.is_empty() { | ||
| TargetFilter::Typed(TypedFilter::card()) | ||
| } else { | ||
| parse_type_phrase(filter_for_parse).0 | ||
| }; |
There was a problem hiding this comment.
[HIGH] Unchecked remainder from type phrase parsing. Evidence:
crates/engine/src/parser/oracle_static/cost_mod.rs:718.
Why it matters: Ignoring the unconsumed remainder ofparse_type_phrasecan lead to silent parsing bugs where unsupported or unmodeled qualifiers in the spell filter are dropped, resulting in an over-broad alternative cost grant.
Suggested fix: Verify that the remainder ofparse_type_phraseis empty, returningNoneotherwise.
let base_filter = if filter_for_parse.is_empty() {
TargetFilter::Typed(TypedFilter::card())
} else {
let (filter, remainder) = parse_type_phrase(filter_for_parse);
if !remainder.trim().is_empty() {
return None;
}
filter
};References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. Convenience shortcuts that get rules wrong are not simpler; they are wrong. (link)
| const DEMON_ORACLE: &str = "Once during each of your turns, you may cast an \ | ||
| enchantment spell by paying life equal to its mana value rather than paying \ | ||
| its mana cost."; |
There was a problem hiding this comment.
[MEDIUM] Fragile trailing whitespace in multiline string literal. Evidence:
crates/engine/tests/integration/demon_of_fates_design.rs:18.
Why it matters: Trailing whitespaces before line-continuation backslashes are easily stripped by IDEs or formatters on save, which would silently merge words (e.g., 'anenchantment') and break the parser match.
Suggested fix: Useconcat!to safely split the string across multiple lines.
| const DEMON_ORACLE: &str = "Once during each of your turns, you may cast an \ | |
| enchantment spell by paying life equal to its mana value rather than paying \ | |
| its mana cost."; | |
| const DEMON_ORACLE: &str = concat!( | |
| "Once during each of your turns, you may cast an ", | |
| "enchantment spell by paying life equal to its mana value rather than paying ", | |
| "its mana cost." | |
| ); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba8122eac5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (tp, frequency) = match parse_alt_cost_frequency_prefix(tp.lower) { | ||
| Ok((rest_lower, freq)) => { | ||
| let consumed = tp.lower.len() - rest_lower.len(); | ||
| (TextPair::new(&tp.original[consumed..], rest_lower), freq) | ||
| } |
There was a problem hiding this comment.
When the printed prefix is Once during each of your turns, this path strips the whole prefix and keeps only CastFrequency::OncePerTurn; granted_spell_alternative_cost_for later checks only that frequency and the affected filter. If another effect lets the controller cast enchantments at instant speed on an opponent's turn, Demon will incorrectly offer the pay-life alternative cost even though the Oracle text limits it to that player's own turns. Thread a StaticCondition::DuringYourTurn (or equivalent timing gate) in addition to the per-turn frequency for this prefix.
Useful? React with 👍 / 👎.
Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — parse timing semantics, not input length.
🟡 Finding
[MED] The cost modifier uses consumed > 20 as a timing proxy. Evidence: crates/engine/src/game/cost_mod.rs:675. Why it matters: timing behavior depends on incidental prefix length. Suggested fix: parse and carry the explicit timing condition; add an Access Maze regression.
Recommendation: request-changes.
CR 118.9 + CR 601.2b: Parse 'Once during each of your turns, you may cast
an enchantment spell by paying life equal to its mana value rather than
paying its mana cost.' into a CastWithAlternativeCost static with
PayLife { SelfManaValue } cost and OncePerTurn frequency.
Implementation:
- Add parse_cast_by_paying_life_alt_cost() to cost_mod.rs
- Export from oracle_static/mod.rs
- Call from dispatch.rs before try_parse_cast_free_permission
- Parser unit tests (positive + negative guard)
- Integration tests (offer, accept+pay, non-enchantment rejection)
…alt_cost dispatch.rs already imports it via `use super::cost_mod::*`, so the pub(crate) re-export in mod.rs is unnecessary and triggers -Dunused-imports.
- Replace .strip_suffix(" spells") / .strip_suffix(" spell") with
strip_cost_mod_spell_noun_suffix() to satisfy the parser combinator gate.
- Fund P0's mana pool in the non-enchantment test so the cast pipeline
can start (previously panicked with 'Cannot pay mana cost').
…e access The function was private in static_helpers.rs but cost_mod.rs (a sibling module) needs to call it via the support re-export. Promote to pub(super).
1. Check parse_type_phrase remainder is empty before accepting the filter (prevents over-broad matching on unsupported qualifiers). 2. Replace backslash-continuation string literal with concat!() to avoid fragile trailing whitespace that IDEs may strip. 3. Thread StaticCondition::DuringYourTurn when the frequency prefix is "once during each of your turns, " so the grant only functions on the controller's turn (prevents interaction with instant-speed flash enablers on opponents' turns). 4. Add assertion in parser unit test for the new condition field.
Address maintainer review (matthewevans):
1. Replace consumed>20 heuristic with explicit nom tag parse of
"once during each of your turns, " to determine DuringYourTurn
condition. The timing gate is now derived from parsing the actual
prefix variant, not from incidental string length.
2. Add Access Maze regression test: "a spell from your hand" parses
with InZone::Hand zone qualifier and card() base filter, confirming
the zone qualifier stripping works correctly.
3. Handle "from your hand" zone qualifier in the filter phrase by
stripping it before the spell-noun suffix and threading it as
FilterProp::InZone { zone: Zone::Hand } through
apply_spell_keyword_subject_constraints.
Whovencroft
force-pushed
the
feat/demon-of-fates-design-alt-cost
branch
from
e349541 to
147a1d5
Compare
Co-authored-by: Whovencroft <6600560+Whovencroft@users.noreply.github.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head review: prior timing blocker is resolved by explicit prefix parsing and Access Maze coverage. Maintainer port retained independent main test additions; parser gates pass. Approving and queueing.
2 participants
Summary
Implements the missing
static_structureeffect for Demon of Fate's Design:This adds a new parser function that lowers the Oracle text into a
CastWithAlternativeCoststatic withPayLife { SelfManaValue }cost andOncePerTurnfrequency, scoped to enchantment spells.Rules References
Implementation
crates/engine/src/parser/oracle_static/cost_mod.rsparse_cast_by_paying_life_alt_cost()functioncrates/engine/src/parser/oracle_static/static_helpers.rsstrip_cost_mod_spell_noun_suffixtopub(super)crates/engine/src/parser/oracle_static/mod.rspub(crate)re-exportcrates/engine/src/parser/oracle_static/dispatch.rstry_parse_cast_free_permissioncrates/engine/src/parser/oracle_static/tests.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/demon_of_fates_design.rsParser Function
Pattern:
[Once during each of your turns, ]you may cast [filter] by paying life equal to its mana value rather than paying its mana cost.Output: