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 (10)
You can disable this status message by setting the WalkthroughRefactors editor integration away from an embedded Python server to uvx/HTTP transports, moves CI/workflows to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Editor as Unity Editor
participant Dispatcher as TransportCommandDispatcher
participant Registry as CommandRegistry
participant Main as Main Thread
Editor->>Dispatcher: ExecuteCommandJsonAsync(commandJson, token)
activate Dispatcher
Dispatcher->>Dispatcher: validate JSON, create PendingCommand (TCS)
Dispatcher-->>Editor: Task<string> (pending)
Main->>Dispatcher: Update loop -> ProcessQueue
activate Dispatcher
Dispatcher->>Registry: ExecuteCommand(cmd)
Registry-->>Dispatcher: Result (sync) or Deferred (async)
Dispatcher->>Dispatcher: Complete TCS, remove pending
Dispatcher-->>Editor: Task completes with response JSON
deactivate Dispatcher
sequenceDiagram
autonumber
participant UI as BridgeControlService
participant Manager as TransportManager
participant Factory as Transport Factory
participant Client as IMcpTransportClient
UI->>Manager: StartAsync(preferredMode)
activate Manager
Manager->>Factory: create client for mode
Factory-->>Manager: IMcpTransportClient
Manager->>Client: StartAsync()
Client-->>Manager: success/failure
Manager-->>UI: Task<bool> result
UI->>Manager: VerifyAsync()
Manager->>Client: VerifyAsync()
Client-->>Manager: TransportState + ping result
Manager-->>UI: BridgeVerificationResult (state + ping)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
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 stdio These only work for JSON configs, we'll have to handle Codex and Claude Code separately
It's better than checking the JSON and it can verify both HTTP and stdio setups
Introduce polling middleware to handle long-running operations that may span domain reloads. Add McpJobStateStore utility to persist tool state in Library folder across reloads. Extend McpForUnityToolAttribute with RequiresPolling and PollAction properties. Update Response helper with Pending method for standardized polling responses. Implement Python-side polling logic in custom_tool_service.py with configurable intervals and 10-minute timeout.
…nymous objects Replace static Response.Success/Error/Pending methods with SuccessResponse, ErrorResponse, and PendingResponse classes. Add IMcpResponse interface for type safety. Include JsonProperty attributes for serialization and JsonIgnore properties for backward compatibility with reflection-based tests. Update all tool and resource classes to use new response types.
…/Linux Rename SetupWizard class to SetupWindowService and update all references throughout the codebase. Implement platform-specific UV detection for macOS and Linux with augmented PATH support, including TryValidateUv methods and BuildAugmentedPath helpers. Split single "Open Installation Links" button into separate Python and UV install buttons. Update UI styling to improve installation section layout with proper containers and button
Lots of feedback, lots of changes
…ature/domain-reload-resilience # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
Feature/domain reload resilience
Slightly more up to date but not final
Just make it more organized, like typical Python projects
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MCPForUnity/Editor/Services/ClientConfigurationService.cs (2)
51-87:IsUvxDetectedcheck likely always passes; UVX “not found” branch may never triggerIn
ConfigureAllDetectedClients, non‑Claude clients are gated by:// Other clients require UVX if (!pathService.IsUvxDetected()) { summary.SkippedCount++; summary.Messages.Add($"➜ {client.name}: UVX not found"); continue; }But the current
PathResolverService.IsUvxDetected()implementation is:public bool IsUvxDetected() { return !string.IsNullOrEmpty(GetUvxPath()); }and
GetUvxPath()always returns either the override fromEditorPrefKeys.UvxPathOverrideor the literal"uvx". That meansIsUvxDetected()will effectively always betrue, even ifuvxis not actually installed or on PATH, and this “UVX not found” skip path will never be taken.If the intent is to skip configuration when UVX isn’t really available, you probably want
IsUvxDetected()(orGetUvxPath(verifyPath: true)) to be wired to the new cross‑platform UV detection (the PlatformDetectors) so it fails when uv/uvx cannot be resolved, instead of only checking for a non‑empty string.
120-235: HTTP config status detection misses non‑VS clients; they’ll always look “incorrect” in HTTP modeIn
CheckClientStatus, HTTP vs stdio status is inferred viaargsvsconfiguredUrl:
- VSCode: reads both
argsandurl/serverUrlout ofservers.unityMCP/mcp.servers.unityMCP(good).- Codex: uses
CodexConfigHelper.TryParseCodexServerto get args and URL (good).- Default branch (Cursor, Claude Desktop, Windsurf, Kiro, Trae, etc.): only assigns
argsfromstandardConfig.mcpServers.unityMCP.argsand never setsconfiguredUrl.Later:
if (configExists) { bool matches = false; if (args != null && args.Length > 0) { // stdio mode: compare uvx --from URL ... } else if (!string.IsNullOrEmpty(configuredUrl)) { // HTTP mode: compare URL via UrlsEqual ... } if (matches) { client.SetStatus(McpStatus.Configured); } ... }For non‑VS JSON clients in HTTP mode,
argswill be null/empty (ConfigJsonBuilder removesargsfor HTTP), andconfiguredUrlremains null because it’s never read. As a result,matchesstaysfalse, and those clients are treated asIncorrectPath(and/or continually auto‑rewritten) even when their HTTP config is perfectly up‑to‑date.One way to fix this without depending on the strong
McpConfigmodel shape is to mirror the VSCode approach for the default case usingJObject:- default: - McpConfig standardConfig = JsonConvert.DeserializeObject<McpConfig>(configJson); - if (standardConfig?.mcpServers?.unityMCP != null) - { - args = standardConfig.mcpServers.unityMCP.args; - configExists = true; - } - break; + default: + var standardToken = JsonConvert.DeserializeObject<JToken>(configJson) as JObject; + if (standardToken != null) + { + var unityToken = standardToken["mcpServers"]?["unityMCP"]; + if (unityToken is JObject unityObj) + { + configExists = true; + + var argsToken = unityObj["args"]; + if (argsToken is JArray) + { + args = argsToken.ToObject<string[]>(); + } + + var urlToken = unityObj["url"] ?? unityObj["serverUrl"]; + if (urlToken != null && urlToken.Type != JTokenType.Null) + { + configuredUrl = urlToken.ToString(); + } + } + } + break;This keeps the existing stdio detection (
args) but also populatesconfiguredUrlfor HTTP configs, allowing theUrlsEqualcomparison to work for non‑VS clients just like it does for VSCode and Codex.
🧹 Nitpick comments (31)
.github/workflows/bump-version.yml (1)
70-82: Minor observation: Silent pattern matching.The sed
-icommands silently succeed if the version pattern is not found in a file (e.g., if Server/README.md doesn't contain the git+https pattern, no error is raised). Withset -euo pipefail, this won't cause the workflow to fail. If incomplete version bumps should be caught, consider adding post-update validation..github/workflows/python-tests.yml (1)
38-45: Consider making artifact upload more defensive.The artifact upload may fail silently if
pytestdoes not create.pytest_cache/or if test results are located elsewhere. Consider adding error handling to gracefully handle missing files.- name: Upload test results uses: actions/upload-artifact@v4 if: always() with: name: pytest-results path: | Server/.pytest_cache/ Server/tests/ + if-no-files-found: warnAlternatively, verify that pytest is configured to generate cache and coverage reports in the expected locations by checking
Server/pyproject.tomlorServer/pytest.inifor pytest configuration.MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
121-135:"MCP Server"recommendation block is likely unreachable nowIn this method,
CheckAllDependenciesonly ever adds Python and UV statuses toresult.Dependencies, somissingcan’t contain a dependency withName == "MCP Server"anymore. Unless something external is mutatingresult.Dependencieswith that name, this branch will never execute and can confuse future readers.You could either remove this block or re‑wire it to whatever now represents the server dependency in the uvx workflow.
- else if (dep.Name == "MCP Server") - { - result.RecommendedActions.Add("MCP Server will be installed automatically when needed."); - }MCPForUnity/Editor/Helpers/McpJobStateStore.cs (1)
42-50: Consider logging deserialization errors.Catching all exceptions silently can hide issues during development (corrupted JSON, deserialization errors, file access problems). Consider logging the exception to aid debugging while still returning the safe default.
Example:
try { var json = File.ReadAllText(path); return JsonConvert.DeserializeObject<T>(json); } -catch (Exception) +catch (Exception ex) { + Debug.LogWarning($"Failed to load state for {toolName}: {ex.Message}"); return default; }MCPForUnity/Editor/Resources/Tests/GetTests.cs (1)
23-36: Consider returning richer error details inErrorResponse.DataThe new
ErrorResponseusages here only return a simple message string; all contextual details (e.g.,modeStr,parseError, exception type/stack) are effectively lost to the client and live only in logs. If MCP clients or tools will act on specific failure modes, consider populatingdatawith a small structured payload (e.g.,{ mode, parseError, exceptionType }) while keeping the human‑readable message as is.Also applies to: 46-74
MCPForUnity/Editor/Helpers/Response.cs (2)
35-67: Consider separating machine error codes from human‑readable messages
ErrorResponsecurrently sets bothCodeandErrorfrom the samemessageOrCodestring. That keeps things simple, but it also couples any machine‑readable logic (based oncode) to user‑facing text. If you foresee clients branching on specific error codes, it may be worth evolving the constructor later to accept distinctcodevsmessageparameters, while keeping the current overload for backward compatibility.
69-107: Clarify how callers should interpretPendingResponse.Success == true
PendingResponsereportsSuccess => truewhile_mcp_statusis"pending". That’s reasonable if “success” means “request accepted, not failed,” but it does assume callers will also check_mcp_status(orStatus) before treating the operation as complete. It may be worth documenting this convention alongside the new networking/transport docs so MCP clients don’t assumesuccess == truealways implies a terminal state.MCPForUnity/Editor/Services/ICustomToolRegistrationService.cs (1)
5-16: ClarifyTask<bool>semantics and consider future cancellation supportThe interface shape looks good for async registration, but two small API polish tweaks could help:
- Document what the
boolresult means (e.g., “returnstrueonly if all tools registered successfully;falseotherwise”) so callers don’t have to inspect the implementation.- Long term, you may want a
CancellationTokenparameter for UI-driven flows that might need to cancel registration mid-flight, especially if HTTP calls can be slow.MCPForUnity/Editor/Migrations/LegacyServerSrcMigration.cs (1)
27-69: Consider how “no matching clients” should affect migration completionThe migration only deletes legacy prefs when
summary.FailureCount == 0andsummary.SuccessCount > 0. That’s safe, but ifConfigureAllDetectedClients()can validly return a summary with zero successes and zero failures (e.g., no clients needed updating), this code will log “incomplete” and retry on every launch.If that scenario is expected and harmless, this is fine. Otherwise, you might want to distinguish “no clients to migrate” from “incomplete” in the summary and treat the former as success so the legacy keys can be cleaned up once and the warning stops repeating.
MCPForUnity/Editor/Helpers/TelemetryHelper.cs (1)
214-223: Consider reusing McpLog’s debug flag instead of re‑reading EditorPrefs
TelemetryHelper.IsDebugEnabled()currently re‑readsEditorPrefsforEditorPrefKeys.DebugLogs, whileMcpLognow centralizes the same flag with a cached, volatile_debugEnabledandSetDebugLoggingEnabled.To keep behavior consistent and avoid duplicating the preference read logic, consider delegating telemetry’s debug gating to
McpLog(e.g., via an internalMcpLog.IsDebugEnabledhelper or by routing debug logs throughMcpLog.Debug/Infoonly). That way, there’s a single source of truth for the debug flag.MCPForUnity/Editor/Helpers/PortManager.cs (1)
42-53: WaitForPortRelease appears unused after GetPortWithFallback simplificationWith the new
GetPortWithFallback(Line 42 onwards) no longer performing any wait‑for‑release logic,WaitForPortRelease(Lines 166‑188) is currently dead code.If you don’t plan to reintroduce the delayed‑release behavior, consider removing
WaitForPortReleaseand any related comments to reduce mental overhead; otherwise, it might be worth adding a TODO or re‑wiring it into the fallback flow where appropriate.Also applies to: 166-188
MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs (1)
39-49: Guard Unity/editor calls from background continuations
StopAsync()andStartAsync()continuations are scheduled viaTaskScheduler.Default(Lines 42–48, 94–111), and from there you call intoMcpLogandMCPForUnityEditorWindow.RequestHealthVerification(). Those eventually touch Unity editor types and windows, which are generally expected to run on the main thread.To avoid potential threading issues, consider marshalling these callbacks back onto the editor loop, e.g.:
- startTask.ContinueWith(t => - { - if (t.IsFaulted) - { - var baseEx = t.Exception?.GetBaseException(); - McpLog.Warn($"Failed to resume HTTP MCP bridge after domain reload: {baseEx?.Message}"); - return; - } - bool started = t.Result; - if (!started) - { - McpLog.Warn("Failed to resume HTTP MCP bridge after domain reload"); - } - else - { - MCPForUnityEditorWindow.RequestHealthVerification(); - } - }, TaskScheduler.Default); + startTask.ContinueWith(t => + { + EditorApplication.delayCall += () => + { + if (t.IsFaulted) + { + var baseEx = t.Exception?.GetBaseException(); + McpLog.Warn($"Failed to resume HTTP MCP bridge after domain reload: {baseEx?.Message}"); + return; + } + + bool started = t.Result; + if (!started) + { + McpLog.Warn("Failed to resume HTTP MCP bridge after domain reload"); + } + else + { + MCPForUnityEditorWindow.RequestHealthVerification(); + } + }; + }, TaskScheduler.Default);A similar pattern can be applied to the
StopAsynccontinuation if you want logging to always occur from the main thread.Also applies to: 89-141
MCPForUnity/Editor/MenuItems/CustomToolsMenuItems.cs (1)
13-61: Avoid double‑setting CustomToolRegistrationEnabled preferenceIn
EnableRegistration/DisableRegistration(Lines 35–49),CustomToolRegistrationProcessor.IsRegistrationEnabledalready writesEditorPrefs.SetBool(EditorPrefKeys.CustomToolRegistrationEnabled, …), so the explicitEditorPrefs.SetBoolcalls here are redundant.You can simplify to rely on the processor property alone:
public static void EnableRegistration() { - CustomToolRegistrationProcessor.IsRegistrationEnabled = true; - EditorPrefs.SetBool(EditorPrefKeys.CustomToolRegistrationEnabled, true); - Debug.Log("MCP Custom Tool Registration enabled"); + CustomToolRegistrationProcessor.IsRegistrationEnabled = true; + Debug.Log("MCP Custom Tool Registration enabled"); } public static void DisableRegistration() { - CustomToolRegistrationProcessor.IsRegistrationEnabled = false; - EditorPrefs.SetBool(EditorPrefKeys.CustomToolRegistrationEnabled, false); - Debug.Log("MCP Custom Tool Registration disabled"); + CustomToolRegistrationProcessor.IsRegistrationEnabled = false; + Debug.Log("MCP Custom Tool Registration disabled"); }Optionally, you may also want to switch the
Debug.Logcalls toMcpLog.Infofor consistency with the rest of the editor tooling.MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs (1)
17-43: Menu wiring and toggle behavior look solidThe setup/menu entry points and the toggle logic (Line 32–37) are clear and safe: using
EditorWindow.HasOpenInstances<MCPForUnityEditorWindow>()before closing all instances avoids unnecessary work. If you ever want to avoidResources.FindObjectsOfTypeAllscans entirely, you could expose aCloseAll()helper onMCPForUnityEditorWindowthat iterates its internalOpenWindowsset instead, but that’s a nice‑to‑have, not required here.MCPForUnity/Editor/Services/Transport/TransportManager.cs (1)
18-99: TransportManager lifecycle and state handling look correctThe start/stop flow is disciplined:
StartAsync(Line 36–59) always tears down any existing client, validates factories, and only commits_active/_activeModeafter a successful start;StopAsync(Line 61–79) reliably clears state even on errors.GetState(Line 90–98) returning a disconnected snapshot when no transport is active keeps callers simple. If you want slightly better diagnosability, you could optionally log whenStartAsyncreturnsfalseor whenVerifyAsyncis invoked with no active transport, but the current behavior is already safe.MCPForUnity/Editor/Services/IToolDiscoveryService.cs (1)
5-52: Tool discovery metadata model is well‑structured and aligned with the implementation
ToolMetadataandParameterMetadatamap cleanly onto whatToolDiscoveryServiceextracts (name/description/parameters/polling flags), and the defaultedDescription/PollActionvalues help ensure tools are always describable to MCP clients. One small robustness improvement would be to initializeParametersto an empty list to guard against null if anyone constructsToolMetadatamanually, but the current usage viaToolDiscoveryServiceis already safe.Based on learnings
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
141-191: Alignuvxhelpers with overrides and harden EditorPrefs readThe new helpers look good and should cover most workflows, but there are a couple of consistency gaps you might want to tighten up:
GetMcpServerGitUrlreadsEditorPrefs.GetString(EditorPrefKeys.GitUrlOverride, "")without a try/catch, whereas other code paths (e.g.,McpLog.ReadDebugPreference,PathResolverService.GetUvxPath) guard EditorPrefs access. A defensive try/catch here would avoid hard failures in odd editor states and mirror the existing pattern.GetUvxCommandcurrently hard‑codes"uvx"and the git URL, ignoring both the UV path override and theGitUrlOverridepreference you just introduced. Consider either:
- delegating to
GetUvxCommandParts()and composing the full shell command from(uvxPath, fromUrl, packageName), or- at least reusing
GetMcpServerGitUrl()so git URL override and version pinning stay in one place.These are mainly for consistency and future maintenance; behavior today is otherwise sound.
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (1)
20-92: PATH-based UV detection looks solid; consider small robustness/logging tweaksThe new
DetectUV+TryFindUvInPathflow is straightforward and safe: you only mark UV available whenuvx/uv --versionsucceeds and the output matchesuv …, and otherwise fall back to a clear “not found in PATH” error.Two optional hardening ideas:
- In
TryFindUvInPath, you currently ignore all exceptions per command; logging at debug level which command failed (and why) would make diagnosing PATH issues easier without spamming normal users.- After
process.WaitForExit(5000), you rely onprocess.ExitCode; if the process somehow hangs beyond 5s, accessingExitCodecan throw. Checking theWaitForExitreturn value and skipping that command if it times out would avoid that edge case.Not blockers; the current implementation should work fine for typical
uvx/uv --versionbehavior.MCPForUnity/Editor/Helpers/CustomToolRegistrationProcessor.cs (2)
19-36: Guard EditorPrefs access for custom-tool registration flag
CustomToolRegistrationProcessor’s static ctor andIsRegistrationEnabledsetter useEditorPrefs.GetBool/SetBooldirectly. Elsewhere in the codebase (e.g.,McpLog,PathResolverService), EditorPrefs access is wrapped in try/catch to avoid unexpected editor/runtime issues.To keep behavior consistent and robust in headless/batch or corrupted-prefs scenarios, consider:
static CustomToolRegistrationProcessor() { try { _isRegistrationEnabled = EditorPrefs.GetBool(EditorPrefKeys.CustomToolRegistrationEnabled, true); } catch { _isRegistrationEnabled = true; } } public static bool IsRegistrationEnabled { get => _isRegistrationEnabled; set { _isRegistrationEnabled = value; try { EditorPrefs.SetBool(EditorPrefKeys.CustomToolRegistrationEnabled, value); } catch { /* ignore preference write failures */ } } }Functional behavior stays the same when prefs work, but you avoid hard failures when they don’t.
101-115: Confirm one-shot auto-registration behavior after script reload
NotifyHttpConnectionHealthyonly triggersRegisterAllTools()when_autoRegistrationPendingis true, and then clears the flag before invoking registration. That means:
- You auto-register once after each script reload, on the first “HTTP connection healthy” notification.
- If that registration attempt fails (e.g., transient server issue), auto-registration will not retry unless the user either forces re-registration or scripts reload again.
If the intent is “best-effort once per reload and otherwise manual,” this is fine. If you’d like auto-registration to retry on failure, you could set
_autoRegistrationPendingback totruewhenRegisterAllTools()returns/flags a failure, or move the pending flag handling intoRegisterAllTools()so it can decide based on the result.Worth double-checking the desired UX here.
MCPForUnity/Editor/Services/ToolDiscoveryService.cs (1)
25-51: Use a saferGetTypespattern so partial reflection failures don’t drop whole assembliesThe discovery pipeline is correct overall, but this bit is a little blunt:
foreach (var assembly in assemblies) { try { var types = assembly.GetTypes(); foreach (var type in types) { ... } } catch (Exception ex) { // Skip assemblies that can't be reflected McpLog.Info($"Skipping assembly {assembly.FullName}: {ex.Message}"); } }If an assembly throws
ReflectionTypeLoadException, you currently skip all types in that assembly, even though most of them may be valid. There’s already aSafeGetTypeshelper inManageScriptthat handles this more gracefully by usingReflectionTypeLoadException.Typesand filtering out nulls. Based on learnings.You could either call that helper or inline the same pattern here, e.g.:
private static IEnumerable<Type> SafeGetTypes(Assembly assembly) { try { return assembly.GetTypes(); } catch (ReflectionTypeLoadException rtle) { return rtle.Types.Where(t => t != null); } catch { return Array.Empty<Type>(); } } ... foreach (var assembly in assemblies) { foreach (var type in SafeGetTypes(assembly)) { var toolAttr = type.GetCustomAttribute<McpForUnityToolAttribute>(); if (toolAttr == null) continue; var metadata = ExtractToolMetadata(type, toolAttr); if (metadata != null) { _cachedTools[metadata.Name] = metadata; } } }That way, a single problematic type doesn’t prevent discovering other tools in the same assembly.
Also applies to: 105-139
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
93-252: Linux UV detection logic looks sound; consider de‑duplicating PATH augmentationDetectUV/TryValidateUv correctly try
uv/uvxwith an augmented PATH, then fall back to/usr/bin/which, and surface clear “not in PATH” errors. This matches the macOS behavior and should be robust for typical Linux installs.You might want to DRY up the PATH additions by reusing
GetPathAdditions()insideTryFindInPath(and possiblyTryValidatePython) to keep all Linux PATH tweaks in one place.MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
91-251: macOS UV detection mirrors Linux and looks correct; small DRY opportunityThe new
DetectUV/TryValidateUvpath is consistent with the Linux implementation: tryuv/uvxfirst with an augmented PATH, then fall back to/usr/bin/which, and emit clear guidance when not found. PATH augmentation viaGetPathAdditionsis appropriate for common Homebrew and system locations.You could optionally reduce duplication by sharing
GetPathAdditions()between Python and UV detection (and perhaps betweenTryFindInPathandBuildAugmentedPath) so that any future PATH tweak for macOS only needs to be updated once.MCPForUnity/Editor/Services/ServerManagementService.cs (1)
65-220: Shell/terminal command quoting is fragile; consider centralizing escaping
TryGetLocalHttpServerCommandconstructs a single shell command string that is then embedded into various terminal invocations (osascript → Terminal,cmd.exe /c start,gnome-terminal/xterm/konsole/xfce4-terminal). This works for the common case (no spaces/quotes in uvx path or overrides), but it will break or behave unexpectedly if:
uvxPathcontains spaces (e.g., a custom override under a path with spaces), or- any user‑overridden Git URL/base URL somehow introduces quotes or other shell‑sensitive characters.
A more robust approach would be to centralize escaping for
command(e.g., helper that produces correctly quoted/escaped fragments per platform), or to avoid nesting shell levels where possible (e.g., passFileName = uvxPathandArguments = ...directly on Windows/Linux, and keep the terminal launcher responsible only for opening a window).Not urgent for typical setups, but worth tightening before users start customizing paths heavily.
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)
24-117: UVX resolution anduvPathparameter are currently redundant in downstream builders
WriteMcpConfigurationandConfigureCodexClientboth resolve auvxPath, but:
ConfigJsonBuilder.ApplyUnityServerToExistingConfig(existingRoot, uvxPath, mcpClient)ultimately usesAssetPathUtility.GetUvxCommandParts(), which callsMCPServiceLocator.Paths.GetUvxPath()again and ignores theuvPathargument.CodexConfigHelper.UpsertCodexServerBlock(existingToml, uvxPath)likewise routes throughCreateUnityMcpTable(uvPath), which currently ignoresuvPathand again callsAssetPathUtility.GetUvxCommandParts().So today:
- The
"UV package manager not found"branches are effectively unreachable with the currentPathResolverService.GetUvxPathimplementation.- The
uvPathvalues you pass into the helpers are unused and could be misleading to future maintainers who expect overrides to matter.If this is intentional (centralizing all path resolution inside
AssetPathUtility), consider either:
- Dropping the
uvPathparameters fromConfigJsonBuilder/CodexConfigHelperand from these calls, or- Wiring those parameters through as overrides in
GetUvxCommandPartsso callers can actually influence the command.This is a cleanliness/maintainability point rather than a functional bug right now.
Also applies to: 122-164
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
13-37: Transport-aware config generation looks correct; consider tighteninguvPathusageThe new
PopulateUnityNodelogic cleanly:
- Switches between HTTP and stdio via
EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true).- Uses
urlvsserverUrlcorrectly for Windsurf and cleans up the stale counterpart.- Removes
command/argswhen switching to HTTP and removesurl/serverUrlwhen switching to stdio.- Sets
"type"only for VSCode and strips it for non‑VSCode clients.- Normalizes
env/disabledfor Kiro/Windsurf in a way that aligns with the existing learnings around per‑client quirks (Codex is handled separately in TOML). Based on learnings.Functionally this looks solid.
The only smell is that
uvPathis now threaded throughBuildManualConfigJson→ApplyUnityServerToExistingConfig→PopulateUnityNodebut never actually used (all uvx pieces come fromAssetPathUtility.GetUvxCommandParts()). As inMcpConfigurationHelper, that can mislead callers who think passing a customuvPathmatters.If you don’t plan to reintroduce explicit uv overrides here, consider:
- public static string BuildManualConfigJson(string uvPath, McpClient client) + public static string BuildManualConfigJson(string uvPath, McpClient client) { var root = new JObject(); ... - PopulateUnityNode(unity, uvPath, client, isVSCode); + PopulateUnityNode(unity, client, isVSCode); } - public static JObject ApplyUnityServerToExistingConfig(JObject root, string uvPath, McpClient client) + public static JObject ApplyUnityServerToExistingConfig(JObject root, string uvPath, McpClient client) { ... - PopulateUnityNode(unity, uvPath, client, isVSCode); + PopulateUnityNode(unity, client, isVSCode); } - private static void PopulateUnityNode(JObject unity, string uvPath, McpClient client, bool isVSCode) + private static void PopulateUnityNode(JObject unity, McpClient client, bool isVSCode)(and update call sites accordingly), or wiring
uvPathintoAssetPathUtility.GetUvxCommandPartsas an override.Not urgent, but it will reduce future confusion.
Also applies to: 41-137
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1)
18-23:uvPathparameter is unused; all uvx details come fromAssetPathUtility.GetUvxCommandPartsIn both
BuildCodexServerBlock(string uvPath)andCreateUnityMcpTable(string uvPath):
- The
uvPathargument is never referenced.- Stdio config instead uses
AssetPathUtility.GetUvxCommandParts()to resolveuvxPath,fromUrl, andpackageName.Given that:
- Callers passing a custom
uvPathcurrently have no effect.- CodexConfigHelper is now internally authoritative about uvx resolution, mirroring the JSON config side.
Consider either:
- Removing the
uvPathparameter fromBuildCodexServerBlock,UpsertCodexServerBlock,CreateUnityMcpTable, and their call sites, or- Updating
GetUvxCommandParts()to accept an optional override and wiringuvPaththrough so explicit paths still matter.This is a maintainability/clarity tweak; behavior today is consistent but slightly misleading.
Also applies to: 69-83, 159-201
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
19-23: New service wiring in MCPServiceLocator looks consistent; only minor testability nitsThe additions for
IToolDiscoveryService,ICustomToolRegistrationService,ICacheManagementService,IServerManagementService, andTransportManagerare wired consistently with the existing locator pattern (lazy properties,Register<T>overrides, andReset()disposal/nulling). This should play nicely with tests that inject custom implementations viaRegister<T>().Two small, non-blocking thoughts:
- If you ever want to mock/replace
TransportManagerwithout depending on the concrete type in tests, consider introducing an interface (e.g.,ITransportManager) and branching on that instead ofTransportManagerinRegister<T>.CustomToolRegistration’s default ctor captures the currentToolDiscoveryinstance; if tests overrideIToolDiscoveryServiceafterCustomToolRegistrationhas been accessed, they’ll also need to overrideICustomToolRegistrationServiceto keep things in sync. Worth calling out in docs/comments if that pattern becomes common in tests.Also applies to: 31-35, 56-66, 79-83, 91-95
MCPForUnity/Editor/Services/PathResolverService.cs (3)
18-37: UnusedverifyPathparameter and override semantics inGetUvxPathThe
verifyPathparameter onGetUvxPath(bool verifyPath = true)is currently unused, and the method simply returns the override (if any) or"uvx". That matches the “default to uvx and allow override” objective, but the signature suggests callers might expect some validation behavior.Two options, depending on intent:
- If you don’t plan to do filesystem validation anymore, consider documenting that
verifyPathis intentionally ignored (and perhaps rename in the interface in a future breaking change).- If you do want optional validation, gate a
File.Existscheck onverifyPathand fall back to"uvx"(ornull) when the override is invalid.
97-123: Python detection is robust;IsUvxDetectedis effectively a configuration check now
IsPythonDetected()is a reasonable lightweight probe (short timeout, full try/catch around process start/exit), and should fail closed in odd environments.For
IsUvxDetected(), note thatGetUvxPath()always returns either an override or"uvx", so this method will always reporttrueas long as a non‑empty string is returned. That aligns with “stop detecting uv path” (no process probe), but semantically it’s closer to “we have some configured command string” than “uvx is actually installed.” If any callers still rely on real detection, you may want to:
- Either update call‑sites to treat this as “is configured” rather than “is installed,” or
- Reintroduce an optional, explicit detection path (e.g., a separate
ProbeUvxAsyncused only on demand in tooling UI).
125-193:GetUvxPackageSourcePathworks but ignores UV/UVX overridesThe
uv cache dir+ git‑checkouts scan is a nice way to locate the Server source in the uv cache, and the error logging guidance is helpful when it fails.One behavioral gap: this always shells out to
FileName = "uv", regardless of any UV/UVX override configured viaSetUvxPathOverride/GetUvxPath(). On machines where only an overriddenuvx/uvis available (or installed in a non‑standard location), this helper could fail even though the rest of the system can run uvx via the override. Consider:
- Deriving the binary from
GetUvxPath()and, if it looks like anuvxpath, mapping it to an adjacentuvbinary (similar toCacheManagementService.ClearUvxCache()), or- Reusing a small shared helper that encapsulates the “uv vs uvx + override” resolution for all uv invocations, so behavior stays consistent across features.
| if [ -f Server/pyproject.toml ]; then | ||
| uv pip install -e Server | ||
| elif [ -f Server/requirements.txt ]; then | ||
| uv pip install -r Server/requirements.txt | ||
| elif [ -f Server/pyproject.toml ]; then | ||
| uv pip install -e Server | ||
| elif [ -f Server/requirements.txt ]; then | ||
| uv pip install -r Server/requirements.txt |
There was a problem hiding this comment.
Remove duplicate conditional checks in MCP server installation.
Lines 62–65 are exact duplicates of lines 58–61. The second elif for Server/pyproject.toml can never be reached if the first one matched, and the same applies to requirements.txt. This is a logic error introduced during refactoring.
Apply this diff to remove the duplicate checks:
- name: Install MCP server
run: |
set -eux
uv venv
echo "VIRTUAL_ENV=$GITHUB_WORKSPACE/.venv" >> "$GITHUB_ENV"
echo "$GITHUB_WORKSPACE/.venv/bin" >> "$GITHUB_PATH"
if [ -f Server/pyproject.toml ]; then
uv pip install -e Server
elif [ -f Server/requirements.txt ]; then
uv pip install -r Server/requirements.txt
- elif [ -f Server/pyproject.toml ]; then
- uv pip install -e Server
- elif [ -f Server/requirements.txt ]; then
- uv pip install -r Server/requirements.txt
else
echo "No MCP Python deps found (skipping)"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -f Server/pyproject.toml ]; then | |
| uv pip install -e Server | |
| elif [ -f Server/requirements.txt ]; then | |
| uv pip install -r Server/requirements.txt | |
| elif [ -f Server/pyproject.toml ]; then | |
| uv pip install -e Server | |
| elif [ -f Server/requirements.txt ]; then | |
| uv pip install -r Server/requirements.txt | |
| if [ -f Server/pyproject.toml ]; then | |
| uv pip install -e Server | |
| elif [ -f Server/requirements.txt ]; then | |
| uv pip install -r Server/requirements.txt | |
| else | |
| echo "No MCP Python deps found (skipping)" | |
| fi |
🤖 Prompt for AI Agents
In .github/workflows/claude-nl-suite.yml around lines 58 to 65 there are
duplicated conditional checks for Server/pyproject.toml and
Server/requirements.txt; remove the repeated elif blocks so each file check
appears only once (keep one if/elif/elif/else sequence for installing the MCP
server), ensuring the installation commands run for pyproject.toml or
requirements.txt without redundant duplicate branches.
| public static void SaveState<T>(string toolName, T state) | ||
| { | ||
| var path = GetStatePath(toolName); | ||
| Directory.CreateDirectory(Path.GetDirectoryName(path)); | ||
| var json = JsonConvert.SerializeObject(state ?? Activator.CreateInstance<T>()); | ||
| File.WriteAllText(path, json); | ||
| } |
There was a problem hiding this comment.
Document or constrain the generic type parameter.
Line 30 uses Activator.CreateInstance<T>() which requires T to have a parameterless constructor. If state is null and T lacks this constructor, a runtime exception will be thrown.
Consider either:
- Adding a generic constraint:
where T : new() - Removing the fallback and documenting that callers must never pass null
- Wrapping in a try-catch and logging or re-throwing with context
Additionally, file I/O operations can fail (permissions, disk space). Consider wrapping in try-catch for better error messages if SaveState failures become an issue during development.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Helpers/McpJobStateStore.cs around lines 26 to 32, the use
of Activator.CreateInstance<T>() when state is null requires T to have a
parameterless constructor and will throw at runtime for types without one; add a
generic constraint "where T : new()" to the SaveState<T> method signature to
guarantee a parameterless constructor (or alternatively remove the null fallback
and document callers must not pass null), and wrap the directory/file operations
in a try-catch that logs or rethrows a contextual exception to surface IO
failures (permissions/disk) with clear messages.
| /// Get the port to use from storage, or return the default if none has been saved yet. | ||
| /// </summary> | ||
| /// <returns>Port number to use</returns> | ||
| public static int GetPortWithFallback() | ||
| { | ||
| // Try to load stored port first, but only if it's from the current project | ||
| var storedConfig = GetStoredPortConfig(); | ||
| if (storedConfig != null && | ||
| storedConfig.unity_port > 0 && | ||
| string.Equals(storedConfig.project_path ?? string.Empty, Application.dataPath ?? string.Empty, StringComparison.OrdinalIgnoreCase) && | ||
| IsPortAvailable(storedConfig.unity_port)) | ||
| string.Equals(storedConfig.project_path ?? string.Empty, Application.dataPath ?? string.Empty, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| if (IsDebugEnabled()) Debug.Log($"<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>: Using stored port {storedConfig.unity_port} for current project"); | ||
| return storedConfig.unity_port; | ||
| } | ||
|
|
||
| // If stored port exists but is currently busy, wait briefly for release | ||
| if (storedConfig != null && storedConfig.unity_port > 0) | ||
| { | ||
| if (WaitForPortRelease(storedConfig.unity_port, 1500)) | ||
| { | ||
| if (IsDebugEnabled()) Debug.Log($"<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>: Stored port {storedConfig.unity_port} became available after short wait"); | ||
| return storedConfig.unity_port; | ||
| } | ||
| // Port is still busy after waiting - find a new available port instead | ||
| if (IsDebugEnabled()) Debug.Log($"<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>: Stored port {storedConfig.unity_port} is occupied by another instance, finding alternative..."); | ||
| int newPort = FindAvailablePort(); | ||
| SavePort(newPort); | ||
| return newPort; | ||
| } | ||
|
|
||
| // If no valid stored port, find a new one and save it | ||
| int foundPort = FindAvailablePort(); | ||
| SavePort(foundPort); | ||
| return foundPort; | ||
| return DefaultPort; | ||
| } |
There was a problem hiding this comment.
SetPreferredPort XML summary does not match behavior
Line 67+ XML says “If port is unavailable, the next available port is chosen instead”, but the implementation currently throws InvalidOperationException when the port is in use and never falls back. This mismatch is likely to confuse callers and users.
You can either update the behavior to actually probe for a fallback port, or correct the comment to reflect the current “throws on unavailable port” semantics. For example, if you want fallback behavior:
- /// Persist a user-selected port and return the value actually stored.
- /// If <paramref name="port"/> is unavailable, the next available port is chosen instead.
+ /// Persist a user-selected port and return the value actually stored.
+ /// Throws if <paramref name="port"/> is invalid; if it is already in use, a new available port is chosen instead.
@@
- public static int SetPreferredPort(int port)
+ public static int SetPreferredPort(int port)
{
if (port <= 0)
{
throw new ArgumentOutOfRangeException(nameof(port), "Port must be positive.");
}
- if (!IsPortAvailable(port))
- {
- throw new InvalidOperationException($"Port {port} is already in use.");
- }
-
- SavePort(port);
- return port;
+ if (!IsPortAvailable(port))
+ {
+ // Fallback to discovery if requested port is busy
+ port = FindAvailablePort();
+ }
+
+ SavePort(port);
+ return port;
}If you prefer strict behavior instead, just fix the XML summary and drop the “next available port” wording.
Also applies to: 71-85
🤖 Prompt for AI Agents
MCPForUnity/Editor/Helpers/PortManager.cs around lines 67-85: the XML summary
for SetPreferredPort claims it will “choose the next available port” but the
method currently throws InvalidOperationException when the requested port is in
use; either implement actual fallback probing or correct the docs—choose one:
(A) Implement fallback: attempt subsequent ports (e.g., port+1 up to a sensible
max or DefaultPort range), test each for availability, set and persist the first
free port and return it; ensure exceptions are thrown only if no port found. (B)
Keep strict behavior: update the XML summary to remove the “next available port”
wording and explicitly state that the method throws InvalidOperationException
when the requested port is unavailable and that callers must handle fallback
themselves.
| private static bool JsonConfigUsesStdIo(McpClient client) | ||
| { | ||
| string configPath = McpConfigurationHelper.GetClientConfigPath(client); | ||
| if (string.IsNullOrEmpty(configPath) || !File.Exists(configPath)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| var root = JObject.Parse(File.ReadAllText(configPath)); | ||
|
|
||
| JToken unityNode = null; | ||
| if (client.mcpType == McpTypes.VSCode) | ||
| { | ||
| unityNode = root.SelectToken("servers.unityMCP") | ||
| ?? root.SelectToken("mcp.servers.unityMCP"); | ||
| } | ||
| else | ||
| { | ||
| unityNode = root.SelectToken("mcpServers.unityMCP"); | ||
| } | ||
|
|
||
| if (unityNode == null) return false; | ||
|
|
||
| return unityNode["command"] != null; | ||
| } | ||
| catch | ||
| { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Likely typo in VSCode JSON config detection path
In JsonConfigUsesStdIo:
if (client.mcpType == McpTypes.VSCode)
{
unityNode = root.SelectToken("servers.unityMCP")
?? root.SelectToken("mcp.servers.unityMCP");
}All generated configs for VSCode use either servers.unityMCP (current) or historically mcpServers.unityMCP (camel‑cased property name), but there is no producer for a nested mcp.servers.unityMCP path.
This means the fallback selector is almost certainly a typo and won’t match any real config, while failing to cover a potential legacy mcpServers container.
Suggest:
- unityNode = root.SelectToken("servers.unityMCP")
- ?? root.SelectToken("mcp.servers.unityMCP");
+ unityNode = root.SelectToken("servers.unityMCP")
+ ?? root.SelectToken("mcpServers.unityMCP");This would let the migration also catch older VSCode configs, if any existed, without affecting current ones.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Migrations/StdIoVersionMigration.cs around lines 102 to
133, the VSCode JSON path fallback uses an incorrect selector
"mcp.servers.unityMCP" which never matches real configs; change the fallback to
check "mcpServers.unityMCP" (i.e., use root.SelectToken("servers.unityMCP") ??
root.SelectToken("mcpServers.unityMCP")) so legacy camel-cased containers are
detected, keeping the existing primary selector intact.
| public BridgeVerificationResult Verify(int port) | ||
| { | ||
| byte[] buffer = new byte[count]; | ||
| int offset = 0; | ||
| stream.ReadTimeout = timeoutMs; | ||
| while (offset < count) | ||
| { | ||
| int read = stream.Read(buffer, offset, count - offset); | ||
| if (read <= 0) throw new IOException("Connection closed before reading expected bytes"); | ||
| offset += read; | ||
| } | ||
| return buffer; | ||
| } | ||
| var mode = _transportManager.ActiveMode ?? ResolvePreferredMode(); | ||
| bool pingSucceeded = _transportManager.VerifyAsync().GetAwaiter().GetResult(); | ||
| var state = _transportManager.GetState(); | ||
|
|
||
| private static string ReadLineAscii(NetworkStream stream, int timeoutMs, int maxLen = 512) | ||
| { | ||
| stream.ReadTimeout = timeoutMs; | ||
| using (var ms = new MemoryStream()) | ||
| if (mode == TransportMode.Stdio) | ||
| { | ||
| byte[] one = new byte[1]; | ||
| while (ms.Length < maxLen) | ||
| { | ||
| int n = stream.Read(one, 0, 1); | ||
| if (n <= 0) break; | ||
| if (one[0] == (byte)'\n') break; | ||
| ms.WriteByte(one[0]); | ||
| } | ||
| return Encoding.ASCII.GetString(ms.ToArray()); | ||
| bool handshakeValid = state.IsConnected && port == CurrentPort; | ||
| string message = handshakeValid | ||
| ? $"STDIO transport listening on port {CurrentPort}" | ||
| : $"STDIO transport port mismatch (expected {CurrentPort}, got {port})"; | ||
| return BuildVerificationResult(state, mode, pingSucceeded && handshakeValid, message, handshakeValid); | ||
| } | ||
|
|
||
| return BuildVerificationResult(state, mode, pingSucceeded); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Blocking on VerifyAsync() in Verify(int port) can deadlock in async contexts
Verify(int port) currently does:
var mode = _transportManager.ActiveMode ?? ResolvePreferredMode();
bool pingSucceeded = _transportManager.VerifyAsync().GetAwaiter().GetResult();
var state = _transportManager.GetState();
...If any IMcpTransportClient.VerifyAsync() implementation ever performs real async work that captures the Unity synchronization context (e.g., WebSocket I/O with await but no ConfigureAwait(false)), this sync wait can deadlock the editor thread.
Safer options:
- Keep
Verify(int)as a thin wrapper over the async path and push the port check into a shared helper:
public async Task<BridgeVerificationResult> VerifyAsync(int? port = null)
{
var mode = _transportManager.ActiveMode ?? ResolvePreferredMode();
bool pingSucceeded = await _transportManager.VerifyAsync();
var state = _transportManager.GetState();
if (mode == TransportMode.Stdio && port.HasValue)
{
bool handshakeValid = state.IsConnected && port.Value == CurrentPort;
string message = handshakeValid
? $"STDIO transport listening on port {CurrentPort}"
: $"STDIO transport port mismatch (expected {CurrentPort}, got {port.Value})";
return BuildVerificationResult(state, mode, pingSucceeded && handshakeValid, message, handshakeValid);
}
return BuildVerificationResult(state, mode, pingSucceeded);
}
public BridgeVerificationResult Verify(int port)
{
return VerifyAsync(port).GetAwaiter().GetResult();
}- Or, if you must keep heavy work off the main context, wrap the verify call in
Task.Runso the async method runs on a thread-pool context without capturing Unity’s synchronization context.
In both cases, also ensuring that transport implementations use ConfigureAwait(false) internally would further reduce deadlock risk.
I'll verify the deadlock concern by examining the codebase structure, callers, and async implementation patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all callers of the Verify method
rg -nP --type=csharp '\bVerify\s*\(' -A2 -B2 | head -100
Length of output: 923
🏁 Script executed:
#!/bin/bash
# Locate and inspect the BridgeControlService class
fd -e cs BridgeControlService.csLength of output: 104
🏁 Script executed:
#!/bin/bash
# Search for VerifyAsync implementation in TransportManager
rg -nP --type=csharp 'VerifyAsync|IMcpTransportClient' -A5 -B2 | head -150Length of output: 12717
🏁 Script executed:
#!/bin/bash
# Check if GetAwaiter().GetResult() pattern is used elsewhere
rg -nP --type=csharp 'GetAwaiter\s*\(\s*\)\s*\.GetResult\s*\(' -B3 -A1Length of output: 570
🏁 Script executed:
#!/bin/bash
# Examine the broader BridgeControlService structure
fd -e cs BridgeControlService.cs -x catLength of output: 7386
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for all calls to Verify method with int parameter (not VerifyAsync)
rg -nP --type=csharp '\.Verify\s*\(\s*\d+\s*\)|\.Verify\s*\([a-zA-Z_]\w*\s*\)' -B3 -A1
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Alternative search - look for references to Verify without Async suffix
rg -nP --type=csharp '\bVerify\s*\(\s*(?!Async)' -B2 -A2 | grep -v 'VerifyAsync' | head -50Length of output: 923
🏁 Script executed:
#!/bin/bash
# Check the full WebSocketTransportClient.VerifyAsync to understand what async work it does
fd -e cs WebSocketTransportClient.cs -x catLength of output: 18316
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for any test files or other references to Verify(int method
rg -nP --type=csharp 'Verify\s*\(' --type=cs -g '**/*Test*' -g '**/*test*'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if IBridgeControlService.Verify is implemented elsewhere or if there are other implementations
rg -nP --type=csharp 'IBridgeControlService|class.*BridgeControlService' -A10 | head -60Length of output: 6174
🏁 Script executed:
#!/bin/bash
# Search in all file types for references to .Verify( with port, not just C#
rg 'Verify\s*\(\s*[a-zA-Z0-9_]+\s*\)' -g '!node_modules' -g '!.git' -B2 -A1 | head -80Length of output: 281
Blocking VerifyAsync() in Verify(int port) is a confirmed architectural issue that should be addressed
The review concern is valid. While no internal callers of Verify(int port) were found in the codebase, the method is part of the public IBridgeControlService contract (documented in v6_NEW_UI_CHANGES.md) and could be used externally or retained for backward compatibility.
The underlying problem is real:
- Line 114 blocks with
GetAwaiter().GetResult()on an async operation WebSocketTransportClient.VerifyAsync()performs genuine async I/O (_socket.SendAsync()with 5-second timeout)- Calling this from the Unity editor main thread could deadlock if the async method captures the synchronization context
The UI code correctly uses VerifyAsync() with await (McpConnectionSection.cs), but the synchronous wrapper remains a footgun for external callers or future internal use.
Recommended fixes:
Either wrap the async call in Task.Run to escape the Unity context:
public BridgeVerificationResult Verify(int port)
{
return Task.Run(async () => await VerifyAsync(port)).GetAwaiter().GetResult();
}Or refactor both methods to share logic as suggested in the review, with the sync method deferring to the async path.
🤖 Prompt for AI Agents
MCPForUnity/Editor/Services/BridgeControlService.cs lines 111-127: the sync
Verify(int port) blocks on VerifyAsync() via GetAwaiter().GetResult(), which can
deadlock the Unity editor; fix by avoiding direct blocking — either have Verify
call the async implementation off the Unity context using Task.Run to execute
VerifyAsync(port) and synchronously return its result, or refactor so both
Verify and VerifyAsync share a common implementation where the sync path
delegates to the async logic executed on a background thread; ensure the
returned BridgeVerificationResult and any port/handshake logic are preserved.
| if (hasOverride) | ||
| { | ||
| string overridePath = pathService.GetUvxPath(); | ||
|
|
||
| if (!string.IsNullOrEmpty(overridePath) && File.Exists(overridePath)) | ||
| { | ||
| string overrideDirectory = Path.GetDirectoryName(overridePath); | ||
| string overrideExtension = Path.GetExtension(overridePath); | ||
| string overrideName = Path.GetFileNameWithoutExtension(overridePath); | ||
|
|
||
| if (!string.IsNullOrEmpty(overrideDirectory) && overrideName.Equals("uvx", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| string uvSibling = Path.Combine(overrideDirectory, string.IsNullOrEmpty(overrideExtension) ? "uv" : $"uv{overrideExtension}"); | ||
| if (File.Exists(uvSibling)) | ||
| { | ||
| uvCommand = uvSibling; | ||
| McpLog.Debug($"Using UV executable inferred from override: {uvSibling}"); | ||
| } | ||
| else | ||
| { | ||
| uvCommand = overridePath; | ||
| McpLog.Debug($"Using override executable: {overridePath}"); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| uvCommand = overridePath; | ||
| McpLog.Debug($"Using override executable: {overridePath}"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid falling back to uvx for cache clean when no sibling uv exists
When the override points to uvx and no sibling uv executable is found, the code currently falls back to invoking the override path (i.e., uvx cache clean mcp-for-unity). If, as uv’s CLI docs suggest, cache clean is a uv subcommand (and not a valid uvx invocation), this path will always fail instead of gracefully falling back to uv on PATH.
A safer behavior would be: if the override is uvx but there is no sibling uv, keep uvCommand as the default "uv" so the later PATH-based branch is used.
You could adjust the override handling like this:
- if (!string.IsNullOrEmpty(overrideDirectory) && overrideName.Equals("uvx", StringComparison.OrdinalIgnoreCase))
- {
- string uvSibling = Path.Combine(overrideDirectory, string.IsNullOrEmpty(overrideExtension) ? "uv" : $"uv{overrideExtension}");
- if (File.Exists(uvSibling))
- {
- uvCommand = uvSibling;
- McpLog.Debug($"Using UV executable inferred from override: {uvSibling}");
- }
- else
- {
- uvCommand = overridePath;
- McpLog.Debug($"Using override executable: {overridePath}");
- }
- }
+ if (!string.IsNullOrEmpty(overrideDirectory) && overrideName.Equals("uvx", StringComparison.OrdinalIgnoreCase))
+ {
+ string uvSibling = Path.Combine(
+ overrideDirectory,
+ string.IsNullOrEmpty(overrideExtension) ? "uv" : $"uv{overrideExtension}");
+
+ if (File.Exists(uvSibling))
+ {
+ uvCommand = uvSibling;
+ McpLog.Debug($"Using UV executable inferred from override: {uvSibling}");
+ }
+ else
+ {
+ // Leave uvCommand as the default "uv" so we fall back to PATH‑based resolution.
+ McpLog.Debug(
+ $"UVX override specified at '{overridePath}' but no sibling 'uv' executable was found; falling back to 'uv' from PATH.");
+ }
+ }
else
{
uvCommand = overridePath;
McpLog.Debug($"Using override executable: {overridePath}");
}This keeps the “smart sibling” behavior when both uvx and uv are installed, but avoids an almost‑certainly invalid uvx cache clean ... call when only uvx is present.
Also applies to: 77-80
I'll now verify this review comment by inspecting the actual code and understanding the uvx/uv behavior.
🏁 Script executed:
#!/bin/bash
# Find and inspect the CacheManagementService.cs file
find . -name "CacheManagementService.cs" -type f 2>/dev/null | head -5Length of output: 55
🏁 Script executed:
#!/bin/bash
# Display lines 27-55 and 77-80 of the file
if [ -f "./MCPForUnity/Editor/Services/CacheManagementService.cs" ]; then
echo "=== Lines 27-55 ==="
sed -n '27,55p' "./MCPForUnity/Editor/Services/CacheManagementService.cs"
echo ""
echo "=== Lines 77-80 ==="
sed -n '77,80p' "./MCPForUnity/Editor/Services/CacheManagementService.cs"
fiLength of output: 1867
🏁 Script executed:
#!/bin/bash
# Check the full context around the override handling to understand the complete logic
if [ -f "./MCPForUnity/Editor/Services/CacheManagementService.cs" ]; then
echo "=== Full method context (lines 15-95) ==="
sed -n '15,95p' "./MCPForUnity/Editor/Services/CacheManagementService.cs"
fiLength of output: 3614
🏁 Script executed:
#!/bin/bash
# Search for other uses of uvCommand in the file to understand where it's used
rg -n "uvCommand" "./MCPForUnity/Editor/Services/CacheManagementService.cs" -A 2 -B 2Length of output: 1895
🏁 Script executed:
#!/bin/bash
# Check how uvCommand is used downstream (e.g., in cache clean invocations)
rg -n "cache.*clean" ./MCPForUnity/ --type cs -C 3Length of output: 735
🌐 Web query:
uv CLI cache clean command documentation - is it a uv subcommand or uvx subcommand?
💡 Result:
"uv cache clean" is a uv subcommand (not an uvx subcommand). See the uv CLI reference which lists "uv cache clean", and the tools docs that explain uvx is an alias for "uv tool run" (different purpose). [1][2]
Sources:
[1] uv CLI reference / commands (lists "uv cache clean").
[2] uv tools docs (explains uvx = alias for uv tool run).
Fix uvCommand fallback when override is uvx with no sibling uv executable
When the override points to uvx but no sibling uv exists, the code sets uvCommand = overridePath and later attempts to run uvx cache clean mcp-for-unity. Since cache clean is a uv subcommand (not supported by uvx), this command will fail.
Update lines 45–49 to leave uvCommand as the default "uv" when only uvx is found:
- else
- {
- uvCommand = overridePath;
- McpLog.Debug($"Using override executable: {overridePath}");
- }
+ else
+ {
+ McpLog.Debug($"UVX override specified at '{overridePath}' but no sibling 'uv' executable was found; falling back to 'uv' from PATH.");
+ }This allows the PATH-based uv cache clean fallback (lines 81+) to execute instead of the invalid uvx cache clean call.
🤖 Prompt for AI Agents
MCPForUnity/Editor/Services/CacheManagementService.cs around lines 27 to 55:
when the provided override points to an executable named "uvx" but there is no
sibling "uv" executable, do not set uvCommand to the overridePath (which would
cause later "uvx cache clean" to fail); instead leave uvCommand as the default
"uv" (i.e., do not overwrite it) and update the debug log to indicate the uvx
override was found but ignored in favor of the PATH "uv" fallback so that the
later PATH-based "uv cache clean" logic can run.
| if (response.success) | ||
| { | ||
| McpLog.Info($"Successfully registered {response.registered?.Count ?? 0} tools with MCP server"); | ||
| return true; | ||
| } | ||
|
|
||
| McpLog.Error($"Failed to register tools: {response.error ?? "Unknown error"}"); | ||
| return false; |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify logging for 409/duplicate registrations to avoid misleading counts
The registration flow is generally solid and handles 409 conflicts gracefully, but there’s a small mismatch in how success is reported:
- In
SendRegistrationAsync, forHttpStatusCode.Conflictyou log:
McpLog.Info($"Tool registration skipped - already registered ({duplicateList})");
return new RegisterToolsResponse
{
success = true,
registered = duplicates
};- In
RegisterAllToolsAsync, anyresponse.success == truepath logs:
McpLog.Info($"Successfully registered {response.registered?.Count ?? 0} tools with MCP server");On a pure-duplicates 409, this will log “Successfully registered N tools” where N is actually the count of already-registered tools, not newly registered ones, and you also get two Info logs for the same event.
Two easy ways to tighten this up:
- Keep duplicates and newly registered tools separate, and adjust the top-level message, for example:
if (response.success)
{
var registeredCount = response.registered?.Count ?? 0;
var duplicateCount = response.duplicates?.Count ?? 0;
if (registeredCount == 0 && duplicateCount > 0)
McpLog.Info($"Tool registration skipped – {duplicateCount} tools already registered");
else
McpLog.Info($"Successfully registered {registeredCount} tools ({duplicateCount} already registered)");
return true;
}- Or, in the 409 branch, leave
registerednull and only populateduplicates, so the existing success log prints0for newly registered tools.
Either way, the log output will more accurately reflect what actually happened while keeping the “409 == non-fatal” behavior.
Also applies to: 113-122
🏁 Script executed:
#!/bin/bash
# Search for SendRegistrationAsync and RegisterToolsResponse to understand structure
rg -n "SendRegistrationAsync|RegisterToolsResponse" --type=cs -A 15 MCPForUnity/Editor/Services/CustomToolRegistrationService.csLength of output: 4021
🏁 Script executed:
#!/bin/bash
# Examine the full RegisterAllToolsAsync method context around lines 49-56 and 113-122
rg -n "RegisterAllToolsAsync|SendRegistrationAsync" --type=cs -A 50 MCPForUnity/Editor/Services/CustomToolRegistrationService.cs | head -200Length of output: 5174
🏁 Script executed:
#!/bin/bash
# Look for HttpStatusCode.Conflict handling
rg -n "Conflict" --type=cs -B 5 -A 10 MCPForUnity/Editor/Services/CustomToolRegistrationService.csLength of output: 875
Clarify logging for 409/duplicate registrations to avoid double-logging and misleading counts
The issue is confirmed: on a 409 Conflict response, two Info logs fire in sequence:
- Line 117 in
SendRegistrationAsync:McpLog.Info($"Tool registration skipped - already registered ({duplicateList})") - Line 51 in
RegisterAllToolsAsync:McpLog.Info($"Successfully registered {response.registered?.Count ?? 0} tools")
This creates both redundant logging and a misleading success message—the count reflects already-registered duplicates, not newly-registered tools.
The suggested fix of either keeping registered and duplicates separate (with adjusted logging) or nullifying registered on 409 so it stays null/empty is sound and will resolve both issues.
| private static void CancelPending(string id, CancellationToken token) | ||
| { | ||
| PendingCommand pending = null; | ||
| lock (PendingLock) | ||
| { | ||
| if (Pending.Remove(id, out pending)) | ||
| { | ||
| UnhookUpdateIfIdle(); | ||
| } | ||
| } | ||
|
|
||
| pending?.TrySetCanceled(); | ||
| pending?.Dispose(); | ||
| } | ||
|
|
||
| private static void RemovePending(string id, PendingCommand pending) | ||
| { | ||
| lock (PendingLock) | ||
| { | ||
| Pending.Remove(id); | ||
| UnhookUpdateIfIdle(); | ||
| } | ||
|
|
||
| pending.Dispose(); | ||
| } |
There was a problem hiding this comment.
Avoid calling Unity editor APIs from CancellationToken callbacks
CancelPending is registered directly on the passed CancellationToken and can run on arbitrary thread‑pool threads. Inside the lock, it calls UnhookUpdateIfIdle(), which in turn touches EditorApplication.update (Unity editor API). Unity generally expects all editor API usage, especially event subscription/unsubscription, to occur on the main thread only.
You can safely drop the unhook from CancelPending and let the next editor update perform the idle unhook on the main thread via ProcessQueue:
private static void CancelPending(string id, CancellationToken token)
{
PendingCommand pending = null;
lock (PendingLock)
{
- if (Pending.Remove(id, out pending))
- {
- UnhookUpdateIfIdle();
- }
+ Pending.Remove(id, out pending);
}
pending?.TrySetCanceled();
pending?.Dispose();
}With this change, EditorApplication.update is only hooked/unhooked from code paths already known to run on the main thread (ProcessQueue), avoiding cross‑thread Unity API usage while still ensuring the update hook is released once the queue is empty.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static void CancelPending(string id, CancellationToken token) | |
| { | |
| PendingCommand pending = null; | |
| lock (PendingLock) | |
| { | |
| if (Pending.Remove(id, out pending)) | |
| { | |
| UnhookUpdateIfIdle(); | |
| } | |
| } | |
| pending?.TrySetCanceled(); | |
| pending?.Dispose(); | |
| } | |
| private static void RemovePending(string id, PendingCommand pending) | |
| { | |
| lock (PendingLock) | |
| { | |
| Pending.Remove(id); | |
| UnhookUpdateIfIdle(); | |
| } | |
| pending.Dispose(); | |
| } | |
| private static void CancelPending(string id, CancellationToken token) | |
| { | |
| PendingCommand pending = null; | |
| lock (PendingLock) | |
| { | |
| Pending.Remove(id, out pending); | |
| } | |
| pending?.TrySetCanceled(); | |
| pending?.Dispose(); | |
| } | |
| private static void RemovePending(string id, PendingCommand pending) | |
| { | |
| lock (PendingLock) | |
| { | |
| Pending.Remove(id); | |
| UnhookUpdateIfIdle(); | |
| } | |
| pending.Dispose(); | |
| } |
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs around
lines 252 to 276, CancelPending is being invoked from a CancellationToken
callback which may run off the main thread but currently calls
UnhookUpdateIfIdle (which touches EditorApplication.update) inside the lock;
remove the UnhookUpdateIfIdle call from CancelPending so the method only
performs the thread-safe Pending.Remove under the lock and then
TrySetCanceled/Dispose the pending outside the lock, leaving the existing
RemovePending/ProcessQueue logic (which runs on the main thread) responsible for
unhooking the editor update.
* Fix HTTP instance routing and per-project session IDs * Drop confusing log message * Ensure lock file references later version of uvicorn with key fixes * Fix test imports * Update refs in docs --------- Co-authored-by: David Sarno <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (2)
80-83: Consider calling EnsureIdentityCache for consistency.
GetProjectName()returns the cached value directly, whereasGetProjectHash()callsEnsureIdentityCache()before returning. While the static constructor schedules a cache refresh, explicitly ensuring the cache would make the behavior more predictable and consistent withGetProjectHash().Apply this diff to improve consistency:
public static string GetProjectName() { + EnsureIdentityCache(); return _cachedProjectName; }
85-103: Simplify hash truncation logic.SHA1 always produces 20 bytes (40 hex characters), so
Math.Min(8, sb.Length)on line 97 will always evaluate to8. TheMath.Minadds unnecessary complexity.Apply this diff to simplify:
- return sb.ToString(0, Math.Min(8, sb.Length)); + return sb.ToString(0, 8);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-39)
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (2)
125-150: Excellent project-scoped session ID design.The implementation correctly scopes the session ID per project by incorporating the project hash into the pref key. The fallback to a runtime GUID when EditorPrefs are unavailable (e.g., batch mode) ensures robustness across different Unity execution contexts.
204-222: Robust fallback hash computation for batch mode.The fallback logic correctly handles scenarios where
Application.dataPathis unavailable (e.g., batch mode or tests) by computing the hash from the current working directory with "Assets" appended. The normalization of trailing separators ensures hash stability across different path representations.
We also make the identifying hashes longer
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (2)
61-64: Add debug logging to exception handler.Silent exception swallowing reduces observability. Consider logging to aid debugging when identity cache updates fail.
Apply this diff to add debug logging:
catch { - // Ignore and keep defaults + McpLog.Debug("Failed to update identity cache, keeping defaults."); }
80-83: Consider ensuring cache for consistency.Unlike
GetProjectHash()which callsEnsureIdentityCache(), this method directly returns the cached value. For API consistency and robustness, consider adding the same guard.Apply this diff:
public static string GetProjectName() { + EnsureIdentityCache(); return _cachedProjectName; }MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (3)
165-200: Note: Socket keep-alive interval updates require reconnection.The
_socket.Options.KeepAliveIntervalis set during connection (line 169), butApplyWelcomecan update_socketKeepAliveInterval(lines 314, 322) based on server configuration. This updated value won't apply to the current socket connection until the next reconnection cycle.This is acceptable behavior since reconnection will pick up the new value, but consider adding a comment documenting this limitation.
_socket = new ClientWebSocket(); +// Note: Keep-alive interval changes from server welcome won't apply until next reconnection _socket.Options.KeepAliveInterval = _socketKeepAliveInterval;
308-324: Consider clarifying the socket keep-alive calculation logic.The calculation of
_socketKeepAliveIntervalin lines 317-322 is complex:int sourceSeconds = keepAliveSeconds ?? serverTimeoutSeconds.Value; int safeSeconds = Math.Max(5, Math.Min(serverTimeoutSeconds.Value, sourceSeconds));The logic ensures a value between 5 seconds and the minimum of
serverTimeoutandkeepAliveInterval, but the intent isn't immediately clear. Consider adding a comment explaining the rationale.if (serverTimeoutSeconds.HasValue) { + // Ensure socket-level keep-alive is safely bounded: + // - At least 5 seconds to avoid excessive traffic + // - At most the smaller of server timeout and app keep-alive interval int sourceSeconds = keepAliveSeconds ?? serverTimeoutSeconds.Value; int safeSeconds = Math.Max(5, Math.Min(serverTimeoutSeconds.Value, sourceSeconds)); _socketKeepAliveInterval = TimeSpan.FromSeconds(safeSeconds); }
381-393: Consider logging parse failures for command responses.The catch block silently swallows JSON parse exceptions when processing command responses. While returning a generic error result is appropriate, logging the parse failure would help diagnose issues where commands return malformed JSON.
try { resultToken = JToken.Parse(responseJson); } -catch +catch (Exception ex) { + McpLog.Warn($"[WebSocket] Failed to parse command response as JSON: {ex.Message}"); resultToken = new JObject { ["status"] = "error", ["error"] = "Invalid response payload" }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs(1 hunks)MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/Transport/Transports/WebSocketTransportClient.csMCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (6)
MCPForUnity/Editor/Services/Transport/IMcpTransportClient.cs (3)
Task(14-14)Task(15-15)Task(16-16)MCPForUnity/Editor/Services/Transport/TransportManager.cs (4)
Task(36-59)Task(61-79)Task(81-88)TransportState(90-98)MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (2)
Task(65-90)TransportCommandDispatcher(19-313)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/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)
🔇 Additional comments (9)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (9)
23-52: LGTM!The reconnection schedule and timeout constants are well-chosen. The use of
volatilefor connection flags provides appropriate visibility guarantees for the flag-checking pattern used throughout the class.
58-81: LGTM!Excellent practice capturing Unity-specific identity values on the main thread before any async context switching. The startup sequence properly cleans up existing state and handles connection failure gracefully.
83-130: LGTM!The shutdown sequence follows best practices: cancel the lifecycle token first, await background tasks, then gracefully close the socket. Defensive exception handling ensures cleanup completes even if individual steps fail.
132-156: LGTM!Straightforward verification using the existing pong mechanism with a reasonable 5-second timeout.
202-271: LGTM!The receive loop properly handles WebSocket lifecycle events and exceptions, triggering reconnection when needed. The chunked message reading with an 8KB buffer and MemoryStream accumulation correctly handles messages of any size.
405-453: LGTM!The keep-alive loop and messaging functions are well-structured. The comment on line 436 clearly documents that session_id is server-authoritative and omitted from registration, which aligns with the session management pattern seen in HandleRegistered.
455-480: LGTM!Thread-safe JSON sending is correctly implemented using a semaphore with proper release in the finally block. The state validation prevents sending on closed sockets.
482-535: LGTM with note on session ID lifecycle during reconnection.The reconnection logic properly guards against concurrent reconnection attempts and follows an appropriate backoff schedule.
Note that line 522 sets the state with the current
_sessionId, which may be stale until the server sends a new "registered" message (handled in HandleRegistered). This is the correct behavior since the server is authoritative for session management, and the session ID will be updated once registration completes.
537-559: LGTM!The WebSocket URI construction properly handles scheme mapping (http→ws, https→wss), preserves the base URL's authority and path, and appends the hub endpoint path. Defensive validation ensures invalid URLs are caught early.
| /// <summary> | ||
| /// Returns the SHA1 hash of the current project path (truncated to 8 characters). | ||
| /// Matches the legacy hash used by the stdio bridge and server registry. |
There was a problem hiding this comment.
Fix documentation inconsistency.
The comment states the hash is "truncated to 8 characters," but line 97 truncates to 16 characters via sb.ToString(0, Math.Min(16, sb.Length)).
Apply this diff to correct the documentation:
/// <summary>
-/// Returns the SHA1 hash of the current project path (truncated to 8 characters).
+/// Returns the SHA1 hash of the current project path (truncated to 16 characters).
/// Matches the legacy hash used by the stdio bridge and server registry.
/// </summary>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Returns the SHA1 hash of the current project path (truncated to 8 characters). | |
| /// Matches the legacy hash used by the stdio bridge and server registry. | |
| /// <summary> | |
| /// Returns the SHA1 hash of the current project path (truncated to 16 characters). | |
| /// Matches the legacy hash used by the stdio bridge and server registry. |
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs around lines 67–69 (and
note the truncation occurs on line ~97), the XML doc summary incorrectly states
the SHA1 hash is "truncated to 8 characters" while the code truncates to 16
characters; update the documentation to say "truncated to 16 characters" (or
otherwise match the actual truncation length used in the sb.ToString call) so
the comment and implementation are consistent.
OK, this is outright the best OSS Unity MCP available
Not required for the fix, but it's a good guarantee regardless of the working directory
Update all Optional[Type] annotations to use the PEP 604 union syntax Type | None throughout the transport layer and mcp_source.py script
Update all Dict[K, V] annotations to use the built-in dict[K, V] syntax across services, transport layer, and models for consistency with PEP 585
Clean up unused imports of Dict, List, and Path types that are no longer needed after migration to modern type hint syntax
It's working, we have a better integration test in any case
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
Improvements
Removals
✏️ Tip: You can customize this high-level summary in your review settings.