HTTP setup overhaul: transport selection (HTTP local/remote vs stdio), safer lifecycle, cleaner UI, better Claude Code integration #499
Conversation
…stering When registering with Claude Code, if a UnityMCP server already exists, remove it first before adding the new registration. This ensures the transport mode (HTTP vs stdio) is always updated to match the current UseHttpTransport EditorPref setting. Previously, if a stdio registration existed and the user tried to register with HTTP, the command would fail with 'already exists' and the old stdio configuration would remain unchanged.
…ctly
The validation code was incorrectly parsing the output of 'claude mcp get UnityMCP' by looking for JSON format ("transport": "http"), but the CLI actually returns human-readable text format ("Type: http"). This caused the transport mismatch detection to never trigger, allowing stdio to be selected in the UI while HTTP was registered with Claude Code.
Changes:
- Fix parsing logic to check for "Type: http" or "Type: stdio" in CLI output
- Add OnTransportChanged event to refresh client status when transport changes
- Wire up event handler to trigger client status refresh on transport dropdown change
This ensures that when the transport mode in Unity doesn't match what's registered with Claude Code, the UI will correctly show an error status with instructions to re-register.
…laude CLI status checks and HTTP simplifications
This commit resolves three issues with Claude Code registration: 1. UI blocking: Removed synchronous CheckStatus() call after registration that was blocking the editor. Status is now set immediately with async verification happening in the background. 2. Thread safety: Fixed "can only be called from the main thread" errors by capturing Application.dataPath and EditorPrefs.GetBool() on the main thread before spawning async status check tasks. 3. Transport mismatch detection: Transport mode changes now trigger immediate status checks to detect HTTP/stdio mismatches, instead of waiting for the 45-second refresh interval. The registration button now turns green immediately after successful registration without blocking, and properly detects transport mismatches when switching between HTTP and stdio modes.
Address code review feedback by making CheckStatusWithProjectDir thread-safe by design rather than by convention: 1. Made projectDir and useHttpTransport parameters non-nullable to prevent accidental background thread calls without captured values 2. Removed nullable fallback to EditorPrefs.GetBool() which would cause thread safety violations if called from background threads 3. Added ArgumentNullException for null projectDir instead of falling back to Application.dataPath (which is main-thread only) 4. Added XML documentation clearly stating threading contracts: - CheckStatus() must be called from main thread - CheckStatusWithProjectDir() is safe for background threads 5. Removed unreachable else branch in async status check code These changes make it impossible to misuse the API from background threads, with compile-time enforcement instead of runtime errors.
Reviewer's GuideOverhauls MCP’s HTTP transport and lifecycle handling: introduces explicit HTTP Local/HTTP Remote/Stdio transport selection with scoped prefs, adds deterministic management of Unity-launched local HTTP servers via pidfiles + instance tokens, refines connection/health UI and consolidated Start/Stop behavior, tightens shutdown cleanup, and makes Claude CLI integration non-blocking and transport-aware, including server-side support for lifecycle tokens via new CLI args. Sequence diagram for HTTP Local Start Server and deterministic lifecyclesequenceDiagram
actor User
participant McpConnectionSection
participant BridgeControlService
participant TransportManager
participant ServerManagementService
participant PythonServerMain as PythonServer_main_py
User->>McpConnectionSection: Click Start Server (HTTP Local)
McpConnectionSection->>ServerManagementService: StartLocalHttpServer()
ServerManagementService->>ServerManagementService: StopLocalHttpServerInternal(quiet=true)
ServerManagementService->>ServerManagementService: Check port free via GetListeningProcessIdsForPort
alt port in use
ServerManagementService-->>McpConnectionSection: return false
McpConnectionSection-->>User: Show Port In Use dialog
else port free
ServerManagementService->>ServerManagementService: Create pidfile path and instance token
ServerManagementService->>ServerManagementService: StoreLocalHttpServerHandshake(pidfile, token)
ServerManagementService->>PythonServerMain: Launch terminal process with --pidfile and --unity-instance-token
PythonServerMain->>PythonServerMain: Parse args and write pidfile with os.getpid()
PythonServerMain->>PythonServerMain: Set UNITY_MCP_INSTANCE_TOKEN env var
ServerManagementService-->>McpConnectionSection: return true
McpConnectionSection->>McpConnectionSection: TryAutoStartSessionAfterHttpServerAsync()
McpConnectionSection->>McpConnectionSection: WaitForHttpServerAcceptingConnectionsAsync(timeout)
loop until reachable or timeout
McpConnectionSection->>PythonServerMain: TcpClient.ConnectAsync(host, port)
PythonServerMain-->>McpConnectionSection: accept or fail
end
McpConnectionSection->>BridgeControlService: StartAsync()
BridgeControlService->>TransportManager: StopAsync(TransportMode.Stdio)
BridgeControlService->>TransportManager: StartAsync(TransportMode.Http)
TransportManager-->>BridgeControlService: started bool
alt started
BridgeControlService-->>McpConnectionSection: true
McpConnectionSection->>McpConnectionSection: VerifyBridgeConnectionAsync()
McpConnectionSection-->>User: Session running, Health updated
else not started
BridgeControlService-->>McpConnectionSection: false
McpConnectionSection-->>User: Warn failed to auto start session
end
end
Sequence diagram for HTTP Local Stop Server with pidfile and token validationsequenceDiagram
actor User
participant McpConnectionSection
participant BridgeControlService
participant ServerManagementService
participant PythonServerMain as PythonServer_main_py
User->>McpConnectionSection: Click Stop Server (HTTP Local)
alt session running
McpConnectionSection->>BridgeControlService: StopAsync()
BridgeControlService->>TransportManager: StopAsync(currentMode)
BridgeControlService-->>McpConnectionSection: completed
end
McpConnectionSection->>ServerManagementService: StopLocalHttpServer()
ServerManagementService->>ServerManagementService: TryGetLocalHttpServerHandshake(out pidfile, out token)
alt handshake exists
ServerManagementService->>ServerManagementService: TryReadPidFromPidFile(pidfile)
alt pid invalid
ServerManagementService-->>McpConnectionSection: false (pidfile not ready)
else pid valid
ServerManagementService->>ServerManagementService: GetListeningProcessIdsForPort(port)
alt no listener
ServerManagementService->>ServerManagementService: Delete pidfile and ClearLocalServerPidTracking()
ServerManagementService-->>McpConnectionSection: false
else listener present
ServerManagementService->>ServerManagementService: ProcessCommandLineContainsInstanceToken(pid, token)
alt token and port match
ServerManagementService->>PythonServerMain: TerminateProcess(pid)
PythonServerMain-->>ServerManagementService: process exits
ServerManagementService->>ServerManagementService: Delete pidfile and ClearLocalServerPidTracking()
ServerManagementService-->>McpConnectionSection: true
else validation failed
ServerManagementService-->>McpConnectionSection: false (refusing to stop)
end
end
end
else no handshake
ServerManagementService->>ServerManagementService: TryGetStoredLocalServerPid(port, out storedPid)
ServerManagementService->>ServerManagementService: GetListeningProcessIdsForPort(port)
ServerManagementService->>ServerManagementService: LooksLikeMcpServerProcess(pid)
alt looks like MCP server
ServerManagementService->>PythonServerMain: TerminateProcess(pid)
PythonServerMain-->>ServerManagementService: exited
ServerManagementService-->>McpConnectionSection: true or false
else not MCP server
ServerManagementService-->>McpConnectionSection: false (refusing to stop)
end
end
McpConnectionSection->>McpConnectionSection: UpdateConnectionStatus()
McpConnectionSection-->>User: UI reflects stopped state
Sequence diagram for non-blocking Claude CLI status check with transport-aware validationsequenceDiagram
actor User
participant MCPForUnityEditorWindow as EditorWindow
participant McpClientConfigSection
participant ClaudeCliMcpConfigurator
participant ClaudeCliCli as ClaudeCLI
User->>EditorWindow: Open MCP For Unity window
EditorWindow->>McpClientConfigSection: RefreshSelectedClient(forceImmediate=true)
McpClientConfigSection->>McpClientConfigSection: RefreshClientStatus(selected, forceImmediate)
McpClientConfigSection->>McpClientConfigSection: RefreshClaudeCliStatus(client, forceImmediate)
alt forceImmediate or needsRefresh
McpClientConfigSection->>McpClientConfigSection: ApplyStatusToUi(showChecking=true)
McpClientConfigSection->>McpClientConfigSection: Capture projectDir and useHttpTransport on main thread
McpClientConfigSection->>ClaudeCliMcpConfigurator: CheckStatusWithProjectDir(projectDir, useHttpTransport, attemptAutoRewrite=false) async on background Task
ClaudeCliMcpConfigurator->>ClaudeCLI: ExecPath.TryRun(claude, "mcp list")
ClaudeCLI-->>ClaudeCliMcpConfigurator: stdout with UnityMCP entry or not
alt UnityMCP present
ClaudeCliMcpConfigurator->>ClaudeCLI: ExecPath.TryRun(claude, "mcp get UnityMCP")
ClaudeCLI-->>ClaudeCliMcpConfigurator: stdout with Type http or Type stdio
ClaudeCliMcpConfigurator->>ClaudeCliMcpConfigurator: Compare registered transport vs useHttpTransport
alt mismatch
ClaudeCliMcpConfigurator->>ClaudeCliMcpConfigurator: client.SetStatus(McpStatus.Error, transport mismatch message)
else match
ClaudeCliMcpConfigurator->>ClaudeCliMcpConfigurator: client.SetStatus(McpStatus.Configured)
end
else UnityMCP not present
ClaudeCliMcpConfigurator->>ClaudeCliMcpConfigurator: client.SetStatus(McpStatus.NotConfigured)
end
ClaudeCliMcpConfigurator-->>McpClientConfigSection: completed Task
McpClientConfigSection->>McpClientConfigSection: lastStatusChecks[client] = DateTime.UtcNow
McpClientConfigSection->>McpClientConfigSection: ApplyStatusToUi(client)
McpClientConfigSection-->>User: UI shows up to date Claude CLI status
end
Class diagram for updated transport and server lifecycle managementclassDiagram
class IServerManagementService {
<<interface>>
+bool StartLocalHttpServer()
+bool StopLocalHttpServer()
+bool IsLocalHttpServerRunning()
+bool TryGetLocalHttpServerCommand(out string command, out string error)
}
class ServerManagementService {
-static HashSet~int~ LoggedStopDiagnosticsPids
-static object LocalHttpServerProcessLock
-static Process LocalHttpServerProcess
-static bool OpenedHttpServerLogViewerThisSession
+bool StartLocalHttpServer()
+bool StopLocalHttpServer()
+bool IsLocalHttpServerRunning()
+bool TryGetLocalHttpServerCommand(out string command, out string error)
-bool StopLocalHttpServerInternal(bool quiet)
-void TryCaptureAndStoreLocalServerPid()
-void TryOpenLogInTerminal(string logPath)
-ProcessStartInfo CreateLocalHttpServerProcessStartInfo(string fileName, string arguments)
-bool ProcessCommandLineContainsInstanceToken(int pid, string instanceToken)
-bool TryGetUnixProcessArgs(int pid, out string argsLower)
-bool LooksLikeMcpServerProcess(int pid)
-bool TerminateProcess(int pid)
-bool ProcessExistsUnix(int pid)
-void StoreLocalServerPidTracking(int pid, int port, string argsHash)
-bool TryGetStoredLocalServerPid(int expectedPort, out int pid)
-void ClearLocalServerPidTracking()
-void StoreLocalHttpServerHandshake(string pidFilePath, string instanceToken)
-bool TryGetLocalHttpServerHandshake(out string pidFilePath, out string instanceToken)
-string GetLocalHttpServerPidFilePath(int port)
-bool TryReadPidFromPidFile(string pidFilePath, out int pid)
-string ComputeShortHash(string input)
-void LogStopDiagnosticsOnce(int pid, string details)
-string TrimForLog(string s)
-ProcessStartInfo CreateTerminalProcessStartInfo(string command)
-bool TryGetLocalHttpServerCommandParts(out string fileName, out string arguments, out string displayCommand, out string error)
}
class TransportProtocol {
<<enumeration>>
HTTPLocal
HTTPRemote
Stdio
}
class McpConnectionSection {
-TransportProtocol transportDropdownValue
-Button connectionToggleButton
-Button startHttpServerButton
-Button testConnectionButton
-VisualElement healthIndicator
-VisualElement healthRow
-Label healthStatusLabel
-bool connectionToggleInProgress
-bool autoStartInProgress
-bool httpServerToggleInProgress
-double lastLocalServerRunningPollTime
-bool lastLocalServerRunning
+event Action OnManualConfigUpdateRequested
+event Action OnTransportChanged
+void InitializeUI()
+void RegisterCallbacks()
+void UpdateConnectionStatus()
+void UpdateHttpServerCommandDisplay()
+void RefreshHttpUi()
+Task VerifyBridgeConnectionAsync()
-void PersistHttpUrlFromField()
-void UpdateHttpFieldVisibility()
-bool IsHttpLocalSelected()
-void UpdateStartHttpButtonState()
-async void OnHttpServerToggleClicked()
-async Task TryAutoStartSessionAfterHttpServerAsync()
-static async Task~bool~ WaitForHttpServerAcceptingConnectionsAsync(TimeSpan timeout)
}
class BridgeControlService {
-ITransportManager _transportManager
+Task~bool~ StartAsync()
+Task StopAsync()
-TransportMode ResolvePreferredMode()
}
class McpEditorShutdownCleanup {
<<static>>
+static void OnEditorQuitting()
}
class ClaudeCliMcpConfigurator {
+ClaudeCliMcpConfigurator(McpClient client)
+override string GetConfigPath()
+override McpStatus CheckStatus(bool attemptAutoRewrite)
+internal McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTransport, bool attemptAutoRewrite)
-void Register()
-void Unregister()
}
class McpClientConfigSection {
-int selectedClientIndex
-List~IMcpClientConfigurator~ configurators
-Dictionary~IMcpClientConfigurator, DateTime~ lastStatusChecks
-HashSet~IMcpClientConfigurator~ statusRefreshInFlight
+void RefreshSelectedClient(bool forceImmediate)
-void RefreshClientStatus(IMcpClientConfigurator client, bool forceImmediate)
-void RefreshClaudeCliStatus(IMcpClientConfigurator client, bool forceImmediate)
}
class MCPForUnityEditorWindow {
-McpConnectionSection connectionSection
-McpClientConfigSection clientConfigSection
+void CreateGUI()
}
class EditorPrefKeys {
<<static>>
+const string UseHttpTransport
+const string HttpTransportScope
+const string LastLocalHttpServerPid
+const string LastLocalHttpServerPort
+const string LastLocalHttpServerStartedUtc
+const string LastLocalHttpServerPidArgsHash
+const string LastLocalHttpServerPidFilePath
+const string LastLocalHttpServerInstanceToken
}
class TransportManager {
+Task~bool~ StartAsync(TransportMode mode)
+Task StopAsync(TransportMode mode)
}
class McpServerMainPy {
+def main()
}
IServerManagementService <|.. ServerManagementService
McpConnectionSection ..> IServerManagementService : uses
McpConnectionSection ..> BridgeControlService : uses via MCPServiceLocator
BridgeControlService ..> TransportManager : uses
McpEditorShutdownCleanup ..> TransportManager : uses
McpEditorShutdownCleanup ..> IServerManagementService : uses
MCPForUnityEditorWindow o--> McpConnectionSection
MCPForUnityEditorWindow o--> McpClientConfigSection
McpClientConfigSection ..> ClaudeCliMcpConfigurator : uses
ClaudeCliMcpConfigurator ..> McpServerMainPy : configures HTTP or stdio
ServerManagementService ..> McpServerMainPy : starts local HTTP server
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds mutual exclusivity for HTTP vs stdio transports, comprehensive local HTTP server lifecycle (pidfile/token, start/stop, platform process handling), editor shutdown cleanup, centralized main-thread command dispatcher, Claude CLI thread-safe status checks, UI changes for local/remote HTTP, and Python server pidfile/token CLI options. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as McpConnectionSection (UI)
participant Bridge as BridgeControlService
participant Transports as Transport(s)
participant Server as MCP Server
UI->>Bridge: StartAsync(requestedTransport)
rect rgb(220,240,255)
Note over Bridge: Ensure mutual exclusivity
Bridge->>Transports: Stop(otherTransport)
Transports-->>Bridge: Stopped / Warning
end
rect rgb(240,220,255)
Note over Bridge: Start requested transport
Bridge->>Transports: Start(requestedTransport)
Transports->>Server: Connect
Server-->>Transports: Connected
Transports-->>Bridge: Started
end
Bridge-->>UI: Completed
UI->>UI: Raise OnTransportChanged
sequenceDiagram
participant UI as McpConnectionSection
participant ServerMgmt as ServerManagementService
participant Terminal as Terminal/OS
participant Server as Local HTTP Server
participant Handshake as Handshake (pidfile/token)
UI->>ServerMgmt: StartLocalHttpServer()
ServerMgmt->>ServerMgmt: StopManagedLocalHttpServer() if present
ServerMgmt->>ServerMgmt: Verify port availability
ServerMgmt->>Handshake: Generate pidfile & token
ServerMgmt->>Terminal: Launch command with --pidfile --unity-instance-token
Terminal->>Server: Server process starts
Server->>Handshake: Write pidfile
Handshake-->>ServerMgmt: Confirmed
ServerMgmt-->>UI: Started (persist handshake)
UI->>UI: Poll server readiness -> success
UI->>Bridge: Optionally auto-start session
sequenceDiagram
participant Editor as Unity Editor
participant Cleanup as McpEditorShutdownCleanup
participant Bridge as BridgeControlService
participant ServerMgmt as ServerManagementService
Editor->>Cleanup: EditorApplication.quitting
Cleanup->>Bridge: Stop HTTP & Stdio (concurrent, 750ms bound)
Bridge-->>Cleanup: Stopped / Warnings
Cleanup->>ServerMgmt: Attempt StopLocalHttpServer() or StopManagedLocalHttpServer()
ServerMgmt-->>Cleanup: Attempted / Logged
Cleanup->>Editor: Quit proceeds (errors logged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-09-03T16:00:55.839ZApplied to files:
📚 Learning: 2025-10-13T13:27:23.040ZApplied to files:
📚 Learning: 2025-12-29T15:23:11.613ZApplied to files:
🧬 Code graph analysis (2)MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (5)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
ProcessCommandLineContainsInstanceTokenon Windows, thereturn !ok ? true : false;fallback makes a failed command-line query count as a token match, which seems opposite to the comment about falling back to port+pidfile validation; consider returningfalse(or distinguishingokfromtokenMatch) so failures don’t silently loosen the validation. CreateLocalHttpServerProcessStartInfoinServerManagementServiceis currently unused; either wire it into the HTTP-local launch path (e.g., for non-terminal launches or future refactors) or remove it to avoid dead code and confusion.- In
RefreshClaudeCliStatusyou unconditionally castIMcpClientConfiguratortoClaudeCliMcpConfigurator; to make this more robust against future call sites, consider anascast with a null-check (or constraining the method to the specific type) so an accidental non-Claude client doesn’t cause anInvalidCastException.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ProcessCommandLineContainsInstanceToken` on Windows, the `return !ok ? true : false;` fallback makes a failed command-line query count as a token match, which seems opposite to the comment about falling back to port+pidfile validation; consider returning `false` (or distinguishing `ok` from `tokenMatch`) so failures don’t silently loosen the validation.
- `CreateLocalHttpServerProcessStartInfo` in `ServerManagementService` is currently unused; either wire it into the HTTP-local launch path (e.g., for non-terminal launches or future refactors) or remove it to avoid dead code and confusion.
- In `RefreshClaudeCliStatus` you unconditionally cast `IMcpClientConfigurator` to `ClaudeCliMcpConfigurator`; to make this more robust against future call sites, consider an `as` cast with a null-check (or constraining the method to the specific type) so an accidental non-Claude client doesn’t cause an `InvalidCastException`.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs:661-670` </location>
<code_context>
+ return true; // Don't block if URL cannot be parsed
+ }
+
+ string host = uri.Host;
+ int port = uri.Port;
+
+ var deadline = DateTime.UtcNow + timeout;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Auto-connect polling may misbehave for wildcard/bind-all HTTP hosts like 0.0.0.0/::.
In `WaitForHttpServerAcceptingConnectionsAsync`, using `uri.Host` directly can break for wildcard hosts like `0.0.0.0` or `::`, where a `TcpClient` connect will usually fail even though the server is listening on loopback. That makes the readiness check too pessimistic and can hurt auto-start behavior. Consider normalizing wildcard/empty/`*` hosts to `localhost` (e.g., `127.0.0.1`) before attempting the TCP connection.
```suggestion
string host = uri.Host;
int port = uri.Port;
// Normalize wildcard/bind-all hosts to loopback for readiness checks
string normalizedHost;
if (string.IsNullOrWhiteSpace(host) ||
string.Equals(host, "0.0.0.0", StringComparison.OrdinalIgnoreCase) ||
string.Equals(host, "::", StringComparison.OrdinalIgnoreCase) ||
string.Equals(host, "*", StringComparison.OrdinalIgnoreCase))
{
normalizedHost = "localhost";
}
else
{
normalizedHost = host;
}
var deadline = DateTime.UtcNow + timeout;
while (DateTime.UtcNow < deadline)
{
try
{
using var client = new TcpClient();
var connectTask = client.ConnectAsync(normalizedHost, port);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
522-552: Consider removing unused method.
CreateLocalHttpServerProcessStartInfoappears to be unused after the refactor to terminal-based launching. If it's intentionally kept for potential future use, consider adding a comment explaining this.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Services/BridgeControlService.csMCPForUnity/Editor/Services/IServerManagementService.csMCPForUnity/Editor/Services/McpEditorShutdownCleanup.csMCPForUnity/Editor/Services/McpEditorShutdownCleanup.cs.metaMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Windows/Components/Common.ussMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.uxmlMCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxmlMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csServer/src/main.py
🧰 Additional context used
🧠 Learnings (3)
📚 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/McpEditorShutdownCleanup.cs.metaMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.csMCPForUnity/Editor/Services/IServerManagementService.csMCPForUnity/Editor/Services/BridgeControlService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Services/McpEditorShutdownCleanup.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.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/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Services/ServerManagementService.cs
🧬 Code graph analysis (7)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)
MCPForUnity/Editor/Services/IServerManagementService.cs (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
IsLocalHttpServerRunning(677-737)
MCPForUnity/Editor/Services/BridgeControlService.cs (2)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (2)
StdioBridgeHost(122-171)Stop(415-484)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
RefreshSelectedClient(292-303)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (2)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (2)
ClaudeCliMcpConfigurator(328-613)ClaudeCliMcpConfigurator(330-330)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)
MCPForUnity/Editor/Services/McpEditorShutdownCleanup.cs (2)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)MCPForUnity/Editor/Services/IServerManagementService.cs (2)
IsLocalUrl(44-44)StopLocalHttpServer(24-24)
MCPForUnity/Editor/Services/ServerManagementService.cs (3)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)MCPForUnity/Editor/Helpers/ExecPath.cs (1)
ExecPath(12-272)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
HttpEndpointUtility(12-85)GetBaseUrl(20-24)
🪛 Ruff (0.14.10)
Server/src/main.py
447-447: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (39)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml (1)
11-11: UI label clarification aligns well with PR's polish goals.The change from "Show Debug Logs:" to "Debug Mode:" is a clear, concise improvement to the label that better communicates the setting's purpose. This fits well within the PR's broader UI/UX enhancements.
Server/src/main.py (2)
394-409: LGTM! Clear lifecycle management arguments.The new CLI arguments are well-documented and appropriately optional, enabling Unity to coordinate server lifecycle without breaking existing usage patterns.
437-449: LGTM! Robust lifecycle handshake implementation.The pidfile and token setup correctly handles edge cases (missing directories, varied path formats) and uses defensive exception handling appropriate for startup code. The broad exception catch flagged by Ruff (line 447) is intentional here—consistent with the pattern at lines 86-88—to ensure lifecycle setup never breaks server startup while still logging failures for debugging.
MCPForUnity/Editor/Services/IServerManagementService.cs (1)
26-30: LGTM: Well-documented interface addition.The new
IsLocalHttpServerRunning()method is appropriately scoped for UI state management with clear "best-effort" semantics documented. The implementation (from relevant snippets) shows proper fallback logic using handshake validation, PID checks, and port listener detection.MCPForUnity/Editor/Services/BridgeControlService.cs (1)
85-101: LGTM: Proper transport mutual exclusion with appropriate error handling.The mutual exclusion logic prevents duplicate sessions when switching transports. Error handling correctly logs warnings but allows startup to proceed, which is appropriate for best-effort cleanup. The legacy stdio stop provides defensive shutdown of any orphaned stdio transport state.
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (1)
54-54: LGTM: Consistent EditorPref type mapping.The addition of
HttpTransportScopeto the known string preferences follows the established pattern and aligns with the new EditorPref key definition for transport scope management.MCPForUnity/Editor/Services/McpEditorShutdownCleanup.cs.meta (1)
1-11: LGTM: Standard Unity metadata file.This is a standard Unity .meta file containing MonoImporter metadata for the corresponding shutdown cleanup class. No functional changes to review.
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
178-179: LGTM: Appropriate reactive binding for transport changes.The event binding ensures client configuration is immediately refreshed when the transport changes, maintaining UI consistency. The
forceImmediate: trueparameter ensures synchronous updates, which is appropriate for user-driven transport switching.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.uxml (2)
22-22: LGTM: Consolidated server control button.The replacement of multiple buttons with a single
start-server-buttonaligns with the PR's goal of consolidating local HTTP server UX into a unified Start/Stop button. The CSS classes properly reference the styling defined inCommon.uss.
36-36: LGTM: Named health row for programmatic access.Adding
name="health-row"to the health indicators section enables proper programmatic access for health status updates, consistent with the improved health verification workflow.MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
10-16: LGTM: Well-organized EditorPrefs keys for server lifecycle.The seven new constants follow the established naming convention and properly support the local HTTP server lifecycle management introduced in this PR. The inline comment for
HttpTransportScopeusefully documents the expected values ("local" | "remote").MCPForUnity/Editor/Windows/Components/Common.uss (3)
196-199: LGTM: Proper button alignment for server controls.The
start-server-buttonrule correctly aligns the button flush left (margin-left: 0px), consistent with other full-width controls in the manual config section.
201-212: LGTM: Appropriate danger styling for active server state.The
server-runningstate styling properly uses red-toned colors to indicate a running server/session, with appropriate hover and active states. This provides clear visual feedback that stopping the server is a significant action.
380-380: LGTM: Correct Unity UI Toolkit property.The change from
font-style: italicto-unity-font-style: italicis correct for Unity UI Toolkit, which requires the-unity-prefix for font styling properties.MCPForUnity/Editor/Services/McpEditorShutdownCleanup.cs (3)
1-23: LGTM! Clean shutdown hook initialization.The static constructor pattern with guard against duplicate subscriptions is appropriate for handling domain reloads. The empty catch block on line 21 is acceptable here since unsubscription failure is benign.
25-41: Transport shutdown with bounded wait looks correct.The 750ms timeout for
Task.WaitAllis reasonable for best-effort cleanup during editor quit. The inner empty catch block (line 35) appropriately swallowsAggregateExceptionfrom timeouts or task failures without blocking the quit process.
43-71: Local server cleanup logic with backward-compatible scope detection.The fallback from explicit
HttpTransportScopepref toIsLocalUrl()heuristics (lines 56-57) correctly maintains backward compatibility for users who haven't yet set the new scope preference.MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (4)
337-347: Good thread-safety pattern for main-thread value capture.Capturing
Application.dataPathandEditorPrefsvalues on the main thread before delegating to the thread-safe method is the correct approach for Unity editor code that needs to run background checks.
349-371: Thread-safe method with enforced contract.The
ArgumentNullExceptionforprojectDir(lines 368-371) correctly enforces the thread-safety contract at compile/runtime, preventing accidental fallback toApplication.dataPathfrom background threads.
395-426: Transport mismatch detection improves UX.Detecting mismatches between the registered transport mode and current setting, then providing an actionable error message ("Click Configure to re-register"), is a good user experience improvement. The case-insensitive
Containscheck for "Type: http" / "Type: stdio" is appropriate for CLI output parsing.
500-522: Pre-removal ensures clean registration state.Removing existing
UnityMCPregistration before adding a new one (lines 500-510) ensures the transport mode is always up-to-date. Setting status toConfiguredimmediately after successful registration (line 522) avoids blocking the UI thread with a verification call.MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (2)
292-303: Parameter addition maintains backward compatibility.The
forceImmediate = falsedefault preserves existing behavior for callers that don't pass the parameter. The logic at line 298 correctly forces immediate refresh for non-Claude CLI clients while respecting the parameter for Claude CLI.
336-377: Thread-safe async status refresh pattern.The implementation correctly:
- Captures main-thread values (
projectDir,useHttpTransport) beforeTask.Run(lines 341-343)- Uses the thread-safe
CheckStatusWithProjectDirmethod (line 350)- Marshals UI updates back to main thread via
EditorApplication.delayCall(line 363)The cast at line 349 is safe due to the type check at line 307.
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (7)
22-27: Transport enum expansion supports explicit local/remote distinction.Splitting HTTP into
HTTPLocalandHTTPRemotemakes the transport mode explicit rather than relying solely on URL heuristics.
99-117: Backward-compatible transport initialization.The scope inference from
IsLocalUrl()whenHttpTransportScopepref is empty (lines 111-114) maintains backward compatibility for existing users upgrading to this version.
165-181: Best-effort stop of deselected transport prevents duplicate sessions.When switching between HTTP and Stdio, stopping the deselected transport (lines 171-180) is a good UX improvement. The fire-and-forget
ContinueWithpattern is appropriate since failure to stop is non-critical.
184-194: URL field persistence on focus-out/Enter improves UX.Not normalizing/overwriting on every keystroke (as noted in the comment) prevents fighting with user input and avoids duplicated scheme issues.
424-434: Throttled server status polling avoids performance issues.Caching
IsLocalHttpServerRunningresults for 0.75s (line 428) prevents expensive port/PID checks on every UI tick, which could otherwise cause noticeable UI stalls.
597-649: Auto-start session with TCP readiness check.Waiting for the HTTP server to accept TCP connections before attempting session start (lines 617-623) reduces transient "unable to connect" errors. The 10-attempt retry loop with 1s delay provides a reasonable 10s window for slower server startups.
651-691: TCP connection check for server readiness.Using a raw TCP connect (lines 669-675) to detect server readiness is a lightweight and reliable approach. Returning
truewhen URL can't be parsed (line 658) avoids blocking startup in edge cases.MCPForUnity/Editor/Services/ServerManagementService.cs (9)
67-75: Centralized cleanup of EditorPrefs tracking keys.Wrapping each
DeleteKeyin try-catch is defensive but acceptable given EditorPrefs can throw in edge cases.
168-204: Instance token validation reduces PID reuse risk.Checking that the process command line contains the unique instance token (lines 176-198) provides strong validation that the PID hasn't been reused by an unrelated process. The fallback on line 193 (
return !ok ? true : false) allows Stop to proceed if Windows can't read the command line due to permissions.
247-279: 6-hour PID validity window balances reliability and usability.The timestamp-based validity check (lines 263-270) mitigates PID reuse risk while still allowing stop operations after domain reloads within a reasonable session window.
436-458: Pre-start port occupancy check prevents confusing failures.Checking for existing listeners before launch (lines 445-456) and showing a clear dialog explaining why the server can't start is a good UX improvement over letting the launch fail with a cryptic error.
462-500: Per-launch pidfile and instance token enable deterministic stop.Creating a unique instance token and pidfile path (lines 466-473) before launch, then storing them in EditorPrefs (line 499), enables the deterministic stop path that validates both PID and token before termination.
772-843: Deterministic handshake-based stop path is robust.The handshake path validates:
- Pidfile exists and contains a valid PID (lines 778-787)
- PID is still listening on the expected port (line 812)
- Process command line contains the instance token (line 813)
Only when all validations pass does it terminate the process. This prevents accidentally killing unrelated processes that may have reused the PID.
903-930: Fingerprint matching with fallback for older installs.The hash comparison (lines 916-918) provides strong validation, while the fallback to common server indicators (lines 923-929) maintains compatibility with servers started before this feature was added.
1189-1251: Graceful termination with escalation is appropriate.The 8-second window for graceful shutdown (line 1214) before escalating to SIGKILL gives the server time to clean up while ensuring the stop operation eventually succeeds.
1360-1378: Script-based terminal launch avoids AppleScript permission prompts.Using a
.commandscript file opened via/usr/bin/open -a Terminal(macOS) avoids the automation permission prompts that AppleScript would trigger. This is a good cross-platform approach.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
1098-1102: Critical bug: Unity safety check incorrectly blocks termination of MCP server.The check
s.Contains("unity")will match the MCP server's own arguments (e.g., "mcp-for-unity"), causingLooksLikeMcpServerProcessto returnfalsefor legitimate server processes. This prevents the "Stop Server" functionality from working correctly.The check should exclude "mcp-for-unity" variants before testing for Unity Editor/Hub:
🔎 Proposed fix
+ // Check for MCP server indicators first, before the Unity exclusion + bool mentionsMcp = sCompact.Contains("mcp-for-unity") + || sCompact.Contains("mcp_for_unity") + || sCompact.Contains("mcpforunity"); + + // If it explicitly mentions the server package/entrypoint, that is sufficient + if (mentionsMcp) + { + return true; + } + // Explicitly never kill Unity / Unity Hub processes - if (s.Contains("unity") || s.Contains("unityhub") || s.Contains("unity hub")) + // Note: Check after MCP indicators since "mcp-for-unity" contains "unity" + if ((s.Contains("unity") && !mentionsMcp) || s.Contains("unityhub") || s.Contains("unity hub")) { return false; } // Positive indicators bool mentionsUvx = s.Contains("uvx") || s.Contains(" uvx "); bool mentionsUv = s.Contains("uv ") || s.Contains("/uv"); bool mentionsPython = s.Contains("python"); bool mentionsUvicorn = s.Contains("uvicorn"); - bool mentionsMcp = sCompact.Contains("mcp-for-unity") - || sCompact.Contains("mcp_for_unity") - || sCompact.Contains("mcpforunity"); bool mentionsTransport = sCompact.Contains("--transporthttp") || (sCompact.Contains("--transport") && sCompact.Contains("http")); - - // If it explicitly mentions the server package/entrypoint, that is sufficient - // (we already only evaluate this for PIDs that are listening on our configured port). - if (mentionsMcp) - { - return true; - }
♻️ Duplicate comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
651-706: Wildcard host normalization correctly addresses the previous review feedback.The normalization of
0.0.0.0,::, and*tolocalhostfor TCP readiness checks is the correct approach, as servers bound to these addresses accept connections via loopback.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
165-181: Best-effort transport cleanup is reasonable, but async errors are silently swallowed.The fire-and-forget pattern is intentional for best-effort cleanup. However, the empty
ContinueWith(_ => { })will silently swallow any async exceptions fromStopAsync. Consider logging async failures for diagnosability:🔎 Optional: Log async failures for diagnosability
try { var stopTask = MCPServiceLocator.TransportManager.StopAsync(stopMode); - stopTask.ContinueWith(_ => { }, TaskScheduler.Default); + stopTask.ContinueWith(t => + { + if (t.IsFaulted) + { + McpLog.Warn($"Async stop of {stopMode} transport failed: {t.Exception?.GetBaseException()?.Message}"); + } + }, TaskScheduler.Default); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
🧰 Additional context used
🧠 Learnings (3)
📚 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/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.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/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (4)
MCPForUnity/Editor/Services/ServerManagementService.cs (6)
System(1328-1440)IsLocalUrl(1290-1294)IsLocalUrl(1299-1313)IsLocalHttpServerRunning(641-701)StopLocalHttpServer(636-639)StartLocalHttpServer(421-516)MCPForUnity/Editor/Helpers/McpLog.cs (4)
McpLog(7-52)Info(37-41)Warn(43-46)Error(48-51)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (3)
HttpEndpointUtility(12-85)SaveBaseUrl(29-33)GetBaseUrl(20-24)MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
Uri(704-721)
🔇 Additional comments (9)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (2)
292-303: LGTM! Clean delegation with conditional force-immediate logic.The parameter change to
RefreshSelectedClient(bool forceImmediate = false)with the computedshouldForceImmediateprovides a sensible default where non-Claude CLI clients always refresh immediately while Claude CLI clients respect the explicit parameter.
336-353: Good thread-safety pattern: capturing main-thread values before async execution.Capturing
projectDiranduseHttpTransporton the main thread beforeTask.Runis the correct approach sinceApplication.dataPathandEditorPrefsare not thread-safe. The defensive type check inside the task prevents crashes if the method is called with an unexpected configurator type.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (4)
22-27: LGTM! Clear transport protocol enum expansion.Splitting HTTP into HTTPLocal and HTTPRemote provides explicit transport selection, which aligns with the PR objective of making the HTTP workflow more explicit.
99-140: Good back-compat inference for existing installations.The logic correctly infers
HTTPLocalvsHTTPRemotebased onIsLocalUrl()when the scope preference isn't set yet, providing a smooth upgrade path for users.
184-194: Good UX improvement: persist URL on focus-out/Enter instead of every keystroke.This prevents fighting the user during typing and avoids duplicate scheme issues from premature normalization.
424-434: Reasonable polling throttle to avoid expensive port/PID checks.The 0.75-second throttle with an exception for in-progress toggles is a practical optimization. The cached
lastLocalServerRunningstate provides responsive UI while avoiding excessive system calls.MCPForUnity/Editor/Services/ServerManagementService.cs (3)
1164-1209: Graceful process termination with escalation is well-implemented.The pattern of sending SIGTERM, waiting up to 8 seconds, then escalating to SIGKILL is a reasonable approach. The use of
ProcessExistsUnixto verify termination before escalating avoids unnecessary force-kills.However, note that the blocking
Thread.Sleep(100)loop (lines 1191-1197) can block the caller for up to 8 seconds. Since this is called from UI event handlers viaOnHttpServerToggleClicked, this could cause brief UI freezes if the process is slow to terminate.
1336-1370: Good improvement: Script-based terminal launching avoids AppleScript permission prompts.Using
.commandfiles on macOS and.cmdfiles on Windows is more reliable than complex AppleScript or nested quote escaping. This approach is less likely to trigger automation permission dialogs on macOS.
458-468: Handshake-based server launch with pidfile and instance token.The per-launch instance token and pidfile provide deterministic shutdown semantics, which is a significant improvement over pure port/PID heuristics. This aligns well with the PR objective of safer lifecycle management.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
1141-1162: Consider async alternative to Thread.Sleep in termination loop.The graceful termination loop uses
Thread.Sleep(100)which blocks the Unity main thread for up to 8 seconds. While this is bounded and only occurs during server cleanup, consider refactoring toawait Task.Delayto keep the editor responsive.🔎 Example async refactor
- // Wait briefly for graceful shutdown. - var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(8); - while (DateTime.UtcNow < deadline) - { - if (!ProcessExistsUnix(pid)) - { - return true; - } - System.Threading.Thread.Sleep(100); - } + // Wait briefly for graceful shutdown (async to avoid blocking Unity thread). + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(8); + while (DateTime.UtcNow < deadline) + { + if (!ProcessExistsUnix(pid)) + { + return true; + } + await Task.Delay(100); + }Note: This would require changing the method signature to
async Task<bool>and updating all call sites.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Services/ServerManagementService.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/ServerManagementService.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/ServerManagementService.cs
📚 Learning: 2025-12-19T21:09:44.740Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 468
File: Server/DOCKER_OVERVIEW.md:22-22
Timestamp: 2025-12-19T21:09:44.740Z
Learning: In the Unity MCP project, the Docker workflow (.github/workflows/publish-docker.yml) intentionally uses the DOCKER_USERNAME secret instead of hardcoding "msanatan" to allow users who fork the repository to publish to their own Docker Hub repositories, while the documentation (Server/DOCKER_OVERVIEW.md) correctly references the official image location "msanatan/mcp-for-unity-server" for end users.
Applied to files:
MCPForUnity/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/Services/ServerManagementService.cs
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/Editor/Services/ServerManagementService.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/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/Services/ServerManagementService.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (5)
MCPForUnity/Editor/Services/ServerManagementService.cs (4)
IsLocalUrl(1250-1254)IsLocalUrl(1259-1273)StopLocalHttpServer(589-592)StartLocalHttpServer(421-516)MCPForUnity/Editor/Services/IServerManagementService.cs (3)
IsLocalUrl(44-44)StopLocalHttpServer(24-24)StartLocalHttpServer(19-19)MCPForUnity/Editor/Helpers/McpLog.cs (4)
McpLog(7-52)Info(37-41)Warn(43-46)Error(48-51)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (3)
HttpEndpointUtility(12-85)SaveBaseUrl(29-33)GetBaseUrl(20-24)MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
Uri(704-721)
MCPForUnity/Editor/Services/ServerManagementService.cs (4)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)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/Helpers/McpLog.cs (5)
McpLog(7-52)Info(37-41)Warn(43-46)Error(48-51)Debug(31-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (26)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (12)
22-27: LGTM: Transport enum split is clean.The HTTPLocal/HTTPRemote split provides clear semantics for the transport selection and aligns with the PR's goal of making HTTP workflows explicit.
64-65: LGTM: Event-driven architecture.The new events enable clean coordination with the editor window and client config sections.
99-141: LGTM: Back-compat inference is well-designed.The initialization gracefully infers HTTPLocal vs HTTPRemote from the current URL when the explicit scope preference isn't set, ensuring smooth upgrades. The health tooltip addition improves discoverability.
145-193: LGTM: Transport switching with best-effort cleanup.The logic correctly stops the deselected transport only when switching between HTTP (local/remote) and Stdio, avoiding unnecessary stops when toggling HTTPLocal ↔ HTTPRemote. The async continuation on
TaskScheduler.Defaultkeeps the UI responsive.
197-205: LGTM: Debounced URL normalization improves UX.Persisting and normalizing only on focus-out or Enter prevents the editor from fighting with the user during typing. Using
SetValueWithoutNotifyavoids recursive callbacks.Also applies to: 250-262
207-222: LGTM: Consolidated Start/Stop button.Routing both start and stop buttons to
OnHttpServerToggleClickedconsolidates the UX while maintaining backward compatibility with older UXML layouts.
264-332: LGTM: Context-aware UI visibility.The visibility logic reduces clutter by adapting controls based on transport selection, debug mode, and health state. Hiding the manual session toggle when local server controls are active avoids confusion.
364-374: LGTM: Clear validation feedback.Displaying a clear warning when HTTP Local is selected with a non-localhost URL prevents user confusion and misconfiguration.
435-445: LGTM: Smart polling optimization.Throttling
IsLocalHttpServerRunning()checks to once per 0.75 seconds avoids expensive port/PID queries on every UI tick while keeping the UI responsive.
471-525: LGTM: Consolidated server toggle with proper guards.The re-entry guard and three-path logic (stop session+server, stop server only, or start server+session) correctly handle all states. The async flow keeps the UI responsive.
608-660: LGTM: Auto-start with reasonable retry policy.The auto-start flow waits for server readiness (up to 10 seconds) and then retries session start up to 10 times with 1-second delays. This balances robustness with timely feedback.
662-717: LGTM: Readiness polling with wildcard host normalization.The implementation correctly normalizes wildcard/bind-all hosts (0.0.0.0, ::, *, empty) to localhost before polling, addressing the prior review concern. The TCP connection with 250ms timeout is lightweight and non-blocking.
✅ Addresses past review comment about auto-connect polling misbehaving for wildcard/bind-all HTTP hosts.
MCPForUnity/Editor/Services/ServerManagementService.cs (14)
20-47: LGTM: Project-scoped path management.Using
Library/MCPForUnityfor logs, pidfiles, and scripts keeps state project-scoped and out of version control, which is the right approach for Unity projects.
49-65: LGTM: String normalization utilities.
QuoteIfNeededandNormalizeForMatchare simple, focused helpers that improve cross-platform command construction and matching robustness.
67-120: LGTM: Clean handshake state management.The helpers for storing/retrieving pidfile path and instance token provide a clear API for the deterministic stop flow. Swallowing exceptions on EditorPrefs operations is acceptable for resilience.
122-166: LGTM: Robust PID file handling.The helpers construct project-scoped pidfile paths and tolerate extra whitespace/newlines when parsing, which improves robustness against manual edits or platform quirks.
168-200: LGTM: Token validation reduces PID reuse risk.Validating the instance token in the process command line provides strong evidence that the PID still belongs to the Unity-launched server, reducing false positives from PID reuse.
202-241: LGTM: Fingerprinting with short hash.Using a truncated SHA-256 hash as a process fingerprint provides a stable, cross-platform identifier without storing the full command line in EditorPrefs.
243-275: LGTM: 6-hour TTL limits PID reuse risk.The 6-hour time-to-live on stored PIDs is a pragmatic balance between convenience (survives domain reload, editor restart) and safety (limits PID reuse risk).
421-516: LGTM: Deterministic server start with handshake.The start flow creates a per-launch token and pidfile, stores the handshake state, and launches the server with augmented command-line arguments. This enables deterministic stop without relying on heuristics.
518-584: LGTM: Log viewer with session-scoped deduplication.The session-scoped flag prevents spawning multiple terminal windows for the same log file during a single Unity session, which improves UX without requiring complex AppleScript or window tracking.
594-654: LGTM: Multi-tiered server detection.The three-tier approach (handshake → stored PID → heuristic) balances deterministic detection with fallback robustness for edge cases like domain reload or manual server launch.
691-771: LGTM: Deterministic stop via handshake.The handshake-based stop path (pidfile + token validation) provides deterministic termination without relying on port/process heuristics. The fallback to
LooksLikeMcpServerProcesswhen token validation is unavailable is pragmatic.
831-882: LGTM: Fixed Unity process detection logic.The code now correctly checks for MCP indicators (mcp-for-unity, mcp_for_unity, mcpforunity) before checking for "unity" substring, preventing false positives where "mcp-for-unity" would be mistaken for a Unity process.
✅ Addresses past review comment about "unity" substring check matching "mcp-for-unity" and refusing to stop the server.
1191-1245: LGTM: Command construction refactored for flexibility.Splitting the command construction into
TryGetLocalHttpServerCommandPartsallows the terminal launcher to use separatefileNameandarguments, improving cross-platform quoting and escaping.
1057-1072: Critical: "unity" substring check still matches "mcp-for-unity".Lines 1057-1066 correctly check for MCP indicators first, but line 1069 still checks
s.Contains("unity")without excluding MCP processes. Since "mcp-for-unity" contains "unity", this will cause false positives and prevent stopping the server.This is the same bug that was fixed in lines 835-840 but remains here.
🔎 Proposed fix
Apply the same pattern used in lines 835-840:
if (!string.IsNullOrEmpty(s)) { bool mentionsMcp = sCompact.Contains("mcp-for-unity") || sCompact.Contains("mcp_for_unity") || sCompact.Contains("mcpforunity"); // If it explicitly mentions the server package/entrypoint, that is sufficient. // Note: Check before Unity exclusion since "mcp-for-unity" contains "unity". if (mentionsMcp) { return true; } // Explicitly never kill Unity / Unity Hub processes - if (s.Contains("unityhub") || s.Contains("unity hub") || s.Contains("unity")) + if (s.Contains("unityhub") || s.Contains("unity hub") || (s.Contains("unity") && !mentionsMcp)) { return false; }This ensures MCP processes are not excluded by the Unity check.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
MCPForUnity/Editor/Services/ServerManagementService.cs (5)
22-23: Consider resettingOpenedHttpServerLogViewerThisSessionon domain reload.
OpenedHttpServerLogViewerThisSessionprevents duplicate log viewer windows within a session, but Unity domain reloads (entering play mode, script recompilation) may reset this field while the Terminal window remains open. This could lead to multiple Terminal windows after domain reloads.If this is acceptable behavior (one Terminal per domain reload session), document it with a comment. Otherwise, consider persisting this flag via
EditorPrefswith a session ID to survive domain reloads.
49-53:QuoteIfNeededdoes not escape existing quotes in path.If
scontains double quotes, the result will be malformed (e.g.,"/some"path"becomes""/some"path""). While Unity project paths rarely contain quotes, consider escaping or warning when this edge case occurs.🔎 Proposed enhancement
private static string QuoteIfNeeded(string s) { if (string.IsNullOrEmpty(s)) return s; + if (s.Contains("\"")) + { + // Edge case: path contains quotes. Escape for shell safety. + s = s.Replace("\"", "\\\""); + } return s.IndexOf(' ') >= 0 ? $"\"{s}\"" : s; }
498-502: Consider storing the launched process inLocalHttpServerProcessfor tracking.Line 500 starts the server process but doesn't capture the returned
Processobject. TheStopLocalHttpServerInternalmethod (lines 775-797) checksLocalHttpServerProcessand attempts to terminate it first if available.Storing the launched process would enable more reliable cleanup:
- var startInfo = CreateTerminalProcessStartInfo(launchCommand); - System.Diagnostics.Process.Start(startInfo); + var startInfo = CreateTerminalProcessStartInfo(launchCommand); + lock (LocalHttpServerProcessLock) + { + LocalHttpServerProcess = System.Diagnostics.Process.Start(startInfo); + } McpLog.Info($"Started local HTTP server in terminal: {launchCommand}");However, note that on macOS and Windows, the terminal launcher scripts create intermediate processes, so the returned
Processobject may not be the actual server process. Consider documenting this limitation.
775-797: EnsureLocalHttpServerProcessis always disposed even if termination fails.The
finallyblock (lines 792-796) disposesLocalHttpServerProcess, which is correct. However, ifTerminateProcessthrows an exception (line 784), the process handle might leak.Consider wrapping
TerminateProcessin try-catch or ensuring all paths dispose the handle:try { if (LocalHttpServerProcess != null && !LocalHttpServerProcess.HasExited) { int pidToKill = LocalHttpServerProcess.Id; if (unityPid <= 0 || pidToKill != unityPid) { - if (TerminateProcess(pidToKill)) + try + { + if (TerminateProcess(pidToKill)) + { + stoppedAny = true; + } + } + catch (Exception ex) { - stoppedAny = true; + McpLog.Warn($"Exception terminating tracked process: {ex.Message}"); } } } } - catch { } + catch (Exception ex) + { + McpLog.Debug($"Error accessing LocalHttpServerProcess: {ex.Message}"); + } finally { try { LocalHttpServerProcess?.Dispose(); } catch { } LocalHttpServerProcess = null; }
1125-1170: BlockingThread.Sleepin Unix termination may freeze Unity Editor briefly.Lines 1150-1158 use
Thread.Sleep(100)in a loop for up to 8 seconds, blocking the Unity main thread. This may cause editor lag during server shutdown.For typical use cases (shutdown during editor quit or transport switch), brief blocking is acceptable. However, consider warning users if shutdown takes > 2 seconds:
var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(8); + var warnedAt = DateTime.UtcNow + TimeSpan.FromSeconds(2); + bool warned = false; while (DateTime.UtcNow < deadline) { if (!ProcessExistsUnix(pid)) { return true; } + if (!warned && DateTime.UtcNow >= warnedAt) + { + McpLog.Info($"Waiting for server process {pid} to exit gracefully..."); + warned = true; + } System.Threading.Thread.Sleep(100); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Services/ServerManagementService.cs
🧰 Additional context used
🧠 Learnings (7)
📚 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/ServerManagementService.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/ServerManagementService.cs
📚 Learning: 2025-12-19T21:09:44.740Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 468
File: Server/DOCKER_OVERVIEW.md:22-22
Timestamp: 2025-12-19T21:09:44.740Z
Learning: In the Unity MCP project, the Docker workflow (.github/workflows/publish-docker.yml) intentionally uses the DOCKER_USERNAME secret instead of hardcoding "msanatan" to allow users who fork the repository to publish to their own Docker Hub repositories, while the documentation (Server/DOCKER_OVERVIEW.md) correctly references the official image location "msanatan/mcp-for-unity-server" for end users.
Applied to files:
MCPForUnity/Editor/Services/ServerManagementService.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/ServerManagementService.cs
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/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/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/Services/ServerManagementService.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (4)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
Uri(704-721)MCPForUnity/Editor/Helpers/McpLog.cs (5)
McpLog(7-52)Info(37-41)Warn(43-46)Error(48-51)Debug(31-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (9)
MCPForUnity/Editor/Services/ServerManagementService.cs (9)
168-200: LGTM! Token validation reduces PID reuse risk.The per-launch token validation is a solid approach to avoid terminating wrong processes after PID reuse. The fallback to
LooksLikeMcpServerProcesswhen token validation is unavailable (line 740) is appropriate.
202-275: LGTM! Stored PID validation with age limit is well-designed.The 6-hour age limit (line 262) is a pragmatic balance between avoiding PID reuse false positives and surviving editor restarts/domain reloads. The fingerprint hash (lines 221-241) provides additional validation.
518-584: LGTM! Log viewer approach avoids AppleScript permission issues.The
.commandscript approach on macOS (lines 542-563) is a good workaround for automation permission prompts. The note about expected transient errors (lines 528-530) will help users interpret logs.
594-654: LGTM! Best-effort server status check is appropriate for UI state.The tiered approach (handshake → stored PID → heuristics) matches the stop logic and provides a reasonable confidence level for UI state display.
938-962: LGTM! Unix process args fetching is robust.The
-wwflag (line 951) prevents truncation of long command lines, which is essential for reliably detecting "mcp-for-unity" in process arguments. The normalization (line 955) enables consistent matching.
1027-1101: LGTM! Process fingerprinting correctly handles "mcp-for-unity" vs "unity" ambiguity.Lines 1057-1066 check for MCP indicators first, then lines 1069-1073 exclude Unity processes only if MCP indicators are absent. This correctly handles the case where "mcp-for-unity" contains "unity". Per past review comments, this bug was addressed.
1206-1246: LGTM! Command building is clear and handles dev mode correctly.The split between
fileName/arguments(for ProcessStartInfo) anddisplayCommand(for user copy-paste) is clean. Dev mode flags at line 1237 align with the comment at line 456 about cache-busting.
1289-1401: LGTM! Script-based terminal launching avoids brittle quoting issues.Writing scripts to
Library/MCPForUnity/TerminalScripts(lines 1298, 1317) is a robust approach that sidesteps shell quoting complexity. The macOS implementation (lines 1296-1314) avoids AppleScript automation permission prompts, as noted in the comment.
417-516: Verify handshake-based lifecycle in manual testing scenarios.The handshake-based lifecycle (pidfile + token) is a significant improvement for deterministic stop behavior. Ensure manual testing covers:
- Happy path: Start → Stop within same editor session
- Domain reload: Start → Domain reload (enter play mode) → Stop (should still work via pidfile)
- Editor restart: Start → Quit editor → Reopen → Stop (should detect stale pidfile and refuse stop if port reused)
- Manual server launch: Launch via terminal manually (no handshake) → Stop via UI (should fall back to heuristics)
- Port conflict: Start while another process owns the port (should refuse and show clear error)
Per the PR summary, "pytest OK (83 passed, 2 skipped, 7 xpassed); manual Unity verification of pidfile creation and deterministic stop" suggests this is covered. Consider documenting these scenarios in a test plan.
Also applies to: 586-936
| private static string GetLocalHttpServerPidFilePath(int port) | ||
| { | ||
| string dir = GetLocalHttpServerPidDirectory(); | ||
| Directory.CreateDirectory(dir); | ||
| return Path.Combine(dir, $"mcp_http_{port}.pid"); | ||
| } |
There was a problem hiding this comment.
Directory.CreateDirectory may throw if permissions are denied.
If the Library/MCPForUnity/RunState directory cannot be created due to permission issues, this will throw an exception. The caller (StartLocalHttpServer) should handle this gracefully.
Consider wrapping in try-catch and returning a default path or logging an error.
🔎 Proposed fix
private static string GetLocalHttpServerPidFilePath(int port)
{
- string dir = GetLocalHttpServerPidDirectory();
- Directory.CreateDirectory(dir);
- return Path.Combine(dir, $"mcp_http_{port}.pid");
+ try
+ {
+ string dir = GetLocalHttpServerPidDirectory();
+ Directory.CreateDirectory(dir);
+ return Path.Combine(dir, $"mcp_http_{port}.pid");
+ }
+ catch (Exception ex)
+ {
+ McpLog.Warn($"Failed to create pidfile directory: {ex.Message}");
+ return null;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
MCPForUnity/Editor/Services/ServerManagementService.cs around lines 128-133:
Directory.CreateDirectory can throw (e.g.
UnauthorizedAccessException/IOException) when permissions are denied; wrap the
directory creation in a try-catch, catch relevant exceptions, log the error
(using Debug.LogError or the service logger) and return a safe fallback PID path
(for example in Path.GetTempPath() or a known writable location) so
StartLocalHttpServer can continue or handle the failure gracefully instead of
letting an exception propagate.
…-tasks Fix local HTTP server lifecycle and TCP connect observation
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
114-119:Directory.CreateDirectorymay throw if permissions are denied.This was flagged in a previous review. The caller (
StartLocalHttpServer) should handle this gracefully, or wrap the directory creation in try-catch here.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Services/ServerManagementService.cs (2)
20-20: StaticHashSetgrows unbounded during long editor sessions.
LoggedStopDiagnosticsPidsis never cleared, so PIDs accumulate across stop attempts. While unlikely to cause memory issues in practice, consider clearing it periodically (e.g., inClearLocalServerPidTracking) or using a bounded cache.🔎 Proposed fix
private static void ClearLocalServerPidTracking() { + try { LoggedStopDiagnosticsPids.Clear(); } catch { } try { EditorPrefs.DeleteKey(EditorPrefKeys.LastLocalHttpServerPid); } catch { } // ... rest of method }
177-180: Minor: Redundant lowercasing oftokenNeedle.
tokenNeedleis already lowercased (line 164), andNormalizeForMatchalso lowercases. This is harmless but slightly redundant.🔎 Proposed fix
if (TryGetUnixProcessArgs(pid, out var argsLowerNow)) { - containsToken = argsLowerNow.Contains(NormalizeForMatch(tokenNeedle)); + containsToken = argsLowerNow.Contains(tokenNeedle); // argsLowerNow is already normalized return true; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/Services/IServerManagementService.csMCPForUnity/Editor/Services/McpEditorShutdownCleanup.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/services/tools/read_console.py
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/IServerManagementService.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Services/McpEditorShutdownCleanup.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/ServerManagementService.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/ServerManagementService.cs
📚 Learning: 2025-12-19T21:09:44.740Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 468
File: Server/DOCKER_OVERVIEW.md:22-22
Timestamp: 2025-12-19T21:09:44.740Z
Learning: In the Unity MCP project, the Docker workflow (.github/workflows/publish-docker.yml) intentionally uses the DOCKER_USERNAME secret instead of hardcoding "msanatan" to allow users who fork the repository to publish to their own Docker Hub repositories, while the documentation (Server/DOCKER_OVERVIEW.md) correctly references the official image location "msanatan/mcp-for-unity-server" for end users.
Applied to files:
MCPForUnity/Editor/Services/ServerManagementService.cs
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/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/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/Services/ServerManagementService.cs
🧬 Code graph analysis (3)
MCPForUnity/Editor/Services/IServerManagementService.cs (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (2)
StopManagedLocalHttpServer(511-536)IsLocalHttpServerRunning(538-598)
Server/src/services/tools/read_console.py (1)
Server/src/services/tools/utils.py (1)
coerce_int(63-77)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (5)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/IServerManagementService.cs (1)
IsLocalUrl(50-50)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (3)
HttpEndpointUtility(12-85)SaveBaseUrl(29-33)GetBaseUrl(20-24)MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
Uri(704-721)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (17)
MCPForUnity/Editor/Services/ServerManagementService.cs (5)
480-486: Handshake state stored before server confirms startup.
StoreLocalHttpServerHandshakeis called immediately afterProcess.Start, but the server may not have written its PID to the pidfile yet. This is acceptable given the polling inTryAutoStartSessionAfterHttpServerAsync, but be aware thatIsLocalHttpServerRunningmay return false briefly after start.
769-774: Good fix for the "unity" substring false positive.The logic now correctly detects MCP server indicators (
mcp-for-unity,mcp_for_unity,mcpforunity) before checking for Unity processes, preventing false refusals when the command line contains "mcp-for-unity".
1116-1144: Good graceful termination pattern with escalation.The Unix termination logic follows best practices: sends SIGTERM first, waits up to 8 seconds for graceful exit, then escalates to SIGKILL only if necessary. The blocking
Thread.Sleepis acceptable in this synchronous termination context.
1270-1288: Script-based terminal launch avoids macOS automation permission prompts.Good approach: writing a
.commandscript and opening it with Terminal avoids AppleScript permission issues. The script persists inLibrary/MCPForUnity/TerminalScriptsbut is overwritten on subsequent launches, which is acceptable.
1289-1305: Script-based Windows terminal launch avoids quote escaping issues.Writing a
.cmdscript and launching it withcmd.exe /c startis a robust approach that avoids brittle nested-quote escaping in the command line.MCPForUnity/Editor/Services/IServerManagementService.cs (1)
26-37: Interface additions are well-documented and align with implementation.The new
StopManagedLocalHttpServer()andIsLocalHttpServerRunning()methods provide clear semantics for lifecycle management. Documentation accurately describes the intent: managed stop works regardless of transport selection, and running detection is best-effort.MCPForUnity/Editor/Services/McpEditorShutdownCleanup.cs (3)
18-23: Good defensive pattern for domain reload safety.Unsubscribing before subscribing in the static constructor prevents duplicate event handlers across Unity domain reloads. The try-catch around unsubscribe is appropriate since the event may not have a prior subscription.
43-68: Robust cleanup logic with appropriate fallbacks.The two-tier approach works well:
- If HTTP Local is selected, attempt
StopLocalHttpServer()(guarded by PID heuristics)- Always fall back to
StopManagedLocalHttpServer()to catch orphaned Unity-managed serversThis covers scenarios like transport switching or URL changes.
32-35: Bounded wait prevents blocking Unity quit.Using
Task.WaitAllwith a 750ms timeout ensures transport cleanup doesn't hang the editor quit indefinitely. The inner try-catch correctly suppresses exceptions for best-effort semantics.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (8)
22-27: Transport enum updated for explicit local/remote distinction.The
TransportProtocolenum now distinguishesHTTPLocalfromHTTPRemote, enabling clearer UI semantics and transport-specific behavior (local server controls vs. remote URL entry).
165-192: Best-effort stop of deselected transport prevents duplicated sessions.Good UX improvement: when switching between HTTP and Stdio, the previously active transport is stopped asynchronously. The fire-and-forget pattern with error logging via
ContinueWithis appropriate here since we don't want to block the UI callback.
195-205: URL normalization on commit prevents fighting user input.Deferring URL normalization to focus-out or Enter key prevents the jarring experience of the URL field being rewritten mid-typing. This is a significant UX improvement.
435-445: Throttled server-running checks avoid expensive polling.The 0.75-second throttle for
IsLocalHttpServerRunning()checks is a good balance between UI responsiveness and avoiding expensive port/PID queries on every repaint.
471-525: Consolidated Start/Stop toggle handles all server states correctly.The logic correctly handles three states:
- Session active → end session, then stop server
- Server running without session → stop server directly
- Neither → start server and auto-start session
The re-entry guard (
httpServerToggleInProgress) prevents race conditions from rapid clicks.
608-660: Auto-start session with readiness check and retries.Good implementation:
- Waits up to 10 seconds for server to accept TCP connections
- Then retries session start up to 10 times with 1-second delays
This covers the startup race condition where the session start might fail if attempted before the server is fully ready.
675-688: Wildcard host normalization addresses past review feedback.The code now correctly normalizes bind-all hosts (
0.0.0.0,::,*) tolocalhostbefore TCP connection checks. This fixes the issue where TcpClient.Connect would fail for wildcard hosts even though the server is listening on loopback.
693-727: TCP readiness polling with proper async disposal.The polling loop correctly:
- Uses
Task.WhenAnywith a 250ms timeout per attempt- Observes faulted tasks to prevent
UnobservedTaskException- Disposes
TcpClientviausingstatementNote: Disposing a
TcpClientwhileConnectAsyncis pending may throw on some platforms, but the outer catch block handles this gracefully.
| # Coerce count defensively (string/float -> int). | ||
| # Important: leaving count unset previously meant "return all console entries", which can be extremely slow | ||
| # (and can exceed the plugin command timeout when Unity has a large console). | ||
| # To keep the tool responsive by default, we cap the default to a reasonable number of most-recent entries. | ||
| # If a client truly wants everything, it can pass count="all" (or count="*") explicitly. | ||
| if isinstance(count, str) and count.strip().lower() in ("all", "*"): | ||
| count = None | ||
| else: | ||
| count = coerce_int(count) | ||
|
|
||
| if action == "get" and count is None: | ||
| count = 200 |
There was a problem hiding this comment.
Fix the logic bug: explicit "all"/"*" requests are incorrectly capped at 200.
The current implementation has a critical flaw. When a user explicitly passes count="all" or count="*", the code sets count = None (line 54), but then lines 58-59 immediately override it back to 200 for action == "get". This defeats the purpose of the "all"/"*" feature described in the comments.
Trace:
- User passes
count="all"→ line 54 setscount = None - Line 58 condition
action == "get" and count is Noneevaluates toTrue - Line 59 sets
count = 200 - Result: User gets 200 entries instead of all entries
Fix: Track whether the user explicitly requested all entries, and skip the default in that case.
🔎 Proposed fix
- # Coerce count defensively (string/float -> int).
- # Important: leaving count unset previously meant "return all console entries", which can be extremely slow
- # (and can exceed the plugin command timeout when Unity has a large console).
- # To keep the tool responsive by default, we cap the default to a reasonable number of most-recent entries.
- # If a client truly wants everything, it can pass count="all" (or count="*") explicitly.
- if isinstance(count, str) and count.strip().lower() in ("all", "*"):
- count = None
- else:
- count = coerce_int(count)
-
- if action == "get" and count is None:
- count = 200
+ # Coerce count defensively (string/float -> int).
+ # Important: leaving count unset previously meant "return all console entries", which can be extremely slow
+ # (and can exceed the plugin command timeout when Unity has a large console).
+ # To keep the tool responsive by default, we cap the default to a reasonable number of most-recent entries.
+ # If a client truly wants everything, it can pass count="all" (or count="*") explicitly.
+ request_all_entries = isinstance(count, str) and count.strip().lower() in ("all", "*")
+
+ if request_all_entries:
+ count = None
+ else:
+ count = coerce_int(count)
+ if action == "get" and count is None:
+ count = 200🤖 Prompt for AI Agents
In Server/src/services/tools/read_console.py around lines 48 to 59, the logic
that handles count incorrectly overrides an explicit "all" or "*" request with
the 200 default; change it to record when the user explicitly requested all
(e.g., set a boolean explicit_all = True when count is a string and in
("all","*")), otherwise coerce count to int as before, and then only apply the
default cap (if action == "get") when explicit_all is False and count is None
(leaving count None to mean "return all" when explicit_all is True).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (1)
153-165: Consider defensive handling when context is unavailable.If
_mainThreadContextis null (e.g., during domain reload edge cases) and the caller is on a background thread,Invoke()executes on the background thread instead of the main thread—potentially accessing Unity APIs unsafely.Consider adding a fallback or logging when context is unavailable:
🔎 Suggested defensive check
+ if (_mainThreadContext == null) + { + // Context unavailable (domain reload edge case); caller should retry or handle gracefully. + McpLog.Warn("[TransportCommandDispatcher] Main thread context unavailable; cannot guarantee main-thread execution."); + } + if (_mainThreadContext != null && Thread.CurrentThread.ManagedThreadId != _mainThreadId) { _mainThreadContext.Post(_ => Invoke(), null); return tcs.Task; }Server/src/transport/plugin_hub.py (2)
48-71: Moveimport jsonto the top of the file.The
jsonmodule is imported twice inside the function (lines 63 and 67). While Python caches module imports, this pattern is unconventional and slightly inefficient. Consider moving the import to the module level with other imports.🔎 Proposed fix
Add at the top of the file with other imports:
import jsonThen update the function:
def _read_unity_status_file(target_hash: str | None) -> dict | None: """Best-effort read of the Unity status JSON written under ~/.unity-mcp/.""" try: base_path = Path.home().joinpath(".unity-mcp") status_files = sorted( base_path.glob("unity-mcp-status-*.json"), key=lambda p: p.stat().st_mtime, reverse=True, ) if not status_files: return None if target_hash: for status_path in status_files: if status_path.stem.endswith(target_hash): with status_path.open("r") as f: - import json - return json.load(f) with status_files[0].open("r") as f: - import json - return json.load(f) except Exception: return None
84-89: Consider annotating mutable class attributes withClassVar.Per static analysis (RUF012), mutable class attributes that are shared across instances should be annotated with
typing.ClassVarto clarify they are not instance attributes.🔎 Proposed fix
Add to imports:
from typing import Any, ClassVarThen update the attributes:
- _FAST_FAIL_COMMANDS: set[str] = {"read_console", "get_editor_state", "ping"} + _FAST_FAIL_COMMANDS: ClassVar[set[str]] = {"read_console", "get_editor_state", "ping"} _registry: PluginRegistry | None = None - _connections: dict[str, WebSocket] = {} + _connections: ClassVar[dict[str, WebSocket]] = {} # command_id -> {"future": Future, "session_id": str} - _pending: dict[str, dict[str, Any]] = {} + _pending: ClassVar[dict[str, dict[str, Any]]] = {}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.csMCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.csMCPForUnity/Editor/Tools/ReadConsole.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/services/resources/editor_state.pyServer/src/transport/plugin_hub.pyServer/src/transport/unity_transport.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.csServer/src/transport/unity_transport.pyServer/src/transport/plugin_hub.py
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.csMCPForUnity/Editor/Tools/ReadConsole.csMCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.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:
Server/src/transport/unity_transport.pyServer/src/transport/plugin_hub.py
🧬 Code graph analysis (5)
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (1)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (16)
Task(69-74)Task(76-99)Task(101-138)Task(140-164)Task(189-224)Task(231-269)Task(282-312)Task(314-359)Task(361-394)Task(414-426)Task(428-476)Task(478-543)Task(545-569)Task(571-583)Task(585-592)Task(594-619)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
MCPForUnity/Editor/Helpers/Response.cs (2)
ErrorResponse(35-67)ErrorResponse(61-66)
Server/src/transport/unity_transport.py (2)
Server/src/transport/plugin_hub.py (1)
send_command_for_instance(447-499)Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (4)
MCPForUnity/Editor/Services/ServerManagementService.cs (6)
System(1263-1375)IsLocalUrl(1225-1229)IsLocalUrl(1234-1248)IsLocalHttpServerRunning(538-598)StopLocalHttpServer(506-509)StartLocalHttpServer(407-501)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)MCPForUnity/Editor/Services/IServerManagementService.cs (4)
IsLocalUrl(50-50)IsLocalHttpServerRunning(36-36)StopLocalHttpServer(24-24)StartLocalHttpServer(19-19)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (3)
HttpEndpointUtility(12-85)SaveBaseUrl(29-33)GetBaseUrl(20-24)
Server/src/transport/plugin_hub.py (1)
Server/tests/integration/test_helpers.py (2)
model_dump(10-13)error(46-47)
🪛 Ruff (0.14.10)
Server/src/transport/unity_transport.py
102-102: Do not catch blind exception: Exception
(BLE001)
Server/src/transport/plugin_hub.py
70-70: Do not catch blind exception: Exception
(BLE001)
84-84: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
87-87: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
89-89: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
177-177: Do not catch blind exception: Exception
(BLE001)
207-207: Consider moving this statement to an else block
(TRY300)
474-474: Do not catch blind exception: Exception
(BLE001)
482-482: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (25)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
155-160: LGTM! Robust guard against transient editor states.This early-return check prevents tool calls from failing or hanging when the Unity editor is compiling or performing domain reloads—states where reflection APIs and console internals may be unstable. Returning a structured error with a
"retry"hint allows clients to implement appropriate backoff logic rather than hard-failing.The placement is correct: reflection members are validated first (lines 133–153), then editor state, then action processing. This aligns well with the PR's goal of introducing non-blocking CLI status checks and retry semantics.
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (6)
19-24: LGTM!The
[InitializeOnLoad]attribute ensures the static constructor runs on the main thread during editor initialization. Usingintfor_processingFlagis correct forInterlockedoperations.
67-82: LGTM!The static constructor correctly captures the main thread context at editor load time. The permanent update hook avoids "must focus Unity" edge cases when processing commands from background websocket threads.
110-116: LGTM!Proactively waking the main thread after enqueueing improves responsiveness when the editor is unfocused or during domain reload transitions.
167-191: LGTM!The nested
Pump()function and context-aware dispatch are consistent with the pattern inRunOnMainThreadAsync. Best-effort semantics are appropriate for this wakeup mechanism.
204-217: LGTM!Keeping the update hook permanently installed avoids edge cases where commands arrive while Unity is unfocused. The no-op implementations maintain backward compatibility with existing call sites.
219-260: LGTM!The reentrancy guard using
Interlocked.Exchangeprevents concurrentProcessQueueinvocations. Thetry/finallyensures the flag resets correctly even on early returns. Lock scope is appropriately minimal—held only to snapshot ready commands.MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
69-74: LGTM!Clean simplification by delegating to the centralized
RunOnMainThreadAsyncdispatcher. This removes the previous boilerplate (custom TCS,EditorApplication.delayCall, and manual registration handling) while preserving cancellation support and null-safety.Server/src/transport/unity_transport.py (2)
12-12: LGTM: Import enables structured error responses.The MCPResponse import is necessary for the fail-fast error handling below and aligns with the PR's goal of standardized error signaling across transports.
95-107: The core concern about validation errors receivinghint="retry"is incorrect.The validation checks at lines 10–11 and 16–18 raise
ValueErrorandTypeErrorbefore the try/except block begins at line 19. These exceptions propagate immediately and are not caught by the exception handler at line 26, so validation errors will not receivehint="retry".The try/except block (lines 19–31) only wraps the
PluginHub.send_command_for_instance()call andnormalize_unity_response(), catching only failures from those operations. The broadExceptioncatch andhint="retry"response are appropriate for transient errors (e.g., network issues, Unity reloading) that may occur during command execution. This design is consistent with the existing preflight check inPluginHub.send_command_for_instance()that already returns the same structured retry hint when Unity is reloading.Likely an incorrect or invalid review comment.
Server/src/transport/plugin_hub.py (5)
34-45: LGTM!The new
PluginDisconnectedErrorexception and_extract_project_hashhelper are well-implemented. The use ofrpartitionis a safe way to handle the "Project@hash" format.
146-164: LGTM!The fail-fast enhancement for disconnected sessions is well-implemented. Proactively failing in-flight commands with
PluginDisconnectedErrorprevents callers from waiting untilCOMMAND_TIMEOUTwhen the plugin has already disconnected.
191-217: LGTM!The enhanced error handling with structured retry hints for fast-fail commands is well-designed. Returning
MCPResponsewithhint="retry"instead of raising exceptions allows callers to handle transient failures gracefully.The static analysis suggestion (TRY300) to move
return resultto anelseblock is a minor style preference that doesn't affect correctness here.
333-337: LGTM!The update to handle the new
_pendingstructure is correct. Accessing the entry outside the lock is safe since you're reading from an already-retrieved reference, and theisinstancecheck properly handles the case where the entry may have been removed.
453-499: LGTM!The preflight and readiness probing logic is well-designed:
- Status file check - Avoids sending commands when Unity explicitly reports reloading
- Bounded readiness probe - Uses ping to verify Unity's main thread is responsive before issuing fast-fail commands
- Configurable timeout - Allows tuning via
UNITY_MCP_SESSION_READY_WAIT_SECONDSwith sensible bounds (0-30s)The
while-elseconstruct is a clean way to handle the "deadline reached without successful ping" case, returning an appropriate retry hint.Server/src/services/resources/editor_state.py (1)
45-51: Type safety concern at line 51 is unfounded; design intent is well-documented.The code is correct as-is. The
send_with_unity_instancefunction signature guarantees a return type ofdict[str, Any] | MCPResponse, so theisinstancecheck at line 45 properly narrows the type. Line 51 safely returns non-dict responses (which must beMCPResponse), maintaining type safety.Additionally, the extra guards at lines 46–49 are intentional: the comment at lines 42–44 explicitly explains that validation errors from malformed data are expected to bubble up. The code guards specific known failure cases (
success=False,data=None) while allowing other structural mismatches to fail loudly—this is the documented design, not a gap. This pattern is defensive but lighter-weight than the simpler ternary used in other resources (e.g.,windows.py).Likely an incorrect or invalid review comment.
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (9)
22-27: LGTM: Clean transport enum split.The enum clearly distinguishes HTTP Local from HTTP Remote, aligning with the PR's first-class transport selection goals.
165-192: Fire-and-forget async stop is appropriate here.The best-effort transport cleanup intentionally doesn't await to avoid blocking the UI. The ContinueWith error handler ensures exceptions are logged rather than left unobserved.
197-205: Good debouncing pattern for URL field.Persisting and normalizing only on focus-out or Enter prevents the UI from fighting user input during typing.
264-332: LGTM: Clear connection status management.The method appropriately toggles UI visibility based on transport mode and session state, with good separation between HTTP Local controls and manual session controls.
364-374: Good localhost validation for HTTP Local mode.Clear messaging when the URL doesn't meet HTTP Local requirements, preventing user confusion.
438-445: Excellent polling throttle to avoid expensive checks.The 0.75-second throttle for
IsLocalHttpServerRunning()prevents excessive port/PID lookups from impacting editor responsiveness.
471-525: LGTM: Clean consolidated toggle with proper re-entrancy guard.The unified start/stop flow handles session coordination correctly. The broad exception catch is appropriate for a UI handler that needs to display user-friendly error dialogs.
608-660: Good auto-start pattern with server readiness check.Waiting for the HTTP server to accept connections before starting the session reduces race conditions and transient connection failures.
763-828: LGTM: Clean health verification with smart log deduplication.The state tracking via
lastHealthStatusprevents log spam while ensuring status changes are properly recorded.
| private static async Task<bool> WaitForHttpServerAcceptingConnectionsAsync(TimeSpan timeout) | ||
| { | ||
| try | ||
| { | ||
| string baseUrl = HttpEndpointUtility.GetBaseUrl(); | ||
| if (!Uri.TryCreate(baseUrl, UriKind.Absolute, out var uri) || uri.Port <= 0) | ||
| { | ||
| return true; // Don't block if URL cannot be parsed | ||
| } | ||
|
|
||
| string host = uri.Host; | ||
| int port = uri.Port; | ||
|
|
||
| // Normalize wildcard/bind-all hosts to loopback for readiness checks. | ||
| // When the server binds to 0.0.0.0 or ::, clients typically connect via localhost/127.0.0.1. | ||
| string normalizedHost; | ||
| if (string.IsNullOrWhiteSpace(host) | ||
| || string.Equals(host, "0.0.0.0", StringComparison.OrdinalIgnoreCase) | ||
| || string.Equals(host, "::", StringComparison.OrdinalIgnoreCase) | ||
| || string.Equals(host, "*", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| normalizedHost = "localhost"; | ||
| } | ||
| else | ||
| { | ||
| normalizedHost = host; | ||
| } | ||
|
|
||
| var deadline = DateTime.UtcNow + timeout; | ||
| while (DateTime.UtcNow < deadline) | ||
| { | ||
| try | ||
| { | ||
| using var client = new TcpClient(); | ||
| var connectTask = client.ConnectAsync(normalizedHost, port); | ||
| var completed = await Task.WhenAny(connectTask, Task.Delay(250)); | ||
| if (completed != connectTask) | ||
| { | ||
| // Avoid leaving a long-running ConnectAsync in-flight (default OS connect timeout can be very long), | ||
| // which can accumulate across retries and impact overall editor/network responsiveness. | ||
| // Closing the client will cause ConnectAsync to complete quickly (typically with an exception), | ||
| // which we then attempt to observe (bounded) by awaiting. | ||
| try { client.Close(); } catch { } | ||
| } | ||
|
|
||
| try | ||
| { | ||
| // Even after Close(), some platforms may take a moment to complete the connect task. | ||
| // Keep this bounded so the readiness loop can't hang here. | ||
| var connectCompleted = await Task.WhenAny(connectTask, Task.Delay(250)); | ||
| if (connectCompleted == connectTask) | ||
| { | ||
| await connectTask; | ||
| } | ||
| else | ||
| { | ||
| _ = connectTask.ContinueWith( | ||
| t => _ = t.Exception, | ||
| System.Threading.CancellationToken.None, | ||
| TaskContinuationOptions.OnlyOnFaulted, | ||
| TaskScheduler.Default); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // Ignore connection exceptions and retry until timeout. | ||
| } | ||
|
|
||
| if (client.Connected) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // Ignore and retry until timeout | ||
| } | ||
|
|
||
| await Task.Delay(150); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| catch | ||
| { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Complex async cancellation pattern may leak tasks under repeated retries.
The method attempts to work around long-lived TcpClient.ConnectAsync operations by manually closing the client (Line 704) and using fire-and-forget continuations (Lines 718-722) for tasks that don't complete within 250ms. While this is defensive, the fire-and-forget continuation only observes exceptions without actually cancelling the underlying operation, potentially leaving tasks in-flight if auto-start is triggered repeatedly (e.g., rapid server restarts during development).
Consider refactoring to use CancellationTokenSource explicitly with a passed cancellation token, which would provide cleaner cancellation semantics and avoid the double-timeout pattern.
🔎 Suggested refactor using CancellationToken
- private static async Task<bool> WaitForHttpServerAcceptingConnectionsAsync(TimeSpan timeout)
+ private static async Task<bool> WaitForHttpServerAcceptingConnectionsAsync(TimeSpan timeout, System.Threading.CancellationToken cancellationToken = default)
{
try
{
string baseUrl = HttpEndpointUtility.GetBaseUrl();
if (!Uri.TryCreate(baseUrl, UriKind.Absolute, out var uri) || uri.Port <= 0)
{
return true;
}
string host = uri.Host;
int port = uri.Port;
string normalizedHost;
if (string.IsNullOrWhiteSpace(host)
|| string.Equals(host, "0.0.0.0", StringComparison.OrdinalIgnoreCase)
|| string.Equals(host, "::", StringComparison.OrdinalIgnoreCase)
|| string.Equals(host, "*", StringComparison.OrdinalIgnoreCase))
{
normalizedHost = "localhost";
}
else
{
normalizedHost = host;
}
var deadline = DateTime.UtcNow + timeout;
- while (DateTime.UtcNow < deadline)
+ while (DateTime.UtcNow < deadline && !cancellationToken.IsCancellationRequested)
{
try
{
+ using var connectCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+ connectCts.CancelAfter(250);
using var client = new TcpClient();
- var connectTask = client.ConnectAsync(normalizedHost, port);
- var completed = await Task.WhenAny(connectTask, Task.Delay(250));
- if (completed != connectTask)
- {
- try { client.Close(); } catch { }
- }
-
try
{
- var connectCompleted = await Task.WhenAny(connectTask, Task.Delay(250));
- if (connectCompleted == connectTask)
- {
- await connectTask;
- }
- else
- {
- _ = connectTask.ContinueWith(
- t => _ = t.Exception,
- System.Threading.CancellationToken.None,
- TaskContinuationOptions.OnlyOnFaulted,
- TaskScheduler.Default);
- }
+ await client.ConnectAsync(normalizedHost, port, connectCts.Token);
+ if (client.Connected)
+ {
+ return true;
+ }
}
- catch
+ catch (OperationCanceledException)
{
- // Ignore connection exceptions and retry until timeout.
+ // Timeout or cancellation, retry
}
-
- if (client.Connected)
+ catch
{
- return true;
+ // Connection failed, retry
}
}
catch
{
- // Ignore and retry until timeout
+ // Unexpected error, retry
}
await Task.Delay(150);
}
return false;
}
catch
{
return false;
}
}Note: This requires TcpClient.ConnectAsync overload that accepts CancellationToken, which was added in .NET 5. If targeting an older framework, the current pattern is acceptable with the noted limitations.
Note: The wildcard host normalization (Lines 675-688) correctly addresses the prior review concern about bind-all addresses like 0.0.0.0 and ::.
for the host normalization fix.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Server/src/transport/unity_transport.py (1)
95-109: Consider distinguishing retryable from non-retryable errors.The error handling converts all exceptions to
MCPResponsewithhint="retry", but some exceptions (e.g.,TypeError,ValueErrorfor malformed parameters) represent non-transient errors that won't benefit from retrying. Consider categorizing exceptions to provide more accurate retry guidance.Additionally, please verify that
normalize_unity_responsecorrectly handles both input types: raw Unity responses (success path, line 101) andMCPResponse.model_dump()dicts (error path, line 108).🔎 Suggested categorization approach
try: raw = await PluginHub.send_command_for_instance( unity_instance, command_type, params, ) return normalize_unity_response(raw) except Exception as exc: # NOTE: asyncio.TimeoutError has an empty str() by default, which is confusing for clients. err = str(exc) or f"{type(exc).__name__}" - # Fail fast with a retry hint instead of hanging for COMMAND_TIMEOUT. - # The client can decide whether retrying is appropriate for the command. + # Distinguish transient errors (worth retrying) from permanent errors. + is_retryable = isinstance(exc, (asyncio.TimeoutError, ConnectionError, OSError)) + hint = "retry" if is_retryable else None return normalize_unity_response( - MCPResponse(success=False, error=err, hint="retry").model_dump() + MCPResponse(success=False, error=err, hint=hint).model_dump() )Server/src/transport/plugin_hub.py (3)
47-47: AddClassVarannotations to mutable class attributes.The mutable class attributes
_FAST_FAIL_COMMANDS,_connections, and_pendingshould be annotated withtyping.ClassVarto clarify they are class-level shared state and improve type-checker understanding.🔎 Proposed fix
+from typing import Any, ClassVar -from typing import Any class PluginHub(WebSocketEndpoint): """Manages persistent WebSocket connections to Unity plugins.""" encoding = "json" KEEP_ALIVE_INTERVAL = 15 SERVER_TIMEOUT = 30 COMMAND_TIMEOUT = 30 # Fast-path commands should never block the client for long; return a retry hint instead. # This helps avoid the Cursor-side ~30s tool-call timeout when Unity is compiling/reloading # or is throttled while unfocused. - _FAST_FAIL_COMMANDS: set[str] = {"read_console", "get_editor_state", "ping"} + _FAST_FAIL_COMMANDS: ClassVar[set[str]] = {"read_console", "get_editor_state", "ping"} _registry: PluginRegistry | None = None - _connections: dict[str, WebSocket] = {} + _connections: ClassVar[dict[str, WebSocket]] = {} # command_id -> {"future": Future, "session_id": str} - _pending: dict[str, dict[str, Any]] = {} + _pending: ClassVar[dict[str, dict[str, Any]]] = {}Also applies to: 50-50, 52-52
168-169: Consider logging when ignoring invalid timeout parameters.When parsing the requested timeout fails, the exception is silently ignored. Adding a debug log would aid troubleshooting without changing behavior.
🔎 Proposed addition
# Give the server a small cushion beyond the Unity-side timeout to account for transport overhead. server_wait_s = max(server_wait_s, requested_s + 5.0) except Exception: - pass + logger.debug(f"Ignoring invalid timeout parameter: {requested}")
323-327: Consider acquiring lock when accessing_pendingin_handle_command_result.While CPython's GIL provides some protection, accessing
cls._pendingwithout the lock at line 323 could race withon_disconnectcleanup (lines 110-123) or thesend_commandfinally block (line 210). Acquiring the lock would ensure consistency across Python implementations.🔎 Proposed fix
async def _handle_command_result(self, payload: CommandResultMessage) -> None: cls = type(self) lock = cls._lock if lock is None: return command_id = payload.id result = payload.result if not command_id: logger.warning(f"Command result missing id: {payload}") return - async with lock: - entry = cls._pending.get(command_id) - future = entry.get("future") if isinstance(entry, dict) else None + async with lock: + entry = cls._pending.get(command_id) + future = entry.get("future") if isinstance(entry, dict) else None + if future and not future.done(): future.set_result(result)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Server/src/transport/plugin_hub.pyServer/src/transport/unity_transport.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/transport/unity_transport.pyServer/src/transport/plugin_hub.py
📚 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:
Server/src/transport/unity_transport.py
🧬 Code graph analysis (1)
Server/src/transport/plugin_hub.py (1)
Server/tests/integration/test_helpers.py (2)
model_dump(10-13)error(46-47)
🪛 Ruff (0.14.10)
Server/src/transport/unity_transport.py
102-102: Do not catch blind exception: Exception
(BLE001)
Server/src/transport/plugin_hub.py
47-47: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
50-50: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
52-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
145-145: Do not catch blind exception: Exception
(BLE001)
157-157: Do not catch blind exception: Exception
(BLE001)
168-169: try-except-pass detected, consider logging the exception
(S110)
168-168: Do not catch blind exception: Exception
(BLE001)
197-197: Consider moving this statement to an else block
(TRY300)
453-453: Do not catch blind exception: Exception
(BLE001)
461-461: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
Server/src/transport/unity_transport.py (1)
12-12: LGTM!The import is required for the new error handling pathway and follows standard patterns.
Server/src/transport/plugin_hub.py (1)
444-477: LGTM: Readiness probe improves fast-path command reliability.The bounded readiness probe for fast-path commands is well-designed:
- Correctly excludes "ping" to avoid recursion
- Uses a configurable, capped timeout window
- Returns structured retry responses instead of hanging
- Provides a better experience during Unity domain reload/reconnect scenarios
1 participant
This branch makes MCP’s HTTP workflow much easier to use and harder to break by:
What changed (high level)
Transport selection is now first-class in the UI
UseHttpTransport+HttpTransportScope), with back-compat inference if the scope pref doesn’t exist yet.Local HTTP server UX is consolidated + more reliable
HTTP Local shows a manual “launch server command” (copyable) and a single “Start Server / Stop Server” button.
Clicking Start Server launches the local server and then auto-starts the session, waiting for the port to become reachable to reduce transient “start failed then recovers” behavior.
Clicking Stop Server ends the session and attempts to stop the local server.
Start generates a pidfile + per-launch token and launches the server with
--pidfile+--unity-instance-token.Stop prefers the handshake path: pidfile → validate PID on port → validate token → kill, then cleans up.
Reduces reliance on “port scan + heuristics” and avoids “Refusing to stop…” false negatives.
Safer lifecycle semantics across transports
Claude Code client setup improvements
UnityMCPentry first to ensure clean, current registration.User-facing UI polish
Implementation notes
EditorPrefKeys(including scope + handshake storage).McpConnectionSection,BridgeControlService,ServerManagementService, plus shutdown cleanup.--pidfile,--unity-instance-tokenand writes the pidfile on startup.Testing
pytest✅ (83 passed, 2 skipped, 7 xpassed)Files touched (major)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.*MCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Services/BridgeControlService.csMCPForUnity/Editor/Services/McpEditorShutdownCleanup.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.csServer/src/main.pySummary by Sourcery
Overhaul HTTP transport selection and local server lifecycle to make MCP sessions safer, less error‑prone, and better integrated with Claude Code, while adding deterministic shutdown and editor quit cleanup.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.