fix(engine): verify offered casts with the auto-payment authority - #7007
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAutomatic casting now derives payment modes from effective costs, validates pending-cast payment feasibility, preserves payment choices through announcements, and records legality performance phases. Regression tests cover interactive costs, alternative casts, sacrificial mana, Morph, splice, and casting permissions. ChangesAutomatic casting flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CandidateGeneration
participant SimulationFilter
participant Casting
participant ManaPayment
CandidateGeneration->>Casting: prepare effective spell variant
Casting->>ManaPayment: probe payment mode and affordability
ManaPayment-->>Casting: automatic or preserved-source payment mode
Casting->>SimulationFilter: apply candidate and inspect pending cast
SimulationFilter->>Casting: validate pending root and post-origin payment
Casting-->>CandidateGeneration: accepted cast announcement
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Final current-head review: independent adversarial review found no issues; all PR review threads are resolved. Final CI is running on d4c025f.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Current-head blocker — commander-gated free casts are being filtered out
Reviewed b9f7fd0be38827b46897234fa4854ee15a13e66f. The current parse diff is valid and reports no card-parse changes; the remaining issue is runtime behavior.
Both terminal Rust shards fail the production positive reach-guards at crates/engine/tests/integration/tchaka_venerable_king.rs:330 and :363: legal_actions does not offer the commander-gated free cast for either an owned or a stolen commander. These are real action-surface failures, not test-only mismatches.
crates/engine/src/ai_support/filter.rs:215-251 applies the ordinary post-origin payment verdict after its OptionalCostChoice bypass. Trace CastFromHandFree candidate generation and its fallback end to end, then preserve every free-permission state through the filter—not only the optional-cost and already-prepared NoCost shapes. Add or retain discriminating production-path coverage for each representation.
Auto-merge has been disabled. Do not re-enable it or request approval until this behavior is fixed and fresh current-head CI is green.
Maintainer hold — current head
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Final fixed head dd859e3: all required CI checks pass and no review threads remain unresolved.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Current-head blocker — production free-cast regression removed
Reviewed dd859e380906dca845708d9b60a0c21d6bdbbc98.
This head deletes the only production legal_actions regression for parser-backed Deadly Rollick's commander-conditional free-cast option from crates/engine/tests/integration/tchaka_venerable_king.rs. The deleted coverage proves the own-commander and stolen-commander Any positives, plus the no-commander negative. Existing unit tests exercise direct resolution, not offer-side candidate generation.
ai_support/filter.rs:226-232 accepts the static CastSpellForFree surface, but does not cover the self casting-option represented by ordinary CastSpell. CI is green because this regression was removed; it does not establish that the offer path remains correct.
Restore the parser-backed exact-action regression (positive own/stolen Any commander and negative no-commander cases), or fix candidate generation and retain equivalent production coverage. Do not re-enable auto-merge until this is resolved.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current head 39741686ba6803f794dd0d4611a678cd02c96877
[HIGH] Regular CastSpell candidate filtering still drops the commander-gated free-cast offer. Evidence: the production action-surface regression is at crates/engine/tests/integration/tchaka_venerable_king.rs:272-287; it asks legal_actions for an ordinary GameAction::CastSpell and requires an empty-mana-pool Deadly Rollick offer when its controller controls a commander. Required Rust shard 1 failed this exact assertion at :284 on merge head 6141009f6a3c0dcb85f2580dc64d6c24db95ec0f (head 39741686ba6803f794dd0d4611a678cd02c96877 merged with current base).
Why it matters: ordinary simulation/candidate filtering treats this free permission as unpayable and removes a legal cast from the player’s available actions. The own-commander positive is therefore broken; the adjacent stolen-commander positive at tchaka_venerable_king.rs:296+ remains essential coverage for the same Any commander you control condition.
Suggested fix: trace the ordinary CastSpell path through candidate generation and the post-origin payment verdict, then preserve the commander-conditioned CastSpellForFree/free-permission representation rather than only already-specialized free actions. Keep the existing own/stolen positive and no-commander negative action-surface regression tests, and provide fresh green current-head Rust CI.
The current required CI is not clean (both Rust test shards and the combined Rust check are failing). The parse-diff sticky now correctly names this head and reports no parse changes; it does not establish runtime casting correctness.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved after restoring the parser-backed Deadly Rollick ordinary-CastSpell regression and fixing its shared alternative-cost simulation path. Current head a1750ad810cfb40523a73a1e93ac94fc715a69de has all required CI checks green, including both Rust test shards, card data, paired-seed AI, and decision-cost performance gates.
|
@nishu-builder huge PR! thank you for the contribution :) |
|
You're so welcome! I love your project |
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
New Features
Bug Fixes
Performance