Fix Throne of Eldraine#6625
Conversation
github-actions
Bot
added
the
needs-maintainer
|
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 (37)
🚧 Files skipped from review as they are similar to previous changes (30)
📝 WalkthroughWalkthroughThis PR adds chosen-color mana riders, carries activation ability indexes through payment evaluation, extends mana restriction parsing to multiple restrictions, and updates mana eligibility, auto-tap, replacement payment, AI, and integration tests. ChangesChosen-color restriction parsing and contracts
Activation payment context and mana enforcement
Ability-index payment plumbing
Validation and integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)crates/engine/src/game/casting_tests.rsast-grep timed out on this file Comment |
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/engine/src/ai_support/candidates.rs (1)
2118-2141: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSilent fallback masks a should-never-happen state instead of failing loudly.
pending_cast.activation_ability_index.unwrap_or(usize::MAX)converts a missing index (which, forActivationCostOneOfChoice, should be structurally impossible) into "no color restriction" rather than an assertion/error. Ifactivation_ability_indexis ever unexpectedlyNonehere, a chosen-color-restricted activation would silently be treated as unrestricted instead of surfacing the bug. See the consolidated comment for the cross-file fix.🤖 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/ai_support/candidates.rs` around lines 2118 - 2141, Replace the silent unwrap_or fallback in the WaitingFor::ActivationCostOneOfChoice candidate generation with an assertion or equivalent fail-fast handling that requires pending_cast.activation_ability_index to be present. Pass the validated index to casting::can_pay_ability_cost_now so a missing activation ability index cannot be treated as unrestricted.crates/engine/src/ai_support/mod.rs (1)
272-287: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMirrors the
candidates.rsActivationCostOneOfChoicesentinel — same root cause.Same
.unwrap_or(usize::MAX)pattern gating the sameWaitingFor::ActivationCostOneOfChoicelegality check asai_support/candidates.rs. See the consolidated comment.🤖 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/ai_support/mod.rs` around lines 272 - 287, The WaitingFor::ActivationCostOneOfChoice legality check incorrectly uses usize::MAX as the fallback activation ability index. Update the pending_cast.activation_ability_index handling in this match arm to use the same valid fallback and behavior established by candidates.rs, while preserving the existing cost lookup and can_pay_ability_cost_now validation.crates/engine/src/game/keywords.rs (1)
776-786: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
usize::MAXused as an undocumented "no ability index" sentinel.Ninjutsu-family activation has no indexed ability, so
usize::MAXstands in for "not applicable." This works today only because the lookup this feeds into (activation_ability_definition) gracefully falls back toUnrestrictedon a miss — but nothing here documents thatusize::MAXis a deliberate sentinel rather than a real index. See the consolidated comment for the cross-file recommendation (Option<usize>propagation).🤖 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/keywords.rs` around lines 776 - 786, Update pay_ability_cost_for_activation and its activation_ability_definition lookup path to accept Option<usize> for the ability index, passing None from the ninjutsu-family activation call instead of usize::MAX. Preserve the existing Unrestricted fallback behavior when no indexed ability applies, and update all affected callers and signatures consistently.
🤖 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/costs.rs`:
- Around line 201-208: The activation ability index contract uses a magic
usize::MAX sentinel instead of representing the absent case explicitly. In
crates/engine/src/game/costs.rs:201-208, change
PaymentScope::Activation.ability_index to Option<usize>; in
crates/engine/src/game/costs.rs:570-617, update pay_ability_cost_for_activation
and pay_ability_cost_for_activation_with_cost_move_replacement to accept and
propagate Option<usize>. Update all callers to pass None for unindexed
Ninjutsu-family activations and the actual Option value (or an explicit
invariant assertion) for stamped activation choices, including the affected
paths in game/keywords.rs:776-786, ai_support/candidates.rs:2118-2141 and
4033-4047, and ai_support/mod.rs:272-287.
In `@crates/engine/tests/integration/throne_of_eldraine_mana_riders.rs`:
- Around line 197-210: Update the manual-pin/resume test to reach the
mana-payment state through the production GameAction::ActivateAbility path in
manual payment mode, rather than constructing PendingCast and
activation_ability_index directly. Verify the resulting
pending_cast.activation_ability_index is Some(draw) before exercising pin
rejection, while preserving the existing tapped-state and failure-path
assertions.
---
Outside diff comments:
In `@crates/engine/src/ai_support/candidates.rs`:
- Around line 2118-2141: Replace the silent unwrap_or fallback in the
WaitingFor::ActivationCostOneOfChoice candidate generation with an assertion or
equivalent fail-fast handling that requires
pending_cast.activation_ability_index to be present. Pass the validated index to
casting::can_pay_ability_cost_now so a missing activation ability index cannot
be treated as unrestricted.
In `@crates/engine/src/ai_support/mod.rs`:
- Around line 272-287: The WaitingFor::ActivationCostOneOfChoice legality check
incorrectly uses usize::MAX as the fallback activation ability index. Update the
pending_cast.activation_ability_index handling in this match arm to use the same
valid fallback and behavior established by candidates.rs, while preserving the
existing cost lookup and can_pay_ability_cost_now validation.
In `@crates/engine/src/game/keywords.rs`:
- Around line 776-786: Update pay_ability_cost_for_activation and its
activation_ability_definition lookup path to accept Option<usize> for the
ability index, passing None from the ninjutsu-family activation call instead of
usize::MAX. Preserve the existing Unrestricted fallback behavior when no indexed
ability applies, and update all affected callers and signatures consistently.
🪄 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: 74ea3e82-3984-4e71-a310-cf1dbd2948ca
📒 Files selected for processing (35)
crates/engine/src/ai_support/candidates.rscrates/engine/src/ai_support/mod.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/cost_payability.rscrates/engine/src/game/costs.rscrates/engine/src/game/effects/mana.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/keywords.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_payment.rscrates/engine/src/game/mana_sources.rscrates/engine/src/game/planeswalker.rscrates/engine/src/game/replacement.rscrates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_effect/mana.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/types/ability.rscrates/engine/src/types/mana.rscrates/engine/tests/integration/companion_special_action.rscrates/engine/tests/integration/issue_2862_teferi_loyalty.rscrates/engine/tests/integration/issue_4220_agatha_soul_cauldron.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/restricted_mana_face_down_and_face_up.rscrates/engine/tests/integration/restricted_mana_mv_or_x.rscrates/engine/tests/integration/restricted_mana_x_cost_only.rscrates/engine/tests/integration/throne_of_eldraine_mana_riders.rscrates/phase-ai/src/policies/land_animation.rscrates/server-core/src/game_action_payload_guard.rs
| let mut pending = PendingCast::new( | ||
| throne, | ||
| CardId(0xED), | ||
| ResolvedAbility::new((*draw_ability.effect).clone(), Vec::new(), throne, P0), | ||
| ManaCost::generic(3), | ||
| ); | ||
| pending.activation_ability_index = Some(draw); | ||
| state.pending_cast = Some(Box::new(pending)); | ||
| state.objects.get_mut(&throne).unwrap().tapped = true; | ||
| state.waiting_for = WaitingFor::ManaPayment { | ||
| player: P0, | ||
| convoke_mode: None, | ||
| }; | ||
| assert!(matches!(state.waiting_for, WaitingFor::ManaPayment { .. })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
The manual-pin section hand-seeds the exact state the fix is supposed to produce.
PendingCast::new(...) + pending.activation_ability_index = Some(draw) + state.waiting_for = ManaPayment + tapped = true construct post-activation state directly. handle_spend_pool_mana derives its PaymentContext::Activation from pending.activation_ability_index, so if the production activation path ever stopped populating that field (or populated a wrong index), this assertion pair at Lines 212-220 would still pass — the pin rejection is proven only against a fixture-supplied index, not against what ActivateAbility actually writes.
The auto path above covers production wiring, so the rider isn't unguarded overall; the gap is specifically the manual pin/resume branch. Reaching WaitingFor::ManaPayment for an activation through a real GameAction::ActivateAbility (manual payment mode) would close it. Failing that, at minimum assert the production-populated value first — activate through the runner, then read back state.pending_cast.activation_ability_index == Some(draw) before pinning.
As per path instructions, "A test must exercise the FAILURE path the fix prevents and drive the engine through its production pipeline" and "Flag constructor shortcuts ... builders that bypass production wiring, setUp that pre-populates post-fix state) that can silently mask the very bug a regression test claims to catch."
🤖 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/tests/integration/throne_of_eldraine_mana_riders.rs` around
lines 197 - 210, Update the manual-pin/resume test to reach the mana-payment
state through the production GameAction::ActivateAbility path in manual payment
mode, rather than constructing PendingCast and activation_ability_index
directly. Verify the resulting pending_cast.activation_ability_index is
Some(draw) before exercising pin rejection, while preserving the existing
tapped-state and failure-path assertions.
Source: Path instructions
matthewevans
removed
the
needs-maintainer
github-actions
Bot
added
the
needs-maintainer
6587acc to
9bc52dc
Compare
2 participants
Summary
Fixes engine support for Throne of Eldraine's chosen-color mana restrictions.
Files changed
CR references
Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: gpt-5.6
Thinking: high
Tier: Frontier
Verification
Required checks ran clean, or exact unresolved local failures are stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— PASSgit diff --check— PASSCARGO_TARGET_DIR=/tmp/phase-throne-verify cargo test -p engine --test integration throne_of_eldraine_mana_riders— PASS (3 tests)./scripts/check-parser-combinators.sh— PASScargo clippy-strict,cargo test -p engine,./scripts/gen-card-data.sh,cargo coverage, andcargo semantic-audit— not completed after two local attempts because the shared Cargo build directory remained locked; CI owns the remaining checks.Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=3d772f6a671b78ee09de68c67bb9f021ca210aa7 base=6895ca39a2b1017ceec96c25fb8141d0ee6847a7
Anchored on
tag()dispatch for spend-restriction clauses.ManaRestrictionspell-payment authority.Final review-impl
Final review-impl PASS head=3d772f6a671b78ee09de68c67bb9f021ca210aa7
Claimed parse impact
Validation Failures
None.
CI Failures
Local Cargo commands that require the shared build directory were blocked by its persistent lock after two attempts. The focused integration regression suite passed in an isolated target directory; CI must run the remaining full suite and card-data audits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests