fix(ai): halt empty NamedChoice escape and refuse DB-less restore (#6393) - #6741
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThe adapter now requires a loaded card database before restoration operations. Engine-owned AI fallback actions are exposed through WASM and worker adapters, while the controller halts safely when no escape action exists. ChangesAI fallback and controller safety
Card database restoration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@client/src/adapter/wasm-adapter.ts`:
- Around line 590-596: Factor the card-database load validation currently in the
guard around cardDbLoaded into a shared helper, then invoke it from both the
existing restoration method and resumeMultiplayerHostState before rehydration or
state resume proceeds. Ensure either failed load path throws instead of
continuing with DB-less state, and add regression coverage proving
resumeMultiplayerHostState rejects a failed card database load.
In `@client/src/game/controllers/aiController.ts`:
- Around line 282-303: Remove client-side selection of result.actions[0] from
the non-Priority path in the controller method containing the
adapter.getLegalActions() call. Add or use an adapter API that returns the
engine-selected fallback action, then dispatch that action when available and
return null otherwise; retain the existing Priority handling and pending-cast
behavior.
🪄 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: 5a740f69-2c22-46e9-b5a8-e4d9f46e2c96
📒 Files selected for processing (5)
client/src/adapter/__tests__/wasm-adapter.test.tsclient/src/adapter/wasm-adapter.tsclient/src/game/controllers/__tests__/aiController.test.tsclient/src/game/controllers/aiController.tscrates/phase-ai/src/search.rs
matthewevans
left a comment
There was a problem hiding this comment.
Required changes
[HIGH] Guard P2P host resume against a failed card-database load. Evidence: client/src/adapter/wasm-adapter.ts:662-668 deliberately swallows loadCardDbFromUrl() failures and then calls resumeMultiplayerHostState; client/src/adapter/p2p-adapter.ts:1171-1172 reaches that path for a resumed host; and crates/engine-wasm/src/lib.rs:1658-1662 rehydrates only when CARD_DB is present. Why it matters: a persisted host can resume with definitions absent, reintroducing the DB-less restored-state failure this PR fixes for the ordinary restore path. Suggested fix: factor the successful-load requirement into one shared restore/resume DB guard, use it before both rehydration paths, and add a regression proving a failed DB load rejects P2P host resume without invoking the engine resume call.
[HIGH] Keep non-Priority escape-action selection engine-owned. Evidence: client/src/game/controllers/aiController.ts:296-304 selects result.actions[0] for every non-Priority wait, while the selected-AI action seam is the adapter/WASM getAiAction path at client/src/adapter/wasm-adapter.ts:428-433. Why it matters: legal-action ordering is not an engine decision contract, so the frontend can choose different gameplay merely because enumeration order changes. Suggested fix: expose an engine-selected nullable fallback action through the adapter/WASM boundary; for non-Priority waits, dispatch only that action or halt when it is null. Keep the existing Priority special case.
Current-head check evidence: Rust, frontend, WASM, and card-data checks succeeded. The paired-seed and decision-cost AI gates remain in progress; they are not the basis for these requested changes.
…ase-rs#6393) CardName prompts keep options empty and synthesize from all_card_names; fallback must use legal_actions, and the client must not fabricate PassPriority (or soft-restore without a card DB) after relaunch softlocks. Co-authored-by: Cursor <cursoragent@cursor.com>
…e-rs#6393) P2P host resume now shares restoreState's hard card-DB requirement. Non-Priority AI escape dispatches getAiFallbackAction (WASM fallback_action) instead of inventing from legal-action list order. Co-authored-by: Cursor <cursoragent@cursor.com>
7fb505d to
dcc929a
Compare
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 `@client/src/adapter/wasm-adapter.ts`:
- Around line 673-680: Update resumeMultiplayerHostState in the adapter to await
the fallback branch, ensuring MainThreadFallback.resumeMultiplayerHostState
returns its enqueue() promise typed as Promise<void> so queued WASM execution
and errors remain attached. Keep the direct engine branch behavior unchanged.
🪄 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: abfb8d2c-943f-4ec9-8798-75ff9b325093
⛔ Files ignored due to path filters (1)
client/src/wasm/engine_wasm.d.tsis excluded by!client/src/wasm/**,!**/*.d.ts
📒 Files selected for processing (10)
client/src/adapter/__tests__/wasm-adapter.test.tsclient/src/adapter/engine-worker-client.tsclient/src/adapter/engine-worker.tsclient/src/adapter/types.tsclient/src/adapter/wasm-adapter.tsclient/src/game/controllers/__tests__/aiController.test.tsclient/src/game/controllers/aiController.tscrates/engine-wasm/src/lib.rscrates/phase-ai/src/lib.rscrates/phase-ai/src/search.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/phase-ai/src/search.rs
- client/src/game/controllers/aiController.ts
|
Maintainer fixup |
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review complete: the current head carries the fallback-resume error-propagation fix and its regression coverage. Required CI checks are green; remaining AI advisory checks may continue under merge-when-ready.
|
Current-head maintainer review is approved. Required Rust and frontend checks are green; the paired-seed AI and decision-cost advisory gates remain in progress. I also attempted the authorized auto-merge action, but GitHub still reports |
Summary
fallback_actionnow picks a legalChooseOptionfromlegal_actions(fixes CardName prompts with emptyoptionsafter restore whenall_card_namesis populated).PassPriorityfor non-Priority waits — empty legal set notifiesai-controller-stuck:<type>and stops immediately.restoreStatehard-fails if the card DB failed to load, so we never soft-restore into a CardName softlock without rehydrate.Closes #6393
Test plan
cargo test -p phase-ai --lib named_choice_card_name_fallbackaiController.test.ts+wasm-adapter.test.ts(51 passed)Validation Failures
/review-implsubagent unavailable (API/model routing); Bugbot found no bugs. Plan was CLEAN before implement.Summary by CodeRabbit