fix(engine): verify offered casts with the auto-payment authority - #7007
fix(engine): verify offered casts with the auto-payment authority#7007nishu-builder wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAutomatic casting now computes effective costs and payment modes for priority, Sneak, and Web Slinging candidates. Validation handles target-dependent costs, interactive payment states, pending-cast provenance, and atomic mana affordability. The change adds legality instrumentation and broad regression coverage. ChangesAutomatic casting flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CandidateGeneration
participant SimulationFilter
participant Casting
participant ManaPayment
participant GameState
CandidateGeneration->>Casting: prepare effective spell cost
Casting->>ManaPayment: probe payment mode and feasibility
ManaPayment->>GameState: simulate taps and payment choices
ManaPayment-->>Casting: payment mode or rejection
Casting->>SimulationFilter: apply candidate and inspect pending root
SimulationFilter-->>CandidateGeneration: retain or reject candidate
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/ai_support/candidates.rs (1)
3616-3638: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCompute sacrificial payment mode per candidate spell.
cast_payment_modeis computed before the spell loop, so everyCastSpellcandidate inherits a board-levelAutoExceptSacrificialManaeven when its final cost needs no mana. A spell with costNoCostor floating mana already fully payable is then sent toenter_payment_step;finalize_automatic_mana_paymentis gated out byAutoExceptSacrificialMana, while the sacrificial source list has already been excluded. Derive the mode after each spell's payment/cost is established, and keep the modeAutofor cases that do not require sacrificial payment.Also move
activatable_mana_source_selectionsbehind thespell_objects_available_to_castcheck, and align the free/mana-pay alternatives (CastSpellForFree,CastSpellAsSneak,CastSpellAsWebSlinging) so mana-paying alternatives do not remainCastPaymentMode::Autowhen every available source requires sacrifice.🤖 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 3616 - 3638, Update the candidate-generation flow around `spell_objects_available_to_cast`, `CastPaymentMode`, and the `CastSpell`/`CastSpellForFree`/`CastSpellAsSneak`/`CastSpellAsWebSlinging` actions so payment mode is computed per candidate after its final cost or payment alternative is established. Move `activatable_mana_source_selections` behind the available-spell check, use `AutoExceptSacrificialMana` only when the candidate actually requires mana and every available source is sacrificial, and retain `Auto` for free, `NoCost`, or already fully payable candidates. Apply the same mode selection to mana-paying alternatives so they do not remain unconditionally `Auto`.
🧹 Nitpick comments (7)
crates/engine/tests/integration/offer_side_auto_payment.rs (1)
502-509: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe tolerant
OptionalEffectChoicebranch can hide a flow regression.The fixture ability is built with
.optional(). The production flow must therefore presentWaitingFor::OptionalEffectChoicebeforeEffectZoneChoice. The currentif matches!(...)accepts both outcomes. If the engine stops offering the optional choice, this fixture keeps passing and the two Face-of-Boe tests still report green on a changed pipeline.Assert the intermediate state instead of tolerating its absence.
♻️ Proposed change
- if matches!( - runner.state().waiting_for, - WaitingFor::OptionalEffectChoice { .. } - ) { - runner - .act(GameAction::DecideOptionalEffect { accept: true }) - .expect("the production 'you may cast' choice must be accepted"); - } + assert!( + matches!( + runner.state().waiting_for, + WaitingFor::OptionalEffectChoice { .. } + ), + "the optional CastFromZone effect must present its 'you may cast' choice, got {:?}", + runner.state().waiting_for + ); + runner + .act(GameAction::DecideOptionalEffect { accept: true }) + .expect("the production 'you may cast' choice must be accepted");🤖 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/offer_side_auto_payment.rs` around lines 502 - 509, Replace the conditional `OptionalEffectChoice` handling in the test flow with an unconditional assertion that `runner.state().waiting_for` is `WaitingFor::OptionalEffectChoice` before dispatching `GameAction::DecideOptionalEffect { accept: true }`. Preserve the existing expectation message and action result handling so the fixture fails if the optional choice is skipped.Source: Path instructions
crates/engine/src/ai_support/filter.rs (1)
188-188: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDrop the discarded
PendingCastclone in thebeforeread.
pending_spell_rootalways clones thePendingCast. Line 188 uses only the provenance and discards the clone.PendingCastowns a boxedResolvedAbilityand severalVecfields, so this is a deep clone on everyfallback_simulationcall.Split the provenance read from the clone. The
afterpath still needs an ownedPendingCast, becausepost_origin_auto_payment_verdicttakes&mut sim.♻️ Proposed change
- let before = pending_spell_root(state).map(|(provenance, _)| provenance); + let before = pending_spell_root_provenance(state);fn pending_spell_root_ref(state: &GameState) -> Option<&PendingCast> { state .waiting_for .pending_cast_ref() .or(state.pending_cast.as_deref()) .filter(|pending| pending.activation_ability_index.is_none()) } fn pending_spell_root_provenance(state: &GameState) -> Option<SpellRootProvenance> { pending_spell_root_ref(state) .map(|pending| (pending.object_id, pending.casting_permission_index)) } fn pending_spell_root(state: &GameState) -> Option<(SpellRootProvenance, PendingCast)> { pending_spell_root_ref(state).map(|pending| { ( (pending.object_id, pending.casting_permission_index), pending.clone(), ) }) }Also applies to: 236-248
🤖 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/filter.rs` at line 188, Avoid cloning PendingCast in the before provenance read within fallback_simulation. Add or reuse a borrowed pending-spell-root helper and a provenance-only helper, then update the before path to use the borrowed provenance result while retaining pending_spell_root’s owned clone for the after path and post_origin_auto_payment_verdict.crates/engine/src/game/casting.rs (1)
13869-13884: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce the Assist search from a linear scan to one probe per helper.
The loop tests every contribution in
1..=genericagainst every candidate. Each iteration runs twocan_feasibly_pay_mana_cost_with_probecalls, and the caster-side call is unprobed for the helper, so the cost isO(generic × candidates)payment simulations. For an{X}spell with a large chosenXin a four-player game this runs on the candidate-generation path.Both predicates are monotone in
contribution: a helper that can payngeneric can payn-1, and the caster's residualgeneric - contributiononly shrinks ascontributiongrows. Find each helper's maximum payable generic amount once, then test the caster once at that amount.🤖 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 13869 - 13884, Replace the nested contribution scan in the Assist payment logic with one calculation per candidate helper that finds its maximum payable generic contribution, then perform a single caster feasibility probe using that contribution and the corresponding residual generic cost. Preserve the existing shard handling, source ID, and probe arguments, while retaining the monotonic behavior that accepts a helper whenever its maximum contribution leaves a caster-payable remainder.crates/engine/src/game/casting_costs.rs (1)
12107-12111: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDrop the cloned
PendingCaston the auto-finalization path.
eligible_tap_payment_mode,choice_free_auto_payment_verdict, andcan_pay_cost_after_auto_tapall usestate.pending_castimmutably, andfinalize_automatic_mana_paymentruns only after those reads complete. Usingas_deref()avoids cloning the boxedPendingCastbefore entering payment.🤖 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 12107 - 12111, Update the pending-cast access in the auto-finalization path to use an immutable dereference via as_deref() instead of cloning through map and as_ref. Keep the existing control flow and downstream calls to eligible_tap_payment_mode, choice_free_auto_payment_verdict, can_pay_cost_after_auto_tap, and finalize_automatic_mana_payment unchanged.crates/engine/src/game/perf_counters.rs (2)
210-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the phase gating of the two post-apply counters.
record_post_apply_uncached_source_collectionincrements only duringLegalityClonePhase::PostApplyCore.record_post_apply_auto_payment_core_callincrements unconditionally. A post-apply payment check that runs outside any legality phase therefore raisespost_apply_auto_payment_core_callswithout raisingpost_apply_uncached_source_collections. That breaks the one-to-one pairing thatoffer_side_auto_payment_phase_accounting_has_exact_clone_ownershipasserts incrates/engine/src/ai_support/mod.rs(both expected to equalN). Gate both counters the same way, or document why the call counter is phase-independent.🤖 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/perf_counters.rs` around lines 210 - 218, Update record_post_apply_auto_payment_core_call to use the same LEGALITY_CLONE_PHASE == Some(LegalityClonePhase::PostApplyCore) gating as record_post_apply_uncached_source_collection, preserving the one-to-one counter pairing expected by offer_side_auto_payment_phase_accounting_has_exact_clone_ownership.
146-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one phase mapping for both clone recorders.
record_mana_readiness_state_clonerepeats the whole phase-to-field mapping ofrecord_phase_owned_state_clone. The only difference is the extrastrict_fast_path_mana_readiness_state_clonesincrement. Two copies of the mapping must stay in lockstep whenever a phase is added or a field is renamed.♻️ Proposed consolidation
pub(crate) fn record_mana_readiness_state_clone() { - let phase = LEGALITY_CLONE_PHASE.with(Cell::get); - with_mut(|snapshot| match phase { - Some(LegalityClonePhase::Generation) => snapshot.generation_state_clones += 1, - Some(LegalityClonePhase::StrictFastPath) => { - snapshot.strict_fast_path_state_clones += 1; - snapshot.strict_fast_path_mana_readiness_state_clones += 1; - } - Some(LegalityClonePhase::RawValidation) => { - snapshot.raw_validation_state_clones += 1; - } - Some(LegalityClonePhase::GroupedManaReadiness) => { - snapshot.grouped_mana_readiness_state_clones += 1; - } - Some(LegalityClonePhase::PostApplyCore) => { - snapshot.post_apply_auto_payment_core_state_clones += 1; - } - None => {} - }); + record_phase_owned_state_clone(); + if LEGALITY_CLONE_PHASE.with(Cell::get) == Some(LegalityClonePhase::StrictFastPath) { + with_mut(|snapshot| snapshot.strict_fast_path_mana_readiness_state_clones += 1); + } }🤖 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/perf_counters.rs` around lines 146 - 181, Consolidate the duplicated phase-to-counter mapping in record_phase_owned_state_clone and record_mana_readiness_state_clone by reusing one shared helper or recorder. Preserve the existing per-phase state-clone increments, and keep the additional strict_fast_path_mana_readiness_state_clones increment exclusive to record_mana_readiness_state_clone.crates/engine/src/ai_support/mod.rs (1)
6089-6095: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the two extra clones in the total assertion.
The total combines five named phase counters plus a literal
2. One unit is the priority-cast probe clone, which the sum already includes throughpriority_cast_probe_state_clones, so the origin of the literal is not derivable from the assertion. State each remaining owner as a named term or add a comment. A failure of this assertion is otherwise hard to attribute.🤖 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 6089 - 6095, Update the total assertion near the counters aggregation to replace the unexplained literal 2 with named clone-owner terms or an adjacent comment identifying both extra clones. Preserve the existing priority_cast_probe_state_clones contribution and make the assertion explicitly attribute each remaining unit to its owning phase.
🤖 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_tests.rs`:
- Around line 17045-17050: The test setup currently gives the spell ordinary
green mana, so it does not exercise Defiler-only affordability. Remove the added
green ManaUnit while preserving at least 2 life, then assert both
candidate_actions and legal_actions_full include the cast and that
apply_as_current transitions to WaitingFor::DefilerPayment.
In `@crates/engine/src/game/engine.rs`:
- Around line 13889-13894: Strengthen the test around candidate_actions and
legal_actions by adding a legal earlier CastSpell fixture, then assert each API
includes that exact action before retaining the PlayFaceDown absence assertions.
This positive reach-guard must prove both exact-action APIs produced the
expected available action rather than passing on empty results.
In `@crates/engine/src/game/mana_payment.rs`:
- Around line 2666-2681: Update the final fallback test block to also assert
that the same fallback_pool and fallback_cost are accepted by can_pay_for_spell,
using its existing context and arguments with hand_demand set to None. Keep the
direct select_mana_payment assertion, so the test covers both the atomic
selector and the can_pay_for_spell delegation path.
In `@crates/engine/src/game/splice_tests.rs`:
- Around line 229-238: Extend the assertions in the WaitingFor::SpliceOffer
match to verify that pending_cast retains CastPaymentMode::Auto. Inspect the
pending_cast payment-mode field and assert the Auto variant, while preserving
the existing object_id and eligible assertions so the test fails if begin_offer
drops or replaces the mode.
In `@crates/engine/tests/integration/offer_side_auto_payment.rs`:
- Around line 339-341: Add a suite-level prerequisite check for the shared card
fixture/full export used by setup_prepared_copy and setup_face_of_boe, and fail
the test suite when that data is unavailable instead of allowing dependent tests
to return early. Keep the existing test execution paths unchanged when the card
data is present.
---
Outside diff comments:
In `@crates/engine/src/ai_support/candidates.rs`:
- Around line 3616-3638: Update the candidate-generation flow around
`spell_objects_available_to_cast`, `CastPaymentMode`, and the
`CastSpell`/`CastSpellForFree`/`CastSpellAsSneak`/`CastSpellAsWebSlinging`
actions so payment mode is computed per candidate after its final cost or
payment alternative is established. Move `activatable_mana_source_selections`
behind the available-spell check, use `AutoExceptSacrificialMana` only when the
candidate actually requires mana and every available source is sacrificial, and
retain `Auto` for free, `NoCost`, or already fully payable candidates. Apply the
same mode selection to mana-paying alternatives so they do not remain
unconditionally `Auto`.
---
Nitpick comments:
In `@crates/engine/src/ai_support/filter.rs`:
- Line 188: Avoid cloning PendingCast in the before provenance read within
fallback_simulation. Add or reuse a borrowed pending-spell-root helper and a
provenance-only helper, then update the before path to use the borrowed
provenance result while retaining pending_spell_root’s owned clone for the after
path and post_origin_auto_payment_verdict.
In `@crates/engine/src/ai_support/mod.rs`:
- Around line 6089-6095: Update the total assertion near the counters
aggregation to replace the unexplained literal 2 with named clone-owner terms or
an adjacent comment identifying both extra clones. Preserve the existing
priority_cast_probe_state_clones contribution and make the assertion explicitly
attribute each remaining unit to its owning phase.
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 12107-12111: Update the pending-cast access in the
auto-finalization path to use an immutable dereference via as_deref() instead of
cloning through map and as_ref. Keep the existing control flow and downstream
calls to eligible_tap_payment_mode, choice_free_auto_payment_verdict,
can_pay_cost_after_auto_tap, and finalize_automatic_mana_payment unchanged.
In `@crates/engine/src/game/casting.rs`:
- Around line 13869-13884: Replace the nested contribution scan in the Assist
payment logic with one calculation per candidate helper that finds its maximum
payable generic contribution, then perform a single caster feasibility probe
using that contribution and the corresponding residual generic cost. Preserve
the existing shard handling, source ID, and probe arguments, while retaining the
monotonic behavior that accepts a helper whenever its maximum contribution
leaves a caster-payable remainder.
In `@crates/engine/src/game/perf_counters.rs`:
- Around line 210-218: Update record_post_apply_auto_payment_core_call to use
the same LEGALITY_CLONE_PHASE == Some(LegalityClonePhase::PostApplyCore) gating
as record_post_apply_uncached_source_collection, preserving the one-to-one
counter pairing expected by
offer_side_auto_payment_phase_accounting_has_exact_clone_ownership.
- Around line 146-181: Consolidate the duplicated phase-to-counter mapping in
record_phase_owned_state_clone and record_mana_readiness_state_clone by reusing
one shared helper or recorder. Preserve the existing per-phase state-clone
increments, and keep the additional strict_fast_path_mana_readiness_state_clones
increment exclusive to record_mana_readiness_state_clone.
In `@crates/engine/tests/integration/offer_side_auto_payment.rs`:
- Around line 502-509: Replace the conditional `OptionalEffectChoice` handling
in the test flow with an unconditional assertion that
`runner.state().waiting_for` is `WaitingFor::OptionalEffectChoice` before
dispatching `GameAction::DecideOptionalEffect { accept: true }`. Preserve the
existing expectation message and action result handling so the fixture fails if
the optional choice is skipped.
🪄 Autofix
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: ca022485-4cc0-485f-ac0e-c2a40d1940cf
📒 Files selected for processing (13)
crates/engine/src/ai_support/candidates.rscrates/engine/src/ai_support/filter.rscrates/engine/src/ai_support/mod.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/engine.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_payment.rscrates/engine/src/game/perf_counters.rscrates/engine/src/game/splice_tests.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/offer_side_auto_payment.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — special-action cost generation must preserve manual mana-payment paths.
🔴 Blocker
[HIGH] Sneak and Web-slinging omit legal special actions when their alternate cost can only be paid through a sacrificial/manual mana ability. Evidence: crates/phase-ai/src/policies/candidates.rs:3616-3642 grants AutoExceptSacrificialMana only to ordinary CastSpell, while the Sneak and Web-slinging emitters at :4234-4249 and :4289-4304 force/filter Auto; crates/phase-ai/src/casting_costs.rs:8660-8676 supports manual ability payment when automatic payment cannot finish, and :11846-11859 makes only the Auto choice-free verdict final. Why it matters: the upstream candidates are removed before the existing manual-payment authority can expose a legal choice, so the AI cannot take legal Sneak or Web-slinging actions from a sacrificial-source-only mana position. Suggested fix: derive payment mode/feasibility after the alternate cost is known and route Sneak and Web-slinging through the same choice-preserving authority as ordinary casts; add distinct sacrificial-source-only regressions for each action.
Recommendation: request changes.
|
All five review comments addressed in b0e7db4 — every one confirmed against the code, the outside-diff Major with a scope correction:
Additionally self-caught during this round: four tests were silently skipping when generated card data was absent; the suite now asserts its prerequisite instead of vacuously passing. Verification on head Model: gpt-5.6-sol |
|
Current-head hold: GitHub reports head |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/ai_support/candidates.rs (1)
3512-3530: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct CR citation to this closure.
payment_mode_for_costimplements the CR 601.2g-h sacrifice-mana classification, but the citation for this exact behavior appears only at the usage site around Line 3633, not here. A reader who starts at the closure definition sees no rules citation.Add the citation directly above the closure so the rule is visible at the point of implementation, not only at the point of use.
As per path instructions:
crates/engine/**requires that "rules-touching code with no verifiedCR <number>: <description>annotation" be flagged.🤖 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 3512 - 3530, Add a verified “CR 601.2g-h” citation describing the sacrifice-mana classification directly above the payment_mode_for_cost closure. Keep the existing closure logic unchanged and ensure the annotation is visible at its definition.Source: Path instructions
🤖 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.
Nitpick comments:
In `@crates/engine/src/ai_support/candidates.rs`:
- Around line 3512-3530: Add a verified “CR 601.2g-h” citation describing the
sacrifice-mana classification directly above the payment_mode_for_cost closure.
Keep the existing closure logic unchanged and ensure the annotation is visible
at its definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 671ea99a-44e4-4401-a72c-10c4f9e5cdfe
📒 Files selected for processing (10)
crates/engine/data/mtgjson-vintagecrates/engine/src/ai_support/candidates.rscrates/engine/src/ai_support/mod.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/engine.rscrates/engine/src/game/mana_payment.rscrates/engine/src/game/splice_tests.rscrates/engine/tests/integration/offer_side_auto_payment.rs
💤 Files with no reviewable changes (1)
- crates/engine/src/game/casting_tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/engine/src/game/splice_tests.rs
- crates/engine/src/game/mana_payment.rs
- crates/engine/src/game/engine.rs
- crates/engine/tests/integration/offer_side_auto_payment.rs
- crates/engine/src/game/casting.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.rs`:
- Around line 15136-15143: Update the CastPaymentMode classification around the
mana_source_selections predicate to consider each source’s ability to
participate in a feasible payment for cost, rather than merely checking all
activatable sources. Preserve AutoExceptSacrificialMana when only sacrificial
sources can pay, and select Auto when a non-sacrificial source can contribute;
apply this consistently to regular, Sneak, and Web-slinging announcements and
keep validation and execution aligned through the authoritative engine payment
path.
🪄 Autofix
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: 7e55b9b0-9fbf-4dcd-8f01-c5595b5f0597
📒 Files selected for processing (4)
crates/engine/src/ai_support/candidates.rscrates/engine/src/game/casting.rscrates/engine/src/game/engine.rscrates/engine/tests/integration/offer_side_auto_payment.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/ai_support/candidates.rs
|
Current-head hold — maintainer port required. GitHub reports head This is maintainer-side concurrency, not a contributor rebase request: main's #6977 priority legality fast path overlaps |
|
The blocker is addressed across two commits ( Sneak / Web-slinging manual mana paths [HIGH] — both emitters now derive payment mode and feasibility after the alternate cost is known and route through the same choice-preserving verdict as ordinary casts (no per-emitter special-casing). From a sacrificial-source-only position, both special actions are offered and completable through the manual payment path; genuinely unpayable ones remain hidden. Four new regressions (offered+completable and unpayable-negative, per action). Offer-side module: 22/22. Follow-up hardening surfaced by the final fresh-context review ( Verification on head Model: gpt-5.6-sol |
|
Current-head hold — maintainer port still required. I rechecked
This is maintainer-caused staleness: the merge base is |
|
Correction to the current-head hold: the full |
matthewevans
removed
the
needs-maintainer
2 participants
Summary
Enforces the exact-legal-action contract at the offer seam: a
CastSpell { payment_mode: Auto }(or any action that synthesizes an Auto pending cast) is only offered when a complete Auto payment exists, verified with the same authority Auto payment itself uses — no parallel approximation. Found by an external legal-action-fuzzing harness: the engine offered casts counting mana sources Auto payment cannot actually use (an interactive {T}+exile mana ability), andCastPreparedCopy(no payment-mode field) skipped the completion preview entirely, so both shapes failed at commit time with "Cannot pay mana cost" after targeting. The fix routes an exhaustive cast-origin matrix (including cast-during-resolution zone picks, morph/PlayFaceDown, and the miracle reveal/cast-offer split) through a shared payment preview on the already-disposable post-apply scratch state, deferring to the live gate for unresolved payment-affecting choices (Harmonize, Assist) so legal interactive-affordability casts remain offered.Files changed
CR references
Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: gpt-5.6-sol
Thinking: high
Tier: Frontier
Verification
Required checks ran clean, or the exact CI-owned alternative is 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.
tilt get uiresource clippy— Tilt unavailable in this worktree; used the documented direct fallback.cargo fmt --all/cargo fmt --all -- --check— passed.git diff --check— passed.cargo clippy -p phase-engine --all-targets -- -D warnings— passed on head744622c179da157159017f13841d7efc34b4b7e6.cargo test -p phase-engine— passed on head744622c179da157159017f13841d7efc34b4b7e6: 18,486 unit + 4,491 integration (plus subsequent fix-round additions), 0 failed.Plan verification matrix — 37+ targeted invocations passed, including: interactive-mana negative/positive pair, prepared-copy negative/payable pair, Harmonize-only affordability stays offered, EffectZoneChoice cast-during-resolution negative/payable pair, split PlayFaceDown proofs, hostile irrelevant-cost-static fixture, hostile composed
Or/Notfilter fixtures, and the performance-counter regression (exact clone/collection equalities; zero additional whole-state clones on the offer path).Revert probes — interactive-mana and prepared-copy discriminating regressions each fail with the production change reverted and pass restored (transcripts in the pipeline artifacts).
./scripts/gen-card-data.sh— passed on head744622c179da157159017f13841d7efc34b4b7e6: generated card data for ~35684 cards.cargo coverage— passed on head744622c179da157159017f13841d7efc34b4b7e6: timeless legal 15124/16180 fully supported (93.5%); vintage legal 29841/32268 fully supported (92.5%).cargo semantic-audit— passed on head744622c179da157159017f13841d7efc34b4b7e6: 32732 cards audited, 297 existing findings.Gate A
Gate A PASS head=744622c179da157159017f13841d7efc34b4b7e6 base=6d7821dced9623609edea342b47dd9c704ff0b36
Anchored on
reduce_cost_by_poolscratch-pool dry run (PR fix(engine): make mana payments atomic #5793 heritage) — the same simulate-without-mutating discipline the offer preview extends to whole payments.can_pay_cost_after_auto_tap_with_probepayment authority; the offer-side preview calls this shared authority rather than approximating it.Final review-impl
Final review-impl PASS head=744622c179da157159017f13841d7efc34b4b7e6
Claimed parse impact
None.
Validation Failures
Contributor-environment note per the engine-implementer skill: pipeline steps ran as isolated fresh contexts (Codex CLI sessions) with artifact-only handoffs rather than spawned Claude subagents. Plan review: 5 rounds to clean (3 → 3 → 2 → 2 → 0 findings), including one executor STOP_AND_RETURN that identified a logically unconstructible fixture specification (single-fixture PlayFaceDown proof vs. first-wins preflight family ordering), resolved by splitting the proof obligations. Implementation review: 3 rounds to clean (2 → 1 → 0), tightening target-sensitive static handling to production applicability gates and composed-filter analysis to a three-state classification.
CI Failures
None.
Related
Independent of #6989 and #6997 (serialization fixes) from the same contributor; no overlapping concerns.
Summary by CodeRabbit
Bug Fixes
Improvements