fix(ai): choose issued flexible mana products - #7057
Conversation
|
Warning Review limit reached
Next review available in: 34 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 ignored due to path filters (2)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMana-color AI decisions now honor engine-issued action domains and semantic-owner scoping. Tests cover capped fallback, owner-specific scoring, and restoration of a compressed saved-state mana prompt. ChangesMana choice domain enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AI Search
participant AiDecisionContract
participant Mana Choice Resolver
AI Search->>AiDecisionContract: Request issued actions for semantic owner
AiDecisionContract-->>AI Search: Return bounded action domain
AI Search->>Mana Choice Resolver: Evaluate preferred mana choice
Mana Choice Resolver->>AiDecisionContract: Validate choice against issued domain
AiDecisionContract-->>Mana Choice Resolver: Accept or reject candidate
Mana Choice Resolver-->>AI Search: Return valid or ranked issued choice
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/phase-ai/src/search.rs (1)
2353-2421: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep resolving-effect mana preferences when no issued
ChooseManaColorexists.
quiesce()passes the quiesce candidates as&actions, anddeterministic_choice()routes resolve resolving-effectChooseManaColoronly from the passed slice. When that slice contains noChooseManaColor,resolving_effect_mana_choicenow returnsNone; return thepreferredcalculation when no matching issued option is supplied.🤖 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 2353 - 2421, Update resolving_effect_mana_choice so it returns the computed preferred action when issued_actions contains no matching ChooseManaColor, rather than falling through to None. Preserve the existing matching-issued-action preference and fallback behavior when issued choices are available.
🧹 Nitpick comments (3)
crates/phase-ai/src/search.rs (3)
6689-6696: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
build_decision_context_for_semantic_ownerin the test helper.
issued_actionsre-derives the owner's action domain fromAiDecisionContract::issue. Production derives the same list throughbuild_decision_context_for_semantic_ownerat Line 2867. If that production derivation ever adds filtering, the helper silently diverges and the tests stop covering the real domain.♻️ Proposed helper change
fn issued_actions(state: &GameState, owner: PlayerId) -> Vec<GameAction> { - AiDecisionContract::issue(state, owner) + build_decision_context_for_semantic_owner(state, owner) .candidates .into_iter() .map(|candidate| candidate.action) .collect() }🤖 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 6689 - 6696, The test helper issued_actions should reuse build_decision_context_for_semantic_owner instead of calling AiDecisionContract::issue directly, then extract and collect the resulting candidate actions while preserving the owner-specific action domain used by production.Source: Coding guidelines
6890-6906: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNeither new capped-domain test pins the selected mana product. Both tests assert only that the answer is inside the engine-issued domain. Neither states which of the 64 products the AI must choose, so both pass for a demand-blind allocation and neither would detect a change in the engine's candidate ordering.
crates/phase-ai/src/search.rs#L6890-L6906: replace theissued.first()-derivedexpectedwith the literalManaChoice::Combinationthe four-green demand must produce.crates/phase-ai/tests/scenarios.rs#L101-L117: add an assertion onrun[0].actionnaming the expectedManaChoice::Combinationfor this capture's color demand.🤖 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 6890 - 6906, Pin both capped-domain tests to the expected mana product instead of deriving it from issued_actions: in crates/phase-ai/src/search.rs lines 6890-6906, replace the issued.first()-derived expected value with the literal ManaChoice::Combination required for the four-green demand; in crates/phase-ai/tests/scenarios.rs lines 101-117, assert run[0].action equals the expected ManaChoice::Combination for that capture’s color demand.
2860-2887: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the owner-scoped decision context.
score_candidates_corebuildsbuild_decision_context_for_semantic_owner(state, ai_player)in the mana branch, then builds the same context again later when the earlier early-exit branches miss. SinceAiDecisionContract::issueenumerates candidates and sorts them, the mana-miss path pays for two identical enumerations. Build the context once above the early branches and reuse it.🤖 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 2860 - 2887, In score_candidates_core, build build_decision_context_for_semantic_owner(state, ai_player) once before the resolving-effect mana-choice branch, then reuse that context when collecting issued_actions and in the later scored path. Remove the duplicate context construction while preserving the existing candidate enumeration and early-return behavior.
🤖 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/search.rs`:
- Around line 2403-2421: Update the fallback in the preferred-action selection
to rank all issued ChooseManaColor actions by demand instead of taking the first
stable match. Add issued_mana_rank beside mana_product_rank, handling
SingleColor and Combination choices and returning i32::MIN for other actions,
then select the maximum-ranked issued action while preserving the issued-domain
constraint. Adjust mana_product_rank parameter types to use this module’s
existing ColorDemand types.
---
Outside diff comments:
In `@crates/phase-ai/src/search.rs`:
- Around line 2353-2421: Update resolving_effect_mana_choice so it returns the
computed preferred action when issued_actions contains no matching
ChooseManaColor, rather than falling through to None. Preserve the existing
matching-issued-action preference and fallback behavior when issued choices are
available.
---
Nitpick comments:
In `@crates/phase-ai/src/search.rs`:
- Around line 6689-6696: The test helper issued_actions should reuse
build_decision_context_for_semantic_owner instead of calling
AiDecisionContract::issue directly, then extract and collect the resulting
candidate actions while preserving the owner-specific action domain used by
production.
- Around line 6890-6906: Pin both capped-domain tests to the expected mana
product instead of deriving it from issued_actions: in
crates/phase-ai/src/search.rs lines 6890-6906, replace the
issued.first()-derived expected value with the literal ManaChoice::Combination
required for the four-green demand; in crates/phase-ai/tests/scenarios.rs lines
101-117, assert run[0].action equals the expected ManaChoice::Combination for
that capture’s color demand.
- Around line 2860-2887: In score_candidates_core, build
build_decision_context_for_semantic_owner(state, ai_player) once before the
resolving-effect mana-choice branch, then reuse that context when collecting
issued_actions and in the later scored path. Remove the duplicate context
construction while preserving the existing candidate enumeration and
early-return behavior.
🪄 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: cbeac124-d22a-4271-965a-cd0ab582c9c7
⛔ Files ignored due to path filters (1)
crates/phase-ai/fixtures/scenarios/invisible-woman-cosmic-crucible-mana.json.gzis excluded by!**/*.gz
📒 Files selected for processing (3)
crates/phase-ai/Cargo.tomlcrates/phase-ai/src/search.rscrates/phase-ai/tests/scenarios.rs
8992a42 to
50a574f
Compare
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
Summary by CodeRabbit
Bug Fixes
Tests