fix(parser): wire up "sacrifice one or more" cost and its reflexive copy trigger - #5810
fix(parser): wire up "sacrifice one or more" cost and its reflexive copy trigger#5810tryeverything24 wants to merge 6 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 implements support for parsing optional ranged-sacrifice additional costs and synthesizing their trailing reflexive copy triggers, resolving issue #1108 (Plumb the Forbidden). It introduces parser logic to split and build these triggers, updates the sacrifice cost parser to recognize 'one or more' and 'at least one' quantifiers, and adds comprehensive integration tests. The review feedback highlights several incorrect Magic Comprehensive Rules (CR) citations in the comments and test descriptions, specifically pointing out that reflexive triggered abilities are governed by CR 603.12 (not CR 603.2b) and optional additional costs are governed by CR 118.12 (not CR 113.7).
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.
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
Per review feedback on PR phase-rs#5810: - Reflexive triggered abilities ("When you do, ...") are governed by CR 603.12, not CR 603.2b (phase/step beginning triggers) -- 8 sites. - Optional additional costs are governed by CR 118.12, not CR 113.7 (source of an ability) -- 1 site. Comment/doc-string only; no logic changed. Both tests still pass.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — unsupported reflexive tails must remain strict failures.
🟡 Finding
[MED] The parser strips every . When you do tail before supported-build validation. Evidence: crates/engine/src/parser/oracle.rs:3638-3670. Why it matters: an unsupported reflexive continuation is silently dropped and its card appears supported. Suggested fix: preserve the tail as an explicit strict failure unless the full continuation is implemented.
Recommendation: request-changes.
Per review feedback on PR phase-rs#5810: - Reflexive triggered abilities ("When you do, ...") are governed by CR 603.12, not CR 603.2b (phase/step beginning triggers) -- 8 sites. - Optional additional costs are governed by CR 118.12, not CR 113.7 (source of an ability) -- 1 site. Comment/doc-string only; no logic changed. Both tests still pass.
115e23a to
f18bdee
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the prior coverage-honesty finding remains on current head f18bdee7.
🔴 Blocker
[MED] crates/engine/src/parser/oracle.rs:3894-3935 unconditionally removes every trailing . When you do, ... sentence, then emits a trigger only if both the additional cost and build_additional_cost_reflexive_copy_trigger match. When either check declines, the parser still advances i and the removed rules text has no Effect::unimplemented/diagnostic path. This contradicts the comment at oracle_casting.rs:72-75: an unsupported reflexive tail is not left for the swallow check; it is silently discarded. That makes a card with an unsupported reflexive continuation appear supported.
Preserve the suffix unless its full supported form is recognized, or emit the existing strict-failure marker for the unsupported continuation. Add a positive coverage-honesty regression for a non-copy When you do tail; the existing Plumb runtime test is good evidence for the supported copy path but cannot exercise this decline branch.
Evidence: current-head local diff and the parser control flow above. Confidence: high.
Recommendation: request changes — repair the unsupported-tail path, then regenerate current-head parser evidence.
…opy trigger
Plumb the Forbidden ("As an additional cost to cast this spell, you may
sacrifice one or more creatures. When you do, copy this spell for each
creature sacrificed this way. You draw a card and lose 1 life.") only ever
drew one card and lost one life, regardless of how many creatures were
sacrificed -- two distinct parser gaps, both confirmed empirically by
dumping the real parsed AST before/after:
1. oracle_cost.rs's ranged-sacrifice branch recognized "sacrifice any
number of X" but had no case for "sacrifice one or more X" -- that
phrase fell through to the plain-numeral fallback ("one" -> exactly 1,
mandatory), so the cost was never actually optional/ranged.
2. The trailing "When you do, copy this spell for each creature sacrificed
this way" reflexive trigger was never wired into anything -- the
additional-cost line parser only special-cased a trailing mana-reduction
clause (Rottenmouth Viper shape); everything else past the first
sentence was silently dropped (confirmed via a SwallowedClause
diagnostic on the untouched line).
The underlying "copy N times" mechanism (Effect::CopySpell + repeat_for)
is proven, tested infrastructure (Replicate/Casualty/Squad) -- this is a
parser-wiring fix, not a new engine capability.
Anchored on:
- crates/engine/src/database/synthesis.rs:2404-2457 (synthesize_replicate)
+ :2342-2385 -- the "optional/repeatable additional cost -> SpellCast
trigger gated on AdditionalCostPaid, executing a CopySpell with
repeat_for" shape (CR 702.56a), mirrored here from raw Oracle text
instead of a named keyword.
- crates/engine/src/parser/oracle_nom/condition.rs:8650-8669
(parse_you_sacrifice_this_way_clause) -- the existing "when you sacrifice
one or more/any number of/at least one X this way" quantifier vocabulary
(built for Nyssa of Traken), reused so the cost parser and its condition
sibling agree.
- crates/engine/src/game/casting_costs.rs:1971-1975 +
crates/engine/src/game/quantity.rs:1813-1839 -- the existing
QuantityRef::Variable("X")/cost_x_paid mechanism used to carry "how many
creatures were sacrificed" from cost-payment time to the trigger.
Closes phase-rs#1108
Per review feedback on PR phase-rs#5810: - Reflexive triggered abilities ("When you do, ...") are governed by CR 603.12, not CR 603.2b (phase/step beginning triggers) -- 8 sites. - Optional additional costs are governed by CR 118.12, not CR 113.7 (source of an ability) -- 1 site. Comment/doc-string only; no logic changed. Both tests still pass.
Review follow-up (phase-rs#1108): the parser unconditionally stripped every trailing '. When you do, ...' sentence from an additional-cost line BEFORE supported-build validation, so when either the cost shape or the copy-trigger builder declined, the removed rules text had no Effect::unimplemented/diagnostic path — a card with an unsupported reflexive continuation appeared supported while its trigger silently vanished. The split is now committed only when the WHOLE continuation is the supported form (optional ranged-sacrifice cost + recognized copy-for-each-sacrificed body): both halves are validated on the split candidate first, and only then recorded + the trigger emitted. On decline, NOTHING is recorded for the line — re-parsing the un-split two-sentence text is not a safe fallback (the cost grammar partially matches it into a wrong cost, e.g. 'sacrifice one' out of 'sacrifice one or more creatures. When you do, ...'), so with zero lowered evidence the untouched line surfaces through the swallow audit as a SwallowedClause strict failure. Added the requested coverage-honesty regressions: the supported Plumb the Forbidden shape parses clean (cost + exactly one synthesized trigger, no swallowed clause), and a non-copy 'When you do, draw a card for each creature sacrificed this way' tail stays a strict failure (no trigger, no recorded cost, SwallowedClause emitted). Verified in an isolated CARGO_TARGET_DIR: cargo fmt --all --check clean; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -D warnings clean; engine lib 17522 passed; engine integration 3750 passed.
|
Addressed the coverage-honesty blocker on the current head. The Added both requested regressions: the supported Plumb the Forbidden shape parses clean (recorded cost + exactly one synthesized SpellCast trigger + no swallowed clause), and a NON-copy tail ("When you do, draw a card for each creature sacrificed this way") stays a strict failure — no trigger, no recorded cost, Verified locally in an isolated target dir: fmt clean, workspace clippy |
f18bdee to
3bd022d
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughChangesPlumb the Forbidden support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant OracleParser
participant GameEngine
participant Stack
Player->>GameEngine: cast Plumb the Forbidden
GameEngine->>OracleParser: parse optional sacrifice and reflexive copy text
OracleParser-->>GameEngine: additional cost and SpellCast copy trigger
GameEngine->>GameEngine: validate sacrifice minimum and choose X
GameEngine->>Stack: resolve original spell and generated copies
Stack-->>Player: draw cards and lose life
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the new sacrifice-cost representation permits a rules-wrong zero selection.
🔴 Blocker
crates/engine/src/parser/oracle_cost.rs:656-685 encodes both “one or more” and “at least one” as u32::MAX, the same sentinel whose range is (0, eligible_len) in crates/engine/src/game/casting.rs:354-375. The accepted optional-cost path then exposes ChooseXValue with the default minimum of zero at crates/engine/src/game/casting_costs.rs:5961-5983 and accepts the chosen zero at :6243-6267. A player can accept Plumb’s optional cost, choose zero, sacrifice nothing, and still cast; a mandatory “at least one” form has the same lower-bound bug.
Use a typed ranged/minimum requirement rather than the any-number sentinel, and add runtime regressions that accepting “one or more” rejects zero and that a mandatory sibling also rejects zero. The current tests cover positive selection and decline only.
✅ Clean
The previous coverage-honesty blocker is resolved: declining the clause leaves no lowered parse evidence, and the parser test asserts SwallowedClause.
Recommendation: request changes for the minimum-bound model and the two zero-selection regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/parser/oracle_cost.rs (1)
669-685: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the minimum-1 floor for
one or more/at least oneincrates/engine/src/parser/oracle_cost.rs:669-684— these forms still collapse to the sameu32::MAXpath asany number of, andsacrifice_cost_bounds()treats that sentinel as(0, eligible_len), so a cost that should require one sacrifice can be paid with zero.🤖 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 669 - 685, The quantity parsing branch using `nom_on_lower` must preserve a minimum sacrifice count of one for “one or more” and “at least one”; only “any number of” should retain the `u32::MAX` sentinel path. Update the `AbilityCost::Sacrifice` construction and related parsing flow so these bounded forms produce the existing representation for a required minimum of one, while preserving the current target parsing and unbounded behavior.Source: Path instructions
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_casting.rs (1)
130-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared “sacrificed this way” vocabulary
This shape check repeats the suffix list for “sacrificed this way”. Pull it behind a shared helper or shared suffix constants so the accepted forms stay in sync.
🤖 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 130 - 158, Extract the repeated “sacrificed this way” suffixes from is_copy_for_each_sacrificed_this_way_shape into shared constants or a helper, and reuse that shared vocabulary here. Preserve all currently accepted singular, plural, and no-relative-pronoun forms while ensuring future changes remain synchronized.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/engine/src/parser/oracle_cost.rs`:
- Around line 669-685: The quantity parsing branch using `nom_on_lower` must
preserve a minimum sacrifice count of one for “one or more” and “at least one”;
only “any number of” should retain the `u32::MAX` sentinel path. Update the
`AbilityCost::Sacrifice` construction and related parsing flow so these bounded
forms produce the existing representation for a required minimum of one, while
preserving the current target parsing and unbounded behavior.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_casting.rs`:
- Around line 130-158: Extract the repeated “sacrificed this way” suffixes from
is_copy_for_each_sacrificed_this_way_shape into shared constants or a helper,
and reuse that shared vocabulary here. Preserve all currently accepted singular,
plural, and no-relative-pronoun forms while ensuring future changes remain
synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7477ef46-1143-420e-bedc-e0bc17804e5b
📒 Files selected for processing (6)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_casting.rscrates/engine/src/parser/oracle_cost.rscrates/engine/src/parser/oracle_tests.rscrates/engine/tests/integration/issue_1108_plumb_the_forbidden.rscrates/engine/tests/integration/main.rs
Encode "sacrifice one or more" / "sacrifice at least one" as a typed
SacrificeRequirement::AtLeast { min } instead of reusing the zero-floor
any-number u32::MAX sentinel. The floor now threads from the parser
through sacrifice_cost_bounds and the ChooseXValue announcement range,
so accepting the optional cost (or paying the mandatory sibling) can no
longer announce X=0 and sacrifice nothing. "Any number of" costs keep
the zero-floor sentinel semantics unchanged.
Regressions: accepting Plumb the Forbidden's optional cost rejects
X=0, and a mandatory "at least one" sibling rejects X=0; both announce
min=1 and complete normally at the floor.
|
Addressed the zero-selection blocker in df4b415. Typed minimum model. Where the bound binds. Parser encoding ( Regressions (red -> green). Both new runtime tests fail on the previous head with
The existing positive-selection and decline tests are unchanged and green, and the coverage-honesty tests (SwallowedClause assertions) still pass. Totals. engine lib: 17522 passed / 0 failed; integration: 3752 passed / 0 failed; Parse diff. Only cards whose cost text uses the "one or more" / "at least one" quantifiers change lowered form (the Plumb the Forbidden class this PR adds); "any number of" cards keep their previous encoding byte-for-byte. |
|
Held for maintainer port — no contributor action requested. The new head is textually conflicting with current The handler must port this branch across the maintainer-side churn before implementation re-review. This session lacks the authorized fork-push credential ( |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/mana_abilities.rs (1)
3576-3590: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
fixed_count()doesn't guard the "any number of" sentinel like it guardsAtLeast.
SacrificeRequirement::Count { count: u32::MAX }(the "any number of" sentinel) also returnsSome(u32::MAX)fromfixed_count(), so this function would reportcount = u32::MAXfor that shape instead of declining it the same way it declinesAtLeast. This contradicts the stated intent in the comment ("mana-ability sacrifice costs carry fixed counts only") and mirrors the exact casecasting::sacrifice_cost_boundsspecial-cases elsewhere (Count { count: u32::MAX } => (0, eligible_len)).🐛 Proposed fix to also decline the "any number of" sentinel
let (requirement, filter) = super::casting::find_non_self_sacrifice_cost(cost.as_ref()?)?; - // Mana-ability sacrifice costs carry fixed counts only; a typed ranged - // floor ("sacrifice one or more", issue `#1108`) has no mana-ability - // announcement channel, so decline rather than treat the floor as exact. - let count = requirement.fixed_count()?; + // Mana-ability sacrifice costs carry fixed counts only; a typed ranged + // floor ("sacrifice one or more", issue `#1108`) or the "any number of" + // sentinel has no mana-ability announcement channel, so decline rather + // than treat either as an exact count. + let count = match requirement.fixed_count() { + Some(count) if count != u32::MAX => count, + _ => return None, + };🤖 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/mana_abilities.rs` around lines 3576 - 3590, Update sacrifice_cost_choice to explicitly reject SacrificeRequirement::Count with count == u32::MAX before or while deriving the fixed count, so the “any number of” sentinel returns None like AtLeast requirements. Preserve normal fixed-count handling and eligible-target collection for valid finite counts.
🧹 Nitpick comments (1)
crates/engine/src/types/ability.rs (1)
7972-7983: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between
chosen_range_minandmin_selection_count.These two methods differ only in how they treat the plain
Count { count }case (NonevsSome(count)); theAtLeast/Aggregate/any-number-sentinel arms are identical. Could be expressed asmin_selection_countdelegating to a shared helper, but the duplication is small and the two methods have genuinely distinct semantics (chosen-at-payment-time floor vs. legal-payment floor), so this is optional.🤖 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/types/ability.rs` around lines 7972 - 7983, Optionally reduce duplication between chosen_range_min and min_selection_count by extracting their shared AtLeast, Aggregate, and Count { count: u32::MAX } handling into a helper. Preserve their distinct Count { count } behavior: chosen_range_min must return None, while min_selection_count returns Some(count).
🤖 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_costs.rs`:
- Around line 5974-5982: Update the four X-announcement comments in the
casting-cost handling, including the comment near pending.ability.min_x_value
and additional_cost_x_min(&cost), to replace the incorrect CR 107.2 citation
with CR 107.3a. Leave the surrounding explanatory text and implementation
unchanged.
In `@crates/engine/src/game/casting.rs`:
- Around line 354-357: Update the CR citation in sacrifice_cost_bounds and its
matching comment below from CR 107.2 to CR 107.1c, preserving the existing
description and logic.
In `@crates/engine/src/game/cost_payability.rs`:
- Around line 284-289: Update the comment above the Count and AtLeast sacrifice
requirement match in cost payability to replace the incorrect CR 107.2 citation
with the applicable sacrifice-cost rule citation, while preserving the existing
CR 701.21 reference and explanatory text.
---
Outside diff comments:
In `@crates/engine/src/game/mana_abilities.rs`:
- Around line 3576-3590: Update sacrifice_cost_choice to explicitly reject
SacrificeRequirement::Count with count == u32::MAX before or while deriving the
fixed count, so the “any number of” sentinel returns None like AtLeast
requirements. Preserve normal fixed-count handling and eligible-target
collection for valid finite counts.
---
Nitpick comments:
In `@crates/engine/src/types/ability.rs`:
- Around line 7972-7983: Optionally reduce duplication between chosen_range_min
and min_selection_count by extracting their shared AtLeast, Aggregate, and Count
{ count: u32::MAX } handling into a helper. Preserve their distinct Count {
count } behavior: chosen_range_min must return None, while min_selection_count
returns Some(count).
🪄 Autofix (Beta)
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: 0f74e58e-8bd7-4a39-974e-b456a9f1b984
📒 Files selected for processing (12)
crates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/cost_payability.rscrates/engine/src/game/engine_modes.rscrates/engine/src/game/engine_payment_choices.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/replacement.rscrates/engine/src/parser/oracle_casting.rscrates/engine/src/parser/oracle_cost.rscrates/engine/src/parser/oracle_keyword.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_1108_plumb_the_forbidden.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/parser/oracle_cost.rs
- crates/engine/src/parser/oracle_casting.rs
| // CR 601.2b + CR 107.2: the announced X range's floor is the | ||
| // larger of the spell's own minimum-X annotation and the typed | ||
| // ranged floor carried by the additional cost itself ("sacrifice | ||
| // one or more" / "at least one" — issue #1108); accepting such a | ||
| // cost can never announce X=0. | ||
| let min = pending | ||
| .ability | ||
| .min_x_value | ||
| .max(additional_cost_x_min(&cost)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- CR 107.2 text ---'
rg -n "^107\.2|^107\." docs/MagicCompRules.txt | sed -n '1,20p'
echo
echo '--- all CR 107.2 mentions in casting_costs.rs ---'
rg -n "CR 107\.2" crates/engine/src/game/casting_costs.rs
echo
echo '--- nearby code around the cited locations ---'
sed -n '5960,5995p' crates/engine/src/game/casting_costs.rs
echo
sed -n '6825,6940p' crates/engine/src/game/casting_costs.rs
echo
sed -n '10675,10705p' crates/engine/src/game/casting_costs.rsRepository: phase-rs/phase
Length of output: 238
🌐 Web query:
Magic Comprehensive Rules 107.2 601.2b X value announcement minimum cost floor
💡 Result:
In Magic: The Gathering, the value of X in a spell's mana cost is determined during the casting process [1][2]. According to the Comprehensive Rules, rule 601.2b requires a player to announce the value of any variable cost, such as an {X} in a spell's mana cost, as part of the process of casting the spell [1][2]. Regarding the minimum cost floor: If a player is casting a spell with {X} in its mana cost and an effect allows them to cast that spell without paying its mana cost or any alternative cost that includes X, the only legal choice for X is 0 (Rule 107.3b) [3][4]. This restriction does not apply to effects that merely reduce a cost, even if those reductions result in a total cost of zero [3][4]. Rule 107.2 acts as a general fallback, stating that if any value needs to be determined (either as a result or in a calculation) and cannot be, it defaults to 0 [5][3]. While 107.3b provides a specific constraint for casting spells without paying costs, 107.2 ensures that indeterminate values do not halt the game [3].
Citations:
- 1: https://ancestral.vision/spells-abilities-and-effects/casting-spells.html
- 2: https://mtg-rules.vercel.app/chapters/601
- 3: https://media.wizards.com/2026/downloads/MagicCompRules%2020260227.pdf
- 4: https://media.wizards.com/2025/downloads/MagicCompRules%2020250606.pdf
- 5: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.txt
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- files matching MagicCompRules ---'
fd -a 'MagicCompRules*' .
echo
echo '--- locate casting_costs.rs citation sites ---'
rg -n "CR 107\.2|CR 601\.2b|additional_cost_x_min|additional_cost_x_max|enter_payment_step" crates/engine/src/game/casting_costs.rs
echo
echo '--- surrounding code at the first cited site ---'
sed -n '5968,5990p' crates/engine/src/game/casting_costs.rsRepository: phase-rs/phase
Length of output: 10150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- repo files matching rules docs ---'
fd -a 'MagicCompRules*|*CompRules*|*rules*.txt' . | sed -n '1,80p'
echo
echo '--- locate the four citations and nearby rule comments ---'
rg -n -C 2 "CR 107\.2|CR 601\.2b|CR 107\.3b|CR 107\.2" crates/engine/src/game/casting_costs.rs
echo
echo '--- one-line file outline around cited helper functions ---'
ast-grep outline crates/engine/src/game/casting_costs.rs --view expanded | sed -n '1,220p'Repository: phase-rs/phase
Length of output: 526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- repo files matching rules docs ---'
fd -a 'MagicCompRules*|*CompRules*|*rules*.txt' . | sed -n '1,80p'
echo
echo '--- locate the four citations and nearby rule comments ---'
rg -n -C 2 "CR 107\.2|CR 601\.2b|CR 107\.3b" crates/engine/src/game/casting_costs.rs
echo
echo '--- AST outline for casting_costs.rs ---'
ast-grep outline crates/engine/src/game/casting_costs.rs --view expanded | sed -n '1,220p'Repository: phase-rs/phase
Length of output: 526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate rules docs files ---'
find . -maxdepth 4 -type f \( -iname '*comp*rule*' -o -iname 'MagicCompRules*' -o -iname '*rules*.txt' \) | sed -n '1,120p'
printf '\n%s\n' '--- all CR 107.2 mentions in casting_costs.rs ---'
rg -n "CR 107\.2" crates/engine/src/game/casting_costs.rs
printf '\n%s\n' '--- all CR 601.2b / 107.3b mentions in casting_costs.rs ---'
rg -n "CR 601\.2b|CR 107\.3b" crates/engine/src/game/casting_costs.rsRepository: phase-rs/phase
Length of output: 7397
🌐 Web query:
Magic Comprehensive Rules 601.2b announced X additional cost range floor 107.2 107.3a
💡 Result:
In the Magic: The Gathering Comprehensive Rules, the sections you referenced relate to casting spells and defining variable values like X [1][2]. Rule 601.2b governs the announcement of choices made while casting a spell, including modal choices, splice intentions, and—crucially—the announcement of variable values (like X) or intentions to pay additional or alternative costs [3][2]. Rule 107.2 and 107.3 define how numbers and the placeholder X are handled [2][4]: 107.2 states: If anything needs to use a number that can’t be determined, either as a result or in a calculation, it uses 0 instead [2][5]. 107.3a specifies: If a spell or activated ability has a mana cost, alternative cost, additional cost, and/or activation cost with an {X}, [-X], or X in it, and the value of X isn’t defined by the text of that spell or ability, the controller of that spell or ability chooses and announces the value of X as part of casting the spell or activating the ability [1][2]. There is no "additional cost range floor" explicitly defined by these specific rule numbers; rather, these rules establish the mechanics for determining and announcing costs (601.2b) and setting variable values (107.3a) [3][2]. The "floor" for any cost, including those involving X, is generally 0, as governed by rule 601.2f, which states that if the mana component of a total cost is reduced, it cannot be reduced to less than {0} [6].
Citations:
- 1: https://media.wizards.com/2025/downloads/MagicCompRules%2020250404.pdf
- 2: https://media.wizards.com/2025/downloads/MagicCompRules%2020250606.pdf
- 3: https://ancestral.vision/spells-abilities-and-effects/casting-spells.html
- 4: https://crypticstudy.com/rules/
- 5: https://media.wizards.com/2026/downloads/MagicCompRules%2020260227.pdf
- 6: https://mtg-rules.vercel.app/chapters/601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '6978,7090p' crates/engine/src/game/casting_costs.rs
echo
sed -n '10935,10955p' crates/engine/src/game/casting_costs.rs
echo
sed -n '6116,6129p' crates/engine/src/game/casting_costs.rsRepository: phase-rs/phase
Length of output: 6346
Replace the CR 107.2 citation in the X-announcement comments
CR 107.2 is the wrong rule here; these four comments should cite the X-announcement rule (CR 107.3a) instead.
🤖 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_costs.rs` around lines 5974 - 5982, Update the
four X-announcement comments in the casting-cost handling, including the comment
near pending.ability.min_x_value and additional_cost_x_min(&cost), to replace
the incorrect CR 107.2 citation with CR 107.3a. Leave the surrounding
explanatory text and implementation unchanged.
Source: Path instructions
| /// CR 107.2: ranged requirements select between their typed floor and the | ||
| /// eligible pool — zero for the "any number of" sentinel, `min` for the | ||
| /// "one or more" / "at least one" `AtLeast` form (issue #1108). | ||
| pub(crate) fn sacrifice_cost_bounds( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify CR 107.1c vs 107.2 content against the repo's authoritative rules text.
fd -a MagicCompRules.txt | xargs -I{} rg -n -A4 '^107\.1c|^107\.2\.' {}Repository: phase-rs/phase
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the cited implementation and nearby context.
FILE="crates/engine/src/game/casting.rs"
sed -n '330,410p' "$FILE" | cat -n
# Locate the Comprehensive Rules text in the repo and show the relevant rule numbers.
RULES=$(fd -a MagicCompRules.txt | head -n 1)
echo "RULES=$RULES"
rg -n -A4 -B2 '^107\.1c|^107\.2\.' "$RULES"Repository: phase-rs/phase
Length of output: 4176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the rules text location and inspect the relevant rule numbers.
git ls-files | rg 'MagicCompRules\.txt$|CLAUDE\.md$|crates/engine/src/game/casting\.rs$'
fd -a MagicCompRules.txt .
fd -a casting.rs crates/engine/src/game
# If the rules text exists, print the relevant rules.
RULES=$(fd -a MagicCompRules.txt . | head -n 1)
if [ -n "${RULES:-}" ]; then
echo "RULES=$RULES"
rg -n -A4 -B2 '^107\.1c|^107\.2\.' "$RULES"
fiRepository: phase-rs/phase
Length of output: 340
🌐 Web query:
Comprehensive Rules 107.1c 107.2 choose any number sacrifice additional cost
💡 Result:
According to the Magic: The Gathering Comprehensive Rules, the specific rules you inquired about are as follows [1][2]: Rule 107.1c states: "If a rule or ability instructs a player to choose 'any number,' that player may choose any positive number or zero" [1][3]. Rule 107.2 states: "If anything needs to use a number that can’t be determined, either as a result or in a calculation, it uses 0 instead" [1][4]. Regarding "sacrifice" as an additional cost: An additional cost is a cost listed in a spell’s rules text or applied to a spell or ability that its controller must pay at the same time they pay the spell’s mana cost or the ability’s activation cost [5]. If a card instructs you to sacrifice a permanent as an additional cost (e.g., "As an additional cost to cast this spell, sacrifice a creature"), you must choose and sacrifice the required number of permanents at the time you pay the cost [6][5]. If an effect requires you to "choose any number" of permanents to sacrifice as part of a cost, rule 107.1c allows you to choose any positive number or zero [1][3]. If you choose zero, you pay zero costs associated with that choice.
Citations:
- 1: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.pdf
- 2: https://media.wizards.com/2026/downloads/MagicCompRules%2020260227.pdf
- 3: https://mtg-rules.vercel.app/chapters/107
- 4: https://magic-rulebook.vercel.app/rules/107
- 5: https://magic-rulebook.vercel.app/rules/118
- 6: https://crypticstudy.com/rules/
Use CR 107.1c here, not CR 107.2. 107.2 is the indeterminate-number-to-0 rule; this logic is about the “any number”/floor-bounded sacrifice selection rule, so the citation should be updated here and in the matching comment below.
🤖 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 354 - 357, Update the CR
citation in sacrifice_cost_bounds and its matching comment below from CR 107.2
to CR 107.1c, preserving the existing description and logic.
Source: Path instructions
| // CR 701.21 + CR 107.2: counted requirements — fixed counts, | ||
| // the ranged sentinel, and the typed "at least" floor | ||
| // (issue #1108) — are payable when the eligible pool covers | ||
| // the requirement's minimum selection. | ||
| crate::types::ability::SacrificeRequirement::Count { .. } | ||
| | crate::types::ability::SacrificeRequirement::AtLeast { .. } => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== cost_payability slice ==\n'
sed -n '250,320p' crates/engine/src/game/cost_payability.rs
printf '\n== CR 107.2 search ==\n'
rg -n "107\\.2|sacrifice|at least|counted requirements|indeterminate" docs/MagicCompRules.txt | sed -n '1,120p'Repository: phase-rs/phase
Length of output: 3940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '280,300p' crates/engine/src/game/cost_payability.rs
echo '---'
grep -nA4 -B4 '107\.2' docs/MagicCompRules.txtRepository: phase-rs/phase
Length of output: 1422
🌐 Web query:
Magic: The Gathering Comprehensive Rules 107.2 sacrifice cost at least one counted requirements
💡 Result:
In the Magic: The Gathering Comprehensive Rules, rule 107.2 states: "If anything needs to use a number that can’t be determined, either as a result or in a calculation, it uses 0 instead." [1][2][3] This rule is a general guideline for handling indeterminate numerical values within the game, such as when a calculation cannot be resolved, and is not specific to sacrifice costs. [1][2] There is no rule in the Comprehensive Rules that specifically links rule 107.2 to the requirements for a sacrifice cost. [4][1][5] Costs involving sacrifice are governed by rule 118, "Costs," and rule 701.21, "Sacrifice." [6][5] Specifically, rule 118.10 dictates that each payment of a cost applies to only one spell, ability, or effect, meaning a player cannot sacrifice a single permanent to satisfy the sacrifice requirements of multiple different spells or abilities. [6] To sacrifice a permanent, its controller must move it from the battlefield to its owner's graveyard, and they can only sacrifice a permanent they control. [5]
Citations:
- 1: https://media.wizards.com/2024/downloads/MagicCompRules%2020240802.pdf
- 2: https://media.wizards.com/2022/downloads/Comprehensive%20Rules%2020221007.pdf
- 3: https://mtg-rules.vercel.app/chapters/107
- 4: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.txt
- 5: https://mtg.wiki/page/Sacrifice
- 6: https://ancestral.vision/game-concepts/costs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('crates/engine/src/game/cost_payability.rs')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 280 <= i <= 295:
print(f"{i}: {line}")
PY
echo '---'
python3 - <<'PY'
from pathlib import Path
p = Path('docs/MagicCompRules.txt')
for i, line in enumerate(p.read_text().splitlines(), 1):
if '107.2' in line or 'sacrifice' in line.lower():
print(f"{i}: {line}")
PYRepository: phase-rs/phase
Length of output: 1725
Fix the CR citation here — CR 107.2 is about indeterminate numbers defaulting to 0, not sacrifice payability; cite the sacrifice-cost rule instead.
🤖 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 284 - 289, Update the
comment above the Count and AtLeast sacrifice requirement match in cost
payability to replace the incorrect CR 107.2 citation with the applicable
sacrifice-cost rule citation, while preserving the existing CR 701.21 reference
and explanatory text.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — current head 4e521b79f3d0d9e46f1af4480ec581f356099ad4 cannot be merged as reviewed.
[HIGH] The PR is textually conflicting with current main, so its green checks and parse-diff are not evidence for the merge-queue candidate. Evidence: GitHub reports mergeable: CONFLICTING / mergeStateStatus: DIRTY; locally, origin/main is not an ancestor of this head (merge-base is d4a5a37a7e7a5e8c5e7f1a8834e831b78bb5f948). Why it matters: this 15-file / 1,051-line engine change must be ported through current engine interfaces before correctness, parser impact, and runtime tests can be assessed for the code that would actually merge. Suggested fix: a credentialed maintainer must merge current main into the contributor branch, resolve the resulting conflicts at the relevant engine authorities, push the new head, and rerun current-head CI plus the parse-diff artifact.
[MED] The current head adds rules annotations that cite CR 107.2 for sacrifice-selection bounds and X-announcement floors, but those paths implement choice/payment constraints rather than the indeterminate-number fallback. Evidence: crates/engine/src/game/casting.rs:355-357, crates/engine/src/game/casting.rs:385-388, crates/engine/src/game/casting_costs.rs:6121-6125, crates/engine/src/game/casting_costs.rs:7072-7076, crates/engine/src/game/casting_costs.rs:10941-10943, and crates/engine/src/game/cost_payability.rs:286-289. Why it matters: incorrect CR annotations give reviewers false confidence about rules validation in the new typed minimum model. Suggested fix: during the port, verify the applicable CR text and replace each citation with the rule that actually authorizes the specific behavior; do not retain CR 107.2 for these bounds.
The earlier coverage-honesty and zero-selection findings are resolved on this head: the parser test covers a non-copy reflexive tail as SwallowedClause, and the integration tests exercise accepted/mandatory minimum-one paths. The parse-diff sticky comment is present and current to this head, but it is baseline main 9a0a31632f79, not the current merge target.
Recommendation: request changes. Re-review only the pushed, conflict-resolved head; no approval, auto-merge, or enqueue is appropriate now.
|
This PR still has unresolved requested changes on its current head ( If the requested changes are not addressed within 7 days, this PR will be automatically closed. A fresh head or contributor response before then will return it to review rather than expiry. |
|
Closing per the requested-changes expiry warning posted on 2026-08-07. The warned current head |
2 participants
Summary
Fixes #1108. Plumb the Forbidden ("As an additional cost to cast this spell, you may
sacrifice one or more creatures. When you do, copy this spell for each creature sacrificed
this way. You draw a card and lose 1 life.") only ever drew one card and lost one life,
regardless of how many creatures were sacrificed as the additional cost.
Root cause
Two distinct parser gaps, both confirmed empirically by dumping the real parsed AST before
and after the fix (not inferred from reading code alone):
oracle_cost.rs: the ranged-sacrifice branch recognized"sacrifice any number of X"but had no case for"sacrifice one or more X"— that phrase fell through to theplain-numeral fallback (
"one"→ exactly 1, mandatory), so the cost was parsed as amandatory single sacrifice instead of an optional, ranged one.
oracle.rs/oracle_casting.rs: the trailing"When you do, copy this spell for each creature sacrificed this way"reflexive trigger was never wired into anything —the additional-cost line parser only special-cased a trailing mana-reduction clause
(Rottenmouth Viper shape); everything past the first sentence was silently dropped
(confirmed via a
SwallowedClausediagnostic firing on the untouched line).The underlying "copy N times" engine mechanism (
Effect::CopySpell+repeat_for) isproven, tested infrastructure already powering Replicate/Casualty/Squad — this is a parser
wiring fix, not a new engine capability.
Anchored on
crates/engine/src/database/synthesis.rs:2404-2457(synthesize_replicate) +:2342-2385— the "optional/repeatable additional cost →SpellCasttrigger gated onAdditionalCostPaid, executing aCopySpellwithrepeat_for" shape (CR 702.56a),mirrored here from raw Oracle text instead of a named keyword.
crates/engine/src/parser/oracle_nom/condition.rs:8650-8669(
parse_you_sacrifice_this_way_clause) — the existing "when you sacrifice one ormore/any number of/at least one X this way" quantifier vocabulary (built for Nyssa of
Traken), reused so the cost parser and its condition sibling agree.
crates/engine/src/game/casting_costs.rs:1971-1975+crates/engine/src/game/quantity.rs:1813-1839— the existingQuantityRef::Variable("X")/cost_x_paidmechanism used to carry "how many creatureswere actually sacrificed" from cost-payment time to the later-resolving trigger.
Testing
Added
crates/engine/tests/integration/issue_1108_plumb_the_forbidden.rs:plumb_the_forbidden_copies_once_per_creature_sacrificed— sacrificing 2 creaturesyields 3 total draws/life-losses (1 original + 2 copies), both creatures in the
graveyard.
plumb_the_forbidden_declined_cost_resolves_once— declining the additional cost yieldsexactly 1 draw/life-loss, creature stays on the battlefield.
Both pass on a clean, isolated
cargo test -p engine --test integration issue_1108run:Also ran a broader regression sweep on adjacent test modules (
oracle_cost::,oracle_casting::,copy_spell::) — 102/102, 72/72, 36/36 passed, confirming noregressions (Rottenmouth Viper's "any number of" cost path in particular still parses
correctly).
cargo fmt --allclean.Validation Failures (honesty disclosure, per docs/AI-CONTRIBUTOR.md)
Implemented and verified by a Claude Code agent (empirical AST-diff diagnosis, minimal fix
composing only existing engine primitives, end-to-end game-level test, adjacent-suite
regression sweep,
cargo fmtclean), but not through the formal/engine-implementermulti-agent plan → review-plan → implement → review-impl pipeline — that skill wasn't
available in the environment this was built in. No
Gate Acombinator-purity scriptoutput or discriminating-test coverage map / maintainer-simulation matrix is included.
Flagging this plainly rather than claiming a review loop that didn't run. The reflexive-
trigger synthesizer added here is deliberately narrow (only recognizes
"copy (this spell|that spell|it) for each <filter> [that was/were] sacrificed this way") rather than ageneral "additional cost → When you do, [any effect]" synthesizer — other reflexive-trigger
bodies still fall through to the existing
SwallowedClausediagnostic.Model: claude-sonnet-5
Tier: Standard
Summary by CodeRabbit