Repository navigation
fix: Harden IPC contracts, empty RPC errors, and Settings async UI - #1778
Merged
Merged
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
#1776) Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds shared contract validation for IPC endpoints, compile responses, and CLI errors; introduces typed no-response handling; applies ChangesContracts and protocol behavior
Async continuation control
Settings and setup orchestration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
Packages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.cs (1)
37-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCatch
JsonExceptionto align with the Fail Fast policy.Based on learnings, avoid broad defensive
catch (Exception)blocks and only catch expected, domain-specific exceptions. Since this block is intended to recover from serialization failures gracefully, consider catchingJsonException(the base class for Newtonsoft.Json exceptions) so that severe runtime errors (likeOutOfMemoryException) continue to fail fast instead of being swallowed into an RPC error frame.♻️ Proposed refactor
- catch (Exception ex) + catch (JsonException ex) { // Why error frame instead of success+error object: clients treat jsonrpc success as // completed tool results; a fake success would hide serialization failure from agents.🤖 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 `@Packages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.cs` around lines 37 - 40, Update the exception handler in JsonRpcResponseFactory to catch only JsonException for expected serialization failures, replacing the broad catch (Exception) while preserving the existing error-frame handling for that exception. Allow severe runtime exceptions to propagate fail fast.Source: Learnings
Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs (1)
117-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale ConfigureAwait rationale.
The comment at Lines 103-107 says these awaits intentionally avoid
ConfigureAwait(false), but all three awaits now use it. Update the rationale—or revert the awaits if Unity-context preservation is still required.🤖 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 `@Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs` around lines 117 - 127, Update the stale ConfigureAwait rationale near _waitForTestRunnerCleanupAsync to accurately reflect that the test execution and cleanup awaits use ConfigureAwait(false), preserving the current await behavior unless Unity-context preservation is explicitly required by the surrounding implementation.
🤖 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 `@Packages/src/Editor/FirstPartyTools/Compile/CompileLifecycleWatchdog.cs`:
- Line 105: Keep the awaits on the Unity thread by removing
ConfigureAwait(false) or otherwise preserving Unity-context continuation at
Packages/src/Editor/FirstPartyTools/Compile/CompileLifecycleWatchdog.cs:105 for
_waitForPollAsync,
Packages/src/Editor/FirstPartyTools/Compile/CompileUseCase.cs:106 and :174 for
the play-mode polling loops, and
Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/AutoUsingResolver.cs:43
and :106 for retry paths invoking buildFunc.
In
`@Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunner.cs`:
- Around line 69-71: Update ExecuteAsync around the initial
MainThreadSwitcher.SwitchToMainThread call so OperationCanceledException from a
pre-cancelled token or queued cancellation is handled by the existing
cancellation path and returns CreateCancelledResult(). Keep the main-thread
switch and normal execution behavior unchanged for non-cancellation failures and
successful calls.
In
`@Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/PlayModeTestExecuter.cs`:
- Around line 126-128: Keep the async continuation after
taskCompletionSource.Task on the Unity Editor thread by removing
ConfigureAwait(false) or explicitly dispatching to the Editor synchronization
context. Ensure UnifiedTestCallback.Dispose(), the cancellation recovery calls
to RunTestsCancelStopRestore.StopAndRestoreAsync(), and the polling at
Packages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.cs:224
execute on that thread; update the affected sites in
Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/PlayModeTestExecuter.cs:126-128,
:55-67, and :83-92, while preserving cancellation behavior.
In `@Packages/src/Editor/FirstPartyTools/Watch/WatchTools.cs`:
- Around line 191-192: Update the await in the watch compilation flow around
WatchExpressionServices.Compiler.CompileAsync so continuation resumes on the
captured Editor context before any subsequent Registry.Register or
EnsureMonitorStarted mutations; remove ConfigureAwait(false) for this
Editor-scoped operation while preserving cancellation and result handling.
In `@Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs`:
- Line 76: Update the isCliInstalled assignment in
UnityCliLoopSettingsCliSetupPresenter to treat both null and empty cached CLI
versions from GetCachedCliVersion() as not installed, ensuring the setup flow
selects Install/Update rather than Uninstall.
---
Nitpick comments:
In `@Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs`:
- Around line 117-127: Update the stale ConfigureAwait rationale near
_waitForTestRunnerCleanupAsync to accurately reflect that the test execution and
cleanup awaits use ConfigureAwait(false), preserving the current await behavior
unless Unity-context preservation is explicitly required by the surrounding
implementation.
In `@Packages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.cs`:
- Around line 37-40: Update the exception handler in JsonRpcResponseFactory to
catch only JsonException for expected serialization failures, replacing the
broad catch (Exception) while preserving the existing error-frame handling for
that exception. Allow severe runtime exceptions to propagate fail fast.
🪄 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
Run ID: 071ad40f-c1d3-4125-88cf-3bb2f942f2c6
⛔ Files ignored due to path filters (6)
Assets/Tests/Editor/CliUpdateRequiredErrorContractTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/GetCompileStatusResponseContractTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/JsonRpcResponseFactorySerializationFallbackTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/SettingsPresentationDecisionPolicyTests.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/CliPathSetupCheckPolicy.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/SkillInstallDialogPolicy.cs.metais excluded by none and included by none
📒 Files selected for processing (52)
Assets/Tests/Editor/BridgeTransportEndpointTests.csAssets/Tests/Editor/CliUpdateRequiredErrorContractTests.csAssets/Tests/Editor/GetCompileStatusResponseContractTests.csAssets/Tests/Editor/JsonRpcResponseFactorySerializationFallbackTests.csAssets/Tests/Editor/RunTestsTestFrameworkResultTests.csAssets/Tests/Editor/SettingsPresentationDecisionPolicyTests.csAssets/Tests/Editor/StaticFacadeStateGuardTests.csAssets/Tests/Editor/UnityCliLoopSettingsWindowCliActionTests.csPackages/src/Editor/Domain/CliPathSetupCheckPolicy.csPackages/src/Editor/Domain/SkillInstallDialogPolicy.csPackages/src/Editor/FirstPartyTools/Compile/CompilationExecutionService.csPackages/src/Editor/FirstPartyTools/Compile/CompileController.csPackages/src/Editor/FirstPartyTools/Compile/CompileLifecycleWatchdog.csPackages/src/Editor/FirstPartyTools/Compile/CompileUseCase.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCodeServices.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/AutoUsingResolver.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/CompiledAssemblyBuilder.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/DynamicCodeCompiler.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunner.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionFacade.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionScheduler.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionSchedulerHooks.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutor.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeForegroundWarmupRunner.csPackages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputUseCase.csPackages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.csPackages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.csPackages/src/Editor/FirstPartyTools/RunTests/TestFramework/PlayModeTestExecuter.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.csPackages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputUseCase.csPackages/src/Editor/FirstPartyTools/Watch/WatchTools.csPackages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.csPackages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.csPackages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.csPackages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.csPackages/src/Editor/Presentation/UnityCliLoopSettingsToolSettingsPresenter.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cscli/common/errors/cli_update_required_error_contract_test.gocli/common/errors/transport_errors.gocli/common/errors/transport_errors_test.gocli/common/project/endpoint_contract_test.gocli/common/unityipc/client.gocli/dispatcher/shared-inputs-stamp.jsoncli/project-runner/internal/projectrunner/compile_status_response_contract_test.gocli/project-runner/shared-inputs-stamp.jsontests/UnityCliLoop.CodeComplexity.Tests/ConfigureAwaitGuardTests.cstests/contracts/cli_update_required_error_contract.jsontests/contracts/compile_status_response_contract.jsontests/contracts/endpoint_contract.jsontools/UnityCliLoop.CodeComplexity/ConfigureAwaitGuard.cs
💤 Files with no reviewable changes (1)
- Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs
PR-4's bulk ConfigureAwait(false) left Unity API continuations on worker threads. Switch back before Editor callbacks (watch, compile watchdog, run-tests), keep AssemblyBuilder/play-mode polls on the Editor context, map pre-cancelled CommandRunner switches to CreateCancelledResult, and treat empty cached CLI versions as not installed. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore ConfigureAwait(false) with SwitchToMainThread for CompileUseCase and AutoUsingResolver so the FirstPartyTools guard stays green, and use CancellationToken.None on the watchdog main-thread switch so cancelled polls still reach the loop-head AbortCompile path. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Merged
hatayama
added a commit
that referenced
this pull request
Jul 17, 2026
simulate-mouse-input and simulate-keyboard always failed with "set_runInBackground can only be called from the main thread" (UNITY_RPC_ERROR) even though the simulation itself succeeded, and the thrown exception additionally left PlayMode in Error Pause. The executor awaits in both use cases were given .ConfigureAwait(false) by the ConfigureAwait guard (#1778). Their completions come from a TCS created with RunContinuationsAsynchronously, so the method always resumed on a thread-pool thread, and the `using` scope's Dispose then set Application.runInBackground (a main-thread-only API) off the main thread, discarding the successful response. Follow the existing PlayModeTestExecuter pattern: replace `using` with an explicit scope + try-finally that switches back to the main thread via InputSystemUpdateHelper.SwitchToMainThreadIfNeeded before Dispose. Also assert the main-thread precondition inside InputSimulationRunInBackgroundScope.Dispose so future regressions fail fast (requires the ToolContracts asmdef reference).
RyanXie123
pushed a commit
to RyanXie123/unity-cli-loop
that referenced
this pull request
Sep 22, 2026
…atayama#1778) Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
async voidfrom swallowing UI exceptions.User Impact
NoResponseErrors, serialization faults return error frames, and Settings/Setup async failures are logged while UI behavior stays the same.Changes
Child PRs merged into
feature/design-review-fixes-integration:cli_update_requiredcontract fixturesNoResponseErrorfor empty Unity RPC results (+ release-input stamps)async void→Task+Forget(), shallow Settings presenter split, Domain dialog/PATH policiesOut of scope by design: protocolVersion / pin / CHANGELOG bumps; deeper Settings View construction split (SettingsWindow still ~860 lines; further cut to <500 is a follow-up).
Verification
uloop compile0/0 and EditMode suites as recorded on each PR/reviewwith no required fixesManual check (already completed for Presentation)
Review notes
/review(bugbot or security)