fix(parser): UNSUPPORTED one-off: The Face of Boe#4619
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 implements AbilityCost::KeywordCostOfCastSpell to support casting spells by paying alternative mana costs borrowed from keywords (such as Suspend) under CR 118.9 and CR 702.62a. While the implementation is mostly robust and includes thorough runtime tests, there is a critical issue on the lingering branch in casting_costs.rs where a failure to resolve the keyword cost silently defaults to a free cast (ManaCost::zero) instead of aborting, which violates defensive programming principles and could lead to incorrect game states.
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 cost = | ||
| super::keywords::effective_keyword_mana_cost(state, pending.object_id, keyword) | ||
| .unwrap_or_else(ManaCost::zero); |
There was a problem hiding this comment.
[HIGH] Silently defaulting to ManaCost::zero when effective_keyword_mana_cost returns None on the lingering branch.
Why it matters: If effective_keyword_mana_cost returns None (which represents a defensive refusal for a missing keyword or misparse), defaulting to zero allows the spell to be cast for free. This is a critical correctness issue and directly contradicts the defensive programming principles established in cast_from_zone.rs (where unreadable costs abort the cast instead of defaulting to zero).
Suggested fix: Handle the None case by returning an error or failing the cast/payment instead of using unwrap_or_else(ManaCost::zero).
References
- Avoid swallowing all errors (e.g., using
.ok()) when only specific errors are expected to be handled or ignored. Propagate unexpected errors (such as invariant violations or invalid actions) to prevent masking critical bugs.
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward. I found one blocker that needs fixing before this is safe to land.
crates/engine/src/game/casting_costs.rs:4145: theKeywordCostOfCastSpellarm resolves an unreadable keyword cost withunwrap_or_else(ManaCost::zero). This path is reachable through the lingering permission flow: the parser can attach the rider to a priorCastFromZone,grant_lingering_permissionsstampsExileWithAltAbilityCost, and this casting-cost branch consumes it. If the selected spell does not expose that keyword cost, or a future keyword kind is not representable as a mana cost, the engine silently turns the alternative cost into a free cast.
The during-resolution path already treats this exact condition as an abort/no-op (complete_hand_pick_cast_from_zone returns before calling initiate_cast_during_resolution), so the lingering path should use the same refusal semantics rather than zeroing the cost. Please make the KeywordCostOfCastSpell lingering branch fail/abort instead of defaulting to {0}, and add regression coverage for an ExileWithAltAbilityCost { cost: KeywordCostOfCastSpell { .. } } permission whose target card cannot provide that keyword cost.
# Conflicts: # crates/engine/src/parser/oracle_effect/mod.rs
# Conflicts: # crates/engine/src/parser/oracle_effect/tests.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The lingering keyword-cost branch still turns an unreadable borrowed keyword cost into a free cast. Evidence: crates/engine/src/game/casting_costs.rs:4157. Why it matters: effective_keyword_mana_cost documents None as a defensive refusal for misparsed or unsupported borrowed keyword costs, and the during-resolution Face of Boe path now aborts on None, but the lingering ExileWithAltAbilityCost consumer still uses unwrap_or_else(ManaCost::zero), silently miscosting the spell as {0}. Suggested fix: make this branch return an error/refusal on None, matching the during-resolution path's contract.
Reviewed current head ad7248f61cef146b51044ff1d2fcb096347bc237.
2 participants
Summary
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED one-off: The Face of Boe
Cards corrected
Fix
Fully implemented The Face of Boe (cluster-70, UNSUPPORTED one-off) per the approved plan. The card's "{T}: You may cast a spell with suspend from your hand. If you do, pay its suspend cost rather than its mana cost." now parses to a single CastFromZone (0 Unimplemented, was 1) carrying the new parameterized alternative cost and a DuringResolution driver, and casts the picked suspend card during resolution charging its colored suspend cost.
WHAT CHANGED (surgical, by subsystem):
TYPES — crates/engine/src/types/ability.rs: added the parameterized struct variant AbilityCost::KeywordCostOfCastSpell { keyword: KeywordKind } (avoids the forbidden SuspendCostOfCastSpell sibling cluster; backed by the existing per-keyword reader family). Added benign/no-op arms in the three AbilityCost helper matches (all_components_cheap_gate_covered, consumes_source → false; categories → ManaOnly).
KEYWORDS — crates/engine/src/game/keywords.rs: added effective_suspend_cost (mirror of effective_sneak_cost) and the single dispatch authority effective_keyword_mana_cost(state, id, kind) -> Option over Suspend/Sneak/Mayhem/Harmonize/Disturb (None for compound kinds).
PARSER — crates/engine/src/parser/oracle_effect/mod.rs: broadened the alt-cost-rider head guard with the verbless "rather than its mana cost" form, added the "its suspend cost" body recognizer (ordered after discard/pay-life), extracted the duration-blind shared filter-form DRIVER authority during_resolution_for_filter_cast_clause + cast_target_is_hand_origin, refactored Branch 2 to call it, and made the alt-cost fold (attach_alt_cost_to_prior_cast_from_zone) re-derive the driver structurally; documented the intentional condition:None discard at dispatch. lower.rs: benign where-X no-op arm. Branch 1 (anaphor) intentionally untouched.
RUNTIME — cast_from_zone.rs: threaded Option alt_mana_cost through cast_single_target_during_resolution and computed it from the picked card via effective_keyword_mana_cost in complete_hand_pick_cast_from_zone (resolve-path passes None). casting.rs: added ResolutionCastRequest.alt_mana_cost and used it in initiate_cast_during_resolution (replacing the hard-coded zero). engine_resolution_choices.rs: alt_mana_cost:None on the 4 existing literals. casting_costs.rs: added the lingering-branch pay_additional_cost arm (Step F) for the variant's exile-origin class. Benign arms added in analysis/ability_graph.rs, cost_payability.rs (true, like Mana), costs.rs (payment_failed + supported_at_resolution/can_pay_resolution false + lockstep test sample), engine_payment_choices.rs, printed_cards.rs, replacement.rs, and the dormant mtgish-import/convert/action.rs (to keep the workspace compiling).
TESTS — 6 new building-block tests, all pass: suspend rider recognition; discard/pay-life regression guard; full Face-of-Boe clause fold (alt cost + DuringResolution + no Unimplemented); the duration-blind driver authority (6 cases incl. twinning-glass/Sen-Triplets/Xander's-Pact/land-play); keyword reader (colored off-zone suspend cost + Flashback refusal); and the MANDATORY runtime cast-pipeline proof that the {1}{U} suspend cost (not the printed {5}) is auto-paid from the pool and the spell lands on the stack.
VERIFICATION (worktree run directly, not Tilt): cargo fmt clean; cargo clippy -p engine --all-targets -- -D warnings exit 0 clean (fixed one unused-import warning it caught); cargo test -p engine --lib = 14186 passed / 0 failed; targeted parser diff gate over my uncommitted changes = clean (no .contains/.starts_with/.find dispatch; I use the sanctioned nom_primitives::scan_contains); gen-card-data.sh regenerated; corpus driver-diff over the full 635-card CastFromZone corpus shows the ONLY change is the face of boe Lingering→DuringResolution (all 251 existing DuringResolution casts preserved); Face of Boe re-parse = 0 Unimplemented with effect CastFromZone{alt: KeywordCostOfCastSpell{Suspend}, driver: DuringResolution, sub_ability: null}; corpus audit confirms only Face of Boe carries KeywordCostOfCastSpell and the 25 verbless-"rather than its mana cost" cards (Spectacle/Mayhem/Airbending) are unaffected because their bodies don't match discard/pay-life/suspend.
JUDGEMENT CALLS: (1) In categories() I classified KeywordCostOfCastSpell as CostCategory::ManaOnly (it resolves to a mana payment, matching Mana/ManaDynamic) rather than the empty Unimplemented group — semantically accurate and harmless to the AI/coverage classifier. (2) In the main costs.rs pay_cost I return payment_failed with a loud message (not a silent no-op) because reaching that arm means a misrouted cost; in cost_payability.is_payable I return true like Mana (mana affordability is decided by the separate CR 601.2g step). All other sites grouped with the benign Unimplemented arm per the plan. NOTE: the standalone ./scripts/check-parser-combinators.sh exits 1, but that is a pre-existing worktree artifact — it diffs against a base commit 2310 commits behind HEAD and flags the cumulative delta of unrelated merged PRs; the task's targeted per-file diff gate over my changes is clean.
No stop-and-return items; no CR ambiguity; no whole-subsystem deferral; the card is fully shipped with a passing runtime proof. The only plan deviation is C.2, which the approved plan itself already dropped.
Files changed
CR references
Verification
cargo fmt --all— pass./scripts/check-parser-combinators.sh <upstream/main merge-base>— passcargo clippy -p engine --all-targets -- -D warnings— passcargo test -p engine— passoracle-gen data --filter 'the face of boe'— passCards confirmed re-parsed correctly: the face of boe
🤖 Generated with Claude Code