Feat: Local-only package resolution + Claude CLI resolver; quieter installer logs #206
Conversation
…stall logs; guarded auto-registration
WalkthroughAdds ExecPath and ServerPathResolver helpers, centralizes embedded server source lookup, refactors UnityMcpEditorWindow to use unified tool/path resolution and execution, hardens ServerInstaller error handling and uv discovery, and bumps package version to 2.0.1. Changes
Sequence Diagram(s)sequenceDiagram
participant UE as UnityMcpEditorWindow
participant EP as ExecPath
participant SPR as ServerPathResolver
participant Claude as Claude CLI
participant UV as uv
UE->>EP: ResolveClaude()
alt Claude found
UE->>SPR: TryFindEmbeddedServerSource()
SPR-->>UE: srcDir or null
UE->>EP: ResolveUv()
EP-->>UE: uvPath or null
UE->>EP: TryRun(claude, mcp add UnityMCP -- "<uv>" run --directory "<srcDir>" server.py)
EP->>Claude: Execute command (with prepended PATH)
Claude->>UV: invokes uv to run server.py
Claude-->>EP: exit code
EP-->>UE: success/failure
else Claude missing
UE-->>UE: Skip auto-setup
end
sequenceDiagram
participant SI as ServerInstaller
participant SPR as ServerPathResolver
SI->>SI: Attempt server install
alt install throws
SI->>SI: Check server.py at server path
alt exists
SI-->>SI: Log warning and return (tolerant)
else
SI->>SPR: TryFindEmbeddedServerSource()
alt found
SI-->>SI: Log warning and return (tolerant)
else
SI-->>SI: Rethrow/propagate error
end
end
else install succeeds
SI-->>SI: Return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (5)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
198-218: Potential deadlock reading process streams synchronouslyReading StandardOutput and StandardError to the end before the process exits can deadlock if one buffer fills. Use async reads or read both after the process exits. This is especially risky for verbose uv sync.
Apply this diff to use async readers safely:
- using var p = System.Diagnostics.Process.Start(psi); - string stdout = p.StandardOutput.ReadToEnd(); - string stderr = p.StandardError.ReadToEnd(); - p.WaitForExit(60000); + using var p = System.Diagnostics.Process.Start(psi); + var stdoutSb = new System.Text.StringBuilder(); + var stderrSb = new System.Text.StringBuilder(); + p.OutputDataReceived += (_, e) => { if (e.Data != null) stdoutSb.AppendLine(e.Data); }; + p.ErrorDataReceived += (_, e) => { if (e.Data != null) stderrSb.AppendLine(e.Data); }; + p.BeginOutputReadLine(); + p.BeginErrorReadLine(); + bool exited = p.WaitForExit(60000); + if (!exited) { try { p.Kill(); } catch { } } + // Ensure async event handlers drain fully + p.WaitForExit(); + string stdout = stdoutSb.ToString(); + string stderr = stderrSb.ToString();Add this using at the top of the file:
using System.Text;UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (4)
1370-1448: Code duplication detected with ManageScript.cs FindUvPath.This
FindUvPathmethod appears to duplicate significant logic fromUnityMcpBridge/Editor/Tools/ManageScript.cs. Since the newExecPath.ResolveUv()delegates toServerInstaller.FindUvPath(), this method should be updated to use that centralized implementation instead.private string FindUvPath() { - string uvPath = null; - - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - uvPath = FindWindowsUvPath(); - } - else - { - // macOS/Linux paths... [truncated for brevity] - } - - // If no specific path found, fall back to using 'uv' from PATH - if (uvPath == null) - { - // Test if 'uv' is available in PATH by trying to run it - string uvCommand = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "uv.exe" : "uv"; - if (IsValidUvInstallation(uvCommand)) - { - uvPath = uvCommand; - } - } - - if (uvPath == null) - { - UnityEngine.Debug.LogError("UV package manager not found! Please install UV first:\n" + - "• macOS/Linux: curl -LsSf https://astral.sh/uv/install.sh | sh\n" + - "• Windows: pip install uv\n" + - "• Or visit: https://docs.astral.sh/uv/getting-started/installation"); - return null; - } - - return uvPath; + return ExecPath.ResolveUv(); }
1486-1648: Remove redundant FindWindowsUvPath method.This method becomes unnecessary after refactoring
FindUvPathto useExecPath.ResolveUv(). The centralized implementation inServerInstaller.FindUvPath()should handle all UV discovery logic.- private string FindWindowsUvPath() - { - // [entire method implementation removed for brevity] - }
1450-1484: Remove redundant IsValidUvInstallation method.This validation method is likely duplicated in the centralized UV resolution logic. Verify if
ServerInstallerhas equivalent validation and remove this duplicate.#!/bin/bash # Description: Check for UV validation method duplication across the codebase # Search for UV validation methods rg -A 10 "IsValidUv|ValidateUv" --type cs # Look for UV version checking patterns rg -A 5 "uv.*--version|--version.*uv" --type cs
1650-1736: Remove redundant FindClaudeCommand method.This method appears to duplicate functionality that's now centralized in
ExecPath.ResolveClaude(). The Claude resolution should use the centralized implementation to avoid code duplication and maintenance burden.- private string FindClaudeCommand() - { - // [entire method implementation removed for brevity] - }Any callers of this method should be updated to use
ExecPath.ResolveClaude()instead.
🧹 Nitpick comments (5)
UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
41-60: Package-based resolution is correct; consider result cachingFindForAssembly + GetAllRegisteredPackages provides robust, local-only resolution. If this method is called frequently (EditorWindow redraws, etc.), memoize the successful srcPath for the session to avoid repeated scans.
113-118: Improve legacy-id warning with more actionable contextInclude the resolved package path to help users locate the manifest that needs updating.
Apply this diff to enrich the message:
- Debug.LogWarning( - "UnityMCP: Detected legacy package id 'com.justinpbarnett.unity-mcp'. " + - "Please update Packages/manifest.json to 'com.coplaydev.unity-mcp' to avoid future breakage."); + Debug.LogWarning( + $"UnityMCP: Detected legacy package id '{LegacyId}' at '{p.resolvedPath}'. " + + $"Please update Packages/manifest.json to '{CurrentId}' to avoid future breakage.");UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
4-4: Remove unused usingSystem.Reflection is not used in this file.
Apply this diff:
-using System.Reflection;UnityMcpBridge/Editor/Helpers/ExecPath.cs (1)
19-22: Consider making EditorPrefs access more resilient.While the try-catch block provides basic protection, consider logging warnings for unexpected failures to aid debugging.
try { string pref = EditorPrefs.GetString(PrefClaude, string.Empty); if (!string.IsNullOrEmpty(pref) && File.Exists(pref)) return pref; } - catch { } + catch (Exception ex) + { + UnityEngine.Debug.LogWarning($"Failed to read Claude CLI preference: {ex.Message}"); + }UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
1307-1313: Consider making Claude CLI requirement more explicit.While the error message is clear, consider providing more actionable guidance or a link to installation instructions when Claude CLI is not found.
- UnityEngine.Debug.LogError("UnityMCP: Claude CLI not found. Set a path in this window or install the CLI, then try again."); + UnityEngine.Debug.LogError("UnityMCP: Claude CLI not found. Install via 'npm install -g @anthropic-ai/claude-cli' or set a custom path in this window, then try again.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
UnityMcpBridge/Editor/Helpers/ExecPath.cs(1 hunks)UnityMcpBridge/Editor/Helpers/ExecPath.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/ServerInstaller.cs(5 hunks)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs(1 hunks)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs.meta(1 hunks)UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs(4 hunks)UnityMcpBridge/package.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)
UnityMcpBridge/Editor/Helpers/ExecPath.cs (2)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
UnityMcpBridge(20-1029)UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
FindUvPath(1370-1448)
🔇 Additional comments (16)
UnityMcpBridge/package.json (1)
3-3: Version bump to 2.0.1 — LGTMNo functional risk here. Matches the PR scope.
UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (1)
20-35: Dev-layout discovery looks solidCovers common repo layouts, normalizes to full paths, and validates by checking server.py existence. Good defensive approach.
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (3)
46-56: Graceful fallback when server already exists — aligned with PR objectiveSwitching from hard error to warning when a usable install or embedded source exists will reduce noise and improve UX.
128-129: Centralizing embedded-source detection — good refactorDelegating to ServerPathResolver reduces duplication and keeps resolution logic in one place.
230-230: Expose FindUvPath as internal — appropriate for ExecPath reuseMaking FindUvPath internal enables reuse without expanding the public API surface beyond the assembly. Good.
UnityMcpBridge/Editor/Helpers/ExecPath.cs (6)
1-173: LGTM! Well-designed cross-platform CLI resolution utility.This is a well-structured utility class that centralizes external CLI tool resolution and execution. The implementation handles platform-specific paths correctly and provides robust fallbacks. The code is clean, well-organized, and follows good practices.
86-127: Process execution implementation is robust.The TryRun method handles process execution well with proper timeout, output capture, and PATH manipulation. The exception handling and resource disposal are appropriate.
109-115: PATH environment variable handling looks correct.The PATH prepending logic correctly handles both empty and existing PATH values using the appropriate path separator.
130-148: Which method implementation is solid.The Unix-style which command implementation correctly modifies the PATH environment and validates the output. The timeout handling is appropriate.
152-171: Where method handles Windows command properly.The Windows where command implementation correctly parses multiple results and validates file existence. Good use of string splitting with appropriate options.
37-41: Conditional compilation aligns with runtime checks; no change needed.The
#if UNITY_EDITOR_OSX || UNITY_EDITOR_LINUXguards simply ensure theWhichcall is compiled into both macOS and Linux Editor builds, while the surroundingif (RuntimeInformation.IsOSPlatform(...))blocks ensure it only executes on the correct OS at runtime. The Windows block similarly uses#if UNITY_EDITOR_WINDOWS. This setup works as intended across all Unity Editor platforms. No updates required.UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (5)
69-71: Improved Claude CLI availability check enhances robustness.The change to check Claude CLI availability before auto-setup prevents unnecessary execution attempts when the CLI is not available. This aligns well with the PR objectives to fix PATH/tool discovery issues.
495-501: Enhanced auto-setup logic with proper Claude CLI validation.The additional check for Claude CLI presence before attempting registration is a good improvement that prevents failed registration attempts when the CLI is not available.
991-995: Centralized server path resolution improves maintainability.The migration to use
ServerPathResolver.TryFindEmbeddedServerSource()centralizes the embedded server discovery logic, which is consistent with the PR objectives for local-only package resolution.
1307-1338: Claude registration flow significantly improved.The refactored registration method using
ExecPathutilities provides several improvements:
- Proper Claude CLI path resolution with clear error messaging
- Platform-aware PATH handling for Node.js discovery
- Unified execution pattern with better error handling
- Consistent status updates and logging
This addresses the core issues mentioned in the PR objectives regarding Unity GUI PATH problems on macOS/Apple Silicon.
1340-1368: Unregistration flow properly modernized.The unregistration method follows the same improved pattern as registration, using
ExecPathutilities for consistent CLI resolution and execution. The error handling and status updates are appropriate.
…nd RepairPythonEnvironment; use EnvironmentVariables for PATH; prepend Unix PATH only on macOS/Linux; fix duplicate .meta GUIDs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
355-370: Resolved: Use EnvironmentVariables for PATH on ProcessStartInfo; consider centralizing the prepend listThis addresses the prior review note about ProcessStartInfo.Environment vs EnvironmentVariables; thanks for fixing it. The PATH prepend set matches typical macOS/Linux user locations and should improve discovery from Unity’s GUI env.
To avoid drift, consider centralizing these common bin directories in a helper (e.g., ExecPath) so both uv and other tool lookups share one source of truth.
🧹 Nitpick comments (2)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
47-55: Soft-fail behavior is good; consider explicitly tracking the embedded-fallback stateThis aligns with the PR goal to warn (not error) when a usable server exists. One subtlety: PackageInstaller sets the “installed” flag after EnsureServerInstalled returns; with this soft-fail path, the flag may be set even if nothing was copied, which suppresses future install attempts. If that’s intentional, consider also recording that we’re in an “embedded fallback” mode for diagnostics and future UX decisions.
Suggested addition within this block to set a flag (optional but helpful):
if (hasInstalled || TryGetEmbeddedServerSource(out _)) { + try { EditorPrefs.SetBool("UnityMCP.UsingEmbeddedServerFallback", !hasInstalled); } catch { } Debug.LogWarning($"UnityMCP: Using existing server; skipped install. Details: {ex.Message}"); return; }
- Verifications to consider:
- Confirm downstream code paths that assume GetServerPath() can gracefully fall back to embedded when UsingEmbeddedServerFallback is true (RepairPythonEnvironment already does this).
- If you prefer to keep first-run retry behavior, you could avoid marking “installed” in PackageInstaller when UsingEmbeddedServerFallback is true.
210-240: Robust async process execution; confirm C# language level for ‘using var’ in supported Unity versionsThe async read + timeout handling is solid and addresses common deadlock pitfalls with stdout/stderr. One compatibility guardrail: using declarations (using var …) require C# 8. Please confirm the minimum Unity version for this package ensures C# 8 support across your target audience. If you need broader compatibility (e.g., Unity 2019), swap to a classic using (…) { … } pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
UnityMcpBridge/Editor/Helpers/ExecPath.cs(1 hunks)UnityMcpBridge/Editor/Helpers/ExecPath.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/ServerInstaller.cs(6 hunks)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs.meta(1 hunks)UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs.meta
🚧 Files skipped from review as they are similar to previous changes (3)
- UnityMcpBridge/Editor/Helpers/ExecPath.cs.meta
- UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs
- UnityMcpBridge/Editor/Helpers/ExecPath.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
UnityMcpBridge/Editor/Helpers/PackageInstaller.cs (3)
InstallServerOnFirstLoad(24-41)UnityMcpBridge(4-43)InitializeOnLoad(9-42)
🔇 Additional comments (3)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (3)
4-5: OK: Added using directives for new functionalitySystem.Text is needed for StringBuilder usage below. No issues spotted.
129-129: Good centralization via ServerPathResolver.TryFindEmbeddedServerSourceDelegating to ServerPathResolver removes duplicated search logic and makes maintenance easier.
254-254: Visibility change to internal is appropriateExposing FindUvPath internally matches the PR’s intent to reuse this from ExecPath. Looks good.
…gured using ExecPath on all OSes
1 participant
Summary
Should fix #205, #203
What changed
Editor/Helpers/ServerPathResolver.csPackageInfo.FindForAssembly(...)PackageInfo.GetAllRegisteredPackages()Client.List()via#if.ServerInstaller.TryGetEmbeddedServerSource(...)to use shared resolver.UnityMcpEditorWindow.FindPackagePythonDirectory()to shared resolver.Editor/Helpers/ExecPath.csResolveClaude()finds CLI via EditorPrefs/env/candidates/which/where.ResolveUv()reusesServerInstaller.FindUvPath().TryRun(...)runs child procs with optional PATH prepend; captures stdout/stderr.RegisterWithClaudeCode(...)andUnregisterWithClaudeCode()now useExecPathand inject PATH so/usr/bin/env nodeworks in Unity GUI env.EnsureServerInstalled()warns (not error) if installation failed but a usableserver.pyexists.FindUvPath()checks common locations directly, thenwhich/where, then PATH scan.~/.local/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/binwhen callingwhich uvto match user shell PATH..metafor new scripts; bumped package version to2.0.1.Why
env: node: No such file or directory, missinguv).How to verify
2.0.1installed from branchfeat/local-resolution-and-claude-cli.server.pyexists.EditorPrefs.DeleteKey("UnityMCP.UvPath"), temporarily move uv out of PATH;FindUvPath()tries candidates → which (with PATH prepend) → PATH scan; restore uv and confirm success.Compatibility
PackageInfo.FindForAssemblyandGetAllRegisteredPackages.Client.List()via#if UNITY_2021_2_OR_NEWER.Notes
UnityMCP.ClaudeCliPath(EditorPrefs)UnityMCP.UvPath(EditorPrefs)Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores