fix(ai): reject dominated Colorless convoke-family taps during ManaPayment - #6840
Conversation
…yment
Metallic Rebuke's {2}{U} Improvise cast could dead-end: mana_payment_actions
offers a Colorless TapForConvoke marker for every Improvise/Waterbend/Convoke
eligible permanent, with nothing preferring a dual-purpose permanent's native
colored mana ability while a colored pip is still outstanding. Tapping such a
permanent via the Colorless marker first permanently strands the pip, and the
AI dead-ends in ManaPayment with no legal completion, hitting
fallback_action's debug_assert (a release panic) via search.rs's documented
"can_cast_object_now guarantees completability" invariant -- true for the
pre-cast check, not for tap-channel execution order.
Add convoke_native_tap_still_demanded (mana_colors.rs), the tap-channel
sibling of tap_strands_demanded_color, reusing its existing color_is_demanded
helper rather than duplicating it. Wire it into tactical_gate.rs's
assess_candidate as a new TapForConvoke{Colorless} arm: reject the Colorless
tap only when a sibling native-ability candidate for the same object could
still pay the pending cast's outstanding colored demand -- zero-cost
dominance, not a scoring preference, since the native ability can always
still cover the trailing generic slot afterward.
Four new tactical_gate.rs regression tests cover: the dominated-Colorless
rejection, the sibling native tap staying allowed, the tap being allowed once
colored demand is satisfied (not a blanket ban), and a permanent with no
native colored option being unaffected (gate stays scoped to true dominance).
Closes phase-rs#6837
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe AI detects when a permanent’s native or colored mana action is needed during a pending colored payment. Tactical gating rejects its Colorless convoke-family tap while preserving valid alternatives. ChangesConvoke tap-channel gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ManaPayment
participant TacticalGate
participant ManaColors
participant CandidateActions
ManaPayment->>TacticalGate: evaluate Colorless TapForConvoke
TacticalGate->>ManaColors: inspect pending colored demand
ManaColors->>CandidateActions: inspect sibling mana actions
CandidateActions-->>ManaColors: return matching colored action
ManaColors-->>TacticalGate: return demand status
TacticalGate-->>ManaPayment: reject or allow candidate
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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/phase-ai/src/mana_colors.rs`:
- Around line 179-224: The sibling-demand check in
sibling_native_tap_pays_demand must also recognize colored TapForConvoke actions
for the same object and verify their produced mana color satisfies demand,
preventing colorless candidates from being selected first. Add the corresponding
regression test in tactical_gate.rs covering colored and colorless TapForConvoke
siblings and preserving the colored pip.
🪄 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: 51ae8797-5b7d-40d9-94b6-6f880eb6f32c
📒 Files selected for processing (2)
crates/phase-ai/src/mana_colors.rscrates/phase-ai/src/tactical_gate.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the Colorless gate still misses the colored Convoke sibling it claims to cover.
[HIGH] sibling_native_tap_pays_demand never recognizes GameAction::TapForConvoke { mana_type: <color> }. Evidence: mana_colors.rs:202-222 handles only TapLandForMana and ActivateAbility, while the production Convoke candidate generator emits the same creature's Colorless action and a color-matching TapForConvoke action at candidates.rs:4782-4810. Why it matters: for a Convoke spell with a remaining colored pip (for example, {1}{U} and a blue creature), the Colorless action remains Allow; selecting it taps the creature, strands {U}, and recreates the ManaPayment dead-end. The four tests build an Improvise context with a synthetic native TapLandForMana sibling at tactical_gate.rs:1188-1230, so none would fail if this missing Convoke arm remained absent.
Suggested fix: treat a same-object colored TapForConvoke action whose mana_type satisfies color_is_demanded as a dominating sibling too, then add a regression that obtains the Colorless/colored pair through the production ConvokeMode::Convoke candidate path (and verifies the Colorless candidate is rejected while the colored sibling stays selectable). That test must fail on this head; the existing Improvise/native-mana tests should remain as the separate Metallic Rebuke coverage.
sibling_native_tap_pays_demand only handled TapLandForMana and
ActivateAbility as siblings that could still cover an outstanding colored
demand -- it missed that mana_payment_actions emits a colored TapForConvoke
candidate alongside the Colorless one for CR 702.51a Convoke (not just
Improvise/Waterbend, which are always colorless). A Convoke spell with a
remaining colored pip and a matching-colored creature left the Colorless
marker Allow, since the previous code never recognized the colored sibling
on the same object as dominating it -- reintroducing the exact ManaPayment
dead-end this fix targets, just via Convoke instead of Improvise.
Add a TapForConvoke{same object_id} arm matching color_is_demanded on its
own mana_type; Colorless naturally evaluates false so it can't spuriously
match itself.
New regression drives the real production candidate path
(candidate_actions_broad -> mana_payment_actions) for a Convoke spell with a
colored pip and a matching-colored creature, rather than a synthetic
Improvise-only sibling: asserts the Colorless candidate is Reject while the
real colored sibling stays Allow. Fails on the prior head.
|
Addressed in 87de141. Colorless gate now recognizes the colored Convoke siblingYou're right — Added a New regression ( Verified: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the gate over-rejects tapless mana abilities on the same permanent.
[HIGH] sibling_native_tap_pays_demand treats every same-object ActivateAbility that can produce a demanded color as mutually exclusive with the Colorless TapForConvoke action. Evidence: mana_colors.rs:206-221 does not inspect the ability cost; the engine deliberately includes currently activatable non-tap mana abilities during ManaPayment (mana_sources.rs:248-303), including tapless sacrifice abilities (candidates.rs:6747-6810). Why it matters: with an Improvise-eligible artifact that has a tapless self-sacrifice {U} mana ability and a {1}{U} pending spell, Colorless Improvise first, then the tapless sacrifice ability, is a legal completing sequence. This gate rejects the first action solely because the second is in the candidate set; choosing the native ability first instead leaves {1} unpaid and can recreate the dead end. Suggested fix: only call a native mana action a dominating sibling when its activation cost consumes that same permanent's tap (using the engine's cost-component authority rather than reimplementing cost-shape matching), and add a production ManaPayment regression proving Colorless-then-tapless-mana stays available while a {T} native colored producer still gates Colorless.
The prior requested change is resolved at 87de1411: its colored same-object TapForConvoke arm and production candidate-path regression cover the original Convoke gap. This new finding is independent.
…bling sibling_native_tap_pays_demand's ActivateAbility arm gated the Colorless convoke-family marker off ANY same-object ability that could produce a demanded color, without checking whether that ability's own cost actually taps the permanent. The engine deliberately keeps tapless mana abilities (e.g. a sacrifice-based one) in the ManaPayment candidate set alongside a Colorless Improvise/Convoke tap on the same object -- they are not mutually exclusive (Colorless first, then the tapless ability, is a legal completing sequence). The prior code rejected the Colorless candidate solely because the tapless sibling existed, which can itself recreate the ManaPayment dead-end this fix targets, just in the opposite direction. Gate the ActivateAbility arm on the ability's own cost categories (AbilityCost::categories(), CR 118) containing CostCategory::TapsSelf, using the engine's existing cost-component authority rather than re-matching cost shapes by hand -- it already flattens Composite costs. Two new regressions drive the real production ManaPayment candidate set (candidate_actions_broad -> mana_payment_actions) for an Improvise-eligible artifact with a same-object mana ability: a tapless (Sacrifice-cost) one leaves the Colorless candidate Allow (fails on the prior head), and a tap-cost one still gates it to Reject (regression guard for the existing behavior).
|
Addressed in 99b3650. Colorless gate no longer over-rejects tapless mana abilitiesConfirmed — Two new regressions, both driving the real production
Verified: |
|
Current-head maintainer review is clean at The two prior blocking findings are resolved:
I also merged current |
|
Following up on the CI run for the current head (`b7c4922d`, after you merged `main` in) — it isn't fully green yet, so flagging with evidence rather than assuming it'll resolve itself: Decision-cost perf gate: FAILED (7/29 counters over threshold: `crew_eligibility_scans`, `layers_full_eval`, `legend_rule_mode_gate_scans`, `mana_aura_trigger_scans`, `restriction_static_mode_gate_scans`, `sba_battlefield_snapshot_builds`, `state_clone_for_legality`). The gate's own log self-diagnoses: "card-data hash changed ... likely a card-data-driven trajectory shift, not a cost-per-node regression." None of these counters touch `ManaPayment`/convoke/mana-ability code — this PR's diff is scoped entirely to `sibling_native_tap_pays_demand`, reachable only from the `TapForConvoke{Colorless}` gate arm. I also checked #5437 ("Nightly decision-cost perf drift," open since 07-09): the same class of counters swings unpredictably between PASS/FAIL across nightly runs against bare `main` with no PR involved at all — e.g. its 07-18 run shows `mana_aura_trigger_scans` +10225 and `restriction_static_mode_gate_scans` +13227 on main alone. That's the same pre-existing baseline-staleness pattern, not something this diff introduced. Paired-seed AI gate: CANCELLED — hit the workflow's own 1-hour execution ceiling (its own comment notes runner speed varies ~2x and can overrun on a slow one), not a game panic or assertion. Inconclusive on its own; may just need a re-run. Not asking you to take my word for the perf-gate read — flagging with the specific evidence so it's easy to confirm. Let me know if you'd like me to do anything else here (e.g. nothing in this PR should need a baseline refresh, since that's a repo-wide main-tracking concern per #5437, not scoped to this change). |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review complete: the ManaPayment gate is at the right policy seam, both previously requested sibling cases are covered through the production candidate path, and required branch-protection checks are green.
2 participants
Closes #6837
Summary
Metallic Rebuke's{2}{U}Improvise cast could dead-end the AI:mana_payment_actionsoffers a ColorlessTapForConvokemarker for every Improvise/Waterbend/Convoke-eligible permanent, with nothing preferring a dual-purpose permanent's native colored mana ability while a colored pip is still outstanding. Tapping such a permanent via the Colorless marker first permanently strands the pip, and the AI dead-ends inManaPaymentwith no legal completion — hittingsearch::fallback_action'sdebug_assert!("can_cast_object_nowhas a gap that allowed an uncompletable cast through"), discovered via PR #6662's "Paired-seed AI gate" CI run (affinity-mirrormatchup, which plays Metallic Rebuke).What changed
crates/phase-ai/src/mana_colors.rs: newconvoke_native_tap_still_demanded, the tap-channel-selection sibling of the existingtap_strands_demanded_color(reuses itscolor_is_demandedhelper rather than duplicating it).crates/phase-ai/src/tactical_gate.rs: wires this intoassess_candidateas a newTapForConvoke { mana_type: Colorless }arm — reject only when a sibling candidate for the same object could still pay the pending cast's outstanding colored demand via its native ability. Zero-cost dominance, not a scoring preference: the native ability can always still cover a trailing generic slot afterward.Test plan
cargo fmt --allcargo check -p phase-ai --libcargo clippy -p phase-ai --all-targets -- -D warningscargo test -p phase-ai --lib -- tactical_gate:: mana_colors::— 17 passed (4 new: the dominated-Colorless rejection, the sibling native tap staying allowed, the tap being allowed again once colored demand is satisfied, and a permanent with no native colored option being unaffected; 13 pre-existing, no regressions)Closes #6837
Summary by CodeRabbit