Refactor server paths and add path override support #458
Conversation
- Move Server/ directory to MCPForUnity/Server~/ for Unity package integration - Implement FastMCP server with logging, telemetry, and Unity connection management - Add comprehensive tool and resource registry system - Include Python wrapper for Windows line ending fixes - Update CI workflows to reflect new server location - Add tool auto-discovery and Unity context helpers - Implement transport layer with plugin hub and middleware Python 서버를 MCPForUnity 패키지 구조로 마이그레이션 - Unity 패키지 통합을 위해 Server/ 디렉토리를 MCPForUnity/Server~/로 이동 - 로깅, 텔레메트리, Unity 연결 관리를 포함한 FastMCP 서버 구현 - 포괄적인 도구 및 리소스 레지스트리 시스템 추가 - Windows 줄 끝 수정을 위한 Python wrapper 포함 - 새 서버 위치를 반영하도록 CI 워크플로우 업데이트 - 도구 자동 검색 및 Unity 컨텍스트 헬퍼 추가 - 플러그인 허브 및 미들웨어를 사용한 전송 계층 구현
WalkthroughMoves the server into Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as "Unity Editor"
participant Setup as "McpSetupSection"
participant DepMgr as "DependencyManager"
participant Detector as "PlatformDetector"
participant Env as "ServerEnvironmentSetup"
User->>Editor: Open MCP window / click Refresh
Editor->>Setup: RefreshStatus()
Setup->>DepMgr: CheckAllDependenciesAsync(context)
DepMgr->>Detector: DetectPython(overridePath)
alt override present
Detector-->>DepMgr: custom-path status
else
Detector-->>DepMgr: PATH/system status
end
DepMgr->>Detector: DetectNode(overridePath)
Detector-->>DepMgr: node status
DepMgr->>Env: IsEnvironmentReady(packageRoot)
Env-->>DepMgr: readiness (venv, uv, python)
DepMgr-->>Setup: DependencyCheckResult
Setup->>Editor: Update UI
alt user chooses Install Server
User->>Setup: Install Server Environment
Setup->>Env: InstallServerEnvironment()
Env->>Env: create venv / install uv / pip deps
Env-->>Setup: install complete
Setup->>Editor: OnSetupComplete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (2)
297-319: Comment contradicts behavior: dropdown change does not trigger a status check anymore.
The block comment says status check happens when the user changes the client dropdown, but the dropdown handler only callsApplyStatusToUi(...). This can mislead future edits.- // This happens when: - // - User clicks Configure button - // - User changes client dropdown - // - User manually refreshes + // This happens when: + // - User clicks Configure button + // - User manually refreshes (RefreshSelectedClient)
40-45: Remove the unusedstatusRefreshInFlightfield.This
HashSet<IMcpClientConfigurator>is declared on line 43 but never used anywhere in the codebase. It is leftover state from the removal of background/periodic refresh logic and should be deleted.MCPForUnity/Editor/Services/PathResolverService.cs (1)
118-139:IsPythonDetected()can throw if the process doesn’t exit within 2s.If
WaitForExit(2000)returns false,p.ExitCodecan throw. AlsoProcess.Start(...)can return null. Suggested hardening:using var p = Process.Start(psi); - p.WaitForExit(2000); - return p.ExitCode == 0; + if (p == null) return false; + if (!p.WaitForExit(2000)) + { + try { p.Kill(); } catch { /* ignore */ } + return false; + } + return p.ExitCode == 0;MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
170-224: Fix Python version validation (comment says 3.11+, code allows 3.10).
Line 211 says “3.11+” but Line 214 allows3.10(minor >= 10). That will misclassify installs and contradict UI/recommendations.- // Validate minimum version (Python 4+ or python 3.11+) + // Validate minimum version (Python 4+ or Python 3.11+) if (TryParseVersion(version, out var major, out var minor)) { - return major > 3 || (major >= 3 && minor >= 10); + return major > 3 || (major == 3 && minor >= 11); }
170-224: Python detection likely broken on macOS due to reading only stdout for--version.
python --versioncommonly writes to stderr; current code reads onlyStandardOutput, sooutputmay be empty and detection fails.- string output = process.StandardOutput.ReadToEnd().Trim(); - process.WaitForExit(5000); + process.WaitForExit(5000); + string output = process.StandardOutput.ReadToEnd().Trim(); + string error = process.StandardError.ReadToEnd().Trim(); + string combined = string.IsNullOrEmpty(output) ? error : output; - if (process.ExitCode == 0 && output.StartsWith("Python ")) + if (process.ExitCode == 0 && combined.StartsWith("Python ")) { - version = output.Substring(7); // Remove "Python " prefix + version = combined.Substring(7); // Remove "Python " prefix fullPath = pythonPath; ... }Also applies to: 19-72
MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
195-219: Update stale “Setup Window” recommendation (menu path no longer exists).
Line 217 referencesWindow > MCP for Unity > Setup Window, but the menu item now opensWindow/MCP For Unity.- result.RecommendedActions.Add("Use the Setup Window (Window > MCP for Unity > Setup Window) for guided installation."); + result.RecommendedActions.Add("Open the setup UI from: Window > MCP For Unity");MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
215-219: Version threshold mismatch: comment says 3.11+ but code accepts 3.10+.The comment at line 215 states "Python 4+ or python 3.11+" but the validation at line 218 accepts
minor >= 10, allowing Python 3.10. This is inconsistent with line 71 which tells users to install "python 3.11+".- // Validate minimum version (Python 4+ or python 3.11+) + // Validate minimum version (Python 4+ or Python 3.10+) if (TryParseVersion(version, out var major, out var minor)) { return major > 3 || (major >= 3 && minor >= 10); }Or align the code with the 3.11+ requirement:
// Validate minimum version (Python 4+ or python 3.11+) if (TryParseVersion(version, out var major, out var minor)) { - return major > 3 || (major >= 3 && minor >= 10); + return major > 3 || (major >= 3 && minor >= 11); }MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
400-410: Async void lambda can cause unobserved exceptions.
EditorApplication.delayCallexpects anAction, so the async lambda becomesasync void. Exceptions thrown byVerifyBridgeConnectionAsyncwill be unobserved and could crash Unity's editor process or produce confusing errors.private void ScheduleHealthCheck() { EditorApplication.delayCall += async () => { - if (this == null || connectionSection == null) + try { - return; + if (this == null || connectionSection == null) + { + return; + } + await connectionSection.VerifyBridgeConnectionAsync(); + } + catch (Exception ex) + { + McpLog.Error($"Health check failed: {ex.Message}"); } - await connectionSection.VerifyBridgeConnectionAsync(); }; }
🟡 Minor comments (10)
MCPForUnity/Editor/Services/ServerManagementService.cs-83-94 (1)
83-94: Return value semantics are misleading.Returning
trueon failure contradicts the method's documented behavior: "Returns: True if successful, false otherwise" (lines 44-45). Callers checking the return value will incorrectly believe the cache was cleared.If cache clearing is truly non-critical, consider either:
- Returning
voidand documenting it as best-effort- Returning
falsebut having callers ignore it (current caller already does)- Keeping
truebut updating the doc comment to reflect "best-effort, always returns true"/// <summary> /// Clear the local uvx cache for the MCP server package /// </summary> - /// <returns>True if successful, false otherwise</returns> + /// <returns>Always returns true; cache clearing is best-effort and failures are logged but not propagated</returns> public bool ClearUvxCache()Committable suggestion skipped: line range outside the PR's diff.
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs-524-526 (1)
524-526: Handle TryAdd failure for diagnostics.While unlikely (each client is a unique
TcpClientinstance), ifTryAddreturns false, the client handler task continues running but isn't tracked. This means it won't be awaited duringStop(), potentially causing a resource leak if the task hangs or runs long.Consider logging a warning if the add operation fails:
// Create and track the client handler task var clientTask = Task.Run(() => HandleClientAsync(client, token), token); -activeClientTasks.TryAdd(client, clientTask); +if (!activeClientTasks.TryAdd(client, clientTask)) +{ + McpLog.Warn($"[StdioBridge] Failed to track client task for {client.Client?.RemoteEndPoint}"); +}MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs.meta-1-11 (1)
1-11: GUID pattern suggests hand-authoring; regenerate via Unity to ensure proper generation.The GUID
a1b2c3d4e5f607182930415263748596has a sequential pattern (a1b2c3d4e5f607...) that differs significantly from other randomly-distributed GUIDs in the codebase. While it is currently unique, the pattern indicates it may have been hand-authored rather than generated by Unity. Regenerate this GUID through Unity's normal workflow.MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml.meta-1-10 (1)
1-10: Placeholder GUID detected.The GUID
f1e2d3c4b5a697887766554433221100appears to be manually created rather than Unity-generated (sequential hex pattern). While less common than1234..., this could still cause asset reference conflicts. Let Unity regenerate proper GUIDs by removing these meta files and re-importing in the Editor.MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.meta-1-11 (1)
1-11: Placeholder GUID detected - may cause asset conflicts.The GUID
1234567890abcdef1234567890abcdefappears to be a placeholder rather than a Unity-generated unique identifier. This sequential pattern is commonly used as a placeholder and could conflict with other packages or assets using the same pattern. Let Unity generate a proper GUID by deleting this meta file and re-importing the asset in the Unity Editor.MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uss-62-63 (1)
62-63:cursor: linkis not standard USS syntax.Unity USS uses
cursor: arrow | text | resize-vertical | resize-horizontal | link | slide-arrow | resize-up-right | resize-up-left | move-arrow | rotate-arrow | scale-arrow | arrow-plus | arrow-minus | pan | orbit | zoom | fps | split-resize-up-down | split-resize-left-rightbut the property must be applied without the colon format shown. Additionally, the hover pseudo-class may not consistently apply cursor changes in all Unity versions.MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml-19-21 (1)
19-21: Inconsistent clear button text across the UI.Lines 20, 36, 52, and 72 use lowercase
"x"for clear buttons, but line 100 uses"×"(multiplication sign). Standardize to one character for visual consistency.-<ui:Button name="clear-deploy-source-button" text="×" class="mini-button-icon" style="width: 22px; height: 20px; font-size: 15px; flex-shrink: 0;" tooltip="Clear" /> +<ui:Button name="clear-deploy-source-button" text="x" class="mini-button-icon" style="width: 22px; height: 20px; font-size: 15px; flex-shrink: 0;" tooltip="Clear" />Committable suggestion skipped: line range outside the PR's diff.
MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs-85-94 (1)
85-94: Logic bug: venv creation is skipped when it should run.At line 85-88,
CreateVenv(uvPath)is called only whenuvPathis null/empty (pip fallback). However, insideCreateVenvat line 209-213, the method returnstrueearly whenuvPathis NOT empty. This means whenuvPathIS empty (the actual case when this method is called), the code falls through to line 217 which correctly creates the venv.The confusion arises from the misleading comment and structure. The early return at line 209-213 is dead code since
uvPathwill always be empty whenCreateVenvis called fromInstallServerEnvironment.Consider simplifying by removing the dead code branch:
private static bool CreateVenv(string uvPath) { if (Directory.Exists(VenvPath)) { McpLog.Info("[MCP Setup] Cleaning existing .venv..."); try { Directory.Delete(VenvPath, true); } catch { /* ignore */ } } - // If uv is available, let uv sync handle venv creation automatically. - // But if called explicitly, use python venv as fallback. - if (!string.IsNullOrEmpty(uvPath)) - { - // We can skip manual venv creation if using uv sync later, - // but returning true here keeps the flow consistent. - return true; - } - McpLog.Info($"[MCP Setup] Creating virtual environment at: {VenvPath}"); return RunCommand(GetPythonCommand(), $"-m venv \"{VenvPath}\"", out string output, workingDirectory: ServerRoot); }Also applies to: 199-218
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs-386-388 (1)
386-388: Duplicate method call.
settingsSection?.UpdatePathOverrides()is called twice in succession at lines 386-387. This appears to be a copy-paste error.settingsSection?.UpdatePathOverrides(); - settingsSection?.UpdatePathOverrides(); clientConfigSection?.RefreshSelectedClient();MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs-346-360 (1)
346-360: Missing null checks forsettingsPanel.
settingsPanelcould be null if the UXML query at line 105 fails. The code uses null-conditional operators fortoolsPanel(lines 348-349, 358-359) but directly accessessettingsPanelmethods without null checks (lines 346-347, 356-357).if (isReady) { statusText.text = "System Ready"; statusText.style.color = new StyleColor(new Color(0.3f, 0.8f, 0.3f)); // Greenish setupHint.text = "Configure Settings"; - settingsPanel.RemoveFromClassList("disabled"); - settingsPanel.SetEnabled(true); + settingsPanel?.RemoveFromClassList("disabled"); + settingsPanel?.SetEnabled(true); toolsPanel?.RemoveFromClassList("disabled"); toolsPanel?.SetEnabled(true); } else { statusText.text = "Setup Required"; statusText.style.color = new StyleColor(new Color(0.9f, 0.3f, 0.3f)); // Reddish setupHint.text = "Click arrow to configure environment"; - settingsPanel.AddToClassList("disabled"); - settingsPanel.SetEnabled(false); + settingsPanel?.AddToClassList("disabled"); + settingsPanel?.SetEnabled(false); toolsPanel?.AddToClassList("disabled"); toolsPanel?.SetEnabled(false); }
🧹 Nitpick comments (32)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (3)
90-97: Dropdown selection now paints status without checking it — verify staleness isn’t confusing.
This intentionally stops auto-checking, but ifIMcpClientConfigurator.Statusisn’t kept fresh elsewhere, the indicator can be stale immediately after switching clients. Consider a small “Status may be stale; click Refresh” hint or a lightweight on-demand check.
179-199: After “Configure All”, consider forcing a status check for the currently selected client.
Right now this updates the UI fromclient.Statusonly; ifConfigureAllDetectedClients()doesn’t updateStatus, the UI can keep showing the old state right after a user-initiated action.if (selectedClientIndex >= 0 && selectedClientIndex < configurators.Count) { // Just update UI, don't check status UpdateManualConfiguration(); ApplyStatusToUi(configurators[selectedClientIndex]); + // If UI staleness is observed after Configure All, prefer: + // RefreshClientStatus(configurators[selectedClientIndex], forceImmediate: true); }
229-248: Claude CLI “Browse” flow: OK, but verify UX since status won’t update until “Configure”.
The new behavior is consistent with “no background checks”, but users may expect immediate validation after selecting a path. If that expectation exists, considerRefreshSelectedClient()when the selected client is Claude CLI.MCPForUnity/Editor/Services/ServerManagementService.cs (2)
16-40: Consider thread safety for cleanup registration.The
_cleanupRegisteredcheck-and-set pattern (lines 23 and 39) is not atomic. While Unity Editor typically operates on the main thread, ifEnsureCleanupRegistered()is ever called concurrently, multiple handlers could be registered.Also, line 30 creates a new
ServerManagementServiceinstance inside the lambda. If this class is stateless (which it appears to be), this works, but consider whether a static method or cached instance would be cleaner.- private static bool _cleanupRegistered = false; + private static volatile bool _cleanupRegistered = false; + private static readonly object _cleanupLock = new object(); private static void EnsureCleanupRegistered() { if (_cleanupRegistered) return; - - EditorApplication.quitting += () => + lock (_cleanupLock) { - // Try to stop the HTTP server when Unity exits - try - { - var service = new ServerManagementService(); - service.StopLocalHttpServer(); - } - catch (Exception ex) + if (_cleanupRegistered) return; + + EditorApplication.quitting += () => { - McpLog.Debug($"Cleanup on exit: {ex.Message}"); - } - }; - - _cleanupRegistered = true; + try + { + new ServerManagementService().StopLocalHttpServer(); + } + catch (Exception ex) + { + McpLog.Debug($"Cleanup on exit: {ex.Message}"); + } + }; + + _cleanupRegistered = true; + } }
512-545: Potential deadlock ifwhichoutput exceeds buffer size.The code calls
WaitForExit(5000)without first readingStandardOutput. If the output buffer fills (unlikely forwhich, but possible), the process will block and never exit, leading to a timeout.The safer pattern is to read the output before waiting:
using var which = System.Diagnostics.Process.Start(new System.Diagnostics.ProcessStartInfo { FileName = "which", Arguments = term, UseShellExecute = false, RedirectStandardOutput = true, CreateNoWindow = true }); if (which != null) { + // Read output first to prevent buffer deadlock + string output = which.StandardOutput.ReadToEnd(); if (!which.WaitForExit(5000)) { // Timeout - kill the process try { if (!which.HasExited) { which.Kill(); } } catch { } } else if (which.ExitCode == 0) { terminalCmd = term; break; } }MCPForUnity/Server~/src/main.py (1)
64-74:propagate = Falselikely drops these logs entirely—confirm that’s intended.Since
httpx/urllib3/mcp.server.lowlevel.serveraren’t given a handler here, settingpropagate=Falseeffectively silences them (not just “quiets”). If you still want WARN/ERROR in the rotating file, consider leavingpropagate=True(and relying on stderr) or attaching_fhwhen available.MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
506-516: Consider logging the exception for claudeDir extraction.The silent
catch { }suppresses exceptions when extracting the Claude CLI directory. While this is a best-effort PATH enhancement, logging the exception at debug level would aid troubleshooting if PATH-related issues occur.Apply this diff to add debug logging:
try { string claudeDir = Path.GetDirectoryName(claudePath); if (!string.IsNullOrEmpty(claudeDir)) { pathPrepend = string.IsNullOrEmpty(pathPrepend) ? claudeDir : $"{claudeDir}:{pathPrepend}"; } } -catch { } +catch (Exception ex) +{ + McpLog.Debug($"Failed to extract Claude CLI directory for PATH: {ex.Message}"); +}MCPForUnity/Editor/Windows/MCPSetupWindow.uxml (1)
10-10: UI requirement text update is fine (3.11+).Optional nit: consider capitalizing “Python” for consistency in UI copy.
.github/workflows/claude-mcp-preflight.yml (1)
28-35: Preflight correctly targetsMCPForUnity/Server~now.Minor reliability: you may want
set -euxo pipefailin the “Install MCP server deps” step too (it currently lackspipefail).Also applies to: 51-52
.github/workflows/claude-nl-suite.yml (1)
52-64: Consider centralizingMCPForUnity/Server~as a single env var to avoid future drift.You reference the path in multiple steps; an
env: MCP_SERVER_DIR: MCPForUnity/Server~would reduce maintenance risk..github/workflows/python-tests.yml (1)
3-9:uv sync+uv pip install -e ".[dev]"may be redundant; prefer a single dependency path.If you want dev deps, consider
uv sync --dev(or the repo’s standard approach) and drop the extra editable install unless needed for local packaging behavior.Also applies to: 24-32, 33-37, 38-45
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (1)
90-96: Make the no-opUpdatePathOverrides()explicitly transitional (Obsolete and/or log once).Right now it silently does nothing. If external callers still invoke it, consider marking it
[Obsolete("Handled by Setup Section. This method is a no-op and will be removed.")]and/or emitting a single debug log (guarded) so regressions are diagnosable.MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
40-95:GetPackageAbsolutePath()comment doesn’t match implementation; consider naming/intent clarity.It claims “StackTrace/ScriptableObject trick” but uses
[CallerFilePath]. If this is intentional, update the comment to reflect the actual mechanism and its constraints.MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
32-32: Consider consistent namespace prefix for EditorPrefs keys.
EditorWindowActivePaneluses the prefixMCPForUnityEditorWindowwhile all other keys in this file useMCPForUnity. For consistency and easier filtering of all package-related EditorPrefs, consider usingMCPForUnity.EditorWindow.ActivePanelinstead.- internal const string EditorWindowActivePanel = "MCPForUnityEditorWindow.ActivePanel"; + internal const string EditorWindowActivePanel = "MCPForUnity.EditorWindow.ActivePanel";MCPForUnity/Editor/Services/ToolDiscoveryService.cs (2)
141-149: Commented-out code leaves dead method.The
ExtractSummaryDescriptionmethod (lines 355-405) is now unreachable since this is the only call site. If summary extraction is intentionally deferred for performance, consider either:
- Removing the commented code and the dead method entirely
- Adding a TODO comment explaining when lazy extraction should be implemented
metadata.IsBuiltIn = DetermineIsBuiltIn(type, metadata); - if (metadata.IsBuiltIn) - { - // Skip summary extraction during discovery for performance - // string summaryDescription = ExtractSummaryDescription(type, metadata); - // if (!string.IsNullOrWhiteSpace(summaryDescription)) - // { - // metadata.Description = summaryDescription; - // } - } + // TODO: Summary extraction skipped for discovery performance. + // Consider implementing lazy extraction via a separate method if needed.
319-353: Dead code: AssetPath-based detection branch is unreachable.Since
AssetPathis now alwaysnullduring discovery, the entire path-based detection block (lines 326-345) will never execute. The method effectively reduces to just the assembly name check. Consider simplifying this method to reflect the actual behavior.private bool DetermineIsBuiltIn(Type type, ToolMetadata metadata) { if (metadata == null) { return false; } - if (!string.IsNullOrEmpty(metadata.AssetPath)) - { - string normalizedPath = metadata.AssetPath.Replace("\\", "/"); - string packageRoot = AssetPathUtility.GetMcpPackageRootPath(); - - if (!string.IsNullOrEmpty(packageRoot)) - { - string normalizedRoot = packageRoot.Replace("\\", "/"); - if (!normalizedRoot.EndsWith("/", StringComparison.Ordinal)) - { - normalizedRoot += "/"; - } - - string builtInRoot = normalizedRoot + "Editor/Tools/"; - if (normalizedPath.StartsWith(builtInRoot, StringComparison.OrdinalIgnoreCase)) - { - return true; - } - } - } - - if (!string.IsNullOrEmpty(metadata.AssemblyName) && metadata.AssemblyName.Equals("MCPForUnity.Editor", StringComparison.Ordinal)) - { - return true; - } - - return false; + // AssetPath is null during discovery; rely solely on assembly name + return !string.IsNullOrEmpty(metadata.AssemblyName) + && metadata.AssemblyName.Equals("MCPForUnity.Editor", StringComparison.Ordinal); }MCPForUnity/Editor/Windows/Components/Common.uss (1)
513-598: New dependency and dark theme styles look good.The new dependency row styles are well-organized with clear semantic class names. The mini-button variants provide good visual hierarchy with primary/secondary states and hover feedback.
Consider using USS variables for the repeated color values (e.g.,
#4CAF50,#F44336,#E0E0E0) to improve maintainability, though this is optional given the current codebase patterns.MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (1)
30-33: Consider addingGetNodeInstallUrl()for consistency.The interface includes
GetPythonInstallUrl()andGetUvInstallUrl()but lacks a correspondingGetNodeInstallUrl()method. This could create inconsistency when implementing platform-specific Node.js installation guidance./// <summary> /// Detect Node.js on this platform /// </summary> DependencyStatus DetectNode(string overridePath = null); +/// <summary> +/// Get platform-specific Node.js installation URL +/// </summary> +string GetNodeInstallUrl(); /// <summary> /// Get platform-specific installation recommendations /// </summary>MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml (1)
91-111: Consider moving inline styles to the USS stylesheet.The deployment panel has extensive inline styles (background-color, border-radius, padding, etc.). Moving these to
Common.ussas a dedicated class (e.g.,.deploy-panel) would improve maintainability and align with the pattern used elsewhere in the UI.MCPForUnity/Server~/wrapper.js (2)
51-80: Potential unbounded buffer growth.If the Python process outputs data without newlines, the
buffervariable will grow indefinitely until a newline is received or memory is exhausted. Consider adding a maximum buffer size check.let buffer = ""; +const MAX_BUFFER_SIZE = 10 * 1024 * 1024; // 10MB safety limit pythonProcess.stdout.on("data", (data) => { buffer += data.toString("utf8"); + + if (buffer.length > MAX_BUFFER_SIZE) { + log("Buffer overflow - clearing"); + buffer = ""; + } let newlineIdx;
33-40: Consider avoidingshell: truefor security.Using
shell: truecan introduce command injection vulnerabilities if any arguments are derived from external input. While the current arguments are static, removingshell: truewould be a safer default. Note this may require adjusting how the command is invoked on Windows.MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (1)
96-151: TryFindUvViaPython is a clever fallback strategy.This handles the case where uv is installed via pip but not in PATH. The inline Python script correctly handles cross-platform path construction.
Consider logging the exception in the catch block (line 149) at debug level to aid troubleshooting, though swallowing it is acceptable for a fallback path.
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
19-44: Avoid readingUnityEditor.EditorPrefsinside platform detectors (hurts thread-safety).
If dependency checks are intended to be runnable off the main thread, detectors should rely onoverridePathpassed in via context; falling back toEditorPrefshere mixes Unity APIs into detection.Also applies to: 108-133
MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
73-79:CheckAllDependenciesAsyncis synchronous + should guard null context.
Consider renaming to reflect thread-safety rather than async behavior, and add a defensive null check to avoidNullReferenceException.public static DependencyCheckResult CheckAllDependenciesAsync(DependencyContext context) { + if (context == null) throw new ArgumentNullException(nameof(context)); return CheckAllDependenciesInternal(context); }Also applies to: 81-115
MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (4)
54-72: Redundant Python check after earlier guard.
CheckPython()is called at line 54 and returns early at line 57-61 if it fails. Then it's called again at line 67 with a dialog error. The second check is unreachable unlessCheckPython()returns different results on consecutive calls (which would indicate a race condition or flaky detection).try { - // 1. Check Python - EditorUtility.DisplayProgressBar("MCP Setup", "Checking Python...", 0.1f); - if (!CheckPython()) - { - EditorUtility.ClearProgressBar(); - EditorUtility.DisplayDialog("Error", "python 3.11+ not found in PATH.\nPlease install Python first.", "OK"); - return; - } - // 2. Check/Install uv - EditorUtility.DisplayProgressBar("MCP Setup", "Checking 'uv' package manager...", 0.3f); + EditorUtility.DisplayProgressBar("MCP Setup", "Checking 'uv' package manager...", 0.1f); string uvPath = GetOrInstallUv();
275-284: Potential deadlock when reading stdout and stderr synchronously.Synchronously reading
StandardOutput.ReadToEnd()followed byStandardError.ReadToEnd()can deadlock if the process writes enough to stderr to fill the buffer before stdout is fully consumed. This is a known .NET issue with redirected streams.For the typical use cases here (version checks, pip installs), the output is usually small enough to avoid this, but longer-running commands like
uv syncwith verbose output could potentially hang.Consider reading both streams asynchronously:
- output = p.StandardOutput.ReadToEnd(); - string error = p.StandardError.ReadToEnd(); - - // 2 minute timeout to prevent hanging indefinitely - if (!p.WaitForExit(120000)) + var outputTask = p.StandardOutput.ReadToEndAsync(); + var errorTask = p.StandardError.ReadToEndAsync(); + + // 2 minute timeout to prevent hanging indefinitely + if (!p.WaitForExit(120000)) { try { p.Kill(); } catch {} McpLog.Error($"[MCP Setup] Command timed out: {fileName} {arguments}"); return false; } + + output = outputTask.Result; + string error = errorTask.Result;
203-205: Silent exception swallowing when deleting existing venv.Silently catching and ignoring the exception when deleting the existing
.venvdirectory could mask issues like locked files or permission problems. If deletion fails, subsequent venv creation may fail or behave unexpectedly due to stale files.McpLog.Info("[MCP Setup] Cleaning existing .venv..."); - try { Directory.Delete(VenvPath, true); } catch { /* ignore */ } + try { Directory.Delete(VenvPath, true); } + catch (Exception ex) { McpLog.Warn($"[MCP Setup] Could not fully clean existing .venv: {ex.Message}"); }
19-19: UnusedRequirementsPathproperty.
RequirementsPathis defined but never used in this file. The dependency installation at line 229 usesuv sync(which readspyproject.toml) or at line 245 usespip install -e .. Consider removing this unused property or documenting its intended purpose.MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
28-44: Overly defensive try-catch around EditorPrefs.GetString.
EditorPrefs.GetStringdoesn't throw exceptions for missing keys—it returns the default value. The inner try-catch at lines 31 and 122 adds noise without benefit.// 1. Check Override - if (overridePath == null) - { - try { overridePath = UnityEditor.EditorPrefs.GetString(MCPForUnity.Editor.Constants.EditorPrefKeys.PythonPathOverride, ""); } catch {} - } + overridePath ??= UnityEditor.EditorPrefs.GetString(MCPForUnity.Editor.Constants.EditorPrefKeys.PythonPathOverride, "");MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
381-384: Fire-and-forget async call can lose exceptions.The discarded task
_ = connectionSection?.VerifyBridgeConnectionAsync()means any exception thrown will be unobserved. Consider adding.ContinueWith()to log failures or using a helper method.if (MCPServiceLocator.Bridge.IsRunning) { - _ = connectionSection?.VerifyBridgeConnectionAsync(); + _ = connectionSection?.VerifyBridgeConnectionAsync().ContinueWith(t => + { + if (t.IsFaulted && t.Exception != null) + McpLog.Warn($"Bridge verification failed: {t.Exception.InnerException?.Message}"); + }, TaskScheduler.FromCurrentSynchronizationContext()); }MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
97-107: MissingFile.Existscheck for uv override path.The Python override check at line 34 validates
File.Exists(overridePath)before callingTryValidatePython, but the uv override check at line 97 skips this validation. WhileTryValidateUvhandles non-existent paths gracefully via exception, this is inconsistent with the Python check and theLinuxPlatformDetectorimplementation.- if (!string.IsNullOrEmpty(overridePath)) + if (!string.IsNullOrEmpty(overridePath) && File.Exists(overridePath)) { if (TryValidateUv(overridePath, out string version, out string fullPath))
192-197: PotentialIndexOutOfRangeExceptionwhen parsing uv version.If
uv --versionoutputs an unexpected format without a space (e.g., just "uv"),output.Split(' ')[1]at line 194 will throwIndexOutOfRangeException. The outer try-catch will handle it, but it's better to validate explicitly.if (process.ExitCode == 0 && output.StartsWith("uv")) { - version = output.Split(' ')[1]; + var parts = output.Split(' '); + version = parts.Length > 1 ? parts[1] : output; fullPath = uvPath; return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
MCPForUnity/Server~/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.github/workflows/bump-version.yml(2 hunks).github/workflows/claude-mcp-preflight.yml(2 hunks).github/workflows/claude-nl-suite.yml(8 hunks).github/workflows/python-tests.yml(3 hunks)MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs(6 hunks)MCPForUnity/Editor/Constants/EditorPrefKeys.cs(2 hunks)MCPForUnity/Editor/Dependencies/DependencyManager.cs(4 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs(1 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs(6 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs(5 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs(4 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs(5 hunks)MCPForUnity/Editor/Helpers/AssetPathUtility.cs(2 hunks)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs(2 hunks)MCPForUnity/Editor/Helpers/ExecPath.cs(1 hunks)MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs(1 hunks)MCPForUnity/Editor/Services/ClientConfigurationService.cs(1 hunks)MCPForUnity/Editor/Services/IPathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/ServerManagementService.cs(7 hunks)MCPForUnity/Editor/Services/ToolDiscoveryService.cs(2 hunks)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs(6 hunks)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs(1 hunks)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs.meta(1 hunks)MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs(6 hunks)MCPForUnity/Editor/Windows/Components/Common.uss(2 hunks)MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml(0 hunks)MCPForUnity/Editor/Windows/Components/Setup.meta(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.meta(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml.meta(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(7 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uss(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uxml(1 hunks)MCPForUnity/Editor/Windows/MCPSetupWindow.uxml(1 hunks)MCPForUnity/Server~/pyproject.toml(1 hunks)MCPForUnity/Server~/src/core/telemetry.py(2 hunks)MCPForUnity/Server~/src/main.py(2 hunks)MCPForUnity/Server~/src/services/tools/__init__.py(2 hunks)MCPForUnity/Server~/wrapper.js(1 hunks)Server/pyproject.toml(0 hunks)TestProjects/UnityMCPTests/Packages/manifest.json(1 hunks)
💤 Files with no reviewable changes (2)
- MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml
- Server/pyproject.toml
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
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.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/Editor/Windows/Components/Setup.meta.github/workflows/bump-version.ymlMCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.metaTestProjects/UnityMCPTests/Packages/manifest.jsonMCPForUnity/Server~/pyproject.toml.github/workflows/claude-mcp-preflight.ymlMCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs.metaMCPForUnity/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/Windows/Components/Setup.metaMCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.csMCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.metaTestProjects/UnityMCPTests/Packages/manifest.jsonMCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs.github/workflows/claude-mcp-preflight.ymlMCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs.metaMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Services/PathResolverService.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/Windows/Components/Setup/McpSetupSection.csMCPForUnity/Editor/Windows/MCPSetupWindow.uxmlMCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxmlMCPForUnity/Server~/wrapper.jsMCPForUnity/Server~/pyproject.tomlMCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs.github/workflows/claude-mcp-preflight.ymlMCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs.metaMCPForUnity/Editor/Helpers/AssetPathUtility.cs.github/workflows/claude-nl-suite.yml.github/workflows/python-tests.ymlMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csMCPForUnity/Editor/Setup/ServerEnvironmentSetup.csMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Services/PathResolverService.cs
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
MCPForUnity/Server~/src/services/tools/__init__.pyMCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.metaMCPForUnity/Editor/Windows/MCPSetupWindow.uxmlMCPForUnity/Server~/pyproject.tomlMCPForUnity/Editor/Services/ToolDiscoveryService.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/Dependencies/PlatformDetectors/PlatformDetectorBase.csTestProjects/UnityMCPTests/Packages/manifest.jsonMCPForUnity/Editor/Services/PathResolverService.cs
📚 Learning: 2025-11-27T21:09:35.011Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 401
File: MCPForUnity/Editor/Clients/Configurators/VSCodeConfigurator.cs:10-18
Timestamp: 2025-11-27T21:09:35.011Z
Learning: VS Code GitHub Copilot MCP configuration supports mcp.json files placed directly in the Code/User directory: %APPDATA%\Code\User\mcp.json on Windows, ~/Library/Application Support/Code/User/mcp.json on macOS, and ~/.config/Code/User/mcp.json on Linux. This is in addition to workspace-scoped .vscode/mcp.json files.
Applied to files:
TestProjects/UnityMCPTests/Packages/manifest.json
📚 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/MenuItems/MCPForUnityMenu.csMCPForUnity/Editor/Constants/EditorPrefKeys.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:
.github/workflows/claude-mcp-preflight.yml.github/workflows/claude-nl-suite.yml
📚 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:
.github/workflows/claude-mcp-preflight.yml.github/workflows/claude-nl-suite.yml
🧬 Code graph analysis (11)
MCPForUnity/Editor/Helpers/ExecPath.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Warn(43-46)Debug(31-35)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (3)
DependencyStatus(23-23)DependencyStatus(28-28)DependencyStatus(33-33)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (3)
DependencyStatus(17-17)DependencyStatus(22-94)DependencyStatus(153-198)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (4)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (5)
DependencyStatus(19-79)DependencyStatus(110-171)GetPythonInstallUrl(81-84)GetUvInstallUrl(86-89)GetInstallationRecommendations(91-108)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (5)
DependencyStatus(19-79)DependencyStatus(108-168)GetPythonInstallUrl(81-84)GetUvInstallUrl(86-89)GetInstallationRecommendations(91-106)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (6)
DependencyStatus(23-23)DependencyStatus(28-28)DependencyStatus(33-33)GetPythonInstallUrl(43-43)GetUvInstallUrl(48-48)GetInstallationRecommendations(38-38)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs (1)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (2)
MCPForUnityEditorWindow(18-426)ShowWindow(54-58)
MCPForUnity/Editor/Dependencies/DependencyManager.cs (9)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-334)GetMcpPackageRootPath(157-196)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/PathResolverService.cs (3)
GetPythonPath(98-106)GetNodePath(108-116)GetUvxPath(21-38)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (2)
ServerEnvironmentSetup(14-326)IsEnvironmentReady(21-49)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (4)
DependencyStatus(19-80)DependencyStatus(82-168)GetPythonInstallUrl(203-206)GetUvInstallUrl(208-211)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (4)
DependencyStatus(19-79)DependencyStatus(110-171)GetPythonInstallUrl(81-84)GetUvInstallUrl(86-89)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (4)
DependencyStatus(19-79)DependencyStatus(108-168)GetPythonInstallUrl(81-84)GetUvInstallUrl(86-89)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (5)
DependencyStatus(23-23)DependencyStatus(28-28)DependencyStatus(33-33)GetPythonInstallUrl(43-43)GetUvInstallUrl(48-48)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (5)
DependencyStatus(17-17)DependencyStatus(22-94)DependencyStatus(153-198)GetPythonInstallUrl(18-18)GetUvInstallUrl(19-19)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
MCPForUnity/Editor/Services/PathResolverService.cs (6)
GetPythonPath(98-106)GetNodePath(108-116)SetPythonPathOverride(188-196)ClearPythonPathOverride(198-201)SetNodePathOverride(203-211)ClearNodePathOverride(213-216)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (4)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (4)
AssetPathUtility(15-334)GetWrapperJsPath(100-150)GetMcpServerGitUrl(261-296)uvxPath(302-309)MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (3)
McpConfigurationHelper(22-282)PathsEqual(212-229)ExtractUvxUrl(199-210)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-302)TryRun(161-257)
MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (3)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-334)GetPackageAbsolutePath(40-89)MCPForUnity/Editor/Setup/SetupWindowService.cs (2)
SetupWindowService(27-35)ShowSetupWindow(95-106)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (3)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/ClientConfigurationService.cs (1)
CheckClientStatus(53-58)MCPForUnity/Editor/Services/IClientConfigurationService.cs (1)
CheckClientStatus(30-30)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Services/IPathResolverService.cs (7)
GetPythonPath(24-24)GetNodePath(30-30)IsPythonDetected(36-36)SetPythonPathOverride(70-70)ClearPythonPathOverride(75-75)SetNodePathOverride(81-81)ClearNodePathOverride(86-86)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
104-134: LGTM!The server environment check now correctly sets
ErrorMessageconditionally (null when ready, error message when not), addressing the previous review concern. The try-catch ensures the overall dependency check doesn't fail if server environment validation throws.Based on past review comments, the ErrorMessage conditional logic has been fixed.
MCPForUnity/Editor/Services/PathResolverService.cs (1)
188-203: LGTM!The validation logic correctly implements the previous review suggestion: allowing bare commands like
python3while validating explicit paths. This balances flexibility with safety.Based on past review comments, path validation has been implemented as suggested.
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
141-149: LGTM!The Python environment variables are now correctly scoped to stdio mode only, addressing the previous review concern. This prevents schema violations in HTTP-only configs.
Based on past review comments, the env var scoping has been fixed.
MCPForUnity/Server~/wrapper.js (2)
20-41: LGTM!The PATH augmentation now correctly handles both Windows and Unix platforms, addressing the previous review concern. Common Unix locations like
~/.cargo/binand/usr/local/binare now included.Based on past review comments, Unix PATH handling has been implemented.
44-61: LGTM!The HOME environment variable is now conditionally set for Windows only, preserving the correct HOME value on Unix systems. This addresses the previous review concern.
Based on past review comments, Unix HOME handling has been fixed.
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
96-151: LGTM!The fallback path construction now correctly uses
".."to navigate fromServer~to the parent directory before looking forServer/wrapper.js, addressing the previous review concern.Based on past review comments, the path construction has been fixed.
264-296: CRITICAL: Personal fork hardcoded as default server source.Line 274 hardcodes
choej2303/unity-mcp-gg.gitas the default MCP server source. This is a critical supply-chain and correctness risk for the mainlineCoplayDev/unity-mcprepository. All users without an override will pull from this personal fork instead of the official repository.Apply this diff to restore the official repository as the default:
- // Return hardcoded Git URL as requested by the user who prefers to push changes and use the clean URL - // instead of local file paths. - // Note: This means local changes MUST be pushed to GitHub to take effect in the server! - return "git+https://github.com/choej2303/unity-mcp-gg.git@main#subdirectory=MCPForUnity/Server~"; + // Return official repository URL as default + // Users can override via EditorPrefs.GitUrlOverride for development/testing + return "git+https://github.com/CoplayDev/unity-mcp.git@main#subdirectory=MCPForUnity/Server~";Based on past review comments, this personal fork URL is not acceptable for the mainline repository.
🧹 Nitpick comments (4)
MCPForUnity/Server~/src/main.py (1)
408-427: Replace Korean comments with English for maintainability.The logging suppression implementation correctly addresses the previous review by using WARNING level, disabling propagation, and attaching the file handler. However, the Korean comments (lines 411-413) should be translated to English to maintain consistency and accessibility for international contributors.
Apply this diff to translate the comments:
- # 🚨 [핵심] STDIO 모드일 때만 관련 로거를 CRITICAL로 낮춥니다. - # Uvicorn 및 관련 로거들의 입을 막아 stdout 오염을 방지합니다. - # 🚨 [핵심] STDIO 모드일 때 stdout 오염 방지 + # 🚨 [CRITICAL] In STDIO mode only, suppress related loggers. + # Silence Uvicorn and related loggers to prevent stdout pollution. + # 🚨 [CRITICAL] Prevent stdout pollution in STDIO modeMCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
76-79: Misleading method name: "Async" suggests async/await pattern.The method is named
CheckAllDependenciesAsyncbut returnsDependencyCheckResultsynchronously. The name suggests it's designed for background thread usage (thread-safe), but developers may expectasync Task<DependencyCheckResult>. Consider renaming toCheckAllDependenciesThreadSafeorCheckAllDependenciesWithContextfor clarity.MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
108-123: Consider falling back to "node" if the override path is invalid.The current logic trusts the override path if it exists in EditorPrefs, but if the file has been moved/deleted since being set, the command will fail. Consider:
string nodeCommand = "node"; string nodeOverride = EditorPrefs.GetString(EditorPrefKeys.NodePathOverride, ""); -if (!string.IsNullOrEmpty(nodeOverride) && File.Exists(nodeOverride)) +if (!string.IsNullOrEmpty(nodeOverride)) { - nodeCommand = nodeOverride; + if (File.Exists(nodeOverride)) + { + nodeCommand = nodeOverride; + } + else + { + McpLog.Warn($"Node override path not found: {nodeOverride}, falling back to 'node'"); + } }This provides better resilience against stale configuration.
MCPForUnity/Server~/wrapper.js (1)
115-127: Guard against missing PID in Windows taskkill.If the process fails to spawn,
pythonProcess.pidmay be undefined, causingtaskkillto fail with an invalid PID. Consider:function cleanup() { if (pythonProcess) { try { pythonProcess.kill(); if (process.platform === "win32") { - require("child_process").execSync( - `taskkill /pid ${pythonProcess.pid} /T /F` - ); + if (pythonProcess.pid) { + require("child_process").execSync( + `taskkill /pid ${pythonProcess.pid} /T /F` + ); + } } } catch (e) { /* ignore */ } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
claude_skill_unity.zipis excluded by!**/*.zip
📒 Files selected for processing (12)
MCPForUnity/Editor/Dependencies/DependencyManager.cs(4 hunks)MCPForUnity/Editor/Helpers/AssetPathUtility.cs(2 hunks)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs(2 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)MCPForUnity/Server~/pyproject.toml(1 hunks)MCPForUnity/Server~/src/main.py(2 hunks)MCPForUnity/Server~/wrapper.js(1 hunks)deploy-dev.bat(0 hunks)mcp_source.py(0 hunks)prune_tool_results.py(0 hunks)restore-dev.bat(0 hunks)test_unity_socket_framing.py(0 hunks)
💤 Files with no reviewable changes (5)
- prune_tool_results.py
- mcp_source.py
- deploy-dev.bat
- restore-dev.bat
- test_unity_socket_framing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Server~/pyproject.toml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
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.
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.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/PathResolverService.csMCPForUnity/Editor/Helpers/AssetPathUtility.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/PathResolverService.csMCPForUnity/Editor/Helpers/AssetPathUtility.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/Server~/wrapper.jsMCPForUnity/Editor/Helpers/AssetPathUtility.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/Server~/wrapper.jsMCPForUnity/Editor/Helpers/AssetPathUtility.cs
🧬 Code graph analysis (3)
MCPForUnity/Editor/Dependencies/DependencyManager.cs (4)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-335)GetMcpPackageRootPath(158-197)MCPForUnity/Editor/Services/PathResolverService.cs (3)
GetPythonPath(98-106)GetNodePath(108-116)GetUvxPath(21-38)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (2)
ServerEnvironmentSetup(14-326)IsEnvironmentReady(21-49)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (5)
DependencyStatus(23-23)DependencyStatus(28-28)DependencyStatus(33-33)GetPythonInstallUrl(43-43)GetUvInstallUrl(48-48)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-335)GetWrapperJsPath(100-151)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Services/IPathResolverService.cs (11)
GetPythonPath(24-24)GetNodePath(30-30)IsPythonDetected(36-36)SetUvxPathOverride(48-48)ClearUvxPathOverride(59-59)SetClaudeCliPathOverride(54-54)ClearClaudeCliPathOverride(64-64)SetPythonPathOverride(70-70)ClearPythonPathOverride(75-75)SetNodePathOverride(81-81)ClearNodePathOverride(86-86)
🔇 Additional comments (10)
MCPForUnity/Server~/src/main.py (1)
64-68: LGTM!The multiline tuple formatting improves readability without changing behavior.
MCPForUnity/Editor/Dependencies/DependencyManager.cs (3)
14-24: LGTM!The DependencyContext class provides a clean, thread-safe way to pass path information to dependency checks without accessing Unity APIs on background threads.
59-71: LGTM!Correctly gathers Unity API and EditorPrefs data on the main thread before delegating to the internal method. This design enables safe background execution if needed.
199-208: LGTM!The updated recommendations appropriately reflect the new Node.js wrapper dependency and Python 3.11+ requirement.
MCPForUnity/Editor/Services/PathResolverService.cs (3)
18-19: LGTM!The new properties provide clean API access to check for path overrides.
98-116: LGTM!The getter methods appropriately delegate validation to the setters and simply return stored overrides or sensible defaults.
210-225: LGTM!Consistent validation logic with
SetPythonPathOverride, maintaining a uniform approach across path overrides.MCPForUnity/Server~/wrapper.js (2)
1-16: LGTM!The logging setup appropriately suppresses errors to prevent logging failures from breaking the wrapper process.
66-92: LGTM!The stdout JSON filtering correctly prevents non-JSON output from crashing MCP clients. Line-based parsing is appropriate for the JSON-RPC protocol used by MCP, which sends one JSON object per line.
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
36-94: LGTM!The CallerFilePath approach is clever and provides reliable path resolution for local development scenarios, with appropriate fallbacks for Package Manager and Asset Store installations.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs(6 hunks)MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
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.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (4)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/IPackageDeploymentService.cs (2)
SetStoredSourcePath(8-8)GetStoredSourcePath(7-7)MCPForUnity/Editor/Services/PackageDeploymentService.cs (2)
SetStoredSourcePath(23-27)GetStoredSourcePath(18-21)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
🔇 Additional comments (4)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (4)
31-31: LGTM: Git URL browse button properly declared and cached.The button field and caching are consistent with the existing UI element patterns.
Also applies to: 80-80
124-124: LGTM: Click handler registration follows the established pattern.
247-258: LGTM: Git URL browse handler properly implemented.The folder selection flow correctly updates both the UI field and persisted preferences, and appropriately notifies listeners via events. The implementation is consistent with the existing
OnBrowseUvxClickedhandler.
265-265: LGTM: Displaying empty string is cleaner than "Not set" placeholder.This change improves UI consistency by using an empty string instead of a placeholder value when the source path is not configured.
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (1)
31-32: Remove these unused events or implement their invocation.The
OnGitUrlChangedandOnHttpServerCommandUpdateRequestedevents are declared but never raised anywhere in this file. While they are subscribed to in MCPForUnityEditorWindow.cs (lines 136-137), the events are dead code that will never trigger. TheUpdatePathOverrides()method is empty (lines 91-95), confirming that Git URL override functionality has been removed. Either remove these events or restore the code that raises them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs(2 hunks)MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml(0 hunks)
💤 Files with no reviewable changes (1)
- MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
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.
🔇 Additional comments (1)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (1)
16-17: No issues found. The updated documentation at lines 16-17 accurately referencesMcpSetupSectionandMcpClientConfigSection, both of which exist and properly handle the advanced settings and path overrides that were moved from this section. BothOnGitUrlChangedandOnHttpServerCommandUpdateRequestedevents are actively subscribed to inMCPForUnityEditorWindow.cs, confirming they are not dead code.
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
398-408: Async lambda may access disposed resources after await.After
await connectionSection.VerifyBridgeConnectionAsync()completes,connectionSectioncould have been nulled if the window was closed during the async operation. The null check at line 402 only guards before the await.Add a null check after the await or capture the reference before awaiting:
EditorApplication.delayCall += async () => { - if (this == null || connectionSection == null) + var section = connectionSection; + if (this == null || section == null) { return; } - await connectionSection.VerifyBridgeConnectionAsync(); + await section.VerifyBridgeConnectionAsync(); };
♻️ Duplicate comments (1)
MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (1)
117-143: Critical: Version check accepts 3.10+ but should require 3.11+.Lines 132 and 135 reference Python 3.10+, but:
- Line 70 dialog says "python 3.11+ not found"
- MacOSPlatformDetector and other files in this PR reference 3.11+
- Line 140 returns
truewhen version parsing fails, accepting unvalidated PythonApply this diff:
- if (major > 3 || (major == 3 && minor >= 10)) return true; + if (major > 3 || (major == 3 && minor >= 11)) return true; else { - McpLog.Error($"[MCP Setup] Python version {versionStr} is too old. Required: 3.10+"); + McpLog.Error($"[MCP Setup] Python version {versionStr} is too old. Required: 3.11+"); return false; } } } - return true; + McpLog.Warn($"[MCP Setup] Could not parse Python version: {output}"); + return false;
🧹 Nitpick comments (15)
MCPForUnity/Editor/Helpers/ExecPath.cs (1)
212-243: Enhanced timeout cleanup looks solid overall.The layered exception handling correctly addresses the TOCTOU race between
HasExitedandprocess.Id—if the process exits between those calls, theInvalidOperationExceptioncatch handles it cleanly. The wait-after-kill with warning is a good addition for diagnosing stubborn processes.Minor documentation inconsistency: Line 212 comment says "kill the process and its children," but as the line 220 comment acknowledges, child processes aren't actually killed due to Unity .NET limitations. Consider updating line 212 to avoid confusion:
- // Timeout occurred - kill the process and its children + // Timeout occurred - kill the process (child processes may remain due to Unity .NET limitations)MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
182-203: Consider renaming check comments for clarity.The comment numbering ("Check 3: HTTP", then "Check 1: Node.js Wrapper", "Check 2: UVX") is confusing because Check 3 appears before Checks 1 and 2. Consider using descriptive names instead:
- "HTTP mode: validate URL"
- "Stdio mode: validate Node.js wrapper path"
- "Stdio mode: validate UVX command (legacy fallback)"
Apply this diff:
- // Check 3: HTTP (Only check this if we are in HTTP mode) + // HTTP mode: validate URL match if (!string.IsNullOrEmpty(configuredUrl)) { string expectedUrl = HttpEndpointUtility.GetMcpRpcUrl(); matches = UrlsEqual(configuredUrl, expectedUrl); } } else { - // Check 1: Node.js Wrapper (Stdio) (Only check this if we are in Stdio mode) + // Stdio mode: validate Node.js wrapper path (preferred method) string expectedWrapper = AssetPathUtility.GetWrapperJsPath(); if (!string.IsNullOrEmpty(command) && Path.GetFileNameWithoutExtension(command).Equals("node", StringComparison.OrdinalIgnoreCase) && args != null && args.Length > 0 && !string.IsNullOrEmpty(expectedWrapper)) { if (McpConfigurationHelper.PathsEqual(args[0], expectedWrapper)) { matches = true; } } - // Check 2: UVX (Stdio - Legacy/Fallback) + // Stdio mode: validate UVX command (legacy fallback) else if (args != null && args.Length > 0)MCPForUnity/Editor/Windows/Components/Common.uss (1)
552-583: Mini-button styles assume dark theme.The
.mini-buttonclass usesbackground-color: #444andcolor: #EEEwhich are dark-theme colors. Consider adding light theme variants for consistency.MCPForUnity/Server~/wrapper.js (3)
40-41: Empty paths may be included when environment variables are unset.When
localAppData,userProfile, orhomeDirare empty strings,path.join()will create paths like/Programs/uv(Unix) or\.cargo\bin(Windows) which are invalid but won't cause errors—just unnecessary PATH entries.Filter out empty-based paths:
-const newPath = - extraPaths.join(path.delimiter) + path.delimiter + (process.env.PATH || ""); +const newPath = + extraPaths.filter(p => p && !p.startsWith(path.sep) || p.length > 1) + .join(path.delimiter) + path.delimiter + (process.env.PATH || "");Or filter before joining:
const validExtraPaths = extraPaths.filter(p => { // Exclude paths that start with just a separator (from empty env vars) return p && !p.match(/^[\/\\][^\/\\]/); });
78-84: Misleading comment about JSON parsing.The comment states "We don't parse fully to save perf" but
JSON.parse(line)performs a complete parse. The result is discarded, but the parsing still happens. Consider updating the comment for accuracy.- // Validate JSON. If strictly JSON-RPC, it must be an object. - // We don't parse fully to save perf, but JSON.parse ensures validity. + // Validate JSON by parsing. Result is discarded; we only need to verify it's valid JSON. JSON.parse(line);
115-130: Redundant process termination on Windows.
pythonProcess.kill()is called beforetaskkill. Thetaskkill /T /Fis more thorough (kills process tree), so callingkill()first is redundant. Ifkill()succeeds,taskkillmay fail to find the PID.On Windows, prefer
taskkillalone for process tree cleanup:function cleanup() { if (pythonProcess) { try { - pythonProcess.kill(); if (process.platform === "win32") { if (pythonProcess.pid) { require("child_process").execSync( `taskkill /pid ${pythonProcess.pid} /T /F` ); } + } else { + pythonProcess.kill(); } } catch (e) { /* ignore */ } } }MCPForUnity/Editor/Services/PathResolverService.cs (2)
98-116: Inconsistent error handling for EditorPrefs reads.
GetUvxPath()(lines 21-38) wrapsEditorPrefs.GetStringin a try-catch, butGetPythonPath()andGetNodePath()do not. For consistency and robustness, consider adding the same error handling pattern.public string GetPythonPath() { + try + { string overridePath = EditorPrefs.GetString(EditorPrefKeys.PythonPathOverride, string.Empty); if (!string.IsNullOrEmpty(overridePath)) { return overridePath; } + } + catch + { + McpLog.Debug("No Python path override found, falling back to default command"); + } return RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "python" : "python3"; } public string GetNodePath() { + try + { string overridePath = EditorPrefs.GetString(EditorPrefKeys.NodePathOverride, string.Empty); if (!string.IsNullOrEmpty(overridePath)) { return overridePath; } + } + catch + { + McpLog.Debug("No Node path override found, falling back to default command"); + } return "node"; }
146-160: SetUvxPathOverride validates existence but GetUvxPath doesn't check if override still exists.
SetUvxPathOverridevalidates the file exists at set time, but if the file is later deleted,GetUvxPathwill return a stale path. This is consistent withGetClaudeCliPathwhich does checkFile.Existsfor overrides.Consider validating override existence at read time for consistency with
GetClaudeCliPath:public string GetUvxPath() { try { string overridePath = EditorPrefs.GetString(EditorPrefKeys.UvxPathOverride, string.Empty); - if (!string.IsNullOrEmpty(overridePath)) + if (!string.IsNullOrEmpty(overridePath) && File.Exists(overridePath)) { return overridePath; } }Alternatively, document this as intentional behavior if users want to set paths that don't exist yet.
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
90-97: Status display shows cached data without immediate refresh on dropdown change, but this is intentional design.The comment at line 93 explicitly states "Only update UI, don't check status automatically." When the dropdown changes,
ApplyStatusToUi()displays the cached client status without callingRefreshClientStatus(forceImmediate: true). This is a deliberate design choice—status checks only occur on explicit user actions like clicking Configure or when the window gains focus viaRefreshSelectedClient()called fromOnFocus().The initial window display may briefly show stale status before
OnFocus()triggers a refresh, but this is a minor UX concern given that focus is obtained immediately when the window opens. If immediate status verification on dropdown change is desired, the implementation would need to perform async status checks, which contradicts the stated intent to avoid automatic status checking.MCPForUnity/Editor/Windows/MCPSetupWindow.uxml (1)
10-10: Capitalize "python" to "Python" for consistency.The text uses lowercase "python" while "UV" is uppercase. Python is a proper noun and should be capitalized consistently with standard conventions and other UI elements in the codebase.
Apply this diff:
- <ui:Label text="MCP for Unity requires python 3.11+ and UV package manager to function." class="help-text description-text" /> + <ui:Label text="MCP for Unity requires Python 3.11+ and UV package manager to function." class="help-text description-text" />MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
186-200: Potential deadlock:ReadToEnd()called beforeWaitForExit().If the process writes more data than the buffer can hold,
ReadToEnd()may block waiting for the process to exit while the process blocks waiting for the buffer to drain. For small version outputs this is unlikely, but the safer pattern is to read asynchronously or callWaitForExit()after starting the read but before blocking on it.using var process = Process.Start(psi); if (process == null) return false; -string output = process.StandardOutput.ReadToEnd().Trim(); -process.WaitForExit(3000); +process.WaitForExit(3000); +string output = process.StandardOutput.ReadToEnd().Trim();Note: The same pattern appears in
TryValidatePython(lines 248-249) andTryFindInPath(lines 290-291). Consider applying consistently.
198-200: Empty catch block swallows all exceptions.This makes debugging difficult when validation fails unexpectedly. Consider logging at debug/verbose level or catching specific exception types.
-catch { } +catch (Exception ex) +{ + // Debug logging would help diagnose issues + System.Diagnostics.Debug.WriteLine($"uv validation failed: {ex.Message}"); +}MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (2)
96-151: Python-based uv discovery is a useful fallback.The inline Python script correctly determines the Scripts/bin directory based on OS. However, hardcoding
"python"at line 107 may fail on systems where onlypython3is available.Consider trying both
pythonandpython3:-FileName = "python", // Assume python is in PATH +FileName = GetPythonCommand(), // Try python3 first on non-WindowsOr inline the fallback:
var pythonCommands = new[] { "python3", "python" }; foreach (var cmd in pythonCommands) { // ... try with cmd }
49-62: Same ReadToEnd/WaitForExit ordering concern as Windows detector.Multiple places in this file call
ReadToEnd()beforeWaitForExit()(lines 52-53, 118-119, 137-138, 218-219, 252-253, 296-297). While unlikely to cause issues with small version outputs, consider standardizing the safer pattern.MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
315-375:isCheckingguard prevents re-entrancy but consider making it more robust.The guard pattern works for single-threaded Unity main thread usage. However, if
CheckSystemStatusis ever called from an async context, the guard could fail.For additional safety, consider using a
try-finallypattern that resets the flag even on exceptions:The current implementation already uses
try-finallyat lines 371-374, which is good. The pattern is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
claude_skill_unity.zipis excluded by!**/*.zip
📒 Files selected for processing (42)
.github/workflows/python-tests.yml(3 hunks)MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs(6 hunks)MCPForUnity/Editor/Constants/EditorPrefKeys.cs(2 hunks)MCPForUnity/Editor/Dependencies/DependencyManager.cs(4 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs(1 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs(6 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs(5 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs(4 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs(5 hunks)MCPForUnity/Editor/Helpers/AssetPathUtility.cs(2 hunks)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs(2 hunks)MCPForUnity/Editor/Helpers/ExecPath.cs(1 hunks)MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs(1 hunks)MCPForUnity/Editor/Services/ClientConfigurationService.cs(1 hunks)MCPForUnity/Editor/Services/IPathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/ServerManagementService.cs(7 hunks)MCPForUnity/Editor/Services/ToolDiscoveryService.cs(2 hunks)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs(6 hunks)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs(1 hunks)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs.meta(1 hunks)MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs(6 hunks)MCPForUnity/Editor/Windows/Components/Common.uss(2 hunks)MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml(0 hunks)MCPForUnity/Editor/Windows/Components/Setup.meta(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.meta(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml.meta(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(7 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uss(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uxml(1 hunks)MCPForUnity/Editor/Windows/MCPSetupWindow.uxml(1 hunks)MCPForUnity/Server~/pyproject.toml(1 hunks)MCPForUnity/Server~/src/main.py(2 hunks)MCPForUnity/Server~/wrapper.js(1 hunks)deploy-dev.bat(0 hunks)mcp_source.py(0 hunks)prune_tool_results.py(0 hunks)restore-dev.bat(0 hunks)test_unity_socket_framing.py(0 hunks)
💤 Files with no reviewable changes (6)
- restore-dev.bat
- deploy-dev.bat
- test_unity_socket_framing.py
- MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml
- prune_tool_results.py
- mcp_source.py
✅ Files skipped from review due to trivial changes (1)
- MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.meta
🚧 Files skipped from review as they are similar to previous changes (14)
- MCPForUnity/Server~/src/main.py
- MCPForUnity/Editor/Services/ToolDiscoveryService.cs
- MCPForUnity/Editor/Windows/Components/Setup.meta
- MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uss
- MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs
- MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs
- MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs
- MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml.meta
- MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs.meta
- MCPForUnity/Editor/Services/ClientConfigurationService.cs
- MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml
- MCPForUnity/Editor/Services/ServerManagementService.cs
- MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs
- MCPForUnity/Editor/Services/IPathResolverService.cs
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
📚 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/Server~/wrapper.jsMCPForUnity/Editor/Helpers/AssetPathUtility.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Windows/MCPSetupWindow.uxmlMCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs.github/workflows/python-tests.ymlMCPForUnity/Editor/Setup/ServerEnvironmentSetup.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Server~/pyproject.toml
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.cs
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
MCPForUnity/Editor/Windows/MCPSetupWindow.uxmlMCPForUnity/Server~/pyproject.toml
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/Editor/Windows/MCPSetupWindow.uxmlMCPForUnity/Server~/pyproject.toml
📚 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/MenuItems/MCPForUnityMenu.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Constants/EditorPrefKeys.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/MenuItems/MCPForUnityMenu.csMCPForUnity/Editor/Constants/EditorPrefKeys.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/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Services/PathResolverService.cs
🧬 Code graph analysis (8)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs (1)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (2)
MCPForUnityEditorWindow(18-424)ShowWindow(54-58)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (3)
DependencyStatus(23-23)DependencyStatus(28-28)DependencyStatus(33-33)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (5)
DependencyStatus(19-79)DependencyStatus(110-171)TryValidatePython(173-228)TryFindInPath(289-337)TryValidateUv(230-268)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (3)
DependencyStatus(17-17)DependencyStatus(22-94)DependencyStatus(153-198)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (5)
DependencyStatus(19-80)DependencyStatus(82-168)TryValidatePython(228-269)TryFindInPath(271-310)TryValidateUv(170-201)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (3)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/ClientConfigurationService.cs (1)
CheckClientStatus(53-58)MCPForUnity/Editor/Services/IClientConfigurationService.cs (1)
CheckClientStatus(30-30)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (6)
DependencyStatus(23-23)DependencyStatus(28-28)DependencyStatus(33-33)GetPythonInstallUrl(43-43)GetUvInstallUrl(48-48)GetInstallationRecommendations(38-38)MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
GetInstallationRecommendations(156-167)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Services/IPathResolverService.cs (11)
GetPythonPath(24-24)GetNodePath(30-30)IsPythonDetected(36-36)SetUvxPathOverride(48-48)ClearUvxPathOverride(59-59)SetClaudeCliPathOverride(54-54)ClearClaudeCliPathOverride(64-64)SetPythonPathOverride(70-70)ClearPythonPathOverride(75-75)SetNodePathOverride(81-81)ClearNodePathOverride(86-86)
MCPForUnity/Editor/Helpers/ExecPath.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Warn(43-46)Debug(31-35)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (7)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (4)
JObject(204-255)AssetPathUtility(15-334)GetMcpServerGitUrl(262-296)uvxPath(302-309)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
JObject(32-42)JObject(185-191)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
HttpEndpointUtility(13-86)GetMcpRpcUrl(39-42)MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)
McpConfigurationHelper(22-282)MCPForUnity/Editor/Services/PathResolverService.cs (1)
GetClaudeCliPath(40-96)MCPForUnity/Editor/Models/McpClient.cs (1)
SetStatus(42-54)
🔇 Additional comments (44)
.github/workflows/python-tests.yml (3)
28-31: Verifypyproject.tomlexists inMCPForUnity/Server~with correct configuration.The workflow relies on
uv syncand the[dev]extras being defined in the project configuration. Confirm thatpyproject.tomlis present in the newMCPForUnity/Server~/location and contains the expected sections (dependencies, dev-dependencies, optional groups).
25-25: This change is correct—Python 3.11 is the project's minimum required version.The project explicitly declares
requires-python = ">=3.11"inpyproject.toml, and the Pyright configuration also specifies Python 3.11. The workflow was previously testing against 3.10, which is below the project's minimum requirement. This change fixes that discrepancy to ensure CI tests run against a compatible Python version.Note: The README.md badge currently shows "Python-3.10+" which contradicts the
pyproject.tomlrequirement and should be updated separately.
7-7: The path changes are correct and intentional. TheMCPForUnity/Server~directory exists in the codebase and is consistently referenced across all CI/CD workflows (python-tests.yml, claude-mcp-preflight.yml, and claude-nl-suite.yml). No stale references to the oldServer/path remain. TheServer~naming convention is intentional and documented, not a typo. Python 3.11 is consistently applied across all workflows.Likely an incorrect or invalid review comment.
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (3)
46-46: LGTM! Proper tracking structure for client tasks.The
ConcurrentDictionaryis an appropriate choice for tracking active client handler tasks across threads to ensure proper cleanup during shutdown.
708-708: LGTM! Proper cleanup in finally block.Using
TryRemovein the finally block ensures the client task is removed from tracking on all exit paths, including exceptions.
444-444: LGTM! Consistent error tagging improves observability.Adding the
[StdioBridge]prefix to error logs helps with filtering and debugging, making it easier to identify transport-layer issues.Also applies to: 543-543
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (5)
381-382: LGTM! Well-implemented concurrency control.The static lock and running-state flag correctly prevent concurrent Claude CLI operations, which is essential since CLI invocations can conflict if run simultaneously.
142-168: LGTM! Flexible parsing with dual field support.The refactor to JObject parsing and support for both
urlandserverUrlfields enables compatibility with Antigravity stdio connections while maintaining backward compatibility. The comment on Line 160 helpfully documents this intent.
462-546: LGTM! Properly synchronized with try-finally cleanup.The lock-check-set pattern at the start (lines 462-469) combined with the try-finally block (lines 471-545) ensures mutual exclusion and guarantees the running flag is reset even if an exception occurs. This correctly prevents concurrent Claude CLI operations.
550-608: LGTM! Consistent synchronization and defensive checks.The Unregister method mirrors the synchronization pattern from Register (lines 550-557, 601-607) and adds a defensive check to verify the server exists before attempting removal (lines 580-587). This prevents unnecessary errors and provides clear user feedback.
178-211: The transport-mode-specific validation logic is properly mitigated by the existingStdIoVersionMigrationmechanism.The behavioral change you identified is real:
CheckStatusnow validates differently based on theUseHttpTransportpreference (HTTP mode checks URL; Stdio mode checks Node wrapper then falls back to UVX). However, this risk is already addressed by:
- Automatic migration on upgrade:
StdIoVersionMigrationautomatically regenerates Stdio configs when the package updates, ensuring they match the new validation logic.- Default HTTP mode: The default is HTTP transport (
useHttpTransport = true), preserving backward compatibility with existing configurations.- Auto-rewrite fallback: When
attemptAutoRewrite = true, mismatched configs are automatically regenerated viaMcpConfigurationHelper.WriteMcpConfiguration().- Stdio-only client handling: Stdio-exclusive clients like Claude Desktop are explicitly handled with auto-reconfiguration.
Existing configurations will either remain valid (HTTP mode) or be automatically regenerated (Stdio mode on upgrade).
MCPForUnity/Editor/Windows/Components/Common.uss (2)
365-368: LGTM!The
.tool-item-actionsclass provides a clean horizontal layout for tool action buttons.
585-599: LGTM!The dark theme tool overrides properly scope the styling to
.unity-theme-darkand provide appropriate contrast adjustments.MCPForUnity/Server~/wrapper.js (1)
101-109: LGTM!Error and exit handlers are properly implemented, forwarding the exit code appropriately.
MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (1)
23-33: LGTM!The interface additions are well-designed:
- Optional
overridePathparameters maintain backward compatibility- New
DetectNodemethod follows the established pattern- XML documentation is clear and consistent
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (2)
305-319: LGTM - Explicit status checks prevent unexpected config modifications.The change to only check status when
forceImmediate: trueand withattemptAutoRewrite: falseis a good design decision. This ensures:
- Config files are only modified when users explicitly click "Configure"
- No background processes unexpectedly modify user configurations
321-331: Claude CLI status handling is now consistent with other clients.The simplified
RefreshClaudeCliStatusmethod follows the same pattern as the generalRefreshClientStatus, providing consistent behavior across all client types.MCPForUnity/Editor/Services/PathResolverService.cs (1)
188-203: LGTM - Validation correctly allows bare commands while validating explicit paths.The validation logic appropriately distinguishes between:
- Bare commands like
python3ornode(allowed without file existence check)- Explicit paths containing
/or\(validated for existence)This addresses the past review comment and provides a good user experience.
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uxml (1)
1-47: LGTM! Well-structured UI reorganization.The new layout with status header, collapsible setup, and modular section placeholders provides a clean foundation for the lazy-loaded component architecture. The structure is logical and follows Unity UI Toolkit conventions.
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs (1)
8-12: LGTM! Clean consolidation of menu items.The simplified menu structure with a single entry point and shortcut (%#m) provides better UX and reduces code duplication. The direct call to
ShowWindow()aligns with the modular window refactoring.MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
36-89: LGTM! Robust package path resolution.The multi-tier approach (CallerFilePath → PackageInfo → AssetDatabase fallback) handles both development and packaged scenarios well. The virtual path handling for Packages/ and Assets/ is correct.
264-295: Good fix: Now defaults to official repository.The Git URL now correctly defaults to
CoplayDev/unity-mcpinstead of the personal fork, preserving the EditorPrefs override for development. The commented-out local path logic is preserved for potential future use.MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
19-79: LGTM! Override path logic is well-structured.The priority order (explicit override → EditorPrefs → PATH → which fallback) is logical and provides flexibility. The custom path validation with detailed status messages is helpful for debugging.
108-168: LGTM! UV override handling is consistent.The UV detection follows the same robust pattern as Python detection, with proper override checks, PATH augmentation, and fallback logic.
MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (6)
16-19: LGTM! Path resolution is clear.The property-based path definitions using
AssetPathUtility.GetPackageAbsolutePath()provide a clean, centralized approach. The comment clarifying the venv location is helpful.
21-49: LGTM! Environment readiness check with helpful diagnostics.The cross-platform venv Python executable detection (Scripts/python.exe vs bin/python) and diagnostic logging when files are missing will help troubleshoot setup issues.
51-115: LGTM! Well-structured installation orchestration.The step-by-step flow with progress bars, comprehensive error handling, and user-friendly dialogs provides a good installation experience. The timeout handling prevents indefinite hangs.
156-197: LGTM! Robust UV installation with multiple fallbacks.The UV detection and installation logic handles PATH checks, EditorPrefs overrides, pip installation, and Python-based path discovery. Good error handling throughout.
220-247: LGTM! Flexible dependency installation.The dual approach (uv sync vs pip fallback) ensures dependencies can be installed even without UV. The removal of
--frozenflag allows lock file generation if needed.
249-305: LGTM! Robust command execution with proper timeout.The 2-minute timeout prevents hanging, cross-platform process handling is correct, and the Win32Exception catch prevents false-positive errors when checking for missing tools.
MCPForUnity/Editor/Dependencies/DependencyManager.cs (4)
14-24: LGTM! Clean context pattern for thread-safe checks.The
DependencyContextclass encapsulates all paths needed for dependency checking, enabling the async variant to run without Unity API calls. Well-designed separation of concerns.
59-79: LGTM! Effective refactoring to support async checks.The split between
CheckAllDependencies()(main thread context gathering) andCheckAllDependenciesAsync()(thread-safe) with shared internal logic is clean and maintainable.
81-151: LGTM! Override paths correctly propagated.The context-based path overrides are properly passed to all detectors (Python, UV, Node). The server environment check is well-guarded with try/catch, and
ErrorMessageis correctly set tonullwhen the environment is ready (resolving the past review issue).
185-219: LGTM! Recommendations updated to 3.11+.The dependency recommendations now correctly reference "python 3.11+" at line 199, aligning with the updated requirements across the PR.
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
43-54: LGTM! Well-organized preference key additions.The new path override keys (Python, Node, UV) and reorganized deployment/tool config keys follow consistent naming conventions and support the override system introduced throughout this PR. The namespace consolidation improves maintainability.
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (3)
256-259: Version check logic looks correct for Python 3.11+ requirement.The condition
major > 3 || (major >= 3 && minor >= 11)correctly accepts Python 4+ or Python 3.11+, rejecting 3.10 and earlier.
82-168: Well-structured detection flow with good fallback paths.The uv detection method follows a clear priority order: override → PATH → common install locations. The common paths array on Windows (lines 136-142) covers the standard installation scenarios well.
29-44: Override path handling with EditorPrefs looks correct.The try-catch around
EditorPrefs.GetString(line 31) is a reasonable defensive pattern since EditorPrefs can throw in certain Unity states. TheFile.Existscheck before validation is appropriate.MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (2)
153-198: DetectNode implementation is clean and follows the established pattern.The override → PATH detection flow is consistent with Python and uv detection. The LTS recommendation in the error message (line 190) is helpful user guidance.
315-334: TryParseVersion is a solid utility method.Simple and defensive with proper error handling. Correctly handles the common
major.minor.patchversion format.MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (4)
245-271: Lazy loading of tools section is a good UX optimization.Deferring the
toolsSection?.Refresh()call until the Tools tab is first opened (lines 258-263) prevents unnecessary work during window creation.
273-283: Window lifecycle tracking with OnEnable/OnDisable is correct.Adding/removing from
OpenWindowsin the lifecycle methods ensures accurate tracking. The staticHashSetis safe here since Unity editor runs on a single thread.
285-299: Throttled refresh on focus is a good pattern.The 2-second throttle (line 294) prevents excessive
RefreshAllData()calls when rapidly switching focus, which could cause UI lag.
415-423:CloseAllOpenWindowscorrectly creates a copy before iterating.Creating a copy of the collection (line 418) prevents modification-during-iteration issues when
Close()triggersOnDisablewhich removes fromOpenWindows.
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
Outdated
Show resolved
Hide resolved
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs
Show resolved
Hide resolved
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs
Outdated
Show resolved
Hide resolved
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (1)
32-269: Extract common process execution and validation logic to reduce duplication.The process execution pattern with timeout handling and version extraction is duplicated across multiple methods (
DetectUv,TryFindUvViaPython,TryValidateNode,TryFindNodeInPath). Extracting this into a shared helper method would:
- Centralize timeout handling fixes
- Improve consistency across detection methods
- Reduce code volume and improve maintainability
Consider adding a helper method like:
protected bool TryExecuteAndValidateVersion( string executable, string versionArg, string expectedPrefix, out string version, out string fullOutput) { version = null; fullOutput = null; try { var psi = new ProcessStartInfo { FileName = executable, Arguments = versionArg, UseShellExecute = false, RedirectStandardOutput = true, RedirectStandardError = true, CreateNoWindow = true }; using var process = Process.Start(psi); if (process == null) return false; if (!process.WaitForExit(5000)) { process.Kill(); return false; } fullOutput = process.StandardOutput.ReadToEnd().Trim(); if (process.ExitCode == 0 && fullOutput.StartsWith(expectedPrefix)) { version = fullOutput.Substring(expectedPrefix.Length).Trim(); return true; } } catch { } return false; }Then refactor the detection methods to use this helper.
♻️ Duplicate comments (4)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
524-529: Race condition: Task started before being tracked.This is the same issue flagged in the past review comment. The task is started with
Task.Run()before being added toactiveClientTasks. IfHandleClientAsynccompletes extremely quickly (e.g., immediate connection error), its finally block (line 711) will executeTryRemovebeforeTryAddhappens here, potentially leaving a completed task reference in the dictionary or missing the task entirely.The
lock (clientsLock)doesn't prevent this race because:
HandleClientAsynccan start executing immediately whenTask.Run()is called- The finally block's
TryRemove(line 711) is not underclientsLockConcurrentDictionaryis already thread-safe and doesn't need the lockAs suggested in the past review, use a
TaskCompletionSourceto ensure the task is tracked before the handler can complete:- // Create and track the client handler task - lock (clientsLock) - { - var clientTask = Task.Run(() => HandleClientAsync(client, token), token); - activeClientTasks.TryAdd(client, clientTask); - } + // Create and track the client handler task + var tcs = new TaskCompletionSource<object>(); + activeClientTasks.TryAdd(client, tcs.Task); + _ = Task.Run(async () => + { + try + { + await HandleClientAsync(client, token); + tcs.TrySetResult(null); + } + catch (Exception ex) + { + tcs.TrySetException(ex); + } + }, token);MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
381-384: Fire-and-forget async task discards exceptions.This was flagged in a previous review. The discarded task
_ = connectionSection?.VerifyBridgeConnectionAsync();silently swallows exceptions, making debugging difficult.Consider using a helper that logs exceptions:
- if (MCPServiceLocator.Bridge.IsRunning) - { - _ = connectionSection?.VerifyBridgeConnectionAsync(); - } + if (MCPServiceLocator.Bridge.IsRunning) + { + connectionSection?.VerifyBridgeConnectionAsync().ContinueWith(t => + { + if (t.IsFaulted) McpLog.Warning($"Bridge verification failed: {t.Exception?.InnerException?.Message}"); + }, TaskContinuationOptions.OnlyOnFaulted); + }MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (2)
215-226: Check WaitForExit return value (same pattern as earlier methods).The timeout handling issue identified in the uv validation logic also applies here. Verify the return value of
WaitForExit(5000)and kill the process if it times out.
249-261: Check WaitForExit return value (same pattern as earlier methods).The timeout handling issue applies here as well. Verify
WaitForExit(5000)returnstruebefore accessingExitCode.
🧹 Nitpick comments (11)
MCPForUnity/Editor/Helpers/ExecPath.cs (1)
272-278: Low-risk read ordering inWhichandWhere.Calling
WaitForExitbeforeReadToEndcan theoretically deadlock if output exceeds the internal buffer (the process blocks waiting to write while we block waiting for exit). Forwhich/wherecommands that output a single path, this is effectively safe.If you want to be defensive for future reuse, the safer pattern is to read first:
using var p = Process.Start(psi); -p?.WaitForExit(1500); string output = p?.StandardOutput.ReadToEnd().Trim(); +p?.WaitForExit(1500);Not blocking given the limited output size of these commands.
Also applies to: 292-297
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
461-463: Typo in comment and consider removing commented code.Line 461 has a duplicate word: "Collect all active activeClientTasks" should be "Collect all active client tasks".
The commented-out line 463 with its explanation is helpful for understanding the change, but consider removing it entirely to keep the code clean.
Apply this diff:
- // Collect all active activeClientTasks + // Collect all active client tasks clientTasksToWait = activeClientTasks.Values.ToArray(); - // activeClientTasks.Clear(); // Removed to avoid race with TryRemove in handlersMCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (2)
142-169: Consider extracting duplicated command parsing logic.The command extraction pattern (lines 148-152) is identical to the VS Code path (lines 120-124). Consider extracting this into a helper method to reduce duplication and improve maintainability.
Example refactor:
private static string ExtractCommand(JObject configObj) { var commandToken = configObj["command"]; if (commandToken != null && commandToken.Type == JTokenType.String) { return commandToken.ToString(); } return null; }Then use:
command = ExtractCommand(unityObj);in both branches.
496-516: Consider extracting path prepend logic.The path prepend logic (building the PATH environment variable prefix) is duplicated across
CheckStatus(lines 407-427),Register(lines 496-516), andUnregister(lines 570-578). Extracting this into a helper method would improve maintainability.Example refactor:
private string BuildPathPrepend(string claudePath) { string pathPrepend = null; if (Application.platform == RuntimePlatform.OSXEditor) { pathPrepend = "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin"; } else if (Application.platform == RuntimePlatform.LinuxEditor) { pathPrepend = "/usr/local/bin:/usr/bin:/bin"; } try { string claudeDir = Path.GetDirectoryName(claudePath); if (!string.IsNullOrEmpty(claudeDir)) { pathPrepend = string.IsNullOrEmpty(pathPrepend) ? claudeDir : $"{claudeDir}:{pathPrepend}"; } } catch { } return pathPrepend; }Then use:
string pathPrepend = BuildPathPrepend(claudePath);in all three methods.MCPForUnity/Server~/src/services/tools/manage_script.py (1)
4-4: Potentially unused imports after refactoring.The
urlparseandunquoteimports appear to be unused now that the local_split_urifunction has been removed in favor of the shared utility. Consider removing them to keep imports clean.-from urllib.parse import urlparse, unquoteMCPForUnity/Server~/src/services/tools/utils.py (2)
45-45: Consider moving import to module level.The
import mathinside the function works but incurs a small lookup cost on each call. Sincemathis a stdlib module with no circular import risk, consider moving it to the top-level imports.import json import re import os +import math from typing import Any from urllib.parse import urlparse, unquoteThen remove line 45.
245-251: Rename ambiguous loop variable for clarity.The variable
lin the list comprehension is flagged by linters as ambiguous (can be confused with1orI). Consider renaming tolinefor clarity.def index_of(line: int, col: int) -> int: if line <= len(lines): - return sum(len(l) for l in lines[: line - 1]) + (col - 1) - return sum(len(l) for l in lines) + return sum(len(ln) for ln in lines[: line - 1]) + (col - 1) + return sum(len(ln) for ln in lines)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
49-52: Static collection lacks thread safety documentation.
OpenWindowsis a staticHashSetmodified inOnEnable/OnDisable. While Unity's main thread typically handles these callbacks, consider using a concurrent collection or documenting the thread-safety assumptions to prevent future issues if accessed from background threads.MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (1)
54-72: RedundantCheckPython()calls.
CheckPython()is called at line 54 for pre-check, then again at line 67 inside the try block. The second call is redundant since the first already returned early if Python wasn't found.public static void InstallServerEnvironment() { // 0. Pre-check prerequisites (Python & Node) bool hasPython = CheckPython(); bool hasNode = CheckNode(); if (!hasPython || !hasNode) { SetupWindowService.ShowSetupWindow(); return; } try { - // 1. Check Python - EditorUtility.DisplayProgressBar("MCP Setup", "Checking Python...", 0.1f); - if (!CheckPython()) - { - EditorUtility.ClearProgressBar(); - EditorUtility.DisplayDialog("Error", "python 3.11+ not found in PATH.\nPlease install Python first.", "OK"); - return; - } - - // 2. Check/Install uv + // 1. Check/Install uv EditorUtility.DisplayProgressBar("MCP Setup", "Checking 'uv' package manager...", 0.3f);MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (2)
149-149: Consider logging caught exceptions for debugging.The empty catch block silently swallows all exceptions. While returning
falseis appropriate for aTry*method, logging the exception at debug level would help troubleshoot detection failures.- catch { } + catch (Exception ex) + { + UnityEngine.Debug.LogWarning($"TryFindUvViaPython failed: {ex.Message}"); + }
32-35: Extract EditorPrefs reading into a helper method.The pattern of reading EditorPrefs with defensive try-catch is repeated for both
UvPathOverrideandNodePathOverride. Extracting this into a helper reduces duplication and centralizes error handling.protected string GetEditorPrefsSafely(string key, string defaultValue = "") { try { return UnityEditor.EditorPrefs.GetString(key, defaultValue); } catch (Exception ex) { UnityEngine.Debug.LogWarning($"Failed to read EditorPrefs key '{key}': {ex.Message}"); return defaultValue; } }Then use:
overridePath = GetEditorPrefsSafely(EditorPrefKeys.UvPathOverride);Also applies to: 163-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs(6 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs(7 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs(6 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs(6 hunks)MCPForUnity/Editor/Helpers/ExecPath.cs(3 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs(6 hunks)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Common.uss(2 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(7 hunks)MCPForUnity/Editor/Windows/MCPSetupWindow.uxml(1 hunks)MCPForUnity/Server~/src/services/tools/manage_script.py(5 hunks)MCPForUnity/Server~/src/services/tools/script_apply_edits.py(8 hunks)MCPForUnity/Server~/src/services/tools/utils.py(1 hunks)MCPForUnity/Server~/wrapper.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- MCPForUnity/Editor/Windows/MCPSetupWindow.uxml
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Server~/wrapper.js
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.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/Windows/MCPForUnityEditorWindow.csMCPForUnity/Editor/Services/PathResolverService.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/Setup/ServerEnvironmentSetup.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Server~/src/services/tools/utils.py
📚 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/Setup/ServerEnvironmentSetup.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/Setup/ServerEnvironmentSetup.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs
🧬 Code graph analysis (5)
MCPForUnity/Server~/src/services/tools/manage_script.py (1)
MCPForUnity/Server~/src/services/tools/utils.py (1)
split_uri(78-127)
MCPForUnity/Editor/Helpers/ExecPath.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Warn(43-46)Debug(31-35)
MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (3)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-334)GetPackageAbsolutePath(40-89)MCPForUnity/Editor/Helpers/McpLog.cs (4)
McpLog(7-52)Warn(43-46)Error(48-51)Info(37-41)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Services/IPathResolverService.cs (9)
GetUvxPath(12-12)GetPythonPath(24-24)GetNodePath(30-30)IsPythonDetected(36-36)IsClaudeCliDetected(42-42)SetPythonPathOverride(70-70)ClearPythonPathOverride(75-75)SetNodePathOverride(81-81)ClearNodePathOverride(86-86)
MCPForUnity/Server~/src/services/tools/script_apply_edits.py (1)
MCPForUnity/Server~/src/services/tools/utils.py (4)
parse_json_payload(8-22)apply_edits_locally(187-266)normalize_script_locator(130-184)find_best_anchor_match(269-308)
🪛 Ruff (0.14.8)
MCPForUnity/Server~/src/services/tools/utils.py
202-204: Avoid specifying long messages outside the exception class
(TRY003)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
243-243: Avoid specifying long messages outside the exception class
(TRY003)
246-246: Function definition does not bind loop variable lines
(B023)
247-247: Ambiguous variable name: l
(E741)
247-247: Function definition does not bind loop variable lines
(B023)
248-248: Ambiguous variable name: l
(E741)
248-248: Function definition does not bind loop variable lines
(B023)
264-265: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (37)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
212-243: Well-structured timeout cleanup with proper exception handling.The defensive checks (HasExited, InvalidOperationException handling) and tiered logging appropriately handle the various race conditions that can occur during process termination. The comment about Unity .NET limitations is helpful.
One consideration: since child processes may persist (as noted in the comment), callers performing long-running subprocess chains should be aware that timeout cleanup is best-effort.
246-251: Correct pattern for flushing async output buffers.Calling the parameterless
WaitForExit()after the timed version succeeds is the documented approach to ensureOutputDataReceivedandErrorDataReceivedhandlers complete before reading the buffers. Good fix.MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (5)
46-46: Good addition of task tracking.The
ConcurrentDictionaryis appropriate for tracking active client handler tasks in a thread-safe manner.
462-463: Good fix for the first race condition.Removing the
Clear()call and collecting tasks withToArray()properly addresses the race condition flagged in past reviews whereClear()could race withTryRemovecalls from handler finally blocks.
472-482: Good graceful shutdown with timeout.Waiting for all client tasks to complete with a 2-second timeout ensures cleaner shutdown while preventing indefinite hangs.
444-444: Good addition of log tags for filtering.Adding the
[StdioBridge]tag to error logs improves observability by making it easier to filter bridge-related errors.Also applies to: 546-546
710-711: Task cleanup is correctly placed in finally block.The
TryRemovecall ensures that completed client tasks are removed from tracking. Once the race condition inListenerLoopAsync(lines 524-529) is fixed, this cleanup will work reliably.MCPForUnity/Editor/Windows/Components/Common.uss (2)
365-368: LGTM!The
.tool-item-actionsstyling is clean and appropriate for laying out tool action buttons horizontally.
591-605: LGTM!The dark theme tool overrides are properly scoped and use appropriate colors for dark backgrounds.
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (4)
105-105: LGTM - Command extraction properly implemented.The command field extraction includes appropriate null and type checking before assignment.
Also applies to: 120-124
381-382: LGTM - Appropriate synchronization strategy.The static lock and flag prevent concurrent Claude CLI operations across all configurator instances, which is appropriate since they all invoke the same Claude CLI executable.
462-469: LGTM - Solid synchronization pattern.The lock acquisition, guard check, and guaranteed cleanup in the finally block properly prevent concurrent Claude CLI operations and ensure the running flag is always reset.
Also applies to: 539-545
550-557: LGTM - Consistent synchronization and improved error handling.The synchronization pattern matches
Register, and the pre-check for server existence (lines 580-587) is a good improvement that avoids unnecessary error conditions.Also applies to: 580-587, 601-607
MCPForUnity/Server~/src/services/tools/manage_script.py (1)
14-14: Good refactoring to shared utility.The centralization of
split_uriinto a shared utils module improves maintainability and ensures consistent URI handling across the codebase.MCPForUnity/Server~/src/services/tools/script_apply_edits.py (3)
10-15: Clean consolidation of utilities.The refactoring to use shared utilities (
parse_json_payload,apply_edits_locally,normalize_script_locator,find_best_anchor_match) from the utils module improves code organization and reduces duplication.
420-421: Consistent anchor matching behavior.The use of
find_best_anchor_matchwithprefer_last=Trueis appropriate for class-end insertions. The heuristics in the shared utility will help select the correct closing brace.
141-141: Good placement of locator normalization.Normalizing the script locator early ensures all downstream operations use a consistent name/path format.
MCPForUnity/Server~/src/services/tools/utils.py (8)
8-22: Robust JSON parsing utility.The defensive approach of returning the original payload on parse failure is appropriate for handling edge cases gracefully.
25-33: Sensible boolean coercion.The truthy string values cover common conventions. Note that unrecognized strings like
"false"will return the default (False), which is the expected behavior.
78-127: Comprehensive URI handling.The function handles various URI formats well, including UNC paths and Windows drive letters. The case-insensitive
Assetsdetection and percent-decoding ensure robustness across platforms.
130-184: Flexible script locator normalization.The function handles a wide variety of input formats gracefully. The
collapse_duplicate_tailhelper is a nice touch for handling edge cases.
269-308: Good heuristic-based anchor matching.The function provides intelligent match selection for anchor patterns. The delegation to
_find_best_closing_brace_matchfor class-ending brace detection is a good design choice.
311-377: Well-designed scoring heuristics for C# structure.The scoring system effectively distinguishes class-level braces from method braces by considering indentation, position, and context. The combination of penalties and bonuses provides good discrimination.
200-204: Detailed error messages are acceptable here.While static analysis flags the long exception messages (TRY003), these provide valuable context for debugging edit failures. Creating custom exception classes would be over-engineering for internal utilities.
219-219: Verify default position consistency.The default position for
anchor_insertis"before"here, but inscript_apply_edits.py(lines 415, 564), the default is"after". Ensure this difference is intentional, as it could lead to unexpected behavior if edits don't explicitly specify a position.MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (2)
273-283: LGTM - Clean lifecycle management.Proper subscription/unsubscription pattern for
EditorApplication.updateand tracking open windows viaOnEnable/OnDisable.
317-375: LGTM - Robust status checking with guard and cleanup.The
isCheckingflag prevents reentrant calls, and thetry/finallyensures the flag is always reset. Error handling provides user feedback.MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
211-214: LGTM - Version check now correctly requires Python 3.11+.The previous review flagged that the check accepted 3.10+ while messaging said 3.11+. This has been fixed:
major > 3 || (major == 3 && minor >= 11).
28-44: LGTM - Override path handling is well-structured.The fallback from EditorPrefs to empty string with try/catch, followed by file existence check before validation, provides robust override support.
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
132-157: LGTM - Thorough common path fallback for uv detection.Checking standard install locations (LocalApplicationData, Cargo bin) provides good coverage for Windows installations.
260-263: Logic error in Python version check.The condition
major >= 3 && minor >= 11incorrectly rejects Python 4.x (e.g., 4.0 where minor=0). The macOS detector correctly usesmajor > 3 || (major == 3 && minor >= 11).- // Validate minimum version (Python 4+ or python 3.11+) + // Validate minimum version (Python 4+ or Python 3.11+) if (TryParseVersion(version, out var major, out var minor)) { - return major > 3 || (major >= 3 && minor >= 11); + return major > 3 || (major == 3 && minor >= 11); }Likely an incorrect or invalid review comment.
MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (2)
128-141: LGTM - Version check now correctly requires Python 3.11+.The past review flagged inconsistency between 3.10+ check and 3.11+ messaging. Both issues are now fixed: the check requires 3.11+ and parse failures return false with a warning.
200-218: LGTM - CreateVenv logic handles both uv and fallback scenarios.The logic correctly skips manual venv creation when uv is available (since
uv synchandles it), and falls back topython -m venvotherwise.MCPForUnity/Editor/Services/PathResolverService.cs (2)
202-217: LGTM - Path validation addresses prior review concern.The validation at lines 210-214 correctly allows bare commands (like
python3) while validating explicit paths. This addresses the prior review comment about validating Python/Node overrides.
132-153: LGTM - Actual Python verification via process execution.
IsPythonDetected()now properly runspython --versionto verify Python is functional, not just that a path exists. The 2-second timeout prevents hanging.MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (2)
22-27: Past issue resolved: uv now correctly marked as required.The base class now correctly sets
isRequired: truefor uv, aligning with all platform-specific implementations. This addresses the concern raised in the previous review.
153-198: Verify that Node.js is truly required for all scenarios.The method marks Node.js as
isRequired: true, but the AI summary mentions it as a new addition for "Python/Node/uv path overrides." Confirm that Node.js is a hard requirement for the MCP Unity integration, or whether it should be optional for certain workflows. Check howDetectNoderesults are used throughout the codebase and whether there are conditional code paths that handle cases where Node.js is unavailable.
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
Outdated
Show resolved
Hide resolved
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs
Outdated
Show resolved
Hide resolved
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs
Outdated
Show resolved
Hide resolved
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
186-190: Potential deadlock:WaitForExitbeforeReadToEnd.Calling
process.WaitForExit(3000)before readingStandardOutputcan cause a deadlock if the output buffer fills. The process may block waiting for its output to be consumed, while this code blocks waiting for the process to exit.using var process = Process.Start(psi); if (process == null) return false; - process.WaitForExit(3000); string output = process.StandardOutput.ReadToEnd().Trim(); + if (!process.WaitForExit(3000)) + { + try { process.Kill(); } catch { } + return false; + }
294-295: Same deadlock pattern inTryFindInPath.Consistent issue across all process execution methods in this file.
using var process = Process.Start(psi); if (process == null) return false; - process.WaitForExit(3000); string output = process.StandardOutput.ReadToEnd().Trim(); + if (!process.WaitForExit(3000)) + { + try { process.Kill(); } catch { } + return false; + }MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
142-143: Remove duplicate comment.Lines 142-143 contain an identical comment. Remove the duplicate.
// Also check without the ~ just in case - // Also check without the ~ just in case string wrapperPathNoTilde = Path.Combine(Path.GetDirectoryName(wrapperPath), "..", "Server", "wrapper.js");MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (1)
267-276: Potential deadlock: synchronousReadToEndbeforeWaitForExit.Reading
StandardOutputandStandardErrorsynchronously beforeWaitForExitcan deadlock if output buffers fill. The reads block waiting for the process to close its streams, while the process blocks waiting for buffer space.Use async reads or read in separate threads:
using (Process p = Process.Start(psi)) { if (p == null) { McpLog.Error($"[MCP Setup] Failed to start process: {fileName}"); return false; } - output = p.StandardOutput.ReadToEnd(); - string error = p.StandardError.ReadToEnd(); - - // 2 minute timeout to prevent hanging indefinitely - if (!p.WaitForExit(120000)) + // Read streams asynchronously to prevent deadlock on buffer fill + var outputTask = p.StandardOutput.ReadToEndAsync(); + var errorTask = p.StandardError.ReadToEndAsync(); + + // 2 minute timeout to prevent hanging indefinitely + if (!p.WaitForExit(120000)) { try { p.Kill(); } catch {} McpLog.Error($"[MCP Setup] Command timed out: {fileName} {arguments}"); return false; } + + output = outputTask.GetAwaiter().GetResult(); + string error = errorTask.GetAwaiter().GetResult();MCPForUnity/Editor/Services/PathResolverService.cs (1)
98-113: Inconsistent validation:GetPythonPathdoesn't verify override file exists.Unlike
GetUvxPath(line 26) andGetNodePath(line 120),GetPythonPathreturns the override without checkingFile.Exists. If a user sets an override path and later deletes the file, this returns a stale path. This was flagged in a past review but appears unaddressed.public string GetPythonPath() { try { string overridePath = EditorPrefs.GetString(EditorPrefKeys.PythonPathOverride, string.Empty); - if (!string.IsNullOrEmpty(overridePath)) + if (!string.IsNullOrEmpty(overridePath) && File.Exists(overridePath)) { return overridePath; } }
🧹 Nitpick comments (6)
MCPForUnity/Server~/src/services/tools/utils.py (1)
245-248: Closure captures loop-stable variable — acceptable but consider refactoring.The static analysis flagged
index_ofcapturinglinesfrom the outer scope (B023). While this is safe here sincelinesdoesn't change within a singlereplace_rangeoperation, movinglinesintoindex_of's parameters would make the dependency explicit and silence the linter.- def index_of(line: int, col: int) -> int: - if line <= len(lines): - return sum(len(ln) for ln in lines[: line - 1]) + (col - 1) - return sum(len(ln) for ln in lines) + def index_of(line: int, col: int, lines_ref: list[str] = lines) -> int: + if line <= len(lines_ref): + return sum(len(ln) for ln in lines_ref[: line - 1]) + (col - 1) + return sum(len(ln) for ln in lines_ref)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (2)
76-85: Logic inversion:CreateVenvcalled only whenuvPathis null, but receives it anyway.When
uvPathis empty (line 76),CreateVenv(uvPath)is called with the empty value. InsideCreateVenv, ifuvPathis not empty, it returns early (line 201-205). This means the venv creation only happens whenuvPathis null/empty, which is correct, but the parameter passing is confusing. Consider simplifying toCreateVenv()with no parameter since the fallback behavior is self-contained.- if (string.IsNullOrEmpty(uvPath)) - { - EditorUtility.DisplayProgressBar("MCP Setup", "Creating virtual environment...", 0.5f); - if (!CreateVenv(uvPath)) + if (string.IsNullOrEmpty(uvPath)) + { + EditorUtility.DisplayProgressBar("MCP Setup", "Creating virtual environment...", 0.5f); + if (!CreateVenv())
191-210: ParameteruvPathis unused when non-empty; clarify intent.When
uvPathis non-empty, the method returnstruewithout doing anything (lines 201-205). The comment says "we can skip manual venv creation if using uv sync later", but this makes the method's contract unclear. Consider either:
- Removing the parameter since the fallback path never uses it
- Documenting that this method is a no-op when UV is available
- private static bool CreateVenv(string uvPath) + /// <summary> + /// Creates a Python virtual environment manually. No-op if UV is available (UV handles venv creation). + /// </summary> + private static bool CreateVenv() { if (Directory.Exists(VenvPath)) { McpLog.Info("[MCP Setup] Cleaning existing .venv..."); try { Directory.Delete(VenvPath, true); } catch { /* ignore */ } } - // If uv is available, let uv sync handle venv creation automatically. - // But if called explicitly, use python venv as fallback. - if (!string.IsNullOrEmpty(uvPath)) - { - // We can skip manual venv creation if using uv sync later, - // but returning true here keeps the flow consistent. - return true; - } - McpLog.Info($"[MCP Setup] Creating virtual environment at: {VenvPath}"); return RunCommand(GetPythonCommand(), $"-m venv \"{VenvPath}\"", out string output, workingDirectory: ServerRoot); }MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (2)
49-52: StaticOpenWindowsHashSet is not thread-safe.
HashSet<T>is not thread-safe. While Unity Editor operations are typically single-threaded, if any async continuations modifyOpenWindows, this could cause issues. Consider using a concurrent collection or ensuring all access is on the main thread.
317-375: Race condition:isCheckingflag without synchronization.The
isCheckingflag guards re-entry but isn't atomic. IfCheckSystemStatusis called from multiple paths (e.g., button click + setup complete callback simultaneously), both could pass the check before either sets the flag. Consider usingInterlocked.CompareExchangeor ensuring single-threaded access.- private bool isChecking = false; + private int _isChecking = 0; private void CheckSystemStatus() { - if (isChecking) return; - isChecking = true; + if (System.Threading.Interlocked.CompareExchange(ref _isChecking, 1, 0) != 0) return; // ... existing code ... finally { - isChecking = false; + System.Threading.Interlocked.Exchange(ref _isChecking, 0); } }MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
523-538: LGTM: TaskCompletionSource pattern eliminates the race condition.The updated logic correctly addresses the previous review concern where
Task.Runcould start the handler before the task was added toactiveClientTasks. The new pattern:
- Creates a
TaskCompletionSourceand adds its Task to the dictionary first (line 525)- Starts the handler in a wrapper that completes the TCS (lines 527-538)
- Ensures the tracked task exists in the dictionary before the handler can complete
This guarantees that the handler's finally block (line 720) will always find and remove the correct entry.
Consider adding a brief comment above line 524 explaining the TCS pattern for future maintainers:
+ // Use TaskCompletionSource to ensure the task is tracked in activeClientTasks + // before the handler starts, preventing a race where the handler could complete + // and remove itself before being added to the dictionary. var tcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs(9 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs(5 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs(6 hunks)MCPForUnity/Editor/Helpers/AssetPathUtility.cs(2 hunks)MCPForUnity/Editor/MCPForUnity.Editor.asmdef(1 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs(6 hunks)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Common.uss(2 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(7 hunks)MCPForUnity/Server~/src/services/tools/manage_script.py(6 hunks)MCPForUnity/Server~/src/services/tools/utils.py(1 hunks)MCPForUnity/Server~/tests/integration/test_improved_anchor_matching.py(6 hunks)MCPForUnity/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
📚 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/MCPForUnity.Editor.asmdefMCPForUnity/package.jsonMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.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/Server~/src/services/tools/utils.pyMCPForUnity/Server~/src/services/tools/manage_script.pyMCPForUnity/Editor/Setup/ServerEnvironmentSetup.csMCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Services/PathResolverService.cs
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
MCPForUnity/Editor/Windows/Components/Common.uss
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/package.jsonMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.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/Services/PathResolverService.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs
🧬 Code graph analysis (9)
MCPForUnity/Server~/src/services/tools/utils.py (1)
MCPForUnity/Server~/wrapper.js (2)
path(2-2)line(76-76)
MCPForUnity/Server~/src/services/tools/manage_script.py (1)
MCPForUnity/Server~/src/services/tools/utils.py (1)
split_uri(78-127)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (4)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (6)
DependencyStatus(19-19)DependencyStatus(24-81)DependencyStatus(116-162)GetUvInstallUrl(21-21)GetPythonInstallUrl(20-20)GetInstallationRecommendations(22-22)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (6)
DependencyStatus(23-23)DependencyStatus(28-28)DependencyStatus(33-33)GetUvInstallUrl(48-48)GetPythonInstallUrl(43-43)GetInstallationRecommendations(38-38)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (8)
DependencyStatus(19-79)DependencyStatus(108-168)TryValidatePython(170-224)TryFindInPath(287-335)GetUvInstallUrl(86-89)TryValidateUv(226-265)GetPythonInstallUrl(81-84)GetInstallationRecommendations(91-106)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (8)
DependencyStatus(19-79)DependencyStatus(110-171)TryValidatePython(173-228)TryFindInPath(289-337)GetUvInstallUrl(86-89)TryValidateUv(230-268)GetPythonInstallUrl(81-84)GetInstallationRecommendations(91-108)
MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (2)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-334)GetPackageAbsolutePath(40-89)MCPForUnity/Editor/Setup/SetupWindowService.cs (2)
SetupWindowService(27-35)ShowSetupWindow(95-106)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Server~/tests/integration/test_improved_anchor_matching.py (1)
MCPForUnity/Server~/src/services/tools/utils.py (2)
find_best_anchor_match(269-308)apply_edits_locally(187-266)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (8)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (5)
JObject(204-255)AssetPathUtility(15-334)GetWrapperJsPath(100-151)GetMcpServerGitUrl(262-296)uvxPath(302-309)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
JObject(32-42)JObject(185-191)MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
HttpEndpointUtility(13-86)GetMcpRpcUrl(39-42)MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)
McpConfigurationHelper(22-282)MCPForUnity/Editor/Tools/BatchExecute.cs (1)
JArray(153-161)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-302)TryRun(161-257)MCPForUnity/Editor/Models/McpClient.cs (1)
SetStatus(42-54)
MCPForUnity/Editor/Services/PathResolverService.cs (3)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Services/IPathResolverService.cs (10)
GetUvxPath(12-12)GetPythonPath(24-24)GetNodePath(30-30)IsPythonDetected(36-36)IsClaudeCliDetected(42-42)GetClaudeCliPath(18-18)SetPythonPathOverride(70-70)ClearPythonPathOverride(75-75)SetNodePathOverride(81-81)ClearNodePathOverride(86-86)MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Debug(31-35)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (8)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (2)
McpSettingsSection(19-129)McpSettingsSection(45-51)MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (2)
McpConnectionSection(18-501)McpConnectionSection(61-67)MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (2)
McpClientConfigSection(22-378)McpClientConfigSection(49-56)MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (2)
McpToolsSection(17-318)McpToolsSection(30-35)MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Error(48-51)Warn(43-46)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
DependencyManager(29-220)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)
🪛 Ruff (0.14.8)
MCPForUnity/Server~/src/services/tools/utils.py
202-204: Avoid specifying long messages outside the exception class
(TRY003)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
243-243: Avoid specifying long messages outside the exception class
(TRY003)
246-246: Function definition does not bind loop variable lines
(B023)
247-247: Function definition does not bind loop variable lines
(B023)
248-248: Function definition does not bind loop variable lines
(B023)
264-265: Avoid specifying long messages outside the exception class
(TRY003)
MCPForUnity/Server~/tests/integration/test_improved_anchor_matching.py
138-138: Do not catch blind exception: Exception
(BLE001)
148-148: Do not catch blind exception: Exception
(BLE001)
158-158: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (56)
MCPForUnity/Editor/MCPForUnity.Editor.asmdef (1)
6-8: LGTM! Test runner references added correctly.The addition of
UnityEditor.TestRunnerandUnityEngine.TestRunnerreferences is correct and aligns with the test framework dependency added in package.json. This properly enables the Editor assembly to utilize Unity's test framework APIs.MCPForUnity/package.json (1)
10-11: No action needed — version 1.1.33 is correct for Unity 2021.3.The Unity Test Framework version 1.1.33 is explicitly released for and compatible with Unity 2021.3 LTS. The dependency is appropriately specified.
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (3)
36-89: LGTM: Robust package path resolution with multiple fallbacks.The
GetPackageAbsolutePath()method properly handles various installation scenarios (local dev, Package Manager, Asset Store) with appropriate fallbacks. UsingCallerFilePathattribute for source file discovery is a reliable approach for local development.
91-94: LGTM: Clean helper using compiler services.Good use of
CallerFilePathattribute for compile-time path resolution.
264-273: LGTM: Official repository URL restored.The default Git URL now correctly points to
CoplayDev/unity-mcpinstead of a personal fork, addressing the supply-chain concern from the previous review. The EditorPrefs override is preserved for development/testing scenarios.MCPForUnity/Server~/src/services/tools/utils.py (6)
1-7: LGTM: Well-organized imports.Clean import structure with standard library modules and typing support.
9-24: LGTM: Robust JSON parsing with graceful fallback.The
parse_json_payloadfunction correctly handles non-JSON inputs by checking for structural indicators before attempting to parse, returning the original value on failure.
37-74: LGTM: Comprehensive Vec3 coercion with safety checks.Good handling of multiple input formats (list, tuple, JSON string, comma/space-separated) with
math.isfinitevalidation to reject NaN/Inf values.
78-128: LGTM: Thorough URI splitting with platform awareness.The
split_urifunction handles multiple URI schemes (unity://, file://) correctly, including UNC paths on Windows and proper percent-decoding. Good use of case-insensitive matching for the 'Assets' segment.
269-308: LGTM: Well-designed anchor matching with heuristics.The
find_best_anchor_matchfunction provides intelligent brace matching by considering indentation levels and proximity to file end. Good approach for C# class structure detection.
311-378: LGTM: Scoring heuristics for closing brace detection.The
_find_best_closing_brace_matchimplementation uses reasonable heuristics (indentation level, distance from EOF, surrounding context) to identify class-level braces vs method braces.MCPForUnity/Server~/tests/integration/test_improved_anchor_matching.py (4)
7-7: LGTM: Import updated to use centralized utils module.Correctly imports from the new shared utilities location.
32-34: LGTM: Function call updated to use utils_module.Test correctly calls
utils_module.find_best_anchor_matchfrom the centralized module.
94-128: LGTM: Test converted to synchronous with direct utility call.The test properly validates that
apply_edits_locallyuses improved matching by verifying the insertion point is near the class end.
134-160: Broad exception handling acceptable for test runner.The
except Exceptionblocks in the__main__runner are intentional to capture and report all test failures. This pattern is standard for simple test harnesses. The static analysis warnings (BLE001) can be safely ignored here.MCPForUnity/Server~/src/services/tools/manage_script.py (5)
14-14: LGTM: Import from centralized utils module.Clean import of
split_urifrom the shared utilities module.
45-45: LGTM: Usage updated to shared utility.Call site correctly uses the imported
split_urifunction.
375-375: LGTM: Consistent utility usage in delete_script.Properly uses the shared
split_urifunction.
400-400: LGTM: Consistent utility usage in validate_script.Properly uses the shared
split_urifunction.
538-538: LGTM: Consistent utility usage in get_sha.Properly uses the shared
split_urifunction.MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (4)
24-81: LGTM: Well-structured uv detection with override support.The detection flow correctly prioritizes: EditorPrefs override → PATH discovery → Python-based fallback. Process execution properly handles timeouts and kills processes that exceed the limit.
83-114: LGTM: Python-based uv discovery fallback.Good fallback mechanism to locate uv installed via pip but not in PATH. The comment acknowledges the limitation of potentially picking a different Python than configured.
116-162: LGTM: Node.js detection follows established patterns.Consistent implementation with uv detection, supporting override paths and PATH discovery with proper status reporting.
224-267: LGTM: Robust process execution with timeout handling.The
TryExecuteProcessmethod correctly:
- Uses async output reading via
BeginOutputReadLineto avoid buffer deadlocks- Checks
WaitForExitreturn value and kills timed-out processes- Returns exit code check for success determination
This addresses the previous review concerns about timeout handling.
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
19-80: LGTM: Python detection with override support and version validation.Detection flow correctly handles override paths, direct execution, and
wherecommand fallback. Version validation enforces Python 3.11+ requirement.
82-168: LGTM: Comprehensive uv detection with common install locations.Good addition of fallback paths for common Windows installation locations (LocalApplicationData, Cargo bin, etc.) when uv is not in PATH.
MCPForUnity/Editor/Windows/Components/Common.uss (4)
362-368: LGTM: Tool item actions styling.Clean addition of flexbox row layout for tool action buttons.
513-572: LGTM: Dependency row styles with proper theme scoping.Dependency UI styling correctly defines theme-specific colors for both light and dark themes, addressing the previous review feedback. The installed/missing status colors provide good contrast in both themes.
574-636: LGTM: Mini-button styles with comprehensive theme support.All button states (default, hover, secondary, primary) are properly defined for both themes, with appropriate color choices for accessibility.
638-652: LGTM: Dark theme tool overrides.Proper placement of dark theme specific overrides for tool UI elements.
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (7)
105-148: LGTM: Improved config parsing with helper methods.The extraction of
commandandargsparsing into helper methods improves readability. The HTTP/Stdio mode detection is now more explicit and handles both transport types appropriately.
158-196: LGTM: Transport-aware configuration validation.Good separation of HTTP and Stdio validation logic with defensive exception handling for invalid paths.
229-247: LGTM: Clean extraction helpers.
ExtractCommandandExtractArgsprovide reusable JSON extraction with proper null/type checking.
446-508: LGTM: Register with proper synchronization and error handling.The registration flow correctly:
- Acquires lock and sets running flag
- Validates Claude CLI availability
- Handles "already exists" case gracefully
- Resets running flag in finally block
- Refreshes status after operation
511-562: LGTM: Unregister with proper synchronization.Consistent pattern with Register - properly checks if server exists before attempting removal and handles all edge cases with status updates.
565-590: LGTM: Path prepend construction with defensive error handling.Properly builds platform-specific PATH additions and includes Claude directory for resolution.
386-388: The static lock and flag are appropriate here. TheMcpClientRegistrycaches a single instance of each configurator type, includingClaudeCodeConfigurator, so there is only one instance per AppDomain lifetime—not multiple instances competing for the lock. The static synchronization protects against concurrent access to the Claude CLI within that single instance and is not problematic.MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (4)
16-19: LGTM: Property definitions are clear.The path properties correctly compose paths relative to the package root.
21-49: LGTM: Environment readiness check is well-structured.The method correctly handles both Windows and Unix venv paths, includes helpful debug logging, and validates the package root before proceeding.
108-135: LGTM: Python version check correctly requires 3.11+.The version parsing logic and threshold (3.11+) are now consistent with the error message and other parts of the codebase. The fallback on parse failure now correctly returns
falsewith a warning.
299-317: LGTM: Path override handling is consistent.Both
GetPythonCommandandGetNodeCommandcorrectly check for override paths from EditorPrefs and fall back to default commands.MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (7)
60-92: LGTM: UXML and USS loading with proper null checks.The asset loading correctly handles missing resources and logs appropriate errors.
113-178: LGTM: Section instantiation follows a consistent pattern.All sections are loaded from UXML assets into placeholder elements with proper null checks. The event wiring for inter-section communication is clean.
257-264: LGTM: Lazy loading of Tools section.Deferring tool discovery until the tab is opened improves initial window load time.
273-307: LGTM: Lifecycle management with throttled refresh.The 2-second throttle on
OnFocusrefresh prevents excessive calls, and proper cleanup inOnDisableremoves update handlers and window tracking.
377-391: LGTM: Async exception handling properly addressed.The
ContinueWithpattern withTaskContinuationOptions.OnlyOnFaultedcorrectly logs exceptions from the fire-and-forget async operation without swallowing them.
401-415: LGTM: ScheduleHealthCheck properly handles async exceptions.The pattern matches the fix from past reviews, using
ContinueWithto catch and log faulted tasks.
422-430: LGTM: Window cleanup iterates over a copy.Creating a copy of
OpenWindowsbefore iterating prevents collection modification exceptions.MCPForUnity/Editor/Services/PathResolverService.cs (4)
16-19: LGTM: Override detection properties are consistent.The new
HasPythonPathOverrideandHasNodePathOverrideproperties follow the same pattern as existing uvx and Claude CLI properties.
21-38: LGTM:GetUvxPathvalidates override existence.The method correctly checks
File.Existsbefore returning the override path, falling back to the default command.
115-130: LGTM:GetNodePathvalidates override existence.The method correctly checks
File.Existsbefore returning the override, consistent withGetUvxPath.
202-244: LGTM: Override setters validate explicit paths correctly.The validation logic distinguishes between bare commands (like
python3) and explicit paths, only requiringFile.Existsfor the latter. This aligns with the past review feedback.MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (4)
46-46: LGTM: Thread-safe task tracking added.The
ConcurrentDictionaryprovides thread-safe tracking of client handler tasks, enabling the coordinated shutdown sequence introduced in this PR.
419-481: LGTM: Well-coordinated shutdown sequence.The updated
Stopmethod properly collects a snapshot of active client tasks (line 462) and waits for them with a reasonable 2-second timeout (line 475). This approach avoids the race condition withClear()that was flagged in the previous review. The sequence ensures that:
- No new clients are accepted (
isRunning = false)- Active clients are closed, triggering handler exits
- All handler tasks are awaited before shutdown completes
717-721: LGTM: Consistent cleanup in finally block.The
TryRemovecall (line 720) ensures that completed client handlers are removed fromactiveClientTasks, properly completing the task tracking lifecycle. The finally block guarantees cleanup regardless of how the handler exits.
444-444: LGTM: Consistent error logging with [StdioBridge] prefix.The added
[StdioBridge]tags improve log filtering and make it easier to identify transport-layer issues during debugging.Also applies to: 555-555
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
Outdated
Show resolved
Hide resolved
choej2303
changed the title
… 멈추거나 응답이 없어지는(Deadlock) 문제가 생길 수 있었습니다. 해결: 외부 프로그램을 실행할 때 데이터를 안전하게 처리하도록 코드를 고쳐서, 이제 멈추는 일 없이 부드럽게 실행됩니다. 2. 중복 코드 정리 (대청소) 윈도우, 맥, 리눅스용 코드가 각각 따로따로 작성되어 있었는데, 하는 일은 똑같은데 코드가 흩어져 있어서 관리가 어려웠습니다. 해결: 세 곳에 흩어져 있던 똑같은 기능을 하나로 합쳤습니다. 이제 코드가 훨씬 깔끔해졌고, 나중에 고칠 때도 한 곳만 고치면 됩니다.
ServerManagement: Fix execution errors on Windows by using bat script ServerManagement: Force unbuffered stdout (PYTHONUNBUFFERED) for SSE stability AssetPath: Prioritize local server execution over remote git clone WebSocket: Add connection retry logic for robust startup [한글] feat: MCP 서버 실행 안정성 및 통신 신뢰성 개선 ServerManagement: Windows 커맨드 오류 해결을 위해 배치 스크립트 실행 방식 도입 ServerManagement: SSE 스트림 끊김 방지를 위한 Python 비버퍼링(unbuffered) 옵션 적용 AssetPath: Git 원격 실행 대신 로컬 서버 코드 우선 실행하도록 개선 WebSocket: 서버 부팅 대기를 위한 초기 연결 재시도(Retry) 로직 추가
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (1)
14-24: Duplicate XML documentation block.Lines 14-18 and 19-23 contain identical
<summary>documentation. Remove one of these duplicate blocks./// <summary> /// Centralised command execution pipeline shared by all transport implementations. /// Guarantees that MCP commands are executed on the Unity main thread while preserving /// the legacy response format expected by the server. /// </summary> - /// <summary> - /// Centralised command execution pipeline shared by all transport implementations. - /// Guarantees that MCP commands are executed on the Unity main thread while preserving - /// the legacy response format expected by the server. - /// </summary> [InitializeOnLoad] internal static class TransportCommandDispatcherMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
229-249: Consider refreshing status after Claude CLI path override.After the user selects a new Claude CLI path (line 239), the status is not refreshed, which could lead to a confusing UX. For example, if the previous status was "Not found", it will continue to display that message even after the user has selected a valid path. The user must click the Configure button to see the updated status.
Consider calling
ApplyStatusToUi(client)or providing visual feedback that the path has been updated successfully.MCPForUnity/Server~/src/services/tools/manage_script.py (2)
14-46: Add the same Assets-root guard toapply_text_editsthat you already use in delete/validate.Right after
name, directory = split_uri(uri), reject anything not underAssets/(or explicitly document/allow broader scope).name, directory = split_uri(uri) + if not directory or directory.split("/")[0].lower() != "assets": + return {"success": False, "code": "path_outside_assets", "message": "URI must resolve under 'Assets/'."}
529-552:get_shashould also enforce Assets-only (same rationale as delete/validate).name, directory = split_uri(uri) + if not directory or directory.split("/")[0].lower() != "assets": + return {"success": False, "code": "path_outside_assets", "message": "URI must resolve under 'Assets/'."} params = {"action": "get_sha", "name": name, "path": directory}
♻️ Duplicate comments (6)
MCPForUnity/Editor/Services/ToolDiscoveryService.cs (1)
128-128: Critical: AssetPath null still breaks built-in detection.This is the same issue flagged in the previous review. Setting
AssetPath = nulldisables the path-based check inDetermineIsBuiltIn(lines 320-339). The assembly fallback (lines 341-344) only catchesMCPForUnity.Editorassembly, missing tools likeManageRuntimeCompilationinCustomTools/that compile to the default assembly.The comment promises "resolve lazily if needed," but no lazy resolution is implemented. Either:
- Call
ResolveScriptAssetPath(type)here and accept the performance cost, or- Implement actual lazy resolution (e.g., property getter that resolves on first access), or
- Expand the assembly-based fallback to include other known built-in assemblies/namespaces.
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
229-247: Make ExtractArgs resilient to unexpected JSON shapes.
argsToken.ToObject<string[]>()can throw ifargsisn’t a pure string array; that would flip the whole status toErrorunnecessarily.private static string[] ExtractArgs(JObject configObj) { var argsToken = configObj["args"]; if (argsToken is JArray) { - return argsToken.ToObject<string[]>(); + try { return argsToken.ToObject<string[]>(); } + catch (JsonException) { return null; } } return null; }MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs (4)
292-295: Missinguvin prerequisite gating.Line 292 enables Step 2 (Server Environment) when only Python and Node are ready, but
uvis also required and displayed in the UI. The user can attempt server installation withoutuvpresent, which may cause the installation to fail.- bool step1Complete = pythonReady && nodeReady; + bool step1Complete = pythonReady && nodeReady && uvReady;
373-413: File picker filter breaks macOS/Linux executable selection.The
"exe"filter at lines 375, 391, and 407 hides typical Unix executables (which have no extension). Users on macOS/Linux cannot locate Python, Node.js, or uv executables through the file picker.- string path = EditorUtility.OpenFilePanel("Locate Python Executable", "", "exe"); + string path = EditorUtility.OpenFilePanel("Locate Python Executable", "", ""); ... - string path = EditorUtility.OpenFilePanel("Locate Node.js Executable", "", "exe"); + string path = EditorUtility.OpenFilePanel("Locate Node.js Executable", "", ""); ... - string path = EditorUtility.OpenFilePanel("Locate uv Executable", "", "exe"); + string path = EditorUtility.OpenFilePanel("Locate uv Executable", "", "");
443-448: OnSetupComplete fires unconditionally, even on install failure.Line 447 triggers
OnSetupCompleteimmediately after callingInstallServerEnvironment(), regardless of success or failure. This signals completion prematurely and duplicates the transition logic already present inUpdateUI()(lines 321-330).private void OnInstallServerClicked() { MCPForUnity.Editor.Setup.ServerEnvironmentSetup.InstallServerEnvironment(); RefreshStatus(); - OnSetupComplete?.Invoke(); // Trigger main window refresh + // Let UpdateUI() determine when setup is actually complete based on RefreshStatus results }
468-490: Type mismatch in ternary: StyleKeyword vs StyleColor.Lines 474 and 486 mix
StyleKeyword.Null(enum) withnew StyleColor(Color.red)(struct) in a ternary expression. Both branches must return the same type.- deployStatusLabel.style.color = result.Success ? StyleKeyword.Null : new StyleColor(Color.red); + deployStatusLabel.style.color = result.Success + ? new StyleColor(StyleKeyword.Null) + : new StyleColor(Color.red);Apply the same fix to line 486.
🧹 Nitpick comments (21)
MCPForUnity/Editor/Services/ToolDiscoveryService.cs (2)
137-142: Remove commented code or implement it properly.Commented-out code should typically be removed to keep the codebase clean. If summary extraction is permanently disabled for performance, delete these lines. If you plan to re-enable it later, consider implementing proper lazy resolution that only extracts summaries when explicitly requested (e.g., for UI display).
263-311: Consider using or removing the unused ResolveScriptAssetPath method.This method appears to be the intended lazy resolution mechanism mentioned in the comment at line 128, but it's never called. You could either:
- Use it to fix the critical AssetPath issue (call it in line 128 or implement lazy property resolution), or
- Remove it if path resolution is permanently disabled.
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
107-130: LGTM - Node wrapper.js execution with override support.The node path override validation with fallback and warning is well-implemented. The absence of Windows cmd.exe shim here (unlike the uvx fallback) appears intentional since wrapper.js is designed to improve Windows stability.
Minor simplification for the args construction:
- var args = new List<string> { wrapperPath }; - - unity["args"] = JArray.FromObject(args.ToArray()); + unity["args"] = new JArray(wrapperPath);MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (1)
108-135: Minor: Redundant null check.At line 129,
readycan never benullat this point since we passed the early return at line 114 andreadyis assigned at line 116. The null check is harmless but unnecessary.- if (ready == null || ready.Count == 0) return; + if (ready.Count == 0) return;MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
798-811: Consider logging heartbeat failures in debug mode.The empty catch block at line 810 silently swallows all exceptions. While this is defensive, consider logging failures when debug mode is enabled to aid troubleshooting:
catch { } + catch (Exception ex) + { + if (IsDebugEnabled()) McpLog.Debug($"[StdioBridge] Heartbeat error: {ex.Message}"); + }Actually, on second look, this should replace the existing empty catch:
- catch { } + catch (Exception ex) + { + if (IsDebugEnabled()) McpLog.Debug($"[StdioBridge] Heartbeat error: {ex.Message}"); + }MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
565-590: Prefer Path.PathSeparator when building PATH strings.
On macOS/Linux":"is correct, but usingPath.PathSeparatormakes intent explicit and avoids future platform branching issues.- : $"{claudeDir}:{pathPrepend}"; + : $"{claudeDir}{Path.PathSeparator}{pathPrepend}";MCPForUnity/Server~/wrapper.js (1)
136-143: Consider exiting with non-zero code on signals.The signal handlers call
process.exit()with no argument, which defaults to exit code 0 (success). By convention, processes terminated by signals should exit with a non-zero code (often 128 + signal number or simply 1).Apply this diff to exit with code 1 on signals:
process.on("SIGINT", () => { cleanup(); - process.exit(); + process.exit(1); }); process.on("SIGTERM", () => { cleanup(); - process.exit(); + process.exit(1); });MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (2)
116-144: Consider aligning initial connection retry withReconnectSchedule.The initial connection uses a fixed 5-retry strategy with 2-second delays, whereas
AttemptReconnectAsync(line 693) uses theReconnectSchedulearray with exponential backoff. This inconsistency could confuse maintainers and makes behavior less predictable across connection scenarios.Consider one of the following approaches:
Option 1: Reuse
ReconnectSchedulefor initial connection- // Retry logic for initial connection (server might be starting up) - int maxRetries = 5; - for (int i = 0; i < maxRetries; i++) + // Retry logic for initial connection using the same schedule as reconnection + for (int i = 0; i < ReconnectSchedule.Length; i++) { if (await EstablishConnectionAsync(_lifecycleCts.Token)) { - // State is connected but session ID might be pending until 'registered' message _state = TransportState.Connected(TransportDisplayName, sessionId: "pending", details: _endpointUri.ToString()); _isConnected = true; return true; } - if (i < maxRetries - 1) + if (i < ReconnectSchedule.Length - 1) { - McpLog.Info($"[WebSocket] Connection attempt {i + 1} failed, retrying in 2s..."); + var delay = ReconnectSchedule[i + 1]; + McpLog.Info($"[WebSocket] Connection attempt {i + 1} failed, retrying in {delay.TotalSeconds}s..."); try { - await Task.Delay(2000, _lifecycleCts.Token); + await Task.Delay(delay, _lifecycleCts.Token); } catch (OperationCanceledException) { break; } } }Option 2: Document the intentional difference
If the fixed delay is intentional (e.g., faster retries make sense when the server is starting up), add a comment explaining the reasoning.
117-118: Extract hardcoded retry values to named constants.The retry count (5) and delay (2000ms) are hardcoded, which makes them less discoverable and harder to adjust.
+ private const int InitialConnectionMaxRetries = 5; + private static readonly TimeSpan InitialConnectionRetryDelay = TimeSpan.FromSeconds(2); + public async Task<bool> StartAsync() { // ... existing code ... // Retry logic for initial connection (server might be starting up) - int maxRetries = 5; - for (int i = 0; i < maxRetries; i++) + for (int i = 0; i < InitialConnectionMaxRetries; i++) { if (await EstablishConnectionAsync(_lifecycleCts.Token)) { _state = TransportState.Connected(TransportDisplayName, sessionId: "pending", details: _endpointUri.ToString()); _isConnected = true; return true; } - if (i < maxRetries - 1) + if (i < InitialConnectionMaxRetries - 1) { - McpLog.Info($"[WebSocket] Connection attempt {i + 1} failed, retrying in 2s..."); + McpLog.Info($"[WebSocket] Connection attempt {i + 1} failed, retrying in {InitialConnectionRetryDelay.TotalSeconds}s..."); try { - await Task.Delay(2000, _lifecycleCts.Token); + await Task.Delay(InitialConnectionRetryDelay, _lifecycleCts.Token); } catch (OperationCanceledException) { break; } } }MCPForUnity/Editor/Windows/Components/Common.uss (1)
361-368:.tool-item-actionsneeds wrapping/spacing rules (risk of overflow in narrow panes).Right now it’s just
row+margin-top; consider addingflex-wrap: wrap;(and optionallygap) to avoid buttons overflowing when the window is narrow..tool-item-actions { flex-direction: row; margin-top: 6px; + flex-wrap: wrap; + gap: 6px; }MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uss (1)
3-113: Consider adding.unity-theme-lightoverrides (or explicitly scope these as “always dark”).Right now the header uses dark colors unconditionally; if the intent is theme-adaptive, add light-theme variants (or wrap the dark palette under
.unity-theme-dark).MCPForUnity/Editor/Helpers/ExecPath.cs (1)
212-247: Potential indefinite block on final WaitForExit().The timeout handling at lines 212-242 is robust, but line 247 calls
WaitForExit()without a timeout to flush async buffers. If the process is in an unkillable state (rare but possible on some platforms), this could hang indefinitely.Consider adding a timeout or wrapping in a try-catch:
// Ensure async buffers are flushed - process.WaitForExit(); + try { process.WaitForExit(2000); } catch { }MCPForUnity/Editor/Dependencies/DependencyManager.cs (2)
73-79: Misleading method name:CheckAllDependenciesAsyncis synchronous.The method name suggests async behavior (returning
Task<T>or similar), but it executes synchronously. This could confuse callers expecting non-blocking execution.Consider renaming to better reflect its purpose:
- public static DependencyCheckResult CheckAllDependenciesAsync(DependencyContext context) + public static DependencyCheckResult CheckAllDependenciesWithContext(DependencyContext context) { return CheckAllDependenciesInternal(context); }
18-24: Consider makingDependencyContextimmutable.The class uses public setters, allowing mutation after construction. Since this is a context object meant to be passed around, making it immutable would prevent accidental modifications.
public class DependencyContext { - public string PackageRootPath { get; set; } - public string PythonOverridePath { get; set; } - public string NodeOverridePath { get; set; } - public string UvOverridePath { get; set; } + public string PackageRootPath { get; } + public string PythonOverridePath { get; } + public string NodeOverridePath { get; } + public string UvOverridePath { get; } + + public DependencyContext(string packageRootPath, string pythonOverridePath, string nodeOverridePath, string uvOverridePath) + { + PackageRootPath = packageRootPath; + PythonOverridePath = pythonOverridePath; + NodeOverridePath = nodeOverridePath; + UvOverridePath = uvOverridePath; + } }MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
49-49: Consider documenting thread-safety assumptions forOpenWindows.The static
HashSet<MCPForUnityEditorWindow>is accessed without synchronization. This is safe in Unity Editor context (main thread only), but a brief comment would clarify this assumption.- private static readonly HashSet<MCPForUnityEditorWindow> OpenWindows = new(); + // Note: Only accessed from Unity main thread, no synchronization needed + private static readonly HashSet<MCPForUnityEditorWindow> OpenWindows = new();MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
16-16: Inconsistent naming betweenUvxPathOverrideand path override keys.
UvxPathOverrideuses the pattern"MCPForUnity.UvxPath"(without "Override" suffix), while the new keys use"MCPForUnity.*PathOverride". This inconsistency could confuse maintainers.Consider aligning the naming:
- internal const string UvxPathOverride = "MCPForUnity.UvxPath"; + internal const string UvxPathOverride = "MCPForUnity.UvxPathOverride";Note: This would also be a breaking change for existing preferences.
Also applies to: 44-46
MCPForUnity/Server~/src/services/tools/utils.py (1)
176-180: Minor: awkward line break in string slice.The line break in the middle of the slice operation reduces readability.
- base = file_name[:- - 3] if file_name.lower().endswith(".cs") else file_name + base = file_name[:-3] if file_name.lower().endswith(".cs") else file_nameMCPForUnity/Editor/Services/ServerManagementService.cs (2)
16-40: Cleanup handler creates unnecessary service instance.The cleanup lambda creates a new
ServerManagementServiceinstance. SinceStopLocalHttpServerdoesn't rely on instance state, consider making it static or reusing an instance.private static void EnsureCleanupRegistered() { if (_cleanupRegistered) return; EditorApplication.quitting += () => { try { - var service = new ServerManagementService(); - service.StopLocalHttpServer(); + StopLocalHttpServerStatic(); } catch (Exception ex) { McpLog.Debug($"Cleanup on exit: {ex.Message}"); } }; _cleanupRegistered = true; }Alternatively, if the interface contract requires instance methods, the current approach is acceptable.
229-252: Batch file approach is good but leaves temp file behind.The batch file is written to
%TEMP%\start_mcp_server.batbut never cleaned up. While temp files are generally cleaned by the OS eventually, consider cleaning up on successful server stop or using a unique filename to avoid conflicts.Consider storing the path and cleaning up:
private static string _lastBatchFilePath; // In StartLocalHttpServer: _lastBatchFilePath = tempBatPath; // In StopLocalHttpServer or cleanup: if (!string.IsNullOrEmpty(_lastBatchFilePath) && File.Exists(_lastBatchFilePath)) { try { File.Delete(_lastBatchFilePath); } catch { } _lastBatchFilePath = null; }MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (1)
83-114: Consider using configured Python path in the fallback.
TryFindUvViaPython()hardcodes"python"at line 95, which may use a different Python installation than the user's configured override. While the comment acknowledges this trade-off, it could lead to confusion when a user has set a specific Python path override but the fallback uses a different Python.Consider retrieving the configured Python path (if available) via
GetEditorPrefsSafely(EditorPrefKeys.PythonPathOverride)and using it if non-empty, falling back to"python"only if no override is set.MCPForUnity/Server~/src/main.py (1)
411-413: Consolidate redundant comments.Lines 411-413 contain three similar comments expressing the same intent (preventing stdout pollution in STDIO mode). Consider consolidating into a single, clear comment.
Apply this diff:
- # 🚨 [CRITICAL] In STDIO mode only, suppress related loggers. - # Silence Uvicorn and related loggers to prevent stdout pollution. - # 🚨 [CRITICAL] Prevent stdout pollution in STDIO mode + # Suppress noisy loggers in STDIO mode to prevent stdout pollution + # while still capturing their logs in the rotating file handler.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
MCPForUnity/Server~/wrapper.logis excluded by!**/*.logclaude_skill_unity.zipis excluded by!**/*.zip
📒 Files selected for processing (50)
.github/workflows/python-tests.yml(3 hunks)MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs(9 hunks)MCPForUnity/Editor/Constants/EditorPrefKeys.cs(2 hunks)MCPForUnity/Editor/Dependencies/DependencyManager.cs(4 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs(1 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs(8 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs(6 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs(6 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs(4 hunks)MCPForUnity/Editor/Helpers/AssetPathUtility.cs(2 hunks)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs(2 hunks)MCPForUnity/Editor/Helpers/ExecPath.cs(3 hunks)MCPForUnity/Editor/MCPForUnity.Editor.asmdef(1 hunks)MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs(1 hunks)MCPForUnity/Editor/Services/ClientConfigurationService.cs(1 hunks)MCPForUnity/Editor/Services/IPathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)MCPForUnity/Editor/Services/ServerManagementService.cs(8 hunks)MCPForUnity/Editor/Services/ToolDiscoveryService.cs(3 hunks)MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs(5 hunks)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs(11 hunks)MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs(1 hunks)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs(1 hunks)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs.meta(1 hunks)MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs(6 hunks)MCPForUnity/Editor/Windows/Components/Common.uss(2 hunks)MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs(2 hunks)MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml(0 hunks)MCPForUnity/Editor/Windows/Components/Setup.meta(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.meta(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml(1 hunks)MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml.meta(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(7 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uss(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uxml(1 hunks)MCPForUnity/Editor/Windows/MCPSetupWindow.uxml(1 hunks)MCPForUnity/Server~/pyproject.toml(1 hunks)MCPForUnity/Server~/src/main.py(2 hunks)MCPForUnity/Server~/src/services/tools/manage_script.py(6 hunks)MCPForUnity/Server~/src/services/tools/script_apply_edits.py(8 hunks)MCPForUnity/Server~/src/services/tools/utils.py(1 hunks)MCPForUnity/Server~/tests/integration/test_improved_anchor_matching.py(6 hunks)MCPForUnity/Server~/wrapper.js(1 hunks)MCPForUnity/package.json(1 hunks)deploy-dev.bat(0 hunks)mcp_source.py(0 hunks)prune_tool_results.py(0 hunks)restore-dev.bat(0 hunks)test_unity_socket_framing.py(0 hunks)
💤 Files with no reviewable changes (6)
- prune_tool_results.py
- restore-dev.bat
- MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml
- mcp_source.py
- test_unity_socket_framing.py
- deploy-dev.bat
✅ Files skipped from review due to trivial changes (2)
- MCPForUnity/Editor/Windows/Components/Setup.meta
- MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs.meta
🚧 Files skipped from review as they are similar to previous changes (9)
- MCPForUnity/package.json
- MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml
- .github/workflows/python-tests.yml
- MCPForUnity/Editor/Services/ClientConfigurationService.cs
- MCPForUnity/Server~/pyproject.toml
- MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.uxml.meta
- MCPForUnity/Editor/MCPForUnity.Editor.asmdef
- MCPForUnity/Editor/Helpers/AssetPathUtility.cs
- MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
📚 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/MenuItems/MCPForUnityMenu.csMCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.csMCPForUnity/Editor/Services/ToolDiscoveryService.csMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.meta
📚 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/MenuItems/MCPForUnityMenu.csMCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.csMCPForUnity/Editor/Windows/MCPSetupWindow.uxmlMCPForUnity/Editor/Services/ToolDiscoveryService.csMCPForUnity/Server~/src/services/tools/utils.pyMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Setup/ServerEnvironmentSetup.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.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/MenuItems/MCPForUnityMenu.csMCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.csMCPForUnity/Editor/Constants/EditorPrefKeys.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/Windows/Components/Setup/McpSetupSection.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.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/Windows/Components/Setup/McpSetupSection.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Server~/wrapper.jsMCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
MCPForUnity/Editor/Windows/MCPSetupWindow.uxml
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/Editor/Windows/MCPSetupWindow.uxmlMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csMCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs.meta
📚 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/ToolDiscoveryService.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Services/PathResolverService.cs
🧬 Code graph analysis (16)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (3)
AssetPathUtility(15-330)GetWrapperJsPath(100-150)uvxPath(298-305)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs (1)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (2)
MCPForUnityEditorWindow(18-428)ShowWindow(54-58)
MCPForUnity/Editor/Windows/Components/Setup/McpSetupSection.cs (5)
MCPForUnity/Editor/Dependencies/DependencyManager.cs (5)
DependencyCheckResult(59-71)DependencyCheckResult(76-79)DependencyCheckResult(81-151)DependencyManager(29-220)pythonUrl(172-183)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/PathResolverService.cs (9)
GetPythonPath(98-113)GetNodePath(115-130)GetUvxPath(21-38)SetPythonPathOverride(206-221)ClearPythonPathOverride(223-226)SetNodePathOverride(228-243)ClearNodePathOverride(245-248)SetUvxPathOverride(164-178)ClearUvxPathOverride(196-199)MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs (3)
ServerEnvironmentSetup(15-326)InstallUvExplicitly(144-147)InstallServerEnvironment(52-107)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (3)
Task(76-100)TrySetResult(52-55)TransportCommandDispatcher(66-70)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (4)
DependencyStatus(19-19)DependencyStatus(24-81)DependencyStatus(116-162)TryExecuteProcess(224-280)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (3)
DependencyStatus(23-23)DependencyStatus(28-28)DependencyStatus(33-33)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (1)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
InitializeOnLoad(25-966)
MCPForUnity/Editor/Services/ToolDiscoveryService.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Warn(43-46)Info(37-41)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (3)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/ClientConfigurationService.cs (1)
CheckClientStatus(53-58)MCPForUnity/Editor/Services/IClientConfigurationService.cs (1)
CheckClientStatus(30-30)
MCPForUnity/Server~/tests/integration/test_improved_anchor_matching.py (1)
MCPForUnity/Server~/src/services/tools/utils.py (2)
find_best_anchor_match(269-308)apply_edits_locally(187-266)
MCPForUnity/Server~/src/services/tools/utils.py (1)
MCPForUnity/Server~/wrapper.js (2)
path(2-2)line(76-76)
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
MCPForUnity/Editor/Services/PathResolverService.cs (6)
GetPythonPath(98-113)GetNodePath(115-130)SetPythonPathOverride(206-221)ClearPythonPathOverride(223-226)SetNodePathOverride(228-243)ClearNodePathOverride(245-248)
MCPForUnity/Server~/src/main.py (1)
MCPForUnity/Server~/tests/integration/test_helpers.py (1)
info(36-37)
MCPForUnity/Server~/src/services/tools/manage_script.py (1)
MCPForUnity/Server~/src/services/tools/utils.py (1)
split_uri(78-127)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (6)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (5)
DependencyStatus(19-79)DependencyStatus(110-171)GetPythonInstallUrl(81-84)GetUvInstallUrl(86-89)GetInstallationRecommendations(91-108)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (5)
DependencyStatus(19-79)DependencyStatus(108-168)GetPythonInstallUrl(81-84)GetUvInstallUrl(86-89)GetInstallationRecommendations(91-106)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (5)
DependencyStatus(19-80)DependencyStatus(82-168)GetPythonInstallUrl(188-191)GetUvInstallUrl(193-196)GetInstallationRecommendations(198-211)MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs (6)
DependencyStatus(23-23)DependencyStatus(28-28)DependencyStatus(33-33)GetPythonInstallUrl(43-43)GetUvInstallUrl(48-48)GetInstallationRecommendations(38-38)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-55)MCPForUnity/Editor/Services/IPathResolverService.cs (9)
GetUvxPath(12-12)GetPythonPath(24-24)GetNodePath(30-30)IsPythonDetected(36-36)IsClaudeCliDetected(42-42)SetPythonPathOverride(70-70)ClearPythonPathOverride(75-75)SetNodePathOverride(81-81)ClearNodePathOverride(86-86)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (4)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (3)
McpConfigurationHelper(22-282)PathsEqual(212-229)ExtractUvxUrl(199-210)MCPForUnity/Editor/Tools/BatchExecute.cs (1)
JArray(153-161)MCPForUnity/Editor/Services/IPathResolverService.cs (1)
GetClaudeCliPath(18-18)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-340)TryRun(161-257)
🪛 Ruff (0.14.8)
MCPForUnity/Server~/tests/integration/test_improved_anchor_matching.py
138-138: Do not catch blind exception: Exception
(BLE001)
148-148: Do not catch blind exception: Exception
(BLE001)
158-158: Do not catch blind exception: Exception
(BLE001)
MCPForUnity/Server~/src/services/tools/utils.py
202-204: Avoid specifying long messages outside the exception class
(TRY003)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
243-243: Avoid specifying long messages outside the exception class
(TRY003)
264-265: Avoid specifying long messages outside the exception class
(TRY003)
| bool matches = false; | ||
| if (args != null && args.Length > 0) | ||
| bool useHttpTransport = EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true); | ||
|
|
||
| if (useHttpTransport) | ||
| { | ||
| string expectedUvxUrl = AssetPathUtility.GetMcpServerGitUrl(); | ||
| string configuredUvxUrl = McpConfigurationHelper.ExtractUvxUrl(args); | ||
| matches = !string.IsNullOrEmpty(configuredUvxUrl) && | ||
| McpConfigurationHelper.PathsEqual(configuredUvxUrl, expectedUvxUrl); | ||
| // Check: HTTP (Only check this if we are in HTTP mode) | ||
| if (!string.IsNullOrEmpty(configuredUrl)) | ||
| { | ||
| string expectedUrl = HttpEndpointUtility.GetMcpRpcUrl(); | ||
| matches = UrlsEqual(configuredUrl, expectedUrl); | ||
| } | ||
| } | ||
| else if (!string.IsNullOrEmpty(configuredUrl)) | ||
| else | ||
| { | ||
| string expectedUrl = HttpEndpointUtility.GetMcpRpcUrl(); | ||
| matches = UrlsEqual(configuredUrl, expectedUrl); | ||
| // Check: Node.js Wrapper (Stdio) (Only check this if we are in Stdio mode) | ||
| string expectedWrapper = AssetPathUtility.GetWrapperJsPath(); | ||
| if (!string.IsNullOrEmpty(command) && args != null && args.Length > 0 && !string.IsNullOrEmpty(expectedWrapper)) | ||
| { | ||
| try | ||
| { | ||
| if (Path.GetFileNameWithoutExtension(command).Equals("node", StringComparison.OrdinalIgnoreCase) && | ||
| McpConfigurationHelper.PathsEqual(args[0], expectedWrapper)) | ||
| { | ||
| matches = true; | ||
| } | ||
| } | ||
| catch (ArgumentException) | ||
| { | ||
| // Invalid command path, skip Node.js wrapper check | ||
| } | ||
| } | ||
| // Check: UVX (Stdio - Legacy/Fallback) | ||
| else if (args != null && args.Length > 0) | ||
| { | ||
| string expectedUvxUrl = AssetPathUtility.GetMcpServerGitUrl(); | ||
| string configuredUvxUrl = McpConfigurationHelper.ExtractUvxUrl(args); | ||
| matches = !string.IsNullOrEmpty(configuredUvxUrl) && | ||
| McpConfigurationHelper.PathsEqual(configuredUvxUrl, expectedUvxUrl); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix stdio matching: Node-wrapper gate currently blocks UVX fallback, and URL compare uses path comparer.
Right now, any stdio config with command != node but args != null enters the Node-wrapper if and skips the UVX check, so legacy uvx ... --from <gitUrl> setups won’t match. Also, PathsEqual is not URL-safe for --from values.
- // Check: Node.js Wrapper (Stdio) (Only check this if we are in Stdio mode)
+ // Check: Node.js Wrapper (Stdio) vs UVX (Stdio - Legacy/Fallback)
string expectedWrapper = AssetPathUtility.GetWrapperJsPath();
- if (!string.IsNullOrEmpty(command) && args != null && args.Length > 0 && !string.IsNullOrEmpty(expectedWrapper))
+ bool isNode = false;
+ try
+ {
+ isNode = !string.IsNullOrEmpty(command) &&
+ Path.GetFileNameWithoutExtension(command)
+ .Equals("node", StringComparison.OrdinalIgnoreCase);
+ }
+ catch (ArgumentException)
+ {
+ // Invalid command path; treat as not-node and fall back to UVX check below.
+ }
+
+ if (isNode && args != null && args.Length > 0 && !string.IsNullOrEmpty(expectedWrapper))
{
- try
- {
- if (Path.GetFileNameWithoutExtension(command).Equals("node", StringComparison.OrdinalIgnoreCase) &&
- McpConfigurationHelper.PathsEqual(args[0], expectedWrapper))
- {
- matches = true;
- }
- }
- catch (ArgumentException)
- {
- // Invalid command path, skip Node.js wrapper check
- }
+ matches = McpConfigurationHelper.PathsEqual(args[0], expectedWrapper);
}
// Check: UVX (Stdio - Legacy/Fallback)
else if (args != null && args.Length > 0)
{
string expectedUvxUrl = AssetPathUtility.GetMcpServerGitUrl();
string configuredUvxUrl = McpConfigurationHelper.ExtractUvxUrl(args);
matches = !string.IsNullOrEmpty(configuredUvxUrl) &&
- McpConfigurationHelper.PathsEqual(configuredUvxUrl, expectedUvxUrl);
+ UrlsEqual(configuredUvxUrl, expectedUvxUrl);
}Also consider aligning CodexMcpConfigurator.CheckStatus’s --from comparison to use UrlsEqual for consistency.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs around lines 157 to
196, the current stdio matching logic incorrectly prevents the UVX fallback when
a command is present but not the node wrapper and it uses PathsEqual to compare
a URL; change the flow so the Node-wrapper check only runs when the command
clearly indicates node (i.e., test
Path.GetFileNameWithoutExtension(command).Equals("node",...) and on exception
treat as non-node) and do not use that branch to skip the UVX check — perform
the UVX fallback check independently when args exist; additionally replace
PathsEqual with UrlsEqual when comparing the extracted "--from"/configured UVX
URL, and make the same UrlsEqual change in CodexMcpConfigurator.CheckStatus for
the --from comparison to keep behavior consistent.
| // Validate minimum version (Python 4+ or python 3.10+) | ||
| if (TryParseVersion(version, out var major, out var minor)) | ||
| { | ||
| version = output.Substring(7); // Remove "Python " prefix | ||
| fullPath = pythonPath; | ||
|
|
||
| // Validate minimum version (Python 4+ or Python 3.10+) | ||
| if (TryParseVersion(version, out var major, out var minor)) | ||
| { | ||
| return major > 3 || (major >= 3 && minor >= 10); | ||
| } | ||
| return major > 3 || (major >= 3 && minor >= 10); | ||
| } |
There was a problem hiding this comment.
Version check accepts 3.10+ but messaging says 3.11+.
Line 191 accepts Python 3.10+ (minor >= 10), but the error message at line 71 and comments elsewhere in the PR reference 3.11+ as the requirement. This inconsistency exists across platform detectors.
- // Validate minimum version (Python 4+ or python 3.10+)
+ // Validate minimum version (Python 4+ or Python 3.11+)
if (TryParseVersion(version, out var major, out var minor))
{
- return major > 3 || (major >= 3 && minor >= 10);
+ return major > 3 || (major == 3 && minor >= 11);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Validate minimum version (Python 4+ or python 3.10+) | |
| if (TryParseVersion(version, out var major, out var minor)) | |
| { | |
| version = output.Substring(7); // Remove "Python " prefix | |
| fullPath = pythonPath; | |
| // Validate minimum version (Python 4+ or Python 3.10+) | |
| if (TryParseVersion(version, out var major, out var minor)) | |
| { | |
| return major > 3 || (major >= 3 && minor >= 10); | |
| } | |
| return major > 3 || (major >= 3 && minor >= 10); | |
| } | |
| // Validate minimum version (Python 4+ or Python 3.11+) | |
| if (TryParseVersion(version, out var major, out var minor)) | |
| { | |
| return major > 3 || (major == 3 && minor >= 11); | |
| } |
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs
around lines 188-192, the runtime check currently accepts Python 3.10+ while the
project messaging requires Python 3.11+; update the logic to require 3.11 or
higher by changing the minor-version threshold to 11 (i.e., require major > 3 ||
(major == 3 && minor >= 11)), and ensure any nearby comments or error messages
are consistent with 3.11+.
| if (!p.WaitForExit(120000)) | ||
| { | ||
| try { p.Kill(); } catch {} | ||
| McpLog.Error($"[MCP Setup] Command timed out: {fileName} {arguments}"); | ||
| return false; | ||
| } | ||
|
|
||
| output = outputTask.Result; | ||
| string error = errorTask.Result; | ||
|
|
||
| // 2 minute timeout to prevent hanging indefinitely | ||
| if (!p.WaitForExit(120000)) | ||
| { | ||
| try { p.Kill(); } catch {} | ||
| McpLog.Error($"[MCP Setup] Command timed out: {fileName} {arguments}"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Remove duplicate WaitForExit call.
Lines 279-284 duplicate the WaitForExit logic from lines 268-273. The second call is redundant since the process has already been waited on (or killed on timeout).
output = outputTask.Result;
string error = errorTask.Result;
- // 2 minute timeout to prevent hanging indefinitely
- if (!p.WaitForExit(120000))
- {
- try { p.Kill(); } catch {}
- McpLog.Error($"[MCP Setup] Command timed out: {fileName} {arguments}");
- return false;
- }
if (p.ExitCode != 0)🤖 Prompt for AI Agents
In MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs around lines 268-284,
remove the duplicated WaitForExit/timeout block (lines 279-284) — the process is
already waited on (or killed) by the first block, so delete the second
WaitForExit/kill/log/return section and ensure you still assign output =
outputTask.Result and error = errorTask.Result after the single wait; keep the
2-minute timeout comment in the remaining block and preserve the existing
logging/kill behavior.
| private readonly Dictionary<IMcpClientConfigurator, DateTime> lastStatusChecks = new(); | ||
| private readonly HashSet<IMcpClientConfigurator> statusRefreshInFlight = new(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'statusRefreshInFlight' --type csRepository: CoplayDev/unity-mcp
Length of output: 230
Remove the unused statusRefreshInFlight field.
The statusRefreshInFlight field is declared at line 43 but is never referenced anywhere in the codebase. This is dead code left over from the refactoring that removed UpdateClientStatus() and should be removed.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
around lines 42 to 43, remove the dead private readonly field declaration
'statusRefreshInFlight' since it is never referenced after the refactor; delete
the entire field line, ensure no other code relies on it, and run a build/tests
to verify compilation and behavior remain unchanged.
| <uie:Toolbar name="tab-toolbar" class="tab-toolbar" style="margin-top: 10px; margin-bottom: 5px;"> | ||
| <uie:ToolbarToggle name="settings-tab" text="Settings" /> | ||
| <uie:ToolbarToggle name="tools-tab" text="Tools" /> | ||
| </uie:Toolbar> | ||
| <ui:ScrollView name="settings-panel" class="panel-scroll" style="flex-grow: 1;"> | ||
| <ui:VisualElement name="settings-container" class="section-stack" /> | ||
| </ui:ScrollView> | ||
| <ui:ScrollView name="tools-panel" class="panel-scroll hidden" style="flex-grow: 1;"> | ||
| <ui:VisualElement name="tools-container" class="section-stack" /> | ||
| </ui:ScrollView> | ||
| </ui:VisualElement> | ||
|
|
||
| <!-- Main Workspace (Tabbed Content) --> | ||
| <ui:VisualElement name="main-workspace" class="main-workspace"> | ||
|
|
||
| <!-- Settings Panel --> | ||
| <ui:VisualElement name="settings-panel"> | ||
| <ui:VisualElement name="settings-section-placeholder" /> | ||
| <ui:VisualElement name="connection-section-placeholder" /> | ||
| <ui:VisualElement name="client-config-section-placeholder" /> | ||
| </ui:VisualElement> | ||
|
|
||
| <!-- Tools Panel --> | ||
| <ui:VisualElement name="tools-panel" style="display: none;"> | ||
| <ui:VisualElement name="tools-section-placeholder" /> | ||
| </ui:VisualElement> | ||
|
|
There was a problem hiding this comment.
Ensure exactly one tab is selected by default (avoid blank content).
With ToolbarToggle, it’s easy to end up with both toggles off unless initialization forces a default. Consider setting Settings on by default in UXML (and enforce mutual exclusivity in code):
-<uie:ToolbarToggle name="settings-tab" text="Settings" />
+<uie:ToolbarToggle name="settings-tab" text="Settings" value="true" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <uie:Toolbar name="tab-toolbar" class="tab-toolbar" style="margin-top: 10px; margin-bottom: 5px;"> | |
| <uie:ToolbarToggle name="settings-tab" text="Settings" /> | |
| <uie:ToolbarToggle name="tools-tab" text="Tools" /> | |
| </uie:Toolbar> | |
| <ui:ScrollView name="settings-panel" class="panel-scroll" style="flex-grow: 1;"> | |
| <ui:VisualElement name="settings-container" class="section-stack" /> | |
| </ui:ScrollView> | |
| <ui:ScrollView name="tools-panel" class="panel-scroll hidden" style="flex-grow: 1;"> | |
| <ui:VisualElement name="tools-container" class="section-stack" /> | |
| </ui:ScrollView> | |
| </ui:VisualElement> | |
| <!-- Main Workspace (Tabbed Content) --> | |
| <ui:VisualElement name="main-workspace" class="main-workspace"> | |
| <!-- Settings Panel --> | |
| <ui:VisualElement name="settings-panel"> | |
| <ui:VisualElement name="settings-section-placeholder" /> | |
| <ui:VisualElement name="connection-section-placeholder" /> | |
| <ui:VisualElement name="client-config-section-placeholder" /> | |
| </ui:VisualElement> | |
| <!-- Tools Panel --> | |
| <ui:VisualElement name="tools-panel" style="display: none;"> | |
| <ui:VisualElement name="tools-section-placeholder" /> | |
| </ui:VisualElement> | |
| <uie:Toolbar name="tab-toolbar" class="tab-toolbar" style="margin-top: 10px; margin-bottom: 5px;"> | |
| <uie:ToolbarToggle name="settings-tab" text="Settings" value="true" /> | |
| <uie:ToolbarToggle name="tools-tab" text="Tools" /> | |
| </uie:Toolbar> | |
| <!-- Main Workspace (Tabbed Content) --> | |
| <ui:VisualElement name="main-workspace" class="main-workspace"> | |
| <!-- Settings Panel --> | |
| <ui:VisualElement name="settings-panel"> | |
| <ui:VisualElement name="settings-section-placeholder" /> | |
| <ui:VisualElement name="connection-section-placeholder" /> | |
| <ui:VisualElement name="client-config-section-placeholder" /> | |
| </ui:VisualElement> | |
| <!-- Tools Panel --> | |
| <ui:VisualElement name="tools-panel" style="display: none;"> | |
| <ui:VisualElement name="tools-section-placeholder" /> | |
| </ui:VisualElement> | |
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uxml around lines 24–43,
the toolbar toggles can both be off leaving blank content; set the Settings
toggle as selected by default in the UXML (e.g., mark the Settings ToolbarToggle
as checked/selected) and in the accompanying editor C# code enforce mutual
exclusivity by wiring the toggles so when one is turned on the other is turned
off and by ensuring on initialization at least one toggle is active (select
Settings if none).
| for name in ( | ||
| "uvicorn", "uvicorn.error", "uvicorn.access", | ||
| "starlette", | ||
| "docket", "docket.worker", | ||
| "fastmcp", | ||
| ): | ||
| lg = logging.getLogger(name) | ||
| lg.setLevel(logging.WARNING) # ERROR if still too chatty | ||
| lg.propagate = False # prevent duplicate root logs | ||
| # If the rotating file handler was successfully created, attach it | ||
| if '_fh' in globals() and _fh not in lg.handlers: | ||
| lg.addHandler(_fh) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the project dependencies file
find . -name "requirements*.txt" -o -name "pyproject.toml" -o -name "poetry.lock" -o -name "setup.py" | head -20Repository: CoplayDev/unity-mcp
Length of output: 99
🏁 Script executed:
cat MCPForUnity/Server~/pyproject.tomlRepository: CoplayDev/unity-mcp
Length of output: 721
🏁 Script executed:
# Check for docket references in main.py
rg "docket" MCPForUnity/Server~/src/main.py -A 2 -B 2Repository: CoplayDev/unity-mcp
Length of output: 218
🏁 Script executed:
# Search for docket imports/usage in the entire codebase
rg "import.*docket|from.*docket" MCPForUnity/Server~/Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Check what loggers fastmcp actually uses by searching its documentation
# First, let's verify starlette is a transitive dependency and check actual logger names
python3 << 'EOF'
import sys
# Check if fastapi brings in starlette
try:
import fastapi
import starlette
print("✓ starlette is available (transitive via fastapi)")
except ImportError as e:
print(f"✗ starlette import failed: {e}")
# Check if docket is available
try:
import docket
print("✓ docket is available")
except ImportError:
print("✗ docket is NOT available (not in dependencies)")
# Check versions
try:
import fastmcp
print(f"fastmcp version: {getattr(fastmcp, '__version__', 'unknown')}")
except:
print("fastmcp version: unknown")
try:
import uvicorn
print(f"uvicorn version: {getattr(uvicorn, '__version__', 'unknown')}")
except:
print("uvicorn version: unknown")
try:
import starlette
print(f"starlette version: {getattr(starlette, '__version__', 'unknown')}")
except:
print("starlette version: unknown")
EOFRepository: CoplayDev/unity-mcp
Length of output: 238
🏁 Script executed:
# Search for logger instantiations in the main.py file and surrounding context
rg "getLogger" MCPForUnity/Server~/src/main.py -B 5 -A 5Repository: CoplayDev/unity-mcp
Length of output: 2047
🏁 Script executed:
# Search for what loggers fastmcp actually instantiates
# Check fastmcp source or common patterns
rg "logging\.getLogger|getLogger" -t py --max-count=30 | grep -i "fastmcp\|uvicorn\|starlette" | head -20Repository: CoplayDev/unity-mcp
Length of output: 45
🌐 Web query:
fastmcp logging logger names getLogger 2.13
💡 Result:
Do you mean FastMCP (Python) logging (fastmcp.utilities.logging.get_logger which prefixes names with "FastMCP.") — or are you asking about using getLogger/logger names in Scala (Scala 2.13) with SLF4J/Log4j? Please clarify which one you need.
🌐 Web query:
uvicorn 0.38.0 logging getLogger logger names
💡 Result:
Common logger names used by Uvicorn (including v0.38.0) are:
- uvicorn
- uvicorn.error
- uvicorn.access
- uvicorn.asgi
You can configure or get them with logging.getLogger("uvicorn"), logging.getLogger("uvicorn.error"), etc. [1][2]
Sources:
[1] Uvicorn logging examples/docs (formatters, loggers: "uvicorn", "uvicorn.error", "uvicorn.access").
[2] StackOverflow discussion describing Uvicorn's internal loggers including "uvicorn.asgi" and behavior of "uvicorn.error".
🌐 Web query:
starlette 0.50 logging getLogger logger names
💡 Result:
Short answer — logger names you’ll commonly use with Starlette 0.50:
-
Uvicorn server loggers: "uvicorn", "uvicorn.error", "uvicorn.access" (and variants used internally like "uvicorn.asgi"). Use logging.getLogger("uvicorn.error") or logging.getLogger("uvicorn.access") to access them. [1][2]
-
Starlette module loggers: Starlette code uses normal Python module loggers (logging.getLogger(name)), so logger names are the module paths under the starlette package (e.g. "starlette.applications", "starlette.routing", etc.) — you can access them with logging.getLogger("starlette") or logging.getLogger("starlette.") or get the module logger by name. [3]
How to inspect at runtime:
- List/print existing logger objects: import logging; print(logging.root.manager.loggerDict) or call logging.getLogger(name) for the names above. This is the usual way to find and configure them programmatically. [2][3]
Sources:
- Starlette project (repo / releases). [Kludex/starlette on GitHub]. [3]
- Uvicorn / community docs and examples describing uvicorn loggers ("uvicorn", "uvicorn.error", "uvicorn.access"). [1][2]
If you want, I can show exact code examples to configure or silence any of these loggers.
🌐 Web query:
fastmcp 2.13 logging getLogger logger names
💡 Result:
Short answer: FastMCP doesn’t invent a new logger system — use Python’s logging names. The utilities expose a helper get_logger(name) that simply returns a standard logging.Logger (docs say the name will be nested under the FastMCP namespace; the implementation delegates to logging.getLogger). For server→client logs the server may include a logger name in the incoming LogMessage (message.logger) which the client log_handler can prefix or pass to logging.getLogger.
How to use:
- On the server side, get a logger via fastmcp.utilities.logging.get_logger("YourModule") or logging.getLogger("FastMCP.YourModule"). That returns a standard logging.Logger. [1][4]
- On the client side, when handling server log messages, inspect message.logger (may be None) and forward to Python logging (e.g. logging.getLogger(message.logger or "fastmcp.server")). The docs show forwarding MCP LogMessage.data/msg + message.level → Python logging. [2][3]
References:
- fastmcp.utilities.logging (get_logger / configure_logging). [1][3]
- Server logging / LogMessage docs (message.logger and example log_handler). [2]
🏁 Script executed:
# Final verification - check if there are any other logger configurations we should know about
cat MCPForUnity/Server~/src/main.py | grep -A 20 "for name in"Repository: CoplayDev/unity-mcp
Length of output: 682
Remove the non-existent docket logger configuration; it is not a project dependency.
The logger suppression targets docket and docket.worker, but these are not listed in pyproject.toml dependencies and the module is never imported. Configuring loggers for a library that is not in use is unnecessary. Remove these two entries from the logger configuration loop.
Additionally, consider adding uvicorn.asgi to the uvicorn loggers, as it is used internally by uvicorn 0.38.0.
🤖 Prompt for AI Agents
In MCPForUnity/Server~/src/main.py around lines 414 to 425, remove the
non-existent "docket" and "docket.worker" entries from the logger names tuple
and add "uvicorn.asgi" to the uvicorn-related logger names; keep the rest of the
loop logic the same (setLevel(logging.WARNING), propagate=False, and attach the
existing rotating file handler if present).
| from services.tools.utils import ( | ||
| parse_json_payload, | ||
| apply_edits_locally, | ||
| normalize_script_locator, | ||
| find_best_anchor_match | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
LGTM! Good consolidation of shared utilities.
Moving edit application and anchor matching logic to a shared utils module eliminates code duplication and improves maintainability. The migration is complete and consistent across all call sites.
Also applies to: 141-141, 420-421, 569-570, 614-615, 676-676, 689-689
🤖 Prompt for AI Agents
In MCPForUnity/Server~/src/services/tools/script_apply_edits.py around lines 10
to 15 (and similarly at lines 141, 420-421, 569-570, 614-615, 676, 689), the
imports have been correctly consolidated to use shared utilities; no functional
changes are required—keep the current import list (parse_json_payload,
apply_edits_locally, normalize_script_locator, find_best_anchor_match) and
ensure any other call sites listed use these same utility functions to avoid
duplication.
| try: | ||
| test_improved_anchor_matching() | ||
| print("test_improved_anchor_matching passed") | ||
| success1 = True | ||
| except Exception as e: | ||
| print(f"test_improved_anchor_matching failed: {e}") | ||
| success1 = False | ||
|
|
||
| print("\n" + "="*60) | ||
| print("Comparing old vs new behavior...") | ||
| success2 = test_old_vs_new_matching() | ||
| try: | ||
| test_old_vs_new_matching() | ||
| print("test_old_vs_new_matching passed") | ||
| success2 = True | ||
| except Exception as e: | ||
| print(f"test_old_vs_new_matching failed: {e}") | ||
| success2 = False | ||
|
|
||
| print("\n" + "="*60) | ||
| print("Testing _apply_edits_locally with improved matching...") | ||
| success3 = test_apply_edits_with_improved_matching() | ||
| print("Testing apply_edits_locally with improved matching...") | ||
| try: | ||
| test_apply_edits_with_improved_matching() | ||
| print("test_apply_edits_with_improved_matching passed") | ||
| success3 = True | ||
| except Exception as e: | ||
| print(f"test_apply_edits_with_improved_matching failed: {e}") | ||
| success3 = False |
There was a problem hiding this comment.
Ruff BLE001: avoid blind except Exception in the __main__ harness (or suppress explicitly).
If you want “keep going” behavior, splitting AssertionError + suppressing BLE001 is usually clean:
- except Exception as e:
+ except AssertionError as e:
+ print(f"test_improved_anchor_matching failed: {e}")
+ success1 = False
+ except Exception as e: # noqa: BLE001
print(f"test_improved_anchor_matching failed: {e}")
success1 = False(Apply similarly to the other two blocks.)
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
138-138: Do not catch blind exception: Exception
(BLE001)
148-148: Do not catch blind exception: Exception
(BLE001)
158-158: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
MCPForUnity/Server~/tests/integration/test_improved_anchor_matching.py around
lines 134-160: replace the blind "except Exception" blocks with a two-tier
catch: first catch AssertionError to mark the test as failed (print the failure
and set successX = False) and continue, then catch any other Exception and
re-raise it (or let it propagate) so unexpected errors aren’t silently
swallowed; apply the same change to the other two try/except blocks in this
section.
| const newPath = | ||
| extraPaths | ||
| .filter((p) => (p && !p.startsWith(path.sep)) || p.length > 1) | ||
| .join(path.delimiter) + | ||
| path.delimiter + | ||
| (process.env.PATH || ""); |
There was a problem hiding this comment.
Filter relative paths to prevent PATH pollution.
If environment variables (localAppData, userProfile, homeDir) are empty, path.join() produces relative paths (e.g., "Programs/uv", ".cargo/bin"). The current filter (p && !p.startsWith(path.sep)) || p.length > 1 allows these relative paths through, adding meaningless entries to PATH.
Apply this diff to filter only absolute paths:
const newPath =
extraPaths
- .filter((p) => (p && !p.startsWith(path.sep)) || p.length > 1)
+ .filter((p) => p && path.isAbsolute(p))
.join(path.delimiter) +
path.delimiter +
(process.env.PATH || "");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const newPath = | |
| extraPaths | |
| .filter((p) => (p && !p.startsWith(path.sep)) || p.length > 1) | |
| .join(path.delimiter) + | |
| path.delimiter + | |
| (process.env.PATH || ""); | |
| const newPath = | |
| extraPaths | |
| .filter((p) => p && path.isAbsolute(p)) | |
| .join(path.delimiter) + | |
| path.delimiter + | |
| (process.env.PATH || ""); |
🤖 Prompt for AI Agents
In MCPForUnity/Server~/wrapper.js around lines 40 to 45, the filter currently
lets relative paths through and pollutes PATH; change the filter to only allow
truthy absolute paths (e.g., replace the existing predicate with one that checks
p is truthy and path.isAbsolute(p)) so only absolute extraPaths are joined into
newPath.
|
Hi @choej2303 -- thank you for enthusiastically working on this issue. We're always happy to have contributors, and using AI to help code is generally fine. But this pull request is just way too big for us to approve. You're proposing we add 4,300 lines of code, and delete 3,000, including major changes to some of the most fragile and important parts of the server configuration systems. And the main issue you're trying to solve, fixing the windows antigravity configuration, could probably be solved in < 50 lines of code. But again, thanks for your time and effort in submitting this! |
Thank you for the kind words, and I fully understand your concerns regarding this issue. my week-long attempt to implement the gajzzs:main's mcp_wrapper.py solution ended in failure, with intermittent connection issues that mimicked an unstable HTTP link. However, I have faith that your team will definitely find a different and successful outcome. |
2 participants
antigravity did all the work. so, if you have any questions? i need to ask LLM first. haha..
umm.. just test it. it'll work.
antigravity will work in stdio connection.
@coderabbitai generate docstrings
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.