chore: Simplify JSON-RPC execution wiring#1555
Conversation
Introduce JsonRpcRequestProcessor and UnityCliLoopExecutionRouter so JSON-RPC execution can be injected without changing the existing static entrypoints yet. The compatibility wrappers read ApplicationRegistrar.Service at call time to preserve the current test swap seam.
Pass the tool registrar into the server factory, bridge server, and server controller so runtime dispatch no longer reaches ApplicationRegistrar or JsonRpcProcessor static entrypoints from production paths.
Retarget the remaining execution-router tests to the injected router and drop the Application registrar pass-throughs made unreachable by the new production path. Keep the Service swap seam and custom registration wrappers for compatibility.
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR replaces static ChangesTool registrar service injection and JSON-RPC processor split
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant UnityCliLoopExecutionRouter
participant InternalBridgeCommandRouter
participant UnityCliLoopToolRegistrarService
participant JsonRpcRequestProcessor
Caller->>JsonRpcRequestProcessor: ProcessRequestWithEarlyResponseAsync(request)
JsonRpcRequestProcessor->>UnityCliLoopExecutionRouter: ExecuteAsync(commandName, paramsToken, ct)
alt internal command
UnityCliLoopExecutionRouter->>InternalBridgeCommandRouter: Execute(commandName, paramsToken, toolRegistrarService)
InternalBridgeCommandRouter->>UnityCliLoopToolRegistrarService: GetRegistry()
else tool command
UnityCliLoopExecutionRouter->>UnityCliLoopToolRegistrarService: ExecuteToolAsync(commandName, paramsToken, ct)
end
UnityCliLoopToolRegistrarService-->>UnityCliLoopExecutionRouter: UnityCliLoopToolResponse
UnityCliLoopExecutionRouter-->>JsonRpcRequestProcessor: response
JsonRpcRequestProcessor-->>Caller: JSON-RPC response frame
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Assets/Tests/Editor/UnityCliLoopToolRegistryTests.cs (1)
487-501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated
CreateToolRegistrarService()test helper.This helper (real
ToolSettingsRepository,EmptyInternalToolNameProvider,UnityCliLoopToolExecutionService(NoOpEditorRuntimeStatePort), and atoolDiscoveryfunc) is now copy-pasted verbatim inUnityCliLoopServerControllerStartupLockTests.csandUnityCliLoopServerStartupProtectionTests.cs, differing only in the discovery delegate. Extracting a single shared test factory (with an optional discovery-func parameter, mirroring the existingCreateControllerService(...)optional-param pattern) would prevent the three copies drifting whenUnityCliLoopToolRegistrarService's constructor changes.♻️ Example consolidation (shared test-support type)
internal static class ToolRegistrarServiceTestFactory { internal static UnityCliLoopToolRegistrarService Create( Func<IReadOnlyList<IUnityCliLoopTool>> toolDiscovery = null) { IToolSettingsPort toolSettingsPort = new ToolSettingsRepository(); return new UnityCliLoopToolRegistrarService( new EmptyInternalToolNameProvider(), toolSettingsPort, new UnityCliLoopToolExecutionService(new NoOpEditorRuntimeStatePort()), toolDiscovery ?? (() => Array.Empty<IUnityCliLoopTool>())); } }🤖 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 `@Assets/Tests/Editor/UnityCliLoopToolRegistryTests.cs` around lines 487 - 501, The duplicated CreateToolRegistrarService test helper should be consolidated into a shared test factory so the three copies do not drift. Extract the common UnityCliLoopToolRegistrarService setup (ToolSettingsRepository, EmptyInternalToolNameProvider, UnityCliLoopToolExecutionService with NoOpEditorRuntimeStatePort) into a single helper, and make the toolDiscovery delegate optional so UnityCliLoopServerControllerStartupLockTests and UnityCliLoopServerStartupProtectionTests can reuse it while preserving their differing discovery 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.
Nitpick comments:
In `@Assets/Tests/Editor/UnityCliLoopToolRegistryTests.cs`:
- Around line 487-501: The duplicated CreateToolRegistrarService test helper
should be consolidated into a shared test factory so the three copies do not
drift. Extract the common UnityCliLoopToolRegistrarService setup
(ToolSettingsRepository, EmptyInternalToolNameProvider,
UnityCliLoopToolExecutionService with NoOpEditorRuntimeStatePort) into a single
helper, and make the toolDiscovery delegate optional so
UnityCliLoopServerControllerStartupLockTests and
UnityCliLoopServerStartupProtectionTests can reuse it while preserving their
differing discovery behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e676a3f4-bb44-400f-b30f-55ecf680f6c6
⛔ Files ignored due to path filters (3)
Packages/src/Editor/Infrastructure/Api/JsonRpcProcessor.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/Api/UnityCliLoopExecutionRouter.cs.metais excluded by none and included by none
📒 Files selected for processing (12)
Assets/Tests/Editor/UnityCliLoopServerControllerStartupLockTests.csAssets/Tests/Editor/UnityCliLoopServerStartupProtectionTests.csAssets/Tests/Editor/UnityCliLoopToolRegistryTests.csPackages/src/Editor/Application/UnityCliLoopToolRegistrar.csPackages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.csPackages/src/Editor/Infrastructure/Api/GetToolDetailsBridgeCommand.csPackages/src/Editor/Infrastructure/Api/InternalBridgeCommandRouter.csPackages/src/Editor/Infrastructure/Api/JsonRpcProcessor.csPackages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.csPackages/src/Editor/Infrastructure/Api/UnityCliLoopExecutionRouter.csPackages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.csPackages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.cs
💤 Files with no reviewable changes (1)
- Packages/src/Editor/Application/UnityCliLoopToolRegistrar.cs
Share the registrar test factory used by execution-router tests and clean up the wrapper/comment nits found in review.
There was a problem hiding this comment.
5 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/Infrastructure/Api/JsonRpcProcessor.cs">
<violation number="1" location="Packages/src/Editor/Infrastructure/Api/JsonRpcProcessor.cs:42">
P3: The compatibility wrapper's CreateProcessor() constructs a new JsonRpcRequestProcessor (and underlying UnityCliLoopExecutionRouter) on every call. If ProcessRequest or ProcessRequestWithEarlyResponseAsync is called multiple times — which happens on every JSON-RPC request — this allocates a new processor, router, and references every time. Since the underlying toolRegistrarService (ApplicationRegistrar.Service) is effectively a singleton after initialization, consider caching the processor instance (or at least the executionRouter) to avoid per-request allocation pressure on the hot IPC path.</violation>
<violation number="2" location="Packages/src/Editor/Infrastructure/Api/JsonRpcProcessor.cs:44">
P2: The static JSON-RPC compatibility entrypoint can now throw before producing a JSON-RPC response when the application registrar has not been registered yet. Because CreateProcessor reads ApplicationRegistrar.Service before request parsing/error handling, callers not yet migrated to DI lose the previous parse/protocol error behavior; consider deferring service lookup until actual command execution or providing a registered fallback for this path.</violation>
</file>
<file name="Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs">
<violation number="1" location="Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs:163">
P2: Optional capability flags should be tolerant to malformed or future values, but direct `Value<bool>()` conversion can hard-fail request parsing. Checking `JTokenType.Boolean` before reading and defaulting to false would keep this negotiation path robust.</violation>
<violation number="2" location="Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs:207">
P3: HandleFocusWindowNotification() is an empty method with only a comment saying it's intentionally empty because focus-window is handled at the OS level by the CLI. Since the switch case for `focus-window` already calls this method, and the method does nothing, consider either removing the switch case entirely (focus-window notification would fall through to the default case and be silently ignored) or inline the explanation as a comment on the case itself. An empty method with only one call site is unnecessary indirection.</violation>
<violation number="3" location="Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs:460">
P2: If result serialization fails, this path still returns a JSON-RPC success response, so callers can interpret a failed execution as a completed request. Using a normal JSON-RPC error response here would preserve consistent failure handling.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| this.jsonrpc = jsonRpc; | ||
| this.id = id; | ||
| this.error = error; | ||
| UnityCliLoopToolRegistrarService toolRegistrarService = ApplicationRegistrar.Service; |
There was a problem hiding this comment.
P2: The static JSON-RPC compatibility entrypoint can now throw before producing a JSON-RPC response when the application registrar has not been registered yet. Because CreateProcessor reads ApplicationRegistrar.Service before request parsing/error handling, callers not yet migrated to DI lose the previous parse/protocol error behavior; consider deferring service lookup until actual command execution or providing a registered fallback for this path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/Infrastructure/Api/JsonRpcProcessor.cs, line 45:
<comment>The static JSON-RPC compatibility entrypoint can now throw before producing a JSON-RPC response when the application registrar has not been registered yet. Because CreateProcessor reads ApplicationRegistrar.Service before request parsing/error handling, callers not yet migrated to DI lose the previous parse/protocol error behavior; consider deferring service lookup until actual command execution or providing a registered fallback for this path.</comment>
<file context>
@@ -1,706 +1,50 @@
- this.jsonrpc = jsonRpc;
- this.id = id;
- this.error = error;
+ UnityCliLoopToolRegistrarService toolRegistrarService = ApplicationRegistrar.Service;
+ UnityCliLoopExecutionRouter executionRouter = new(toolRegistrarService);
+ return new JsonRpcRequestProcessor(executionRouter);
</file context>
| return false; | ||
| } | ||
|
|
||
| return metadata["acceptsDispatchAck"]?.Value<bool>() ?? false; |
There was a problem hiding this comment.
P2: Optional capability flags should be tolerant to malformed or future values, but direct Value<bool>() conversion can hard-fail request parsing. Checking JTokenType.Boolean before reading and defaulting to false would keep this negotiation path robust.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs, line 163:
<comment>Optional capability flags should be tolerant to malformed or future values, but direct `Value<bool>()` conversion can hard-fail request parsing. Checking `JTokenType.Boolean` before reading and defaulting to false would keep this negotiation path robust.</comment>
<file context>
@@ -0,0 +1,715 @@
+ return false;
+ }
+
+ return metadata["acceptsDispatchAck"]?.Value<bool>() ?? false;
+ }
+
</file context>
| ); | ||
| return JsonConvert.SerializeObject(response, Formatting.None, ResponseSerializerSettings); | ||
| } | ||
| catch (Exception) |
There was a problem hiding this comment.
P2: If result serialization fails, this path still returns a JSON-RPC success response, so callers can interpret a failed execution as a completed request. Using a normal JSON-RPC error response here would preserve consistent failure handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs, line 460:
<comment>If result serialization fails, this path still returns a JSON-RPC success response, so callers can interpret a failed execution as a completed request. Using a normal JSON-RPC error response here would preserve consistent failure handling.</comment>
<file context>
@@ -0,0 +1,715 @@
+ );
+ return JsonConvert.SerializeObject(response, Formatting.None, ResponseSerializerSettings);
+ }
+ catch (Exception)
+ {
+ // Return safe fallback response for any serialization errors
</file context>
| /// Note: focus-window is handled at OS level by the CLI using native process control. | ||
| /// This notification handler remains for protocol compatibility but does nothing. | ||
| /// </summary> | ||
| private static void HandleFocusWindowNotification() |
There was a problem hiding this comment.
P3: HandleFocusWindowNotification() is an empty method with only a comment saying it's intentionally empty because focus-window is handled at the OS level by the CLI. Since the switch case for focus-window already calls this method, and the method does nothing, consider either removing the switch case entirely (focus-window notification would fall through to the default case and be silently ignored) or inline the explanation as a comment on the case itself. An empty method with only one call site is unnecessary indirection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs, line 207:
<comment>HandleFocusWindowNotification() is an empty method with only a comment saying it's intentionally empty because focus-window is handled at the OS level by the CLI. Since the switch case for `focus-window` already calls this method, and the method does nothing, consider either removing the switch case entirely (focus-window notification would fall through to the default case and be silently ignored) or inline the explanation as a comment on the case itself. An empty method with only one call site is unnecessary indirection.</comment>
<file context>
@@ -0,0 +1,715 @@
+ /// Note: focus-window is handled at OS level by the CLI using native process control.
+ /// This notification handler remains for protocol compatibility but does nothing.
+ /// </summary>
+ private static void HandleFocusWindowNotification()
+ {
+ // Intentionally empty because focus-window is handled by the CLI at OS level.
</file context>
| public JsonRpcError error { get; } | ||
|
|
||
| public JsonRpcErrorResponse(string jsonRpc, object id, JsonRpcError error) | ||
| private static JsonRpcRequestProcessor CreateProcessor() |
There was a problem hiding this comment.
P3: The compatibility wrapper's CreateProcessor() constructs a new JsonRpcRequestProcessor (and underlying UnityCliLoopExecutionRouter) on every call. If ProcessRequest or ProcessRequestWithEarlyResponseAsync is called multiple times — which happens on every JSON-RPC request — this allocates a new processor, router, and references every time. Since the underlying toolRegistrarService (ApplicationRegistrar.Service) is effectively a singleton after initialization, consider caching the processor instance (or at least the executionRouter) to avoid per-request allocation pressure on the hot IPC path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/Infrastructure/Api/JsonRpcProcessor.cs, line 43:
<comment>The compatibility wrapper's CreateProcessor() constructs a new JsonRpcRequestProcessor (and underlying UnityCliLoopExecutionRouter) on every call. If ProcessRequest or ProcessRequestWithEarlyResponseAsync is called multiple times — which happens on every JSON-RPC request — this allocates a new processor, router, and references every time. Since the underlying toolRegistrarService (ApplicationRegistrar.Service) is effectively a singleton after initialization, consider caching the processor instance (or at least the executionRouter) to avoid per-request allocation pressure on the hot IPC path.</comment>
<file context>
@@ -1,706 +1,50 @@
- public JsonRpcError error { get; }
-
- public JsonRpcErrorResponse(string jsonRpc, object id, JsonRpcError error)
+ private static JsonRpcRequestProcessor CreateProcessor()
{
- this.jsonrpc = jsonRpc;
</file context>
Summary
User Impact
Changes
Verification
dist/darwin-arm64/uloop compile --project-path <PROJECT_ROOT>: 0 errors / 0 warnings.JsonRpcProcessorCliVersionGateTests|JsonRpcHeartbeatTests|UnityCliLoopToolRegistryTests|UnityCliLoopServerControllerStartupLockTests|UnityCliLoopServerStartupProtectionTests|UnityCliLoopBridgeServerShutdownTests|OnionAssemblyDependencyTests|StaticFacadeStateGuardTests.origin/v3-betaand outside this diff:InputVisualizationCanvasPrefabTests.RuntimeSources_WhenScanned_AreEditorOnly:Packages/src/Runtime/PausePoints/AssemblyInfo.csis already not wrapped in#if UNITY_EDITORon the base branch.UnityCliLoopSettingsWindowCliActionTestscases: the checked-in pin requiresminimumDispatcherVersion3.0.1-beta.6, while these expectations still treat3.0.0as the satisfied boundary.