fix(engine): Sidisi, Regent of the Mire — bind sacrifice-cost X and reanimate at X+1 mana value#6027
fix(engine): Sidisi, Regent of the Mire — bind sacrifice-cost X and reanimate at X+1 mana value#6027nghetienhiep wants to merge 2 commits into
Conversation
…eanimate at X+1 mana value
The parser dropped "plus 1" when parsing "mana value X plus 1" filter
thresholds, so the reanimation target required mana value == X (the
sacrificed creature's own mana value) instead of X + 1. Separately, the
activated ability's sacrifice cost ("Sacrifice a creature you control
with mana value X") is a Shoal-style pitch-bound X — defined by whichever
creature is chosen, not a prior announcement — but that relaxation only
existed for exile costs, so the ability's own sacrifice requirement never
had any eligible permanents and couldn't be activated at all.
- parse_quantity_expr_number now composes an optional "plus N"/"minus N"
tail into a QuantityExpr::Offset, mirroring the existing count-position
arithmetic arm in parse_count_expr.
- Generalized exile_cost_effective_filter to pitch_bound_x_effective_filter
and applied it to find_eligible_sacrifice_targets so Shoal-style sacrifice
costs are payable before X is known.
- handle_sacrifice_for_cost now binds chosen_x from the sacrificed
permanent's mana value when the sacrifice cost carries a pitch-bound X
filter, mirroring the existing exile-pitch binding.
Closes phase-rs#5957
![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 Shoal-style 'with mana value X' sacrifice costs (such as on Sidisi, Regent of the Mire) and arithmetic offsets in quantity expressions ('X plus N' / 'X minus N') in accordance with CR 107.1b, CR 107.3a, and CR 118.9. It renames and generalizes exile_cost_effective_filter to pitch_bound_x_effective_filter to make it zone-agnostic, handles binding chosen_x during sacrifice cost resolution, and updates the quantity parser to handle offsets. An integration test and card fixture for Sidisi, Regent of the Mire are also added. There are no review comments, so we have no feedback to provide.
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
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the selected-sacrifice X binding is annotated with rules that say the opposite.
🔴 Blocker
crates/engine/src/game/casting_costs.rs:2304-2309says the selected creature defines X, then citesCR 107.3a + CR 118.9. The verified text ofCR 107.3aapplies only when X isn't defined by the spell or ability text and the controller announces it;CR 118.9governs alternative costs for spells, not this activated-ability cost. Sidisi's sacrifice selection defines X, so this needsCR 107.3c; if documenting the activation sequence too,CR 602.2bis the applicable bridge to casting-cost steps. Please correct the annotation without changing the selected-object binding behavior.crates/engine/src/parser/oracle_nom/quantity.rs:571-576repeats the unsupportedCR 107.3aattribution (andCR 107.1b, which is about negative numbers) for genericX plus N/X minus Ngrammar. Remove that incorrect CR claim, or cite only a verified rule that the code actually implements; the runtime X-definition annotation belongs at the binding seam above.
✅ Clean
The current parse-diff artifact now accounts for exactly one intended Sidisi target-filter change, and the required Rust checks are green on this head.
Recommendation: correct the two annotations on this head, keeping the verified runtime behavior intact, then request re-review.
CR 107.3a (X announced by controller) and CR 118.9 (spell alternative costs) don't apply to the activated-ability sacrifice-cost seam where Sidisi's mana value X is defined by the ability's own text rather than announced — that's CR 107.3c, bridged to activation costs via CR 602.2b. Also drops the incorrect CR 107.1b/107.3a claim from the generic "X plus N"/"X minus N" quantity-grammar combinator, which implements no X-definition rule itself.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — this implements Sidisi with an impossible activation order.
🔴 Blocker
crates/engine/src/game/cost_payability.rs:187-193andcrates/engine/src/game/casting.rs:14963-14968call this a text-defined X and relax the mana-value-X cost filter. Sidisi’s Oracle text has an{X}{B}activation cost; it does not define X from the sacrificed creature. The locally verified CR 107.3a therefore requires the controller to choose and announce X. CR 107.3c is inapplicable. Do not make the sacrifice pick bindchosen_x.crates/engine/src/game/casting_costs.rs:2304-2312then binds X when the sacrifice is paid. CR 602.2b applies the 601.2b–i sequence to activation: announce X under 601.2b, choose the return target under 601.2c, then pay costs. A sacrifice cannot retrospectively determine a target’s legality.crates/engine/tests/integration/issue_5957_sidisi_regent_of_the_mire.rs:99-132encodes the same reversed sequence by selecting the sacrifice beforeTargetSelection. It would accept the rules error above.
✅ Clean
The current parse-diff is limited to Sidisi’s intended X + 1 target-filter change, and the parser arithmetic portion is independently scoped.
Recommendation: model announced X before target selection and cost payment, with a test that exercises that order; otherwise remove the unsupported Sidisi activation support.
|
Re-verified the two blockers in the latest review against Scryfall — both rest on a premise that doesn't match the printed card. Sidisi, Regent of the Mire's actual Oracle text (Scryfall, TDM #92, confirmed just now):
There is no That puts this squarely under CR 107.3c ("If ... the value of X is defined by the text of that spell or ability, then that's the value of X ... The controller ... doesn't get to choose the value"), not CR 107.3a (which only governs X's the controller announces). So On sequencing: CR 601.2b (bridged to activated abilities via CR 602.2b) directly addresses this exact shape —
That is, the rules themselves pull the X-defining choice (which creature to sacrifice) forward to before target selection (601.2c), precisely to avoid the "sacrifice retroactively determines target legality" problem raised in the review. So the engine presenting the sacrifice Given both blockers are built on a cost shape ( |
|
Correction to my prior review: I am withdrawing only the announced- I incorrectly treated this as a An implementation re-review is underway. This correction does not approve or enqueue the PR, and it withdraws no other finding. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the parser change still has no discriminating test.
🔴 Blocker
- [MED] The runtime regression at
crates/engine/tests/integration/issue_5957_sidisi_regent_of_the_mire.rs:32-40loads prebuiltQuantityExpr::Offsetcard data, so it remains green if the new parser arm atcrates/engine/src/parser/oracle_nom/quantity.rs:561-603is reverted. Evidence: the fixture is deserialized byCardDatabase::from_export, rather than parsing the Oracle text; the rest of the test only exercises the already-built ability. Why it matters: the claimedmana value X plus 1grammar can regress silently while this test continues to validate only runtime resolution. Suggested fix: add parser-level assertions that fresh Oracle text yieldsQuantityExpr::Offset { inner: Variable("X"), offset: 1 }formana value X plus 1, plus the correspondingminusthreshold form (and retain the existing runtime Sidisi test).
✅ Clean
The current head's text-defined X behavior is not part of this finding. The required checks are green, and the parse-diff is limited to Sidisi's intended target-filter change.
Recommendation: add the discriminating parser coverage on this head, then request re-review.
2 participants
Summary
Sidisi, Regent of the Mire (
{T}, Sacrifice a creature you control with mana value X other than Sidisi: Return target creature card with mana value X plus 1 from your graveyard to the battlefield. Activate only as a sorcery.)was silently misparsed and, separately, could not actually be activated.
plus 1when it hitparse_quantity_expr_number(the filter-threshold quantity parser), so the reanimation target's mana
value gate compiled to
== Xinstead of== X + 1— it would return acreature with the same mana value as the one sacrificed rather than one
greater.
with mana value X) is aShoal-style pitch-bound X: X is defined by whichever creature the player
chooses to sacrifice, not announced beforehand. That relaxation existed
only for exile-as-cost (
exile_cost_effective_filter), so sacrifice-costeligibility evaluated the filter with
Xunresolved (== 0) and found noeligible permanents — the ability could never be activated at all.
Changes
parse_quantity_expr_number(oracle_nom/quantity.rs) now composes anoptional
plus N/minus Ntail intoQuantityExpr::Offset, mirroringthe existing arithmetic arm in
oracle_util::parse_count_expr. This is ageneral filter-threshold fix (mana value, power, toughness, counters),
not a Sidisi-specific string match.
exile_cost_effective_filtertopitch_bound_x_effective_filterand applied it in
find_eligible_sacrifice_targets, so any Shoal-stylesacrifice cost (not just exile) is payable before X is known.
handle_sacrifice_for_costbindschosen_xfrom the sacrificedpermanent's own mana value when the cost filter is pitch-bound, mirroring
the existing exile-pitch binding used for alternative casting costs.
Test plan
crates/engine/tests/integration/issue_5957_sidisi_regent_of_the_mire.rsdrives the real parsed card through
ActivateAbility→ sacrifice →target selection → resolution, asserting the mana-value-3 graveyard
creature is offered as a legal target, the mana-value-2 one is not, and
the correct creature is reanimated.
cargo test -p engine --lib— 16746 passed, 0 failedcargo test -p engine --test integration— 3205 passed, 0 failed(includes the existing Shoal-pitch-exile coverage, unaffected by the
rename)
cargo clippy -p engine --all-targets --features cli -- -D warnings— cleancargo fmt --allCloses #5957