chore: extract JSON-RPC response construction into a factory

[hatayama]

Refs: Refactor round 3 ToDo R3-5 (2026-07-09_Unity CLI Loop リファクタ第3弾 ToDo.md)

Summary

• JsonRpcRequestProcessor mixed request parse/dispatch with JSON-RPC response construction (success / error / dispatch-accepted / heartbeat / protocol-mismatch) and 10 co-located response DTOs.
• Moved all response construction into a new stateless JsonRpcResponseFactory and split the 10 DTOs into class-per-file. The processor now owns only parse / dispatch / notification / cancellation handling.
• CreateSuccessResponse / CreateErrorResponse / CreateCliProtocolMismatchResponse / AppendTimingIfRequested widen from private to internal because the processor now calls them across the class boundary. The already-internal CreateDispatchAcceptedResponse / CreateHeartbeatResponse and all DTO visibility are unchanged (byte-equal).
• No wire shape change: protocol version is untouched.

Verification

• Commit 1 added wire-shape characterization tests fixing the exact serialized JSON string for all 5 response categories (success / error / dispatch-accepted x2 / heartbeat / protocol-mismatch) against the pre-move processor.
• Commit 2 performed the pure move; the same characterization tests now target JsonRpcResponseFactory and stayed green unchanged, proving byte equality.
• Normalized two-way diff: residual lines are exactly the approved adaptation set (call sites qualified with JsonRpcResponseFactory., and 4 methods widened private→internal). No logic/body differences.
• baseline 53/53 -> head 54/54 (JsonRpcHeartbeatTests / JsonRpcRequestProcessorCliVersionGateTests / JsonRpcResponseFactoryWireShapeCharacterizationTests / StaticFacadeStateGuardTests / DefaultToolsCatalogDriftTests), Unity compile 0 errors / 0 warnings.
• No mutable static state was moved, so StaticFacadeStateGuardTests tracked paths are unchanged.

Test plan

• uloop compile -- 0 errors / 0 warnings
• uloop run-tests on JSON-RPC related suites -- all green
• CI

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR extracts JSON-RPC response construction from JsonRpcRequestProcessor into a new JsonRpcResponseFactory, introduces new error-data DTOs (SecurityBlockedErrorData, InternalErrorData, ServerBusyErrorData, CliUpdateRequiredErrorData) and response types (JsonRpcError, JsonRpcErrorResponse, JsonRpcSuccessResponse, JsonRpcResponsePhases), and updates/adds tests validating exact wire-shape output.

Changes

Response factory extraction

Layer / File(s)	Summary
New JSON-RPC error and response DTOs Packages/src/Editor/Infrastructure/Api/JsonRpcErrorData.cs, JsonRpcErrorTypes.cs, JsonRpcResponsePhases.cs, SecurityBlockedErrorData.cs, InternalErrorData.cs, ServerBusyErrorData.cs, CliUpdateRequiredErrorData.cs, JsonRpcError.cs, JsonRpcErrorResponse.cs, JsonRpcSuccessResponse.cs	Adds an abstract error-data base class, concrete error-data subtypes, error-type constants, response phase constants, and success/error response DTOs.
JsonRpcResponseFactory implementation Packages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.cs	Adds a static factory with methods for success, error, dispatch-accepted, heartbeat, and CLI protocol-mismatch responses, plus timing augmentation logic.
JsonRpcRequestProcessor refactor Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs	Removes embedded response/error DTOs and local helpers; delegates response creation to JsonRpcResponseFactory throughout exception handling, dispatch-accepted, heartbeat, and success/error paths.
Wire-shape tests Assets/Tests/Editor/JsonRpcHeartbeatTests.cs, Assets/Tests/Editor/JsonRpcResponseFactoryWireShapeCharacterizationTests.cs	Updates heartbeat tests to build expected payloads via the factory, and adds a new characterization test suite with deterministic test doubles asserting exact JSON output for success, error, dispatch-accepted, heartbeat, and protocol-mismatch responses.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant JsonRpcRequestProcessor
  participant JsonRpcResponseFactory
  participant UserFriendlyErrorConverter

  JsonRpcRequestProcessor->>JsonRpcResponseFactory: CreateSuccessResponse(result)
  JsonRpcResponseFactory-->>JsonRpcRequestProcessor: success JSON

  JsonRpcRequestProcessor->>JsonRpcResponseFactory: CreateErrorResponse(exception)
  JsonRpcResponseFactory->>UserFriendlyErrorConverter: convert exception
  UserFriendlyErrorConverter-->>JsonRpcResponseFactory: friendly message
  JsonRpcResponseFactory-->>JsonRpcRequestProcessor: error JSON

  JsonRpcRequestProcessor->>JsonRpcResponseFactory: CreateDispatchAcceptedResponse(heartbeatIntervalSeconds)
  JsonRpcResponseFactory-->>JsonRpcRequestProcessor: accepted JSON

  JsonRpcRequestProcessor->>JsonRpcResponseFactory: CreateCliProtocolMismatchResponse(currentProtocolVersion)
  JsonRpcResponseFactory-->>JsonRpcRequestProcessor: cli_update_required JSON

Loading

Possibly related PRs

• hatayama/unity-cli-loop#1164: Introduces the server_busy JSON-RPC error model via JsonRpcResponseFactory/ServerBusyErrorData that the retrieved PR consumes during dispatch/concurrency handling.
• hatayama/unity-cli-loop#1329: Both touch the cli_update_required protocol-mismatch path via CliUpdateRequiredErrorData and JsonRpcErrorTypes.
• hatayama/unity-cli-loop#1558: Both modify JsonRpcHeartbeatTests.cs, changing which response construction path is used for dispatch-accepted and heartbeat payloads.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes extracting JSON-RPC response construction into a factory, which matches the main change.
Description check	✅ Passed	The description accurately explains the refactor, verification, and lack of wire-format changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/hatayama/extract-jsonrpc-response-factory

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

Packages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.cs (2)

97-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider extracting dispatch-accepted and heartbeat response shapes into named DTOs.

CreateDispatchAcceptedResponse and CreateHeartbeatResponse build wire shapes via anonymous types, while CreateSuccessResponse, CreateErrorResponse, and CreateCliProtocolMismatchResponse use named DTOs (JsonRpcSuccessResponse, JsonRpcErrorResponse). The anonymous approach works but makes the uloop metadata contract implicit rather than explicit, and the shapes can't be referenced in tests or documentation by type.

If the anonymous types are intentional to preserve exact wire shape (e.g., the uloop field isn't part of the standard DTOs), consider adding lightweight named DTOs for these two shapes in a future PR.

Also applies to: 127-145

🤖 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 97 - 125, The dispatch-accepted and heartbeat response payloads are
currently built with anonymous types, which makes the `uloop` contract implicit
and hard to reuse or test. Refactor `CreateDispatchAcceptedResponse` and
`CreateHeartbeatResponse` in `JsonRpcResponseFactory` to use lightweight named
DTOs for the wire shapes, similar to `JsonRpcSuccessResponse` and
`JsonRpcErrorResponse`, while preserving the exact serialized JSON structure for
`accepted`, `phase`, and `heartbeatIntervalSeconds`.

---

37-52: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider logging serialization failures in the fallback catch block.

The broad catch (Exception) silently swallows serialization errors and returns a fallback response. While the fallback behavior is correct (client always gets a valid JSON-RPC frame), the lack of any logging makes serialization issues impossible to diagnose in production. Based on learnings, the Fail Fast policy encourages surfacing unexpected errors rather than silently swallowing them.

A Debug.LogWarning here would preserve the fallback behavior while making failures observable.

🔊 Suggested addition

             catch (Exception ex)
             {
+                Debug.LogWarning($"JSON-RPC serialization failed for id={id}, falling back to safe response: {ex}");
                 // Return safe fallback response for any serialization errors
                 object fallbackResult = new

🤖 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 - 52, The JsonRpcResponseFactory serialization fallback in the catch
block is swallowing exceptions without any observability; keep the safe fallback
response but add a Debug.LogWarning when serialization fails. Use the existing
JsonRpcResponseFactory logic around the JsonConvert.SerializeObject path to log
the exception details and relevant context (such as the response type or
commandType) before returning the fallback JSON-RPC frame.

Source: Learnings

🤖 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 `@Packages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.cs`:
- Around line 97-125: The dispatch-accepted and heartbeat response payloads are
currently built with anonymous types, which makes the `uloop` contract implicit
and hard to reuse or test. Refactor `CreateDispatchAcceptedResponse` and
`CreateHeartbeatResponse` in `JsonRpcResponseFactory` to use lightweight named
DTOs for the wire shapes, similar to `JsonRpcSuccessResponse` and
`JsonRpcErrorResponse`, while preserving the exact serialized JSON structure for
`accepted`, `phase`, and `heartbeatIntervalSeconds`.
- Around line 37-52: The JsonRpcResponseFactory serialization fallback in the
catch block is swallowing exceptions without any observability; keep the safe
fallback response but add a Debug.LogWarning when serialization fails. Use the
existing JsonRpcResponseFactory logic around the JsonConvert.SerializeObject
path to log the exception details and relevant context (such as the response
type or commandType) before returning the fallback JSON-RPC frame.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 670816db-3b86-4f12-afab-81d23056023e

📥 Commits

Reviewing files that changed from the base of the PR and between a04855d and 2534b32.

⛔ Files ignored due to path filters (12)

• Assets/Tests/Editor/JsonRpcResponseFactoryWireShapeCharacterizationTests.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/CliUpdateRequiredErrorData.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/InternalErrorData.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/JsonRpcError.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/JsonRpcErrorData.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/JsonRpcErrorResponse.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/JsonRpcErrorTypes.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/JsonRpcResponsePhases.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/JsonRpcSuccessResponse.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/SecurityBlockedErrorData.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/Api/ServerBusyErrorData.cs.meta is excluded by none and included by none

📒 Files selected for processing (14)

• Assets/Tests/Editor/JsonRpcHeartbeatTests.cs
• Assets/Tests/Editor/JsonRpcResponseFactoryWireShapeCharacterizationTests.cs
• Packages/src/Editor/Infrastructure/Api/CliUpdateRequiredErrorData.cs
• Packages/src/Editor/Infrastructure/Api/InternalErrorData.cs
• Packages/src/Editor/Infrastructure/Api/JsonRpcError.cs
• Packages/src/Editor/Infrastructure/Api/JsonRpcErrorData.cs
• Packages/src/Editor/Infrastructure/Api/JsonRpcErrorResponse.cs
• Packages/src/Editor/Infrastructure/Api/JsonRpcErrorTypes.cs
• Packages/src/Editor/Infrastructure/Api/JsonRpcRequestProcessor.cs
• Packages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.cs
• Packages/src/Editor/Infrastructure/Api/JsonRpcResponsePhases.cs
• Packages/src/Editor/Infrastructure/Api/JsonRpcSuccessResponse.cs
• Packages/src/Editor/Infrastructure/Api/SecurityBlockedErrorData.cs
• Packages/src/Editor/Infrastructure/Api/ServerBusyErrorData.cs

[hatayama]

Thanks for the review, CodeRabbit.

Both nitpicks are valid design ideas but out of scope for this PR:

1. Named DTOs for dispatch-accepted/heartbeat shapes — R3-5's explicit requirement was a byte-for-byte pure move (frozen via JsonRpcResponseFactoryWireShapeCharacterizationTests), not a design change. The anonymous types were carried over unchanged from the original JsonRpcRequestProcessor code. You already noted this could be a follow-up PR — agreed, not doing it here to keep this PR a pure move.

2. Logging in the serialization fallback catch block — this catch (Exception) block is pre-existing behavior moved verbatim, not introduced by this PR. Changing catch-block behavior is outside a pure-move refactor's scope, and per this repo's exception policy, try-catch changes need explicit discussion before altering behavior. Leaving as a possible future improvement rather than bundling into this refactor.

No code changes made for either point.