Merged
Conversation
Contributor
Reviewer's GuideStandardizes MCP transport type configuration across clients, improves Unity console log entry iteration and error logging, and adjusts Kilo Code client layout configuration. Sequence diagram for command processing and error-aware logging in TransportCommandDispatchersequenceDiagram
participant Client
participant TransportCommandDispatcher
participant PendingCommand
participant Handler as CommandHandler
participant McpLogRecord
participant ErrorResponse
Client->>TransportCommandDispatcher: Send command
TransportCommandDispatcher->>PendingCommand: Create and register PendingCommand
TransportCommandDispatcher->>Handler: Execute command
Handler-->>TransportCommandDispatcher: result (may be ErrorResponse)
TransportCommandDispatcher->>TransportCommandDispatcher: Stop stopwatch
alt result is ErrorResponse
TransportCommandDispatcher->>ErrorResponse: Cast result to ErrorResponse
TransportCommandDispatcher->>McpLogRecord: Log(command.type, parameters, logType, ERROR, elapsedMs, errResp.Error)
else result is success
TransportCommandDispatcher->>McpLogRecord: Log(command.type, parameters, logType, SUCCESS, elapsedMs, null)
end
TransportCommandDispatcher->>PendingCommand: TrySetResult({ status = success, result })
PendingCommand-->>Client: Serialized JSON response
Sequence diagram for Unity console log entry iteration using ReadConsolesequenceDiagram
participant Tool as ReadConsole
participant LogEntriesAPI as UnityLogEntries
Tool->>LogEntriesAPI: Invoke StartGettingEntries()
LogEntriesAPI-->>Tool: startResult (entryCount or other)
alt startResult is int
Tool->>Tool: totalEntries = startResult
else startResult not int
Tool->>LogEntriesAPI: Invoke GetCount()
LogEntriesAPI-->>Tool: totalEntries
end
loop Iterate entries
Tool->>LogEntriesAPI: GetEntryInternal(index, logEntry)
LogEntriesAPI-->>Tool: logEntry data
Tool->>Tool: Process and optionally include stacktrace
end
Tool->>LogEntriesAPI: StopGettingEntries()
Flow diagram for MCP transport type configuration in ConfigJsonBuilderflowchart TD
A[Start PopulateUnityNode] --> B{Endpoint uses HTTP?}
B -- Yes --> C{Client is Cline?}
C -- Yes --> D[Set unity.type = streamableHttp]
C -- No --> E[Set unity.type = http]
D --> F[Ensure url/serverUrl set for HTTP]
E --> F[Ensure url/serverUrl set for HTTP]
B -- No (stdio) --> G[Remove url and serverUrl from unity]
G --> H[Set unity.type = stdio]
F --> I[Continue building config]
H --> I[Continue building config]
I --> J[End PopulateUnityNode]
%% Kilo Code layout
K[Configure KiloCode client] --> L[Set IsVsCodeLayout = false]
L --> M[Use generic MCP layout instead of VSCode-specific]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThis PR removes VSCode-specific configuration logic throughout the codebase, standardizes the "type" field in MCP JSON transport configurations to always be present, enhances error logging in command processing, and refines how console entry counts are determined during iteration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 5❌ Failed checks (1 warning, 4 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In ReadConsole, if StartGettingEntries() ever changes to return a non-int, the fallback to GetCount() silently reverts to the old behavior; consider explicitly handling or logging the unexpected return type so issues are easier to diagnose when Unity internals change.
- The logging in TransportCommandDispatcher only treats ErrorResponse as an error case; if other response types can indicate failures, you may want to centralize status determination instead of hardcoding a single error type to keep logs consistent with actual outcomes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In ReadConsole, if StartGettingEntries() ever changes to return a non-int, the fallback to GetCount() silently reverts to the old behavior; consider explicitly handling or logging the unexpected return type so issues are easier to diagnose when Unity internals change.
- The logging in TransportCommandDispatcher only treats ErrorResponse as an error case; if other response types can indicate failures, you may want to centralize status determination instead of hardcoding a single error type to keep logs consistent with actual outcomes.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs" line_range="406-413" />
<code_context>
sw?.Stop();
- McpLogRecord.Log(command.type, parameters, logType, "SUCCESS", sw?.ElapsedMilliseconds ?? 0);
+
+ string syncLogStatus = "SUCCESS";
+ string syncLogError = null;
+ if (result is ErrorResponse errResp)
+ {
+ syncLogStatus = "ERROR";
+ syncLogError = errResp.Error;
+ }
+ McpLogRecord.Log(command.type, parameters, logType, syncLogStatus, sw?.ElapsedMilliseconds ?? 0, syncLogError);
var response = new { status = "success", result };
</code_context>
<issue_to_address>
**issue (bug_risk):** Logged error status is inconsistent with the success status returned to the caller.
Because `syncLogStatus` can be "ERROR" while the returned anonymous object always has `status = "success"`, external callers will treat error payloads as successful responses, while logs treat them as failures. Unless this is deliberate, update the returned `status` to reflect `ErrorResponse` (e.g., reuse `syncLogStatus` or map `ErrorResponse` to a non-success status).
</issue_to_address>
### Comment 2
<location path="TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs" line_range="431-434" />
<code_context>
- Assert.IsNull(unity["type"],
- "Non-VSCode entries should not include type metadata in HTTP mode");
- }
+ // "type" is now included for all clients (standard MCP protocol field).
+ Assert.AreEqual("http", (string)unity["type"],
+ "All entries should advertise HTTP transport type");
}
</code_context>
<issue_to_address>
**issue (bug_risk):** AssertTransportConfiguration now assumes all HTTP entries use "http" type, which contradicts the new Cline "streamableHttp" behavior.
This helper now conflicts with `ConfigJsonBuilder`, which sets `"streamableHttp"` for Cline. If tests use `AssertTransportConfiguration` with a Cline `McpClient`, the assertion will be incorrect and mask the client-specific behavior.
Please either make the assertion depend on the client type (e.g., `"streamableHttp"` for Cline, `"http"` for others) or introduce a separate helper for client-specific expectations. Also ensure there is at least one test that validates a Cline HTTP configuration end-to-end against `"streamableHttp"`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+406
to
+413
| string syncLogStatus = "SUCCESS"; | ||
| string syncLogError = null; | ||
| if (result is ErrorResponse errResp) | ||
| { | ||
| syncLogStatus = "ERROR"; | ||
| syncLogError = errResp.Error; | ||
| } | ||
| McpLogRecord.Log(command.type, parameters, logType, syncLogStatus, sw?.ElapsedMilliseconds ?? 0, syncLogError); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Logged error status is inconsistent with the success status returned to the caller.
Because syncLogStatus can be "ERROR" while the returned anonymous object always has status = "success", external callers will treat error payloads as successful responses, while logs treat them as failures. Unless this is deliberate, update the returned status to reflect ErrorResponse (e.g., reuse syncLogStatus or map ErrorResponse to a non-success status).
Comment on lines
+431
to
434
| // "type" is now included for all clients (standard MCP protocol field). | ||
| Assert.AreEqual("http", (string)unity["type"], | ||
| "All entries should advertise HTTP transport type"); | ||
| } |
Contributor
There was a problem hiding this comment.
issue (bug_risk): AssertTransportConfiguration now assumes all HTTP entries use "http" type, which contradicts the new Cline "streamableHttp" behavior.
This helper now conflicts with ConfigJsonBuilder, which sets "streamableHttp" for Cline. If tests use AssertTransportConfiguration with a Cline McpClient, the assertion will be incorrect and mask the client-specific behavior.
Please either make the assertion depend on the client type (e.g., "streamableHttp" for Cline, "http" for others) or introduce a separate helper for client-specific expectations. Also ensure there is at least one test that validates a Cline HTTP configuration end-to-end against "streamableHttp".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs (1)
431-434: Tests correctly updated for the new always-presenttypefield.The assertion aligns with the
ConfigJsonBuilderchanges. However,ConfigJsonBuildersetstype = "streamableHttp"for Cline clients, while this helper asserts"http". Since no existing tests use a Cline client, they will pass, but consider adding a Cline-specific test case to verify the"streamableHttp"value.📝 Suggested addition: Cline-specific test case
Add a test for Cline HTTP transport configuration:
[Test] public void UsesStreamableHttpType_ForCline() { var configPath = Path.Combine(_tempRoot, "cline.json"); WriteInitialConfig(configPath, isVSCode: false, command: _fakeUvPath, directory: "/old/path"); var client = new McpClient { name = "Cline" }; InvokeWriteToConfig(configPath, client); var root = JObject.Parse(File.ReadAllText(configPath)); var unity = (JObject)root.SelectToken("mcpServers.unityMCP"); Assert.NotNull(unity, "Expected mcpServers.unityMCP node"); Assert.AreEqual("streamableHttp", (string)unity["type"], "Cline should use streamableHttp transport type"); }And update
AssertTransportConfigurationto handle Cline:if (useHttp) { + bool isCline = string.Equals(client.name, "Cline", StringComparison.OrdinalIgnoreCase); string expectedUrl = HttpEndpointUtility.GetMcpRpcUrl(); // ... existing url assertions ... - Assert.AreEqual("http", (string)unity["type"], - "All entries should advertise HTTP transport type"); + string expectedType = isCline ? "streamableHttp" : "http"; + Assert.AreEqual(expectedType, (string)unity["type"], + $"HTTP transport type should be {expectedType}"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs` around lines 431 - 434, The helper currently asserts every client's "type" equals "http" but ConfigJsonBuilder sets Cline clients to "streamableHttp"; add a new test UsesStreamableHttpType_ForCline that creates a McpClient named "Cline", calls WriteInitialConfig and InvokeWriteToConfig, parses the resulting JSON and asserts mcpServers.unityMCP.type == "streamableHttp"; also update the shared helper AssertTransportConfiguration (or its caller) to allow a different expected transport type (e.g., add an expectedType parameter or branch on client.name == "Cline") so the assertion no longer always expects "http".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs`:
- Around line 431-434: The helper currently asserts every client's "type" equals
"http" but ConfigJsonBuilder sets Cline clients to "streamableHttp"; add a new
test UsesStreamableHttpType_ForCline that creates a McpClient named "Cline",
calls WriteInitialConfig and InvokeWriteToConfig, parses the resulting JSON and
asserts mcpServers.unityMCP.type == "streamableHttp"; also update the shared
helper AssertTransportConfiguration (or its caller) to allow a different
expected transport type (e.g., add an expectedType parameter or branch on
client.name == "Cline") so the assertion no longer always expects "http".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bbe8395-1ddf-4fbe-b362-a0486b314bde
📒 Files selected for processing (5)
MCPForUnity/Editor/Clients/Configurators/KiloCodeConfigurator.csMCPForUnity/Editor/Helpers/ConfigJsonBuilder.csMCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.csMCPForUnity/Editor/Tools/ReadConsole.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Unity MCP client configuration generation and related tests to consistently emit a transport type field across client layouts, while also improving console log reading reliability and command execution logging.
Changes:
- Standardize generated MCP config to always include a transport
type(http/stdio), with a Cline-specificstreamableHttpexception. - Update config-writing tests to expect the
typefield for both HTTP and stdio modes. - Improve
ReadConsolelog entry counting and enhance command dispatcher logging to record sync error status/details.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs | Updates transport assertions to always expect type for HTTP/stdio. |
| MCPForUnity/Editor/Tools/ReadConsole.cs | Uses StartGettingEntries() return value as entry count when available to avoid stale counts. |
| MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs | Logs sync command results as ERROR when an ErrorResponse is returned (including error string). |
| MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs | Always emits unity.type for transport; special-cases Cline to streamableHttp in HTTP mode. |
| MCPForUnity/Editor/Clients/Configurators/KiloCodeConfigurator.cs | Switches Kilo Code to non-VSCode layout (mcpServers) behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+432
to
+433
| Assert.AreEqual("http", (string)unity["type"], | ||
| "All entries should advertise HTTP transport type"); |
There was a problem hiding this comment.
AssertTransportConfiguration() now assumes HTTP always uses type=="http". Production config generation has a Cline-specific exception (type=="streamableHttp"), so this helper is no longer generally correct for all clients supported by the repo. Consider making the expected type conditional on the client (or passing expectedType in) to keep the helper aligned with ConfigJsonBuilder.
Suggested change
| Assert.AreEqual("http", (string)unity["type"], | |
| "All entries should advertise HTTP transport type"); | |
| Assert.IsTrue((string)unity["type"] == "http" || (string)unity["type"] == "streamableHttp", | |
| "All HTTP entries should advertise a supported HTTP transport type (\"http\" or \"streamableHttp\")"); |
Comment on lines
+95
to
105
| // Cline expects streamableHttp for HTTP endpoints. | ||
| if (isCline) | ||
| { | ||
| unity["type"] = "http"; | ||
| unity["type"] = "streamableHttp"; | ||
| } | ||
| // Also add type for Claude Code (uses mcpServers layout but needs type field) | ||
| else if (client?.name == "Claude Code") | ||
| else | ||
| { | ||
| // "type" is standard MCP protocol; include for all clients to avoid | ||
| // clients that default to SSE when they see a URL without a type field. | ||
| unity["type"] = "http"; | ||
| } |
There was a problem hiding this comment.
This change introduces/retains a Cline-specific transport type ("streamableHttp") when using HTTP. There isn’t corresponding test coverage in WriteToConfigTests to lock in this behavior, so it’s easy to regress while refactoring the config builder. Consider adding a test case for a McpClient{name="Cline"} that asserts the generated config uses type=="streamableHttp" in HTTP mode (and still uses stdio type when HTTP is disabled).
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
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.
Config-related fix that should fix #909, #820
Related Issues
Additional Notes
Summary by Sourcery
Standardize MCP transport configuration metadata and improve logging behavior for console reading and command dispatch.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Bug Fixes
Refactor