fix(ai): answer selection prompts from the engine's issued domain (#6942) - #6964
Conversation
) The AI fallback returned `SelectCards { cards: Vec::new() }` for a group of `WaitingFor` variants under the comment "empty selection is a valid 'choose nothing'". For six of them the engine rejects an empty selection unconditionally, so the AI proposed an action that could never be accepted and the game stalled. CR 608.2d: a player can't choose an option that's illegal or impossible. The arm made a blanket cardinality assumption across variants whose contracts differ, and several are data-dependent at runtime (`up_to`, `optional`) rather than a property of the variant. A static reclassification would have traded a softlock for a wrong-decision bug. So do not classify at all. Delegate to the engine's own issued candidate set via `build_decision_context_for_semantic_owner`, which is already the authority the gating contract uses. The AI stops restating a cardinality rule it was getting wrong and answers from the domain the engine published -- removing the second hand-maintained list whose disagreement with the first caused this. Two prior drafts mirrored a `pub(crate)` enumerator into phase-ai instead; that would have recreated the defect. The seat is `contract.semantic_owner`, threaded through `fallback_action`. Deriving it from `acting_player()` -- as an earlier draft did -- diverges from the seat the gating contract was issued for, so the fix would have failed precisely in the multi-seat case it exists to serve. The `MulliganDecision` arm's dispatch is moved off `pending.first()` for the same reason, mirroring the existing correct form. Also fixes the engine side of an enumerator/handler disagreement: the candidate enumerator emits `keep_count.min(selectable.len())` for `DigChoice` while the handler demanded exactly `keep_count`, so a dig with fewer legal cards than its keep count was unanswerable. Clamped in the handler, which is the defective side (CR 609.3, CR 101.3). `MultiTargetSelection` is a confirmed member of this defect class and is deliberately NOT converted: the enumerator discards `max_targets` and issues all legal targets at once, so at a min:1/max:1 site with more than one legal target its only non-empty candidate is itself illegal. Delegating there would swap a contract-refusing action for a contract-passing, engine-rejected one. Filed as follow-up rather than half-fixed. Every test's revert baseline is stated per row, because most cannot be observed at bare base at all -- threading the contract changes `fallback_action`'s arity and phase-ai is one compile unit. T11 in particular was observed red on the full fix MINUS ONLY the one-line seat source, which is the only tree that isolates the seat defect; at base+1+2 it would go red merely because the mulligan step is absent.
📝 WalkthroughWalkthroughThe engine now accepts all filter-eligible cards for impossible non- ChangesSelection validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GameState
participant AiDecisionContract
participant fallback_action
participant Engine
GameState->>AiDecisionContract: issue selection candidates
AiDecisionContract->>fallback_action: provide contract and candidates
fallback_action->>AiDecisionContract: choose an issued selection
fallback_action->>Engine: return fallback GameAction
Engine->>GameState: validate and apply selection
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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/engine_resolution_choices.rs`:
- Around line 2991-3002: Correct the CR citation in the DigChoice validation
comment so it references CR 609.3 only (or another rule that directly supports
the behavior), removing the inaccurate CR 101.3 reference. Apply the same
citation correction in
crates/engine/tests/integration/dig_impossible_keep_count.rs lines 5-7; both
sites document the “do as much as possible” handling.
In `@crates/phase-ai/src/search.rs`:
- Around line 1830-1844: Update the WaitingFor::PayCost arm for
CostResume::ManaAbility to select the smallest issued card selection whose
cards.len() is at least the required count, preserving cmp_stable tie-breaking.
Add a sibling helper to issued_selection for this filtered ordering, with
fallback to issued_selection when no qualifying candidate exists, and use it
with the in-scope PayCost count.
🪄 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: 508ca032-5d1b-49e0-8d0c-e43e8fe5a364
📒 Files selected for processing (4)
crates/engine/src/game/engine_resolution_choices.rscrates/engine/tests/integration/dig_impossible_keep_count.rscrates/engine/tests/integration/main.rscrates/phase-ai/src/search.rs
| } else { | ||
| // CR 609.3 + CR 101.3: a dig whose filter (or a short library) | ||
| // leaves fewer selectable cards than `keep_count` must keep as | ||
| // many as possible, not reject every selection. Without the | ||
| // clamp no legal action exists in that state — | ||
| // `validate_dig_selection` below requires every kept id to be in | ||
| // `selectable_cards` while this gate demands more ids than it | ||
| // holds — softlocking every controller. Matches the clamp the | ||
| // candidate enumerator (`ai_support/candidates.rs:1185`) and | ||
| // `cheap_reject_candidate` (`ai_support/mod.rs:702`) already | ||
| // apply. | ||
| let required = keep_count.min(selectable_cards.len()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the duplicated CR 101.3 citation.
CR 101.3 defines “card.” It does not support impossible-instruction handling. CR 609.3 supports the “do as much as possible” behavior. (media.wizards.com)
crates/engine/src/game/engine_resolution_choices.rs#L2991-L3002: remove or replace the CR 101.3 citation in the DigChoice validation comment.crates/engine/tests/integration/dig_impossible_keep_count.rs#L5-L7: remove or replace the matching CR 101.3 citation in the test documentation.
As per path instructions, flag a CR citation whose rule body does not describe the code.
📍 Affects 2 files
crates/engine/src/game/engine_resolution_choices.rs#L2991-L3002(this comment)crates/engine/tests/integration/dig_impossible_keep_count.rs#L5-L7
🤖 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/engine_resolution_choices.rs` around lines 2991 -
3002, Correct the CR citation in the DigChoice validation comment so it
references CR 609.3 only (or another rule that directly supports the behavior),
removing the inaccurate CR 101.3 reference. Apply the same citation correction
in crates/engine/tests/integration/dig_impossible_keep_count.rs lines 5-7; both
sites document the “do as much as possible” handling.
Source: Path instructions
| // carry PendingManaAbility, so CancelCast is not valid here. | ||
| // | ||
| // CR 605.1a names the CLASS this arm matches (what makes an activated | ||
| // ability a mana ability); it is NOT the reason the cost must be paid. | ||
| // CR 118.1 + CR 118.3 are: "to pay a cost, a player carries out the | ||
| // instructions specified", and "a player can't pay a cost without | ||
| // having the necessary resources to pay it FULLY". So a mana ability's | ||
| // cost is not optional, and every mana-ability cost handler demands | ||
| // exactly `count` (`mana_abilities.rs:1130` tap / `:1161` exile / | ||
| // `:1215` sacrifice / `:1267` discard) — an empty selection is rejected | ||
| // in all four (#6942). Answer out of the contract instead. | ||
| WaitingFor::PayCost { | ||
| resume: CostResume::ManaAbility { .. }, | ||
| .. | ||
| } => Some(GameAction::SelectCards { cards: Vec::new() }), | ||
| } => issued_selection(contract), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The sacrifice-cost mana ability still softlocks under this arm.
issued_selection prefers the smallest issued SelectCards. The doc comment at Lines 871-875 records that the enumerator for PayCost { kind: Sacrifice, resume: ManaAbility } issues sizes min_count..=count while the handler demands exactly count. For that shape the smallest issued candidate has min_count cards, so the handler rejects it and the AI keeps re-proposing — the exact #6942 failure mode this arm is meant to remove.
This arm has the PayCost fields in scope. Select the smallest issued selection whose length is at least count instead of the global minimum, so the sacrifice shape is answered too. Alternatively, correct the engine enumerator to emit only size count for the sacrifice cost kind.
Do you want me to open a follow-up issue that tracks the sacrifice-cost enumerator/handler mismatch?
🐛 Sketch: honor the prompt's required count for mana-ability costs
WaitingFor::PayCost {
resume: CostResume::ManaAbility { .. },
+ count,
..
- } => issued_selection(contract),
+ } => issued_selection_with_min_len(contract, *count),Add a sibling helper next to issued_selection that applies the same
smallest-then-cmp_stable ordering over candidates whose cards.len() >= min_len,
and falls back to issued_selection when no such candidate is issued.
🤖 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/phase-ai/src/search.rs` around lines 1830 - 1844, Update the
WaitingFor::PayCost arm for CostResume::ManaAbility to select the smallest
issued card selection whose cards.len() is at least the required count,
preserving cmp_stable tie-breaking. Add a sibling helper to issued_selection for
this filtered ordering, with fallback to issued_selection when no qualifying
candidate exists, and use it with the in-scope PayCost count.
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
AI gate produced no verdict — disclosing rather than treating this as greenBoth This is not specific to this PR. Across the 58 most recent completed CLAUDE.md requires an ai-gate report for AI behavior changes. That requirement is currently unsatisfiable in CI, so I am stating what I can evidence instead of implying coverage I do not have. Why the strength risk is bounded by constructionThe production delta is ~180 lines; 816 of the 973 added lines in In
Divergence is therefore only possible in states where the synthesized pick was not issued. Those are exactly the states this PR fixes, where the old behavior was an unusable action that skipped the Confidence
Required checks ( |
1 participant
Fixes #6942.
The bug
fallback_actionreturnedSelectCards { cards: Vec::new() }for a group ofWaitingForvariants, under the comment "empty selection is a valid 'choose nothing'". For six of them the engine rejects an empty selection unconditionally, so the AI proposed an action that could never be accepted and the game stalled. CR 608.2d: "The player can't choose an option that's illegal or impossible."The issue reported five variants. Opening every handler found six, four of which the report never named.
Why the obvious fix would have been wrong
The arm made a blanket cardinality assumption across variants whose contracts differ — and for several the answer is data-dependent at runtime (
up_to,optional) rather than a property of the variant. Statically reclassifying them would have traded a softlock for a wrong-decision bug: the legitimateup_to == truecases genuinely should answer empty.So this does not classify at all. It delegates to the engine's own issued candidate set via
build_decision_context_for_semantic_owner— already the authority the gating contract uses. The AI stops restating a cardinality rule it was getting wrong and answers from the domain the engine published.That removes the second hand-maintained list whose disagreement with the first caused the bug. Two prior drafts mirrored a
pub(crate)enumerator intophase-aiinstead; one carried an implementer note reading "must mirrorcandidates.rsexactly", which is the defect recreating itself.The seat
The contract is issued for a specific seat, so the answer must be built for
contract.semantic_owner, threaded throughfallback_action. An earlier draft derived it fromacting_player(), which diverges from the contract's seat — the fix would have failed precisely in the multi-seat case it exists to serve. TheMulliganDecisionarm's dispatch is moved offpending.first()for the same reason, mirroring the already-correct form elsewhere in the file.Engine-side: an enumerator/handler disagreement
The candidate enumerator emits
keep_count.min(selectable.len())forDigChoicewhile the handler demanded exactlykeep_count— so a dig with fewer legal cards than its keep count was unanswerable. Clamped in the handler, which is the defective side (CR 609.3, CR 101.3). This changes engine behaviour, not just AI behaviour, so the fourDigChoiceintegration tests were run by name:issue_4271_birthing_ritual_cmc_filter,gonti_lord_of_luxury_exiles_dug_card,fertile_thicket_reveal_to_top_2349,explore_all_doubler_ordering— all pass.Deliberately not converted
MultiTargetSelectionis a confirmed member of this defect class and is excluded anyway. The enumerator destructuresmin_targetsand discardsmax_targets, issuing all legal targets at once, so at amin:1/max:1site with more than one legal target its only non-empty candidate is itself illegal. Delegating would swap a contract-refusing action for a contract-passing, engine-rejected one. At exactly one legal target delegation would be correct, but the arm cannot distinguish, and a singleton-only fix is not a class fix. Filed as follow-up rather than half-fixed.Scoped claim, stated precisely: this converts every arm answering with an unconditional empty
SelectCards, exceptMultiTargetSelection— 4 of the 12SelectCards-answering arms infallback_action. The other seven synthesize a cardinality-plausible answer and are outside the empty-selection half of this issue.Verification — per-row baselines, because bare base is not one
Threading the contract changes
fallback_action's arity, andphase-aiis a single compile unit, so most rows cannot be observed at bare base at all — a compile error is not an observed red. Each row states its own baseline.base + Steps 1 & 2): 9 rows red, every failure the exact mechanism —left: 0against the expected count, plusDigChoice: the escape emitted SelectCards { cards: [] }, which the gating contract refuses.base + 1 + 2T11 would go red merely because the mulligan step is absent.test-ai2071/2071,test-engine22806/22806,clippyclean.Deviation: the plan mispredicted its own test matrix
The plan's T8 row says "fails at base — NO". That is wrong. Observed at
base + Steps 1 + 2,fallback_actionreturnedSome(SelectCards { cards: [] }), notNone, so T8 failed withleft: Some(SelectCards { cards: [] }), right: Noneand does discriminate. The consequent correction: T3 is the only genuine both-directions reach-guard, not one of two. The surprise is in the safe direction — more discrimination than promised — but a plan that mispredicts which of its own tests discriminate is worth recording either way.T2 was also upgraded to collect all offending rows rather than abort on the first, so its failure reports a census rather than a sample: 14 of 16 rows, where the plan predicted 11.
CR annotations — two substitutions
Two of the plan's citations named real rules that do not describe the code. A wrong-but-real citation is worse than none: it creates false confidence that the code was validated, and it survives review because reviewers check code against the stated premise rather than fact-checking the premise.
CR 601.2(the casting procedure — nothing about resolution-time choice cardinality); kept CR 608.2d, which states the rule verbatim.CR 605.1a + CR 605.3bfor "a mana ability's cost is not optional" is a false attribution — 605.1a defines what a mana ability is, 605.3b says it doesn't use the stack. Replaced with CR 118.1 + CR 118.3, quoted inline; 605.1a is retained only to name the ability class, and the comment now says so explicitly.All 13 numbers grep-verified, zero
UNVERIFIED.Notes
_arms (none); the tail arm is preserved verbatim. A futureWaitingForvariant cannot silently inherit the wrong default, which is the defect class itself.cargo ai-gatewas not run locally. Step 5 changes a live AI decision path, so CLAUDE.md requires the paired-seed gate. It is not run here becausecrates/phase-ai/**is a trigger path for.github/workflows/ai-gate.yml, so the paired-seed run fires automatically on this PR and is the authoritative result.Summary by CodeRabbit