HTTP Server, uvx, C# only custom tools #375
Conversation
We'll reference the remote server in GitHub and configure clients to use `uvx`
- Replaced local server execution with uvx package-based configuration for improved reliability - Added GetUvxCommand helper to generate correct package version command string - Updated config generation to use `uvx mcp-for-unity` instead of local Python server - Modified Codex and client configuration validation to support uvx-based setup - Removed unused server source directory handling and related preferences - Updated tests to verify uvx command generation
We don't commit temp folders, tests are expected to clean up after themselves
…comparisons are not precise
- Replaced local server path detection with uvx-based package installation from git repository - Updated all configuration generators to use structured uvx command parts (command, --from URL, package) - Renamed UV path references to UVX for clarity and consistency - Added GetUvxCommandParts() helper to centralize uvx command generation - Added GetMcpServerGitUrl() to handle git repository URL construction - Updated client configuration validation
- Added GetUvxPackageSourcePath method to locate unity-mcp package in uv cache by traversing git checkouts - Replaced hardcoded "Dummy" path in PythonToolSyncProcessor with dynamic path resolution - Added validation for Server directory structure and pyproject.toml to ensure correct package location
Key thing is that MCPForUnity/UnityMcpServer/src is still deleted
…ystem - Removed PythonToolsAsset and file-based sync processor in favor of attribute-based tool discovery - Implemented CustomToolRegistrationProcessor with automatic registration on startup and script reload - Added registration enable/disable preference and force re-registration capability
- Implemented HTTP transport option with configurable URL/port alongside existing stdio mode - Added cache management service with menu item to clear uvx package cache - Updated config builder to generate transport-specific arguments and VSCode type field based on selected mode
- Replaced separate host/port arguments with single --http-url parameter for cleaner configuration - Updated server to parse URL and allow individual host/port overrides when needed - Consolidated HTTP client implementation with connection testing and tool execution support
…rt flag - Replaced --enable-http-server flag with --transport choice parameter (stdio/http) for clearer intent - Removed redundant HTTP port field from UI since HTTP mode uses the same URL/port as MCP client - Simplified server startup logic by consolidating transport mode determination
- Changed HTTP mode to use URL-based configuration instead of command-line arguments - Added proper cleanup of incompatible fields when switching between stdio and HTTP transports - Moved uvx command parsing inside stdio-specific block to avoid unnecessary processing in HTTP mode
- Implemented server management service with menu item to start local HTTP server in new terminal window - Added Git URL override setting in advanced configuration to allow custom server source for uvx --from - Integrated server management into service locator with validation for local-only server startup
- Removed auto-prefixing logic that added "http://" to URLs without protocol - Added placeholder text to guide users on expected URL format - Created dedicated url-field style class for better URL input styling
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (5)
You can disable this status message by setting the WalkthroughThis PR retargets CI to Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Unity Editor
participant BCS as BridgeControlService
participant TM as TransportManager
participant Client as IMcpTransportClient
participant Server as MCP Server
Editor->>BCS: StartAsync()
BCS->>TM: StartAsync(preferredMode)
TM->>Client: Instantiate & StartAsync()
Client->>Server: Connect / Register
Server-->>Client: Welcome / Registered
Client-->>TM: State(Connected)
TM-->>BCS: Started
BCS-->>Editor: Connected
sequenceDiagram
participant Editor as Unity Editor
participant PathSvc as PathResolverService
participant Detector as PlatformDetector
participant System as OS PATH
Editor->>PathSvc: GetUvxPath(verify=true)
alt EditorPref override present
PathSvc-->>Editor: return override
else
PathSvc->>Detector: IsUvxDetected()
Detector->>System: run 'uvx'/'uv' --version
System-->>Detector: version output
Detector-->>PathSvc: found path+version
PathSvc-->>Editor: uvx path
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Potential hotspots to focus review on:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
- Added initialize, ping, and disconnect methods to HttpMcpClient for proper MCP protocol session management - Implemented session ID tracking and header management for stateful HTTP connections - Added cross-platform terminal launcher support for Windows and Linux (previously macOS-only)
- Added proper JSON-RPC 2.0 request/response handling with request ID tracking - Included MCP protocol headers (version, session ID) for standard compliance - Added error handling for JSON-RPC error responses
- Added white-space: normal and flex-shrink properties to section headers and override labels to prevent text overflow - Created new help-text style class for consistent formatting of help text elements
- Added flex-wrap to setting rows to prevent overflow on narrow windows - Set flex-shrink: 0 on labels to maintain consistent width - Replaced max-width and margin-left with flex-basis for better flex behavior
- Capture Unity API calls on main thread before async operations to prevent threading issues - Update RegisterAllTools to use Task.Run pattern instead of GetAwaiter().GetResult() to avoid potential deadlocks - Add optional projectId parameter to RegisterAllToolsAsync for pre-captured values
…d resources Add disposal guard and call StopAsync() in Dispose() to prevent race conditions when disposing the WebSocket transport while background loops are still running. Log warnings if cleanup fails but continue with resource disposal.
…ag to prevent race conditions
… WebSocket message receiving Rent buffer from ArrayPool<byte>.Shared instead of allocating new byte arrays for each receive operation. Pre-size MemoryStream to 8192 bytes and ensure rented buffer is returned in finally block to prevent memory leaks.
By the next version stdio will use Websockets as well, so why be redundant
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
8-12: TheverifyPathparameter appears unused in the implementation.The
GetUvxPath(bool verifyPath = true)signature includes averifyPathparameter, but the implementation inPathResolverService.cs(lines 20-36) doesn't use it—it simply returns the override or defaults to"uvx"without any verification. This could mislead consumers who expect path validation whenverifyPath=true.Consider either implementing the verification logic or removing the parameter if it's not needed.
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)
122-164: Consider: ExplicitverifyPathparameter for consistency.Line 152 calls
GetUvxPath()without explicitly passing theverifyPathparameter, relying on the default value. For consistency with line 99 inWriteMcpConfiguration, consider explicitly passingverifyPath: true.- string uvxPath = MCPServiceLocator.Paths.GetUvxPath(); + string uvxPath = MCPServiceLocator.Paths.GetUvxPath(verifyPath: true);MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (3)
322-326: Reconnect on graceful close may be overly aggressive.Line 324 triggers reconnection for any WebSocket close message, including
NormalClosureinitiated by the server (e.g., during maintenance). Consider distinguishing close statuses: reconnect aggressively for unexpected closures, but handle gracefulNormalClosuremore conservatively (e.g., log and remain disconnected, or attempt a single reconnect without the full backoff schedule).Example approach:
if (result.MessageType == WebSocketMessageType.Close) { - await HandleSocketClosureAsync(result.CloseStatusDescription ?? "Server closed connection").ConfigureAwait(false); + string reason = result.CloseStatusDescription ?? "Server closed connection"; + if (result.CloseStatus == WebSocketCloseStatus.NormalClosure) + { + McpLog.Info($"[WebSocket] Server closed connection gracefully: {reason}"); + // Optionally exit without reconnect, or implement single-attempt reconnect + } + else + { + await HandleSocketClosureAsync(reason).ConfigureAwait(false); + } return null; }
131-155: VerifyAsync doesn't guard against mid-reconnection state.
VerifyAsyncchecks_lifecycleCtsand socket state but doesn't account for active reconnection. During reconnection,_lifecycleCtsremains valid and a new socket might be open, so the checks pass. However, the connection might be in an unstable state (e.g.,_sessionIdpending,_connectionCtsjust cancelled/recreated). Consider checking_isReconnectingFlagor returningfalseif_sessionIdis null/pending.public async Task<bool> VerifyAsync() { + if (Interlocked.CompareExchange(ref _isReconnectingFlag, 0, 0) != 0) + { + return false; // Reconnection in progress + } + if (_socket == null || _socket.State != WebSocketState.Open) { return false; }
387-403: ApplyWelcome keep-alive calculation is dense.Lines 396-402 compute
_socketKeepAliveIntervalwith nested conditionals and ternary operators. The logic is correct but could be clearer. Consider adding comments explaining the intent (e.g., "Ensure socket keep-alive is less than server timeout to prevent premature closure") or refactoring into named local variables.Example refactor for clarity:
int? serverTimeoutSeconds = payload.Value<int?>("serverTimeout"); if (serverTimeoutSeconds.HasValue) { - int sourceSeconds = keepAliveSeconds ?? serverTimeoutSeconds.Value; - int safeSeconds = Math.Max(5, Math.Min(serverTimeoutSeconds.Value, sourceSeconds)); + // Use the shorter of keepAlive or serverTimeout as baseline, with a 5-second floor + int baselineSeconds = keepAliveSeconds ?? serverTimeoutSeconds.Value; + int clampedSeconds = Math.Max(5, Math.Min(serverTimeoutSeconds.Value, baselineSeconds)); - _socketKeepAliveInterval = TimeSpan.FromSeconds(safeSeconds); + _socketKeepAliveInterval = TimeSpan.FromSeconds(clampedSeconds); }MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (1)
20-48: uv PATH-based detection is solid; double-checkPathsemantics and timeout handlingThe new
DetectUv/TryFindUvInPathflow is straightforward and consistent with the Python checks, but two nuances are worth calling out:
DependencyStatus.Pathis now set to the bare command ("uvx"or"uv") rather than a fully-qualified path. If any downstream logic assumesPathis an absolute executable path (vs. just something invocable viaPATH), this could be a behavioral change. If it’s only used for display/logging, you’re fine; otherwise, consider resolving to the actual path (e.g., via a platform-specificwhere/whichorProcess.MainModule.FileNamewhen available).- You call
process.StandardOutput.ReadToEnd()beforeWaitForExit(5000)and then accessExitCode. In pathological cases where the process hangs,WaitForExitcan time out andExitCodeaccess may throw. The outertry/catchwill swallow this, but to avoid surprises you might want to check thatWaitForExitreturnedtruebefore inspectingExitCode.Please verify how
DependencyStatus.Pathis consumed elsewhere (especially for uv) to ensure the new semantics don’t introduce regressions.Also applies to: 50-91
MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
59-61: Revisit uv “required” status and possible leftover MCP Server recommendationTwo design points to confirm:
CheckAllDependenciesalways adds uv and platform detectors construct it withisRequired: true. Combined with the Setup Window text (“requires all dependencies”), this effectively makes uv mandatory. If your HTTP-only workflows genuinely work without uv on the Unity side, consider either marking uv as non-required or gating this check behind whichever features actually need uv. If all supported flows assume uv is present, current behavior is fine.- The
dep.Name == "MCP Server"branch inGenerateRecommendationslooks like a hold‑over now that MCP server detection has been removed fromCheckAllDependencies. If nothing is adding a"MCP Server"dependency anymore, this branch is effectively dead and could be removed or updated to reflect the new transport-first architecture.Please verify whether there are still any producers of a
"MCP Server"dependency in the codebase and whether uv should truly be treated as “required” for all supported setups.Also applies to: 98-107, 121-131, 139-140
MCPForUnity/Editor/Windows/MCPSetupWindow.cs (1)
42-82: Harden setup window against UXML/element mismatchesThe overall flow of
MCPSetupWindowlooks good, but it’s currently assuming that every queried element (python-indicator,uv-indicator, buttons, etc.) always exists in the UXML:
- If any of these IDs change or the UXML fails to load partially, you’ll hit
NullReferenceExceptionwhen wiring.clickedhandlers or inUpdateDependencyStatus.To make this window more robust to future UXML tweaks, consider:
- Adding simple null checks right after each
Q<>(), logging a clear error if a required element is missing, and early‑returning fromCreateGUIinstead of wiring callbacks on nulls.- Optionally guarding
UpdateDependencyStatusagainst a nullindicator/versionLabel/detailsLabelto avoid hard crashes if the layout drifts.Also applies to: 115-148, 150-168
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
28-55: Linux Python/uv detection is solid; consider DRYing PATH augmentationThe Linux detector’s Python and uv flows look functionally sound and nicely aligned (direct invocation with augmented PATH, then
whichfallback). A couple of maintainability nits:
- The PATH augmentation logic appears in three places: inline arrays in
TryValidatePythonandTryFindInPath, plusGetPathAdditionsused byBuildAugmentedPathfor uv. Keeping these lists in sync across methods is easy to forget.- You could simplify by having both
TryValidatePythonandTryFindInPathbuild their PATH from a singleGetPathAdditions/BuildAugmentedPathhelper, so there’s exactly one source of truth for extra search locations.This wouldn’t change behavior but would reduce the chance of subtle differences creeping in between Python and uv detection.
Please double-check that the PATH additions you want for Linux are identical for both Python and uv and, if so, consider consolidating them through
GetPathAdditionsto avoid drift.Also applies to: 93-136, 195-252
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
136-190: Consider refactoring to useBuildAugmentedPath()for consistency.
TryValidatePythonmanually constructspathAdditionsinline (lines 155-161), but the newly addedBuildAugmentedPath()method provides the same functionality. Refactoring would improve maintainability and ensure consistent PATH handling across all detection methods.Additionally, note that the inline
pathAdditionsis missing/bincompared toGetPathAdditions(), creating a minor inconsistency.Apply this diff to use the shared helper:
- // Set PATH to include common locations - var homeDir = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); - var pathAdditions = new[] - { - "/opt/homebrew/bin", - "/usr/local/bin", - "/usr/bin", - Path.Combine(homeDir, ".local", "bin") - }; - - string currentPath = Environment.GetEnvironmentVariable("PATH") ?? ""; - psi.EnvironmentVariables["PATH"] = string.Join(":", pathAdditions) + ":" + currentPath; + psi.EnvironmentVariables["PATH"] = BuildAugmentedPath();
253-301: Eliminate code duplication by usingGetPathAdditions().
TryFindInPathmanually constructspathAdditions(lines 270-278) with the exact same paths returned byGetPathAdditions(). This creates maintenance burden—if paths need updating, they must be changed in multiple places.Apply this diff to reuse the shared helper:
- // Enhance PATH for Unity's GUI environment - var homeDir = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); - var pathAdditions = new[] - { - "/opt/homebrew/bin", - "/usr/local/bin", - "/usr/bin", - "/bin", - Path.Combine(homeDir, ".local", "bin") - }; - - string currentPath = Environment.GetEnvironmentVariable("PATH") ?? ""; - psi.EnvironmentVariables["PATH"] = string.Join(":", pathAdditions) + ":" + currentPath; + psi.EnvironmentVariables["PATH"] = BuildAugmentedPath();MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (2)
82-86: Harden validation level bounds against enum changes.The hardcoded
Mathf.Clamp(savedLevel, 0, 3)on line 84 assumesValidationLevelhas exactly four values (0-3). If the enum is modified (e.g., adding a new level), this constraint will silently produce incorrect behavior.Use enum bounds derived from the enum itself:
- int savedLevel = EditorPrefs.GetInt(EditorPrefKeys.ValidationLevel, 1); - currentValidationLevel = (ValidationLevel)Mathf.Clamp(savedLevel, 0, 3); + int savedLevel = EditorPrefs.GetInt(EditorPrefKeys.ValidationLevel, 1); + var validValues = Enum.GetValues(typeof(ValidationLevel)); + int minValue = (int)validValues.GetValue(0); + int maxValue = (int)validValues.GetValue(validValues.Length - 1); + currentValidationLevel = (ValidationLevel)Mathf.Clamp(savedLevel, minValue, maxValue);Alternatively, validate and fall back to default:
- int savedLevel = EditorPrefs.GetInt(EditorPrefKeys.ValidationLevel, 1); - currentValidationLevel = (ValidationLevel)Mathf.Clamp(savedLevel, 0, 3); + int savedLevel = EditorPrefs.GetInt(EditorPrefKeys.ValidationLevel, 1); + currentValidationLevel = Enum.IsDefined(typeof(ValidationLevel), savedLevel) + ? (ValidationLevel)savedLevel + : ValidationLevel.Standard;
201-206: Improve suggested path for Linux users.The file browser defaults to
ProgramFilesfor non-macOS platforms (line 205), which is Windows-centric. Linux users would benefit from a more appropriate default such as/usr/local/binor/usr/bin.Apply this diff to handle Linux explicitly:
- string suggested = RuntimeInformation.IsOSPlatform(OSPlatform.OSX) - ? "/opt/homebrew/bin" - : Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles); + string suggested; + if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) + suggested = "/opt/homebrew/bin"; + else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + suggested = "/usr/local/bin"; + else + suggested = Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles);MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
360-407: WrapOnTestConnectionClickedin the same error‑handling pattern as the toggle
OnConnectionToggleClickedrobustly guards the bridge operations with a try/catch and user dialog, butOnTestConnectionClickeddirectly awaitsVerifyBridgeConnectionAsync()without protection. IfBridge.VerifyAsync()ever throws, this path would surface as an unobserved async exception and could leave the health UI in a confusing state.Consider mirroring the toggle handler pattern here (try/catch with
McpLog.Error+EditorUtility.DisplayDialog) around the await inOnTestConnectionClicked.Also applies to: 410-447
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
170-229: Avoid killing existing processes before the user confirms starting the server
StartLocalHttpServercallsStopLocalHttpServer()(which aggressively kills whatever owns the configured localhost port) before showing the confirmation dialog. That means simply clicking “Start Local HTTP Server” will attempt to terminate any process bound to that port even if the user then clicks “Cancel”.To make this less surprising while still solving “address already in use”:
- Move
StopLocalHttpServer()inside the confirmation block so it only runs after the user agrees, and/or- Make the dialog text explicit that any process on that port will be terminated prior to starting the MCP server.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
MCPForUnity/Editor/Dependencies/DependencyManager.cs(4 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs(2 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs(5 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs(5 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs(1 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs(3 hunks)MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs(6 hunks)MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs(1 hunks)MCPForUnity/Editor/Services/ClientConfigurationService.cs(16 hunks)MCPForUnity/Editor/Services/IPathResolverService.cs(1 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/ServerManagementService.cs(1 hunks)MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs(1 hunks)MCPForUnity/Editor/Windows/MCPSetupWindow.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Services/ClientConfigurationService.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Services/ClientConfigurationService.csMCPForUnity/Editor/Helpers/ProjectIdentityUtility.csMCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.csMCPForUnity/Editor/Dependencies/DependencyManager.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Services/ClientConfigurationService.csMCPForUnity/Editor/Windows/MCPSetupWindow.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
📚 Learning: 2025-10-24T14:09:08.615Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 348
File: MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs:71-79
Timestamp: 2025-10-24T14:09:08.615Z
Learning: The SystemRoot environment variable on Windows is only required for Codex MCP client configurations due to a Codex bug. Other MCP clients (VSCode, Cursor, Windsurf, Kiro) do not need this environment variable. Codex configurations use TOML format (CodexConfigHelper.cs), while other clients use JSON format (ConfigJsonBuilder.cs).
Applied to files:
MCPForUnity/Editor/Services/ClientConfigurationService.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Editor/Services/ClientConfigurationService.csMCPForUnity/Editor/Helpers/ProjectIdentityUtility.csMCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.csMCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
🧬 Code graph analysis (15)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (5)
MCPForUnity/Editor/Services/ServerManagementService.cs (5)
System(434-514)TryGetLocalHttpServerCommand(359-391)CanStartLocalServer(424-428)StartLocalHttpServer(174-229)StopLocalHttpServer(234-274)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (3)
HttpEndpointUtility(12-85)GetBaseUrl(20-24)SaveBaseUrl(29-33)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-85)MCPForUnity/Editor/Helpers/McpLog.cs (5)
McpLog(7-52)Info(37-41)Warn(43-46)Error(48-51)Debug(31-35)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (4)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (3)
TryValidatePython(138-193)TryFindInPath(254-302)GetUvInstallUrl(69-72)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
TryValidatePython(136-190)TryFindInPath(253-301)GetUvInstallUrl(69-72)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (1)
GetUvInstallUrl(43-43)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (1)
GetUvInstallUrl(17-17)
MCPForUnity/Editor/Services/ClientConfigurationService.cs (11)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-85)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (4)
uvxPath(184-191)JObject(87-138)AssetPathUtility(15-216)GetMcpServerGitUrl(160-178)MCPForUnity/Editor/Services/IPathResolverService.cs (3)
GetUvxPath(12-12)IsUvxDetected(30-30)GetClaudeCliPath(18-18)MCPForUnity/Editor/Services/PathResolverService.cs (3)
GetUvxPath(21-37)IsUvxDetected(120-123)GetClaudeCliPath(39-95)MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (7)
McpConfigurationHelper(22-282)GetClientConfigPath(169-189)EnsureConfigDirectoryExists(194-197)ConfigureCodexClient(122-164)WriteMcpConfiguration(30-117)ExtractUvxUrl(199-210)PathsEqual(212-229)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (3)
JObject(35-45)JObject(139-145)ConfigJsonBuilder(11-146)MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (4)
CodexConfigHelper(16-270)TryParseCodexServer(90-93)TryParseCodexServer(95-128)BuildCodexServerBlock(18-67)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
HttpEndpointUtility(12-85)GetMcpRpcUrl(38-41)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Info(37-41)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (8)
TryValidatePython(136-190)TryFindInPath(253-301)GetUvInstallUrl(69-72)DependencyStatus(19-62)DependencyStatus(91-134)TryValidateUv(192-231)BuildAugmentedPath(233-238)GetPathAdditions(240-251)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (4)
TryValidatePython(89-130)TryFindInPath(132-171)GetUvInstallUrl(69-72)DependencyStatus(19-62)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (3)
GetUvInstallUrl(43-43)DependencyStatus(23-23)DependencyStatus(28-28)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (6)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)SetDebugLoggingEnabled(24-29)Info(37-41)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-85)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (3)
uvxPath(184-191)AssetPathUtility(15-216)GetPackageVersion(197-215)MCPForUnity/Editor/Services/IPathResolverService.cs (3)
GetUvxPath(12-12)SetUvxPathOverride(42-42)ClearUvxPathOverride(53-53)MCPForUnity/Editor/Services/PathResolverService.cs (3)
GetUvxPath(21-37)SetUvxPathOverride(200-214)ClearUvxPathOverride(232-235)
MCPForUnity/Editor/Windows/MCPSetupWindow.cs (1)
MCPForUnity/Editor/Dependencies/DependencyManager.cs (3)
DependencyCheckResult(46-77)DependencyManager(16-142)GetInstallationRecommendations(82-93)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (4)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (8)
TryValidatePython(138-193)TryFindInPath(254-302)GetUvInstallUrl(69-72)DependencyStatus(19-62)DependencyStatus(93-136)TryValidateUv(195-233)BuildAugmentedPath(235-239)GetPathAdditions(241-252)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (4)
TryValidatePython(89-130)TryFindInPath(132-171)GetUvInstallUrl(69-72)DependencyStatus(19-62)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (3)
GetUvInstallUrl(43-43)DependencyStatus(23-23)DependencyStatus(28-28)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (3)
GetUvInstallUrl(17-17)DependencyStatus(15-15)DependencyStatus(20-48)
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
MCPForUnity/Editor/Services/PathResolverService.cs (10)
GetUvxPath(21-37)GetClaudeCliPath(39-95)IsPythonDetected(97-118)IsUvxDetected(120-123)IsClaudeCliDetected(195-198)SetUvxPathOverride(200-214)SetClaudeCliPathOverride(216-230)ClearUvxPathOverride(232-235)ClearClaudeCliPathOverride(237-240)GetUvxPackageSourcePath(125-193)
MCPForUnity/Editor/Services/PathResolverService.cs (4)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Services/IPathResolverService.cs (9)
GetUvxPath(12-12)GetClaudeCliPath(18-18)IsPythonDetected(24-24)IsUvxDetected(30-30)GetUvxPackageSourcePath(74-74)IsClaudeCliDetected(36-36)SetUvxPathOverride(42-42)ClearUvxPathOverride(53-53)ClearClaudeCliPathOverride(58-58)MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Error(48-51)Info(37-41)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-216)GetPackageVersion(197-215)
MCPForUnity/Editor/Services/ServerManagementService.cs (8)
MCPForUnity/Editor/Services/IServerManagementService.cs (6)
ClearUvxCache(12-12)StartLocalHttpServer(19-19)TryGetLocalHttpServerCommand(32-32)StopLocalHttpServer(24-24)IsLocalUrl(38-38)CanStartLocalServer(44-44)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-85)MCPForUnity/Editor/Services/PathResolverService.cs (1)
GetUvxPath(21-37)MCPForUnity/Editor/Helpers/McpLog.cs (5)
McpLog(7-52)Debug(31-35)Error(48-51)Warn(43-46)Info(37-41)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
HttpEndpointUtility(12-85)GetBaseUrl(20-24)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
uvxPath(184-191)AssetPathUtility(15-216)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (5)
MCPForUnity/Editor/Services/Transport/TransportManager.cs (4)
Task(36-59)Task(61-79)Task(81-88)TransportState(90-98)MCPForUnity/Editor/Services/Transport/TransportState.cs (5)
TransportState(6-51)TransportState(15-29)TransportState(31-36)TransportState(38-42)TransportState(44-50)MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (4)
ProjectIdentityUtility(25-29)GetProjectName(82-86)GetProjectHash(72-76)SetSessionId(132-152)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
HttpEndpointUtility(12-85)GetBaseUrl(20-24)MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (1)
TransportCommandDispatcher(19-313)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (8)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Models/McpClient.cs (1)
McpClient(3-46)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
uvxPath(184-191)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-85)MCPForUnity/Editor/Services/IPathResolverService.cs (1)
GetUvxPath(12-12)MCPForUnity/Editor/Services/PathResolverService.cs (1)
GetUvxPath(21-37)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
ConfigJsonBuilder(11-146)MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)
CodexConfigHelper(16-270)UpsertCodexServerBlock(69-88)
MCPForUnity/Editor/Dependencies/DependencyManager.cs (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (2)
GetPythonInstallUrl(38-38)GetUvInstallUrl(43-43)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (2)
GetPythonInstallUrl(64-67)GetUvInstallUrl(69-72)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
GetPythonInstallUrl(64-67)GetUvInstallUrl(69-72)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (2)
GetPythonInstallUrl(16-16)GetUvInstallUrl(17-17)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
GetPythonInstallUrl(64-67)GetUvInstallUrl(69-72)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (4)
GetUvInstallUrl(43-43)GetInstallationRecommendations(33-33)DependencyStatus(23-23)DependencyStatus(28-28)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (4)
GetUvInstallUrl(69-72)GetInstallationRecommendations(74-91)DependencyStatus(19-62)DependencyStatus(93-136)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (4)
GetUvInstallUrl(69-72)GetInstallationRecommendations(74-89)DependencyStatus(19-62)DependencyStatus(91-134)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (3)
GetUvInstallUrl(69-72)GetInstallationRecommendations(74-87)DependencyStatus(19-62)MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
GetInstallationRecommendations(82-93)
🔇 Additional comments (20)
MCPForUnity/Editor/Services/IPathResolverService.cs (2)
70-74: Good addition for package source discovery.The new
GetUvxPackageSourcePath()method provides useful functionality for locating the uvx-installed package source. The implementation correctly uses theuv cache dircommand (sinceuvis the underlying package manager) and searches the git checkouts directory structure.
1-75: Clean refactoring to uvx-centric terminology.The interface has been successfully migrated from MCP server/uv terminology to uvx-centric naming. The method consolidation (replacing
GetMcpServerPath()andGetUvPath()with a unifiedGetUvxPath()), along with the consistent renaming of detection and override methods, improves API clarity and aligns with the PR's goal of moving to a uvx-based workflow.MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (5)
11-66: LGTM: Clean identity caching with proper lifecycle hooks.The caching strategy correctly uses
InitializeOnLoadandEditorApplication.projectChangedto maintain up-to-date project identity. The guard flag_cacheScheduledprevents duplicate refresh scheduling, and the early-return inUpdateIdentityCache(line 54-57) gracefully handles batch mode whereApplication.dataPathmay be unavailable.
68-126: LGTM: Deterministic project identification with appropriate hashing.The hash and name computation correctly derives stable identifiers from
Application.dataPath. SHA1 usage here is appropriate for non-cryptographic project identification purposes. The truncation to 16 characters (line 100) and the "Assets" suffix removal (lines 114-116) align with legacy compatibility requirements mentioned in the documentation.
128-184: LGTM: Robust per-project session ID management.The session ID logic correctly scopes IDs per project using the project hash (lines 144, 164), ensuring multiple Unity projects maintain separate sessions. The fallback for batch mode (lines 174-183) now caches
_fallbackSessionIdto maintain stability across calls, addressing previous review feedback.
186-214: LGTM: Thorough session ID cleanup with legacy migration.The reset logic properly clears the project-scoped session ID and performs one-time cleanup of the legacy global key (lines 202-206). This ensures smooth migration while maintaining test-friendly reset behavior.
216-258: LGTM: Well-designed fallback ensures deterministic per-project identity in batch mode.The cache fallback strategy correctly attempts
UpdateIdentityCachefirst and only falls back to computing a hash from the current working directory (line 252) whenApplication.dataPathis unavailable. The comment (lines 218-220) clearly explains the rationale, and the path normalization (line 251) ensures hash stability across platforms.MCPForUnity/Editor/Services/ClientConfigurationService.cs (4)
46-97: LGTM: Forced re-registration keeps transport settings current.The forced re-registration for ClaudeCode (lines 68-70) ensures transport settings stay synchronized with editor preferences, which is correct given users can switch between HTTP and stdio modes.
253-268: LGTM: Transport-aware registration.The transport mode handling correctly branches between HTTP and stdio modes, using appropriate URLs and commands for each transport type.
567-588: LGTM: Robust URL comparison.The
UrlsEqualhelper provides proper URL normalization usingUri.Comparewith appropriate components and fallback to string comparison. The implementation correctly handles scheme, host, port, path, and percent-encoding differences.
509-565: LGTM: Claude Code configuration check via CLI.The rewritten implementation correctly uses
claude mcp listto check registration status and includes appropriate PATH handling for cross-platform CLI detection. The platform-specific PATH prepending ensures the Claude CLI can be found in various installation scenarios.MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (4)
180-271: Reconnect concurrency issue properly resolved.The per-connection
_connectionCtsarchitecture (Line 185), combined withStopConnectionLoopsAsyncawaiting tasks during reconnection (Line 635 inAttemptReconnectAsync), and the guard inStartBackgroundLoops(Lines 264-267), ensures that only one set of receive/keep-alive loops runs per connection lifecycle. The previous critical issue about spawning multiple concurrent loops on the same socket has been properly addressed.
157-178: Dispose blocks on async operations.Line 167 blocks synchronously on
StopAsync(), which awaits background tasks. This is standard practice for disposing async resources, but be aware that callingDisposefrom certain contexts (e.g., holding locks needed by the tasks, or from a captured synchronization context) could deadlock. The current implementation usesConfigureAwait(false)throughout, which mitigates this risk in Unity editor scenarios.
610-665: Reconnection logic is well-structured.The two-phase stop approach is correct:
HandleSocketClosureAsync(Line 626) callsStopConnectionLoopsAsync(awaitTasks: false)to cancel tokens without blocking the calling task (which might be the receive loop itself).AttemptReconnectAsync(Line 635) callsStopConnectionLoopsAsync()with defaultawaitTasks: trueto ensure tasks fully exit before establishing a new connection.The atomic
_isReconnectingFlagguard (Line 617) prevents concurrent reconnection attempts, and the backoff schedule provides resilience.
667-689: LGTM: URI construction correctly translates HTTP to WebSocket.The logic properly converts HTTP(S) base URLs to WebSocket URIs, preserving scheme security (http→ws, https→wss), authority, and path, then appending the "/hub/plugin" endpoint. This aligns with the HTTP endpoint patterns established in
HttpEndpointUtility.MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
28-55: Windows Python/uv detection changes look consistent and reasonableThe simplified Windows Python detection (direct
python3.exe/python.exe+wherefallback and 3.10+ gating) and the newGetUvInstallUrl/uv install text are consistent with the Linux/macOS detectors and the dependency manager expectations. I don’t see any functional issues here.It would still be good to verify on a couple of Windows setups (Store Python vs. python.org installer, uv installed via the documented script) that detection behaves as expected.
Also applies to: 69-72, 82-86
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
91-134: LGTM! UV detection implementation follows established patterns.The
DetectUvmethod correctly mirrors the approach used inLinuxPlatformDetectorand follows the same two-phase discovery pattern (direct validation → fallback towhich) as the existingDetectPythonmethod in this file.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (2)
82-99: HTTP transport + URL wiring keeps server controls in syncInitializing the transport dropdown from
EditorPrefs, normalizing the HTTP URL viaHttpEndpointUtility, and callingRefreshHttpUi()from both the transport and URL change handlers keeps the Start/Stop buttons and server command display consistent with current settings, including localhost checks. This looks solid and addresses the prior “URL change doesn’t refresh button state” issue.Also applies to: 101-120, 189-242
329-358: Port persistence and validation flow looks correctThe
PersistUnityPortFromFieldpath validates input, delegates availability checks toPortManager.SetPreferredPort, persists the chosen port, and provides a clear recovery dialog on failure before reverting toBridge.CurrentPort. This is a good balance of safety and UX for configuring the Unity socket port.MCPForUnity/Editor/Services/ServerManagementService.cs (1)
231-354: Cross‑platform “kill by port” behavior looks consistentThe
StopLocalHttpServer,GetProcessIdForPort, andKillProcessflow now handles Windows (netstat+taskkill) and POSIX (lsof+kill -9) consistently, including the multi‑PIDlsofcase and logging when no listener is found. The explicit log message about potentially killing non‑MCP processes is helpful context.
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs
Show resolved
Hide resolved
We construct it in parts now
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
396-475: Terminal launch still embeds raw command strings without robust escaping
CreateTerminalProcessStartInfointerpolates the fullcommandstring directly into shell/terminal arguments on all platforms:
- macOS:
/bin/bash -c "osascript ... \\\"{command}\\\""- Windows:
cmd.exe /c start cmd.exe /k "{command}"- Linux: various
bash -c "{command}; exec bash"/bash -c '{command}'patterns.Because
commandincludes user‑configurable pieces (uvx override path and HTTP base URL), any embedded quotes, semicolons,&, or other metacharacters can:
- Break the quoting and make the launch fail, or
- Inject extra shell tokens that run in the same terminal session.
This is the same concern called out in the earlier review and it still applies.
To harden this:
- Prefer launching the terminal binary and passing
commandas a single, properly quoted argument (or as separateFileName/Argumentsfor the uvx binary) without an extrabash -c/cmd /chop where possible; and/or- Introduce platform‑appropriate escaping for
commandbefore embedding it (at least escaping quotes and backslashes correctly for each shell/terminal combination), and consider validating/sanitizing the allowed characters in the configurable pieces.Given this is a helper that builds shell invocations from editor preferences, tightening the quoting/escaping here would significantly reduce fragility and the potential for unintended command execution.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
217-220: Fix typo in comment.Line 219: "new oens" should be "new ones".
Apply this diff:
- /// Particularly useful when reconnecting, we want to ensure that background loops are cancelled correctly before starting new oens + /// Particularly useful when reconnecting, we want to ensure that background loops are cancelled correctly before starting new onesMCPForUnity/Editor/Services/ServerManagementService.cs (1)
136-159: Optionally act onClearUvxCachefailures before starting the server
StartLocalHttpServertreatsClearUvxCache()as best‑effort (you only catch exceptions and ignore the returnedfalse), so a missing/brokenuvwill log an error but still continue to start the server.If you want clearer behavior when cache clearing is important for troubleshooting, consider:
- Checking the boolean result and, on failure, either surfacing a dialog (“Failed to clear uv cache; starting server anyway?”) or aborting the launch with a clearer explanation.
Not strictly necessary, but would make cache‑related issues less confusing for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs(1 hunks)MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs(6 hunks)MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs(1 hunks)MCPForUnity/Editor/Services/IPathResolverService.cs(1 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/ServerManagementService.cs(1 hunks)MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- MCPForUnity/Editor/Constants/EditorPrefKeys.cs
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.csMCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
📚 Learning: 2025-10-24T14:09:08.615Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 348
File: MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs:71-79
Timestamp: 2025-10-24T14:09:08.615Z
Learning: The SystemRoot environment variable on Windows is only required for Codex MCP client configurations due to a Codex bug. Other MCP clients (VSCode, Cursor, Windsurf, Kiro) do not need this environment variable. Codex configurations use TOML format (CodexConfigHelper.cs), while other clients use JSON format (ConfigJsonBuilder.cs).
Applied to files:
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
🧬 Code graph analysis (4)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (5)
MCPForUnity/Editor/Services/Transport/IMcpTransportClient.cs (3)
Task(14-14)Task(15-15)Task(16-16)MCPForUnity/Editor/Services/Transport/TransportState.cs (5)
TransportState(6-51)TransportState(15-29)TransportState(31-36)TransportState(38-42)TransportState(44-50)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
HttpEndpointUtility(12-85)GetBaseUrl(20-24)MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (1)
TransportCommandDispatcher(19-313)MCPForUnity/Editor/Helpers/McpLog.cs (4)
McpLog(7-52)Warn(43-46)Error(48-51)Info(37-41)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)
MCPForUnity/Editor/Services/ServerManagementService.cs (6)
MCPForUnity/Editor/Services/IServerManagementService.cs (6)
ClearUvxCache(12-12)StartLocalHttpServer(19-19)TryGetLocalHttpServerCommand(32-32)StopLocalHttpServer(24-24)IsLocalUrl(38-38)CanStartLocalServer(44-44)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
uvxPath(184-191)AssetPathUtility(15-216)MCPForUnity/Editor/Services/PathResolverService.cs (1)
GetUvxPath(21-38)MCPForUnity/Editor/Helpers/McpLog.cs (5)
McpLog(7-52)Debug(31-35)Error(48-51)Warn(43-46)Info(37-41)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
HttpEndpointUtility(12-85)GetBaseUrl(20-24)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Services/IPathResolverService.cs (6)
GetUvxPath(12-12)GetClaudeCliPath(18-18)IsPythonDetected(24-24)IsClaudeCliDetected(36-36)ClearUvxPathOverride(53-53)ClearClaudeCliPathOverride(58-58)
🔇 Additional comments (20)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (4)
180-271: Previous critical concurrency issue is properly addressed.The reconnection logic now correctly manages loop lifecycle:
EstablishConnectionAsyncstops existing loops first (Line 182) before creating a new_connectionCts(Line 185) and socket (Line 189).StartBackgroundLoopsguards against starting duplicate loops (Lines 264-267).- Each connection lifecycle has its own cancellation token, ensuring old loops exit before new ones start.
This eliminates the risk of multiple concurrent
ReceiveAsynccalls on the sameClientWebSocketinstance that was flagged in previous reviews.
573-580: Previous issue with session_id in pong payload is fixed.The pong message now correctly includes only the message type, complying with the MCP protocol requirement that session identification be handled at the transport layer rather than in message payloads.
262-271: LGTM: Guard prevents duplicate background loops.The guard correctly prevents starting new receive/keep-alive loops while existing loops are still active. Since
IsCompletedreturns true for faulted or cancelled tasks, the check at Lines 264-267 properly handles all task states.
157-178: LGTM: Robust resource management and error handling.The implementation demonstrates good practices:
Disposeproperly ensures background loops are stopped before disposing shared resources (Lines 166-167).ArrayPoolusage inReceiveMessageAsyncefficiently manages buffers with proper cleanup in finally blocks (Lines 312, 348).SemaphoreSlimensures thread-safe WebSocket sends (Lines 593-605).- Comprehensive error handling throughout async paths with appropriate catch blocks and fallbacks.
Note: The blocking async call
StopAsync().GetAwaiter().GetResult()inDispose(Line 167) is acceptable in Unity Editor context but is a pattern to use cautiously in other environments.Also applies to: 305-350, 582-607
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (1)
1-260: Excellent implementation—previous feedback fully addressed.The project identity utility is well-designed and handles all edge cases correctly:
- Deterministic identity: SHA1 hash and project name derived from
Application.dataPathwith fallback to current directory in batch mode.- Project-scoped sessions: Session IDs use project-specific EditorPrefs keys to prevent collisions.
- Batch-mode resilience: Fallback session GUID is now cached (previous concern resolved), and fallback hash uses current directory for deterministic per-project identity.
- Thread safety: All operations correctly marshaled to main thread via
EditorApplication.delayCall.- Documentation accuracy: XML comment now correctly states "16 characters" matching the implementation (previous inconsistency fixed).
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
1-70: LGTM! Clean interface refactoring to uvx terminology.The interface has been consistently updated to replace UV/MCP-specific path terminology with UVX equivalents. The method signatures are clear, well-documented, and follow the established patterns for path resolution and override management.
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (3)
30-117: LGTM! Simplified configuration with centralized path resolution.The removal of the
pythonDirparameter and the use ofMCPServiceLocator.Paths.GetUvxPath()align well with the uvx-based workflow. The early lock check prevents unintended configuration overwrites, and the error handling when uvx is not found provides clear feedback to users.
122-164: LGTM! Consistent Codex configuration updates.The Codex configuration method follows the same improved patterns as
WriteMcpConfiguration: simplified signature, centralized path resolution, and proper lock checking.
199-210: Behavioral change: Now extracts uvx URL with--fromflag.The method has been renamed from
ExtractDirectoryArgtoExtractUvxUrland now searches for the--fromflag instead of--directory. This aligns with the uvx-based workflow where servers are referenced via GitHub URLs rather than local directories.MCPForUnity/Editor/Services/PathResolverService.cs (5)
18-19: LGTM! Clean property implementations.The properties correctly check for non-empty override values in EditorPrefs using the centralized
EditorPrefKeysconstants.
21-38: LGTM! Simple and effective path resolution.The method returns the user-configured override when available, otherwise defaults to the
"uvx"command name for PATH-based resolution. The error handling gracefully falls back to the default when EditorPrefs access fails.
40-96: LGTM! Comprehensive cross-platform path resolution.The Claude CLI path resolution properly handles user overrides and includes reasonable platform-specific candidate paths for Windows, macOS, and Linux. File existence validation ensures only valid paths are returned.
98-119: LGTM! Robust Python detection.The method correctly verifies Python availability by executing the version command with platform-specific executable names. The timeout and exception handling make this a reliable detection mechanism.
131-171: LGTM! Proper validation and cleanup in override methods.The setter methods correctly validate that files exist before storing overrides, handle empty inputs by clearing the override, and provide clear error messages. The clear methods properly clean up EditorPrefs keys using the centralized constants.
MCPForUnity/Editor/Services/ServerManagementService.cs (6)
94-127: Cross‑platform PATH augmentation for uv/uvx looks solid
GetPlatformSpecificPathPrependcovers the expected install locations foruvon macOS, Linux, and Windows (includingLocalApplicationData/Programs/uvandProgramFiles/uv), which should make cache clearing resilient without hard‑coding a single path. No changes needed here.
196-236: StopLocalHttpServer behavior and messaging are clear and constrainedThe stop logic:
- Only operates when the configured URL is local, and
- Explicitly logs that it will terminate whatever owns the port, even if it’s not the MCP server.
This makes the “kill by port” behavior explicit and avoids accidentally targeting remote hosts. Implementation and messaging look good.
238-296: Port→PID resolution handles Windows and Unix cases correctly
GetProcessIdForPort:
- Uses
netstat -ano | findstr :{port}on Windows and parses onlyLISTENINGlines, and- Uses
lsof -i :{port} -ton Unix-like systems, correctly iterating all returned PIDs and logging when multiple are present before picking one.This addresses the multi‑PID
lsofcase and should behave predictably across platforms.
298-316: KillProcess keeps platform differences narrow and containedThe
KillProcesshelper cleanly abstractstaskkill /F /PIDvskill -9and isolates platform-specific details behindExecPath.TryRun, with error logging on exceptions. That’s an acceptable level of robustness for an editor-only “stop server” helper.
321-353: HTTP command construction and local‑only guardrails look good
TryGetLocalHttpServerCommandandCanStartLocalServer:
- Respect the
UseHttpTransportflag,- Enforce that only localhost‑style URLs are eligible for local server launch, and
- Build a straightforward
uvxcommand line, including the optional--fromGit URL.This keeps the local-launch feature constrained and gives useful error messages when settings are incompatible.
Also applies to: 386-390
355-381: Local URL detection is simple and explicit
IsLocalUrlwhitelistslocalhost,127.0.0.1,0.0.0.0, and::1with safe URI parsing and a catch‑all fallback. That’s a clear and predictable set of hostnames for “local only” behavior in the editor.
The flow changed so it checks editor prefs and then defaults to the command line default. So it's always true.
…essStartInfo - Validate command is not empty before processing - Strip carriage returns and newlines from command - macOS: Use osascript directly instead of bash to avoid shell injection, escape backslashes and quotes for AppleScript - Windows: Add window title and escape quotes in command - Linux: Properly escape single quotes for bash -c and double quotes for process arguments
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
MCPForUnity/Editor/Services/ClientConfigurationService.cs (2)
287-290: Redundant null coalescing operator.String interpolation
$"{stdout}\n{stderr}"can never return null, so the?? string.Emptyis unnecessary.- string combined = ($"{stdout}\n{stderr}") ?? string.Empty; + string combined = $"{stdout}\n{stderr}";
264-286: Consider extracting duplicated PATH prepend logic.The path prepend construction logic (lines 264-285) is duplicated in
RegisterClaudeCode,UnregisterClaudeCode(lines 321-324), andCheckClaudeCodeConfiguration(lines 518-539). Consider extracting to a private helper method.Example extraction:
private static string BuildClaudePathPrepend(string claudePath) { string pathPrepend = null; if (Application.platform == RuntimePlatform.OSXEditor) pathPrepend = "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin"; else if (Application.platform == RuntimePlatform.LinuxEditor) pathPrepend = "/usr/local/bin:/usr/bin:/bin"; try { string claudeDir = Path.GetDirectoryName(claudePath); if (!string.IsNullOrEmpty(claudeDir)) { pathPrepend = string.IsNullOrEmpty(pathPrepend) ? claudeDir : $"{claudeDir}:{pathPrepend}"; } } catch { } return pathPrepend; }MCPForUnity/Editor/Services/ServerManagementService.cs (1)
414-424: Windows cmd.exe quote escaping may not work correctly.The
\"escape pattern (line 417) doesn't work reliably in cmd.exe. The shell uses""(doubled quotes) or^"(caret escape) for escaping quotes within quoted strings.In practice, the uvx command being constructed (
uvx --from ... --transport http --http-url http://localhost:...) doesn't contain double quotes, so this is unlikely to cause issues with typical usage. However, if a user somehow configures a URL or path containing quotes, the command would break.If you want to harden this for edge cases:
- string escapedCommandWin = command.Replace("\"", "\\\""); + // cmd.exe uses ^" for escaping quotes in arguments + string escapedCommandWin = command.Replace("\"", "^\"");Or alternatively, use doubled quotes which is more idiomatic for cmd.exe:
- string escapedCommandWin = command.Replace("\"", "\\\""); + string escapedCommandWin = command.Replace("\"", "\"\"");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs(3 hunks)MCPForUnity/Editor/Services/ClientConfigurationService.cs(16 hunks)MCPForUnity/Editor/Services/IPathResolverService.cs(1 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/ServerManagementService.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Services/ClientConfigurationService.csMCPForUnity/Editor/Services/ServerManagementService.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Services/ClientConfigurationService.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Services/ClientConfigurationService.csMCPForUnity/Editor/Services/ServerManagementService.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Services/ClientConfigurationService.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Services/ServerManagementService.cs
📚 Learning: 2025-10-24T14:09:08.615Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 348
File: MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs:71-79
Timestamp: 2025-10-24T14:09:08.615Z
Learning: The SystemRoot environment variable on Windows is only required for Codex MCP client configurations due to a Codex bug. Other MCP clients (VSCode, Cursor, Windsurf, Kiro) do not need this environment variable. Codex configurations use TOML format (CodexConfigHelper.cs), while other clients use JSON format (ConfigJsonBuilder.cs).
Applied to files:
MCPForUnity/Editor/Services/ClientConfigurationService.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (3)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-85)MCPForUnity/Editor/Services/PathResolverService.cs (1)
GetUvxPath(21-38)
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
MCPForUnity/Editor/Services/PathResolverService.cs (8)
GetUvxPath(21-38)GetClaudeCliPath(40-96)IsPythonDetected(98-119)IsClaudeCliDetected(121-124)SetUvxPathOverride(126-140)SetClaudeCliPathOverride(142-156)ClearUvxPathOverride(158-161)ClearClaudeCliPathOverride(163-166)
🔇 Additional comments (23)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (3)
3-4: LGTM!The new imports for
MCPForUnity.Editor.ConstantsandMCPForUnity.Editor.Servicesare correctly added to support the newGetMcpServerGitUrl()andGetUvxCommandParts()methods.
145-163: LGTM! Clear fallback hierarchy for Git URL resolution.The method correctly prioritizes EditorPrefs override, then derives a version-pinned URL from package.json, with a sensible fallback to the main branch when version is unknown. This ensures configs remain valid across development and release scenarios.
169-176: LGTM! Clean tuple-based API for command parts.The method provides a well-structured tuple for downstream consumers, correctly delegating to
MCPServiceLocator.Pathsfor uvx path resolution andGetMcpServerGitUrl()for the URL. The hardcoded"mcp-for-unity"package name aligns with the server package naming convention.MCPForUnity/Editor/Services/IPathResolverService.cs (1)
6-63: LGTM! Clean interface consolidation around uvx terminology.The interface correctly consolidates the path resolution API with consistent uvx-centric naming. The symmetry between
GetUvxPath/SetUvxPathOverride/ClearUvxPathOverride/HasUvxPathOverrideand the corresponding Claude CLI methods provides a predictable API surface.MCPForUnity/Editor/Services/ClientConfigurationService.cs (5)
23-44: LGTM! Simplified configuration flow.The method correctly delegates to the appropriate configuration helper based on client type and properly sets status based on the result. Throwing
InvalidOperationExceptionwith the failure reason provides clear error feedback.
55-73: Good approach: Force fresh registration for Claude Code.Unregistering before re-registering ensures transport settings stay current when users change preferences. This addresses potential stale configuration issues.
166-181: LGTM! Robust URL and path matching logic.The dual-path matching strategy correctly handles both uvx-based (args) and HTTP-based (URL) configurations, using
PathsEqualfor git URLs andUrlsEqualfor HTTP endpoints.
245-260: LGTM! Transport-aware command generation.The conditional logic correctly generates the appropriate
mcp addcommand based on theUseHttpTransportpreference, with proper quoting for paths containing spaces.
559-580: LGTM! Well-implemented URL comparison helper.The
UrlsEqualmethod correctly handles URI normalization withUri.Compareand provides a sensible string-based fallback for cases where URIs can't be parsed. The null/whitespace checks prevent exceptions.MCPForUnity/Editor/Services/PathResolverService.cs (5)
18-19: LGTM! Clean override indicator properties.The properties correctly check for non-empty override values using
EditorPrefs.GetStringwith appropriate null handling.
21-38: LGTM! Sensible default behavior.The method correctly falls back to the bare
"uvx"command when no override is set, which aligns with the PR objective of defaulting to uvx and relying on it being available on PATH.
40-96: LGTM! Comprehensive cross-platform Claude CLI detection.The method correctly checks the EditorPrefs override first, then falls through to platform-specific candidate paths. The candidate lists cover common installation locations for each platform.
98-119: LGTM! Proper Python detection with process execution.The implementation correctly uses process execution to verify Python availability rather than just checking for a path, following the pattern expected for detection methods.
126-140: Incorrect review comment - file picker guarantees full paths.The
pickedvariable passed toSetUvxPathOverridecomes fromEditorUtility.OpenFilePanel()(line 206 in McpSettingsSection.cs), which is a file picker dialog that only returns full paths to files selected from disk. File picker dialogs do not allow users to input bare command names like"uvx"or"uvx-custom"; they only return paths to existing files. Therefore, theFile.Exists()check inSetUvxPathOverrideis appropriate and correct for this use case, not a limitation.Likely an incorrect or invalid review comment.
MCPForUnity/Editor/Services/ServerManagementService.cs (9)
94-130: LGTM!The platform-specific PATH prepend logic covers common uv installation locations on each platform, with proper null filtering for Windows paths.
132-191: LGTM!The server startup flow properly handles error cases: stop failures are acceptable (no existing server), cache clear failures are logged as warnings, and process start exceptions are caught with user-facing error dialogs.
193-236: LGTM!Good defensive checks for local URL validation, clear logging about the port-based kill behavior, and proper exception handling.
238-296: LGTM! Properly handles multiple PIDs from lsof.The implementation correctly splits
lsof -toutput on newlines and parses individual PIDs, with helpful debug logging when multiple processes are found. The fix from the previous review has been properly implemented.
298-316: LGTM!The platform-specific process termination is straightforward. The unused stdout/stderr capture could potentially be logged on failure, but the current error handling via exception catch is sufficient for this cleanup utility.
318-353: LGTM!Clear validation sequence with helpful error messages, and proper command construction with conditional
--fromargument.
355-381: LGTM!Comprehensive local address validation including IPv6 loopback (
::1), with proper case-insensitive comparison and safe exception handling.
383-390: LGTM!Simple and correct capability check combining transport mode and URL locality.
425-492: LGTM! Linux terminal handling is well-implemented.The single-quote escaping with
'\''is the correct idiom for bash, and the terminal detection logic with per-terminal argument formatting covers the major Linux terminal emulators. The fallback to xterm is sensible.
| #if UNITY_EDITOR_OSX | ||
| // macOS: Use osascript directly to avoid shell metacharacter injection via bash | ||
| // Escape for AppleScript: backslash and double quotes | ||
| string escapedCommand = command.Replace("\\", "\\\\").Replace("\"", "\\\""); | ||
| return new System.Diagnostics.ProcessStartInfo | ||
| { | ||
| FileName = "/usr/bin/osascript", | ||
| Arguments = $"-e \"tell application \\\"Terminal\\\" to do script \\\"{escapedCommand}\\\" activate\"", | ||
| UseShellExecute = false, | ||
| CreateNoWindow = true | ||
| }; |
There was a problem hiding this comment.
AppleScript syntax error will prevent server launch on macOS.
The activate at the end of line 410 creates invalid AppleScript syntax. In AppleScript, you cannot chain commands like do script "..." activate without proper syntax.
Apply this diff to fix the AppleScript command:
#if UNITY_EDITOR_OSX
// macOS: Use osascript directly to avoid shell metacharacter injection via bash
// Escape for AppleScript: backslash and double quotes
string escapedCommand = command.Replace("\\", "\\\\").Replace("\"", "\\\"");
return new System.Diagnostics.ProcessStartInfo
{
FileName = "/usr/bin/osascript",
- Arguments = $"-e \"tell application \\\"Terminal\\\" to do script \\\"{escapedCommand}\\\" activate\"",
+ Arguments = $"-e \"tell application \\\"Terminal\\\" to do script \\\"{escapedCommand}\\\"\" -e \"tell application \\\"Terminal\\\" to activate\"",
UseShellExecute = false,
CreateNoWindow = true
};This uses two -e arguments to execute separate AppleScript commands: one to run the script in Terminal, and another to activate (bring to front) the Terminal application.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Services/ServerManagementService.cs around lines 403 to
413, the AppleScript string currently appends "activate" to the same -e command
which produces invalid AppleScript syntax; instead build the
ProcessStartInfo.Arguments to supply two separate -e arguments (one -e to run
the do script in Terminal with the escaped command, and a second -e "activate"
to activate the Terminal app), ensuring proper escaping of backslashes and
double quotes remains applied to the command and that the two -e arguments are
separated so both AppleScript commands execute correctly.
There was a problem hiding this comment.
The start server and stop server buttons works fine for me on my Macbook
There was a problem hiding this comment.
@msanatan, thank you for testing and confirming! If the server start and stop functionality is working correctly on your MacBook, then the current AppleScript command is functioning as intended.
Your manual testing is the most reliable validation—if Terminal is launching properly and executing the server command without issues, then the implementation is good to go. The syntax appears to work fine with osascript in practice.
🧠 Learnings used
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
* 'main' of https://github.com/CoplayDev/unity-mcp: Harden PlayMode test runs (CoplayDev#396) Enable the `rmcp_client` feature so it works with Codex CLI (CoplayDev#395) chore: bump version to 8.0.0 HTTP Server, uvx, C# only custom tools (CoplayDev#375) [CUSTOM TOOLS] Roslyn Runtime Compilation Feature (CoplayDev#371)
2 participants
The current plugin does a few things:
stdioprotocolThis change does the following:
uvxinstead ofuvuvdoes that for us already.uvx(similar tonpxfor Node.js projects), and reference the version of the server that the plugin is on.uvxdownloads it for us -> less files to manageSummary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.