fix: filter uncompletable AI target proposals - #7254
Conversation
|
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:
📝 WalkthroughWalkthroughThe adapter now classifies stale Resolve All responses separately and suppresses their notifications. AI decision contracts now filter final targets that cannot satisfy payment requirements. ChangesResolve All rejection handling
AI final target validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WebSocketAdapter
participant resolveAllRejectionError
participant dispatchResolveAll
participant Notifications
WebSocketAdapter->>resolveAllRejectionError: classify ResolveAllRejected reason
resolveAllRejectionError->>dispatchResolveAll: return AdapterErrorCode
alt STALE_ACTION
dispatchResolveAll->>dispatchResolveAll: clear Resolve All state
else ACTION_REJECTED
dispatchResolveAll->>Notifications: log and show rejection
end
sequenceDiagram
participant AiDecisionContract
participant TargetSelection
participant Reducer
participant ManaState
AiDecisionContract->>TargetSelection: enumerate target choices
TargetSelection->>Reducer: validate each choice
Reducer->>ManaState: check payment feasibility
Reducer-->>AiDecisionContract: retain payable targets
Possibly related PRs
Suggested labels: 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 |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
![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/engine/src/ai_support/mod.rs`:
- Around line 6201-6216: The target-selection fixtures use an empty optional
suffix, which is skipped and cannot exercise genuinely non-final progression. In
crates/engine/src/ai_support/mod.rs lines 6201-6216, update the later
target-selection slot to contain a legal target, then add a separate regression
fixture covering an empty-tail final-validation case. In
crates/manabrew-compat/src/lib.rs lines 5283-5298, mirror the non-empty
later-slot change in the interaction-projection fixture.
🪄 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: 59af5024-ca81-420f-9ad2-06557bb315b1
📒 Files selected for processing (2)
crates/engine/src/ai_support/mod.rscrates/manabrew-compat/src/lib.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/tests/integration/ai_decision_contract.rs`:
- Around line 157-158: Update the borrow of state.waiting_for in the surrounding
test setup to use a mutable reference, so pending_cast and target_slots can be
modified as required. Preserve the existing target constraint assignment and
TargetSelectionSlot construction.
🪄 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: 86411b21-d1cd-48bc-8366-c1d9cebc12f3
📒 Files selected for processing (4)
crates/engine/src/ai_support/context.rscrates/engine/src/ai_support/mod.rscrates/engine/tests/integration/ai_decision_contract.rscrates/manabrew-compat/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/manabrew-compat/src/lib.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/phase-ai/src/search.rs`:
- Around line 8878-8880: Strengthen the regression assertion around the AI
decision in the target-selection test to require Some(GameAction::ChooseTarget {
target: Some(_) }), rather than accepting any action or CancelCast. Keep the
existing reducer clone-count and contract-membership checks, but ensure the test
fails when the target prompt is canceled while legal targets remain.
🪄 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: 76314179-bdf3-4440-b3b8-ae799ba8471c
📒 Files selected for processing (3)
crates/engine/src/ai_support/mod.rscrates/engine/tests/integration/ai_decision_contract.rscrates/phase-ai/src/search.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/ai_support/mod.rs
- crates/engine/tests/integration/ai_decision_contract.rs
| counters.state_clone_for_legality, 3, | ||
| "every target choice and CancelCast must pass through the reducer before the \ | ||
| AI receives the engine-issued decision domain" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require a forward target action in this regression.
The clone count does not prove that the AI selected a target. The later action.is_some() and contract-membership assertions also accept GameAction::CancelCast. If the unmodeled-target path cancels while legal target candidates remain, this test stays green and the AI drops the spell instead of resolving the target prompt. Assert Some(GameAction::ChooseTarget { target: Some(_) }).
Proposed assertion
- assert!(
- action.is_some(),
- "a required target slot with legal choices must always retain a forward action"
- );
+ assert!(
+ matches!(
+ action.as_ref(),
+ Some(GameAction::ChooseTarget { target: Some(_) })
+ ),
+ "a required target slot with legal choices must select a concrete target"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| counters.state_clone_for_legality, 3, | |
| "every target choice and CancelCast must pass through the reducer before the \ | |
| AI receives the engine-issued decision domain" | |
| assert!( | |
| matches!( | |
| action.as_ref(), | |
| Some(GameAction::ChooseTarget { target: Some(_) }) | |
| ), | |
| "a required target slot with legal choices must select a concrete target" | |
| ); |
🤖 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 8878 - 8880, Strengthen the
regression assertion around the AI decision in the target-selection test to
require Some(GameAction::ChooseTarget { target: Some(_) }), rather than
accepting any action or CancelCast. Keep the existing reducer clone-count and
contract-membership checks, but ensure the test fails when the target prompt is
canceled while legal targets remain.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests