fix(engine): thread the "Pay X {E}" X-announcement through Chthonian Nightmare's activated ability#5870
fix(engine): thread the "Pay X {E}" X-announcement through Chthonian Nightmare's activated ability#5870tryeverything24 wants to merge 2 commits into
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 resolves issue #1092 by adding support for variable energy costs (e.g., "Pay X {E}" on Chthonian Nightmare) during parsing and ability activation. The parser was updated to return a QuantityExpr for energy costs, and the engine now correctly handles X-announcement, cost concretization, and energy caps for variable energy costs. Additionally, integration tests were added to verify this behavior. The feedback identifies several incorrect Magic Comprehensive Rules (CR) citations across the modified files, violating the repository's strict rule R6 on verified CR annotations, and suggests replacing CR 107.9 with CR 107.14 and CR 107.3c.
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.
| zone: Some(Zone::Graveyard), | ||
| filter: filter.clone(), | ||
| }, | ||
| // CR 107.9 + CR 601.2b: once X is announced, a variable "Pay X {E}" |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for energy and X-announcement. Evidence: crates/engine/src/game/casting_costs.rs:3624. Why it matters: CR 107.9 describes negative values, whereas CR 107.14 defines energy counters and CR 107.3c defines choosing X. Suggested fix: Replace CR 107.9 with CR 107.14 + CR 107.3c.
| // CR 107.9 + CR 601.2b: once X is announced, a variable "Pay X {E}" | |
| // CR 107.14 + CR 107.3c + CR 601.2b: once X is announced, a variable "Pay X {E}" |
References
- R6. CR annotations are mandatory and verified: Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)
| AbilityCost::PayLife { amount } if amount.contains_x() => { | ||
| Some(max_pay_life_x(state, player)) | ||
| } | ||
| // CR 107.9 + CR 601.2b: X in a variable "Pay X {E}" activation cost |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for energy and X-announcement. Evidence: crates/engine/src/game/casting_costs.rs:5718. Why it matters: CR 107.9 describes negative values, whereas CR 107.14 defines energy counters and CR 107.3c defines choosing X. Suggested fix: Replace CR 107.9 with CR 107.14 + CR 107.3c.
| // CR 107.9 + CR 601.2b: X in a variable "Pay X {E}" activation cost | |
| // CR 107.14 + CR 107.3c + CR 601.2b: X in a variable "Pay X {E}" activation cost |
References
- R6. CR annotations are mandatory and verified: Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)
| fn cost_has_symbolic_counter_removal(cost: &AbilityCost) -> bool { | ||
| /// True when an activated ability's cost carries a symbolic X that must be | ||
| /// announced before payment: a variable counter-removal count (CR 601.2b) or a | ||
| /// variable `{E}` amount (CR 107.9 + CR 601.2b, e.g. "Pay X {E}" — Chthonian |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for energy and X-announcement. Evidence: crates/engine/src/game/casting_costs.rs:5828. Why it matters: CR 107.9 describes negative values, whereas CR 107.14 defines energy counters and CR 107.3c defines choosing X. Suggested fix: Replace CR 107.9 with CR 107.14 + CR 107.3c.
| /// variable `{E}` amount (CR 107.9 + CR 601.2b, e.g. "Pay X {E}" — Chthonian | |
| /// variable {E} amount (CR 107.14 + CR 107.3c + CR 601.2b, e.g. "Pay X {E}" — Chthonian |
References
- R6. CR annotations are mandatory and verified: Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)
|
|
||
| /// CR 107.9: Parse energy costs like "{E}", "{E}{E}", "pay N {e}", "pay eight {e}". | ||
| fn try_parse_energy_cost(lower: &str) -> Option<u32> { | ||
| /// CR 107.9: Parse energy costs like "{E}", "{E}{E}", "pay N {e}", "pay eight {e}", |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for energy and X-announcement. Evidence: crates/engine/src/parser/oracle_cost.rs:1628. Why it matters: CR 107.9 describes negative values, whereas CR 107.14 defines energy counters and CR 107.3c defines choosing X. Suggested fix: Replace CR 107.9 with CR 107.14 + CR 107.3c.
| /// CR 107.9: Parse energy costs like "{E}", "{E}{E}", "pay N {e}", "pay eight {e}", | |
| /// CR 107.14 + CR 107.3c: Parse energy costs like "{E}", "{E}{E}", "pay N {e}", "pay eight {e}", |
References
- R6. CR annotations are mandatory and verified: Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)
Parse changes introduced by this PR✓ No card-parse changes detected. |
…worked (phase-rs#1092) Root cause (two independent gaps, both required for the fix): 1. Parser: `try_parse_energy_cost` (parser/oracle_cost.rs) had no variable-X branch, so "Pay X {E}" silently parsed as `PayEnergy { amount: Fixed(0) }` instead of `PayEnergy { amount: Ref(Variable("X")) }` — mirroring the existing "pay x life" / "pay x speed" branches it sat next to. With X hard-pinned to 0, the ability's own effect ("mana value X") could only ever match a mana-value-0 graveyard creature, so on any realistic board the ability had no legal target and was never activatable — exactly the reported symptom. 2. Runtime: even after the parser fix, the generic X-announcement machinery in casting_costs.rs (`cost_needs_activation_x_announcement`, formerly `cost_has_symbolic_counter_removal`; `additional_cost_x_max`; `concretize_chosen_x_cost`) only recognized `RemoveCounter`-shaped X costs, not `PayEnergy`. Extending just those three functions surfaced `ChooseXValue` correctly, but `handle_activate_ability`'s X-announcement branch (casting.rs) never tagged the resumed `PendingCast` with `ActivationResidual::ManaLeg`, so `push_activated_ability_to_stack`'s existing hand-rolled Sacrifice/Exile/ReturnToHand re-surfacing detour (already correct, written for the mana-leg case) was skipped — the residual Composite[Sacrifice, ReturnToHand] silently no-op'd via the same class of bug already documented for the XMana residual path ("silent Paid no-op" for a non-self battlefield-removal cost). No existing card combines a symbolic-X activation cost with a sibling Sacrifice/Exile/ReturnToHand sub-cost, so this gap was never exercised. Fix: tag the X-announcement resume with `ActivationResidual::ManaLeg` whenever a non-self battlefield-removal cost remains outstanding, so the already-correct re-surfacing logic actually runs. Added `issue_1092_chthonian_nightmare.rs`, driving the real parse_oracle_text + activation pipeline end to end: announce X=2 (bounded by 3 energy counters), sacrifice a creature, choose between two mana-value-2 graveyard creatures (a mana-value-0 decoy proves X is threaded into the target filter, not pinned to 0), reanimate, and confirm energy/zones all land correctly. Verified: cargo fmt --check, cargo clippy -p engine --all-targets -D warnings (clean), cargo test -p engine --lib (16659 passed), cargo test -p engine --test integration (3118 passed) — all green. Closes phase-rs#1092
matthewevans
force-pushed
the
fix/issue-1092
branch
from
51527a1 to
68b2369
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request implements support for variable-X energy costs (e.g., "Pay X {E}") in activated abilities, specifically addressing issue #1092 with Chthonian Nightmare. It updates the oracle cost parser to parse variable energy costs into a QuantityExpr, ensures the variable cost is capped by the player's current energy counters, and handles the announcement and concretization of X during activation. Additionally, it ensures that non-self battlefield-removal sub-costs (like sacrifice or bounce) are correctly surfaced interactively after X-announcement. A comprehensive integration test has been added to verify this behavior. I have no feedback to provide as the implementation is clean and aligns with the repository's architectural guidelines.
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.
2 participants
Fixes #1092.
Chthonian Nightmare: "Pay X {E}, Sacrifice a creature, Return this enchantment to its owner's hand: Return target creature card with mana value X from your graveyard to the battlefield. Activate only as a sorcery." — the ability could never be found/activated at all.
Root cause (two independent gaps)
try_parse_energy_costhad no variable-X branch, so "Pay X {E}" silently parsed asPayEnergy { amount: Fixed(0) }instead ofRef(Variable("X")), unlike the neighboring "pay x life"/"pay x speed" branches. With X pinned to 0, the reanimation target filter ("mana value X") could only ever match a mana-value-0 graveyard creature — no legal target on any realistic board.cost_needs_activation_x_announcement,additional_cost_x_max,concretize_chosen_x_cost) only recognizedRemoveCounter-shaped variable costs, and the resumed-cost re-surfacing for the sibling Sacrifice/ReturnToHand costs needed anActivationResidual::ManaLegtag that was never applied for this combination.Testing
New end-to-end integration test driving the real activation pipeline: announces X=2 bounded by energy, sacrifices a creature, chooses between two identical-mana-value graveyard creatures (plus a wrong-mana-value decoy to prove X is genuinely threaded into the target filter), verifies energy/zones land correctly.
Verified locally (rebased onto main): fmt clean, clippy clean, 16672 lib tests pass, 3121 integration tests pass.