fix: resolve UV path override not being detected in System Requirements .fix: The configuration automatically added by Claude Code is broken. #550 #546
Conversation
Fixes CoplayDev#538 The System Requirements panel showed "UV Package Manager: Not Found" even when a valid UV path override was configured in Advanced Settings. Root cause: PlatformDetectorBase.DetectUv() only searched PATH with bare command names ("uvx", "uv") and never consulted PathResolverService which respects the user's override setting. Changes: - Refactor DetectUv() to use PathResolverService.GetUvxPath() which checks override path first, then system PATH, then falls back to "uvx" - Add TryValidateUvExecutable() to verify executables by running --version instead of just checking File.Exists - Prioritize PATH environment variable in EnumerateUvxCandidates() for better compatibility with official uv install scripts - Fix process output read order (ReadToEnd before WaitForExit) to prevent potential deadlocks Co-Authored-By: ChatGLM 4.7 <[email protected]>
📝 WalkthroughWalkthroughRefactors UV/UVX and Python detection to respect configured overrides and centralizes path resolution via PathResolverService and ExecPath, replacing shell/which-based lookups with augmented-PATH ExecPath validation and updating detectors, settings UI, and related helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Detector as PlatformDetector
participant PathService as PathResolverService
participant Exec as ExecPath
participant OS as OS/Filesystem
Detector->>PathService: GetUvxPath() (respects overrides)
alt override present
PathService->>Exec: TryValidateUvxExecutable(overridePath, augmentedPATH)
Exec->>OS: spawn process (--version) with augmented PATH
OS-->>Exec: stdout/stderr
Exec-->>PathService: success + version
PathService-->>Detector: validated override path + version
else no override
PathService->>Exec: ResolveUvxFromSystem() -> EnumerateCommandCandidates
Exec->>OS: FindInPath(candidate, augmented PATH)
OS-->>Exec: fullPath or null
Exec-->>PathService: candidate path
PathService->>Exec: TryValidateUvxExecutable(candidate, augmentedPATH)
Exec->>OS: spawn process (--version)
OS-->>Exec: stdout/stderr
Exec-->>PathService: success + version
PathService-->>Detector: validated system path + version
end
Detector->>Detector: update DependencyStatus (IsAvailable, Version, Path, Details)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
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/MacOSPlatformDetector.cs (1)
203-240: Process output handling: still misses stderr + timeout outcome; version parsing is a bit too loose.
You redirect stderr but never read it; ifuv/uvx --versionwrites to stderr on some setups, detection will incorrectly fail. AlsoWaitForExit(5000)return value is ignored (ExitCode access can be risky if the process didn’t exit). Finally,Substring(spaceIndex + 1)can capture extra tokens beyond the version.Proposed fix: read stderr too, honor timeout, and parse only the next token as version
using var process = Process.Start(psi); if (process == null) return false; - - string output = process.StandardOutput.ReadToEnd().Trim(); - process.WaitForExit(5000); + // Read both streams to avoid missing output when tools write version to stderr. + string stdout = process.StandardOutput.ReadToEnd(); + string stderr = process.StandardError.ReadToEnd(); + + if (!process.WaitForExit(5000)) + return false; + + string output = (string.IsNullOrWhiteSpace(stdout) ? stderr : stdout).Trim(); if (process.ExitCode == 0 && (output.StartsWith("uv ") || output.StartsWith("uvx "))) { // Extract version: "uvx 0.9.18" -> "0.9.18" int spaceIndex = output.IndexOf(' '); if (spaceIndex >= 0) { - version = output.Substring(spaceIndex + 1).Trim(); + var remainder = output.Substring(spaceIndex + 1).Trim(); + var nextSpace = remainder.IndexOf(' '); + version = (nextSpace >= 0 ? remainder.Substring(0, nextSpace) : remainder).Trim(); fullPath = uvPath; return true; } }
🧹 Nitpick comments (3)
MCPForUnity/Editor/Services/PathResolverService.cs (1)
342-371: Consider renaming for clarity.
FindExecutableInPathappears generic but relies onEnumerateUvxCandidates()(line 351), which is hardcoded to search for "uvx" executables (line 155). While this works for current usage (only called with "uvx" fromGetUvxPath), the method signature suggests broader applicability.💡 Optional: Rename to reflect uvx-specific behavior
Consider renaming to
FindUvxInPath()to match its actual scope, or refactorEnumerateUvxCandidatesto accept a command name parameter if you need true generality.-private string FindExecutableInPath(string commandName) +private string FindUvxInPath() { try { - string exeName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && !commandName.EndsWith(".exe") - ? commandName + ".exe" - : commandName; + string exeName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "uvx.exe" : "uvx"; foreach (string candidate in EnumerateUvxCandidates()) { if (!string.IsNullOrEmpty(candidate) && File.Exists(candidate)) { - string candidateName = Path.GetFileName(candidate); - if (candidateName.Equals(exeName, StringComparison.OrdinalIgnoreCase) || - candidateName.Equals(commandName, StringComparison.OrdinalIgnoreCase)) - { - return candidate; - } + return candidate; } } }And update the call site:
-string fullPath = FindExecutableInPath(uvPath); +string fullPath = FindUvxInPath();MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
206-249: Refactor: Version parsing duplicates PathResolverService logic with less robustness.The version extraction logic (lines 231-240) is simpler than
PathResolverService.TryValidateUvExecutable(lines 314-330 in PathResolverService.cs) and doesn't handle parenthetical build info. For input"uvx 0.9.18 (Homebrew 2025-01-01)", this code would extract"0.9.18 (Homebrew 2025-01-01)"while PathResolverService extracts just"0.9.18".♻️ Refactor to use centralized validation
Consider calling
MCPServiceLocator.Paths.TryValidateUvExecutable()directly instead of duplicating the validation logic:private bool TryValidateUv(string uvPath, out string version, out string fullPath) { version = null; fullPath = null; - try - { - var psi = new ProcessStartInfo - { - FileName = uvPath, - Arguments = "--version", - UseShellExecute = false, - RedirectStandardOutput = true, - RedirectStandardError = true, - CreateNoWindow = true - }; - - psi.EnvironmentVariables["PATH"] = BuildAugmentedPath(); - - using var process = Process.Start(psi); - if (process == null) return false; - - string output = process.StandardOutput.ReadToEnd().Trim(); - process.WaitForExit(5000); - - if (process.ExitCode == 0 && (output.StartsWith("uv ") || output.StartsWith("uvx "))) - { - // Extract version: "uvx 0.9.18" -> "0.9.18" - int spaceIndex = output.IndexOf(' '); - if (spaceIndex >= 0) - { - version = output.Substring(spaceIndex + 1).Trim(); - fullPath = uvPath; - return true; - } - } - } - catch - { - // Ignore validation errors - } - - return false; + if (MCPServiceLocator.Paths.TryValidateUvExecutable(uvPath, out version)) + { + fullPath = uvPath; + return true; + } + return false; }Note: This assumes
PathResolverService.TryValidateUvExecutablecan handle the augmented PATH. If PATH augmentation is critical for Linux, you may need to enhancePathResolverServiceto accept environment variables or continue with the current approach but improve the version parsing.MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
109-133: Potential “Path = uv”/“Path = uvx” ambiguity after successful detection.
When validating viaTryValidateUv("uv"…)/TryValidateUv("uvx"…),fullPathends up being the input token, not necessarily an absolute path—if anything downstream expects an absolute executable path, this can cause mismatches (and could reintroduce “System Requirements says OK but runtime fails”). Consider resolving to an absolute path when the candidate is a bare command.Proposed tweak: resolve bare command to an absolute path before storing
- if (TryValidateUv("uv", out string version, out string fullPath) || - TryValidateUv("uvx", out version, out fullPath)) + if (TryValidateUv("uv", out string version, out string fullPath) || + TryValidateUv("uvx", out version, out fullPath)) { + // If we validated via bare command, try to resolve an absolute path for display/consumers. + if (!string.IsNullOrEmpty(fullPath) && fullPath == "uv" && TryFindInPath("uv", out var resolvedUv)) + fullPath = resolvedUv; + else if (!string.IsNullOrEmpty(fullPath) && fullPath == "uvx" && TryFindInPath("uvx", out var resolvedUvx)) + fullPath = resolvedUvx; + status.IsAvailable = true; status.Version = version; status.Path = fullPath; status.Details = $"Found uv {version} in PATH"; status.ErrorMessage = null; return status; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.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/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.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/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
📚 Learning: 2025-08-13T21:00:17.228Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 209
File: UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:279-281
Timestamp: 2025-08-13T21:00:17.228Z
Learning: WinGet per-user Links directory is %LOCALAPPDATA%\Microsoft\WinGet\Links and machine Links directory is %ProgramFiles%\WinGet\Links based on official Microsoft documentation and winget --info command output.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.cs
🧬 Code graph analysis (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (3)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/IPathResolverService.cs (2)
GetUvxPath(12-12)TryValidateUvExecutable(70-70)MCPForUnity/Editor/Services/PathResolverService.cs (2)
GetUvxPath(22-45)TryValidateUvExecutable(260-340)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (3)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/IPathResolverService.cs (2)
GetUvxPath(12-12)TryValidateUvExecutable(70-70)MCPForUnity/Editor/Services/PathResolverService.cs (2)
GetUvxPath(22-45)TryValidateUvExecutable(260-340)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)
MCPForUnity/Editor/Services/PathResolverService.cs (1)
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
TryValidateUvExecutable(70-70)
🔇 Additional comments (9)
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
64-70: LGTM! Clean interface extension.The new
TryValidateUvExecutablemethod properly centralizes UV validation logic and has clear documentation. This aligns with the PR objective to consolidate path resolution and validation in PathResolverService.MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (1)
201-209: LGTM! Validation logic correctly uses centralized path resolution.The unconditional call to
GetUvxPath()followed byTryValidateUvExecutable()properly respects path overrides and provides accurate validation status. This fixes the core issue where override paths weren't being validated in the UI.MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (1)
20-53: LGTM! Base detector now properly respects path overrides.The refactored
DetectUv()method correctly delegates toPathResolverServicefor path resolution and validation, which was the core fix needed for issue #538. The status messaging clearly distinguishes between override and system paths, and the error guidance points users to Advanced Settings.MCPForUnity/Editor/Services/PathResolverService.cs (2)
157-173: LGTM! PATH prioritization aligns with PR objectives.Moving PATH to Priority 1 in
EnumerateUvxCandidates()ensures the most common user configurations are checked first. The Windows bare command handling (lines 167-171) correctly accounts for PATH entries that may omit the.exeextension.
254-340: LGTM! Robust UV executable validation.The implementation correctly:
- Resolves bare commands to full paths before execution (lines 274-285)
- Reads stdout/stderr before
WaitForExit()to avoid deadlocks (lines 303-306) ✓- Handles both
"uv x.y.z"and"uvx x.y.z"version formats (lines 314-315) ✓- Strips parenthetical build info to extract clean version numbers (lines 323-326)
All key PR objectives for this method are satisfied.
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (2)
96-107: LGTM! Override handling correctly delegates to base implementation.The logic properly respects path overrides by delegating to
base.DetectUv()first (which usesPathResolverService), and returns early if an override is configured. This ensures consistent behavior with the centralized path resolution.
119-119: Good practice: Explicitly clearing error messages on success.Explicitly setting
status.ErrorMessage = nullwhen detection succeeds prevents stale error messages from confusing users. This is a thoughtful improvement.Also applies to: 133-133
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
7-7: Newusing MCPForUnity.Editor.Services;is appropriate (needed forMCPServiceLocator.Paths).
92-105: Override handling is correct and behaves as intended.When a user sets an override path, the system honors that explicit choice and does not fall back to PATH if the override is invalid or missing. This is the correct UX—overrides represent a deliberate user instruction to use a specific path, and silently falling back to PATH would undermine that intent. The code is well-documented with this behavior, and it's consistently applied across all platform detectors (macOS and Linux).
- Read both stdout and stderr when validating uv/uvx executables
- Respect WaitForExit timeout return value instead of ignoring it
- Fix version parsing to handle extra tokens like "(Homebrew 2025-01-01)"
- Resolve bare commands ("uv"/"uvx") to absolute paths after validation
- Rename FindExecutableInPath to FindUvxExecutableInPath for clarity
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @MCPForUnity/Editor/Services/PathResolverService.cs:
- Around line 260-340: TryValidateUvExecutable currently calls ReadToEnd() on
StandardOutput/StandardError before WaitForExit, which can deadlock and ignore
the timeout; change it to asynchronously read both streams (e.g., use
process.OutputDataReceived and process.ErrorDataReceived with StringBuilder
buffers and BeginOutputReadLine()/BeginErrorReadLine()), then call
process.WaitForExit(5000) and if it returns false call process.Kill() and return
false; only after the process has exited, stop async readers and read the
accumulated buffers into local strings for parsing; keep the existing logic that
checks process.ExitCode and parses version, and apply these changes inside
TryValidateUvExecutable (and still call FindUvxExecutableInPath for bare
commands).
- Around line 342-371: FindUvxExecutableInPath is misleading: it accepts a
generic commandName but only searches candidates from EnumerateUvxCandidates
(hardcoded to "uvx"), so callers asking for other commands (e.g., "uv") will
fail; fix by either (A) making the method truly generic—rename to
FindExecutableInPath or keep name and change EnumerateUvxCandidates to a
parameterized EnumerateCandidatesFor(commandName) and implement OS-aware
resolution (search PATH entries or call "where"/"which" per platform) and use
that in FindUvxExecutableInPath, or (B) make the method explicitly
uvx-specific—remove the commandName parameter and/or rename it to
ResolveUvxExecutableInPath/FindUvxOnlyExecutable and ensure GetUvxPath and
callers use the updated name; update all references to the changed symbol(s)
(FindUvxExecutableInPath, EnumerateUvxCandidates, GetUvxPath) accordingly.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Services/PathResolverService.cs (1)
157-209: PATH candidate enumeration: good priority order; consider WinGet Links + quoted PATH entries.On Windows, WinGet installs often surface executables via Links directories (per official
winget --infobehavior), which aren’t included here. Also, some PATH segments can be quoted (e.g.,"C:\Program Files\..."), soTrim('"')avoids missed matches.Proposed diff
--- a/MCPForUnity/Editor/Services/PathResolverService.cs +++ b/MCPForUnity/Editor/Services/PathResolverService.cs @@ foreach (string rawDir in pathEnv.Split(Path.PathSeparator)) { if (string.IsNullOrWhiteSpace(rawDir)) continue; - string dir = rawDir.Trim(); + string dir = rawDir.Trim().Trim('"'); yield return Path.Combine(dir, exeName); @@ else if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { // Priority 4: Windows-specific program directories string localAppData = Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData); string programFiles = Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles); if (!string.IsNullOrEmpty(localAppData)) { + // WinGet per-user Links directory + yield return Path.Combine(localAppData, "Microsoft", "WinGet", "Links", exeName); yield return Path.Combine(localAppData, "Programs", "uv", exeName); } if (!string.IsNullOrEmpty(programFiles)) { + // WinGet machine Links directory + yield return Path.Combine(programFiles, "WinGet", "Links", exeName); yield return Path.Combine(programFiles, "uv", exeName); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.cs
🧰 Additional context used
🧠 Learnings (4)
📚 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/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.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/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.cs
📚 Learning: 2025-08-13T21:00:17.228Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 209
File: UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:279-281
Timestamp: 2025-08-13T21:00:17.228Z
Learning: WinGet per-user Links directory is %LOCALAPPDATA%\Microsoft\WinGet\Links and machine Links directory is %ProgramFiles%\WinGet\Links based on official Microsoft documentation and winget --info command output.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
94-105: DetectUv base-first + override honoring looks correct.Returning early when base finds a valid override/system path keeps behavior consistent across platforms.
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs
Show resolved
Hide resolved
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs
Outdated
Show resolved
Hide resolved
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
Outdated
Show resolved
Hide resolved
Reviewer's GuideRefactors uv/uvx detection to go through PathResolverService with override-aware resolution and robust process execution, adds executable validation and version parsing, and unifies cross-platform PATH augmentation and CLI discovery for uv, Claude CLI, and Python in platform detectors and settings UI. Sequence diagram for uv detection via PathResolverService and platform detectorssequenceDiagram
participant UnityEditor
participant WindowsPlatformDetector
participant PlatformDetectorBase
participant PathResolverService
participant ExecPath
UnityEditor->>WindowsPlatformDetector: DetectUv()
WindowsPlatformDetector->>PlatformDetectorBase: base.DetectUv()
PlatformDetectorBase->>PathResolverService: GetUvxPath()
alt HasUvxPathOverride
PathResolverService->>PathResolverService: TryValidateUvxExecutable(overridePath)
alt OverrideValid
PathResolverService-->>PlatformDetectorBase: uvxPath (override)
PlatformDetectorBase->>PathResolverService: TryValidateUvxExecutable(uvxPath)
PathResolverService-->>PlatformDetectorBase: true, version
PlatformDetectorBase-->>WindowsPlatformDetector: DependencyStatus(IsAvailable=true, Path=overridePath)
WindowsPlatformDetector-->>UnityEditor: status
else OverrideInvalid
PathResolverService-->>PlatformDetectorBase: null
PlatformDetectorBase-->>WindowsPlatformDetector: DependencyStatus(IsAvailable=false)
WindowsPlatformDetector-->>UnityEditor: status
end
else NoOverride
PathResolverService->>PathResolverService: ResolveUvxFromSystem()
PathResolverService->>PathResolverService: EnumerateUvxCandidates(uvx/uv)
PathResolverService-->>PlatformDetectorBase: discoveredPath or null
alt DiscoveredPath
PlatformDetectorBase->>PathResolverService: TryValidateUvxExecutable(discoveredPath)
PathResolverService->>ExecPath: TryRun(discoveredPath, "--version")
ExecPath-->>PathResolverService: success, stdout
PathResolverService-->>PlatformDetectorBase: true, version
PlatformDetectorBase-->>WindowsPlatformDetector: DependencyStatus(IsAvailable=true, Path=discoveredPath)
WindowsPlatformDetector-->>UnityEditor: status
else NotFoundOrInvalid
PlatformDetectorBase-->>WindowsPlatformDetector: DependencyStatus(IsAvailable=false)
WindowsPlatformDetector->>WindowsPlatformDetector: platform specific DetectUv fallback
WindowsPlatformDetector->>WindowsPlatformDetector: BuildAugmentedPath()
WindowsPlatformDetector->>ExecPath: TryRun("uv.exe", "--version", augmentedPath)
alt UvFound
ExecPath-->>WindowsPlatformDetector: success, stdout
WindowsPlatformDetector-->>UnityEditor: DependencyStatus(IsAvailable=true, Path=fullPath)
else UvNotFound
WindowsPlatformDetector->>ExecPath: TryRun("uvx.exe", "--version", augmentedPath)
alt UvxFound
ExecPath-->>WindowsPlatformDetector: success, stdout
WindowsPlatformDetector-->>UnityEditor: DependencyStatus(IsAvailable=true, Path=fullPath)
else NeitherFound
WindowsPlatformDetector-->>UnityEditor: DependencyStatus(IsAvailable=false)
end
end
end
end
Sequence diagram for uvx override validation in settings UIsequenceDiagram
actor User
participant McpSettingsSection
participant EditorPrefs
participant PathResolverService
participant ExecPath
User->>McpSettingsSection: Open settings
McpSettingsSection->>McpSettingsSection: UpdatePathOverrides()
alt OverrideToggleOn
McpSettingsSection->>EditorPrefs: GetString(UvxPathOverride)
EditorPrefs-->>McpSettingsSection: overridePath
McpSettingsSection->>PathResolverService: TryValidateUvxExecutable(overridePath)
PathResolverService->>ExecPath: TryRun(uvxPath, "--version")
ExecPath-->>PathResolverService: success/failure, stdout
alt ValidationSuccess
PathResolverService-->>McpSettingsSection: true, version
McpSettingsSection->>McpSettingsSection: uvxPathStatus add class valid
else ValidationFailure
PathResolverService-->>McpSettingsSection: false, null
McpSettingsSection->>McpSettingsSection: uvxPathStatus add class invalid
end
else OverrideToggleOff
McpSettingsSection->>PathResolverService: GetUvxPath()
PathResolverService-->>McpSettingsSection: systemUvxPath
alt SystemPathValid
McpSettingsSection->>PathResolverService: TryValidateUvxExecutable(systemUvxPath)
PathResolverService->>ExecPath: TryRun(uvxPath, "--version")
ExecPath-->>PathResolverService: success, stdout
PathResolverService-->>McpSettingsSection: true, version
McpSettingsSection->>McpSettingsSection: uvxPathStatus add class valid
else SystemPathInvalid
McpSettingsSection->>McpSettingsSection: uvxPathStatus add class invalid
end
end
Updated class diagram for path resolution and platform detectionclassDiagram
class IPathResolverService {
<<interface>>
+string GetUvxPath()
+string GetClaudeCliPath()
+bool IsPythonDetected()
+bool IsClaudeCliDetected()
+void SetUvxPathOverride(string path)
+void SetClaudeCliPathOverride(string path)
+void ClearUvxPathOverride()
+void ClearClaudeCliPathOverride()
+bool HasUvxPathOverride
+bool HasClaudeCliPathOverride
+bool TryValidateUvxExecutable(string uvPath, out string version)
}
class PathResolverService {
+string GetUvxPath()
-static string ResolveUvxFromSystem()
-static IEnumerable~string~ EnumerateUvxCandidates(string commandName)
-static IEnumerable~string~ EnumerateCommandCandidates(string commandName)
+string GetClaudeCliPath()
+bool IsPythonDetected()
+bool IsClaudeCliDetected()
+void SetUvxPathOverride(string path)
+void SetClaudeCliPathOverride(string path)
+void ClearUvxPathOverride()
+void ClearClaudeCliPathOverride()
+bool TryValidateUvxExecutable(string uvxPath, out string version)
-string FindUvxExecutableInPath(string commandName)
+bool HasUvxPathOverride
+bool HasClaudeCliPathOverride
}
IPathResolverService <|.. PathResolverService
class ExecPath {
<<static>>
+bool TryRun(string exe, string args, string workingDir, out string stdout, out string stderr, int timeoutMs, string extraPathPrepend)
+string FindInPath(string executable, string extraPathPrepend)
-string FindInPathWindows(string exe)
-string Which(string exe, string prependPath)
}
class PlatformDetectorBase {
+DependencyStatus DetectUv()
-bool TryParseVersion(string version, out int major, out int minor)
}
class WindowsPlatformDetector {
+override DependencyStatus DetectUv()
-bool TryValidateUvWithPath(string command, string augmentedPath, out string version, out string fullPath)
-bool TryFindPythonViaUv(out string version, out string fullPath)
-bool TryValidatePython(string pythonPath, out string version, out string fullPath)
-bool TryFindInPath(string executable, out string fullPath)
+string BuildAugmentedPath()
-string[] GetPathAdditions()
}
class MacOSPlatformDetector {
+override DependencyStatus DetectUv()
-bool TryValidatePython(string pythonPath, out string version, out string fullPath)
-bool TryValidateUvWithPath(string command, string augmentedPath, out string version, out string fullPath)
+string BuildAugmentedPath()
-string[] GetPathAdditions()
-bool TryFindInPath(string executable, out string fullPath)
}
class LinuxPlatformDetector {
+override DependencyStatus DetectUv()
-bool TryValidatePython(string pythonPath, out string version, out string fullPath)
-bool TryValidateUvWithPath(string command, string augmentedPath, out string version, out string fullPath)
+string BuildAugmentedPath()
-string[] GetPathAdditions()
-bool TryFindInPath(string executable, out string fullPath)
}
class McpSettingsSection {
+void UpdatePathOverrides()
}
class MCPServiceLocator {
<<static>>
+IPathResolverService Paths
}
PlatformDetectorBase <|-- WindowsPlatformDetector
PlatformDetectorBase <|-- MacOSPlatformDetector
PlatformDetectorBase <|-- LinuxPlatformDetector
WindowsPlatformDetector --> ExecPath : uses
MacOSPlatformDetector --> ExecPath : uses
LinuxPlatformDetector --> ExecPath : uses
PlatformDetectorBase --> MCPServiceLocator : uses
MCPServiceLocator o--> IPathResolverService : Paths
PathResolverService --> ExecPath : uses
McpSettingsSection --> MCPServiceLocator : uses
McpSettingsSection --> IPathResolverService : validates uvx
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The
TryValidateUvimplementations in the Linux and macOS detectors now largely duplicate the newTryValidateUvExecutablelogic (stream handling, version parsing, timeout handling); consider centralizing this inPathResolverServiceto avoid divergence and keep uv validation behavior consistent across platforms. - The 5000ms timeout used in multiple process invocations is currently a magic number; it would be clearer and easier to tune if this were a named constant (e.g.,
UvVersionCheckTimeoutMs) shared across the relevant methods. - In
TryValidateUvExecutableandFindUvxExecutableInPath, all exceptions are swallowed silently; consider at least logging in a debug-only path or narrowing the caught exception types so that unexpected failures are easier to diagnose without impacting users.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `TryValidateUv` implementations in the Linux and macOS detectors now largely duplicate the new `TryValidateUvExecutable` logic (stream handling, version parsing, timeout handling); consider centralizing this in `PathResolverService` to avoid divergence and keep uv validation behavior consistent across platforms.
- The 5000ms timeout used in multiple process invocations is currently a magic number; it would be clearer and easier to tune if this were a named constant (e.g., `UvVersionCheckTimeoutMs`) shared across the relevant methods.
- In `TryValidateUvExecutable` and `FindUvxExecutableInPath`, all exceptions are swallowed silently; consider at least logging in a debug-only path or narrowing the caught exception types so that unexpected failures are easier to diagnose without impacting users.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/PathResolverService.cs:342-351` </location>
<code_context>
+ private string FindUvxExecutableInPath(string commandName)
</code_context>
<issue_to_address>
**issue (bug_risk):** FindUvxExecutableInPath cannot resolve the `uv` command because EnumerateUvxCandidates only yields `uvx` paths.
Because EnumerateUvxCandidates always uses `uvx`/`uvx.exe` as `exeName`, calling FindUvxExecutableInPath with `commandName == "uv"` will only ever produce `uvx` candidates. The filename checks against `exeName` (`uv`/`uv.exe`) or `commandName` (`"uv"`) will therefore never succeed, so `TryValidateUvExecutable("uv", ...)` can’t resolve a `uv` on PATH. Please either introduce a dedicated `EnumerateUvCandidates`, parameterize `EnumerateUvxCandidates` to take the command name, or avoid reusing it here and scan PATH directly for `commandName`/`exeName`.
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs:238-239` </location>
<code_context>
+ string stdout = process.StandardOutput.ReadToEnd();
+ string stderr = process.StandardError.ReadToEnd();
+
+ // Respect timeout - check return value
+ if (!process.WaitForExit(5000))
+ return false;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Processes that exceed the 5s timeout are left running, which can leak child processes over time.
When `WaitForExit(5000)` returns false, the method returns without killing or disposing the process, so timed-out `uv`/`uvx` instances remain running and accumulate. Please explicitly terminate and dispose the process in the timeout path (e.g., `process.Kill(true)` in a try/catch) before returning. The same fix is needed in the macOS detector.
</issue_to_address>
### Comment 3
<location> `MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs:236-237` </location>
<code_context>
+ string stdout = process.StandardOutput.ReadToEnd();
+ string stderr = process.StandardError.ReadToEnd();
+
+ // Respect timeout - check return value
+ if (!process.WaitForExit(5000))
+ return false;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** macOS TryValidateUv shares the same potential process leak on timeout as the Linux implementation.
If `WaitForExit` times out, we return without cleaning up the `uv` process, leaving it running. Apply the same timeout handling you use on Linux (e.g., `Kill` and `Dispose` on timeout) here to avoid orphaned processes and keep behavior consistent across platforms.
</issue_to_address>
### Comment 4
<location> `MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs:201-208` </location>
<code_context>
uvxPathStatus.RemoveFromClassList("invalid");
- if (hasOverride)
+
+ string actualUvxPath = MCPServiceLocator.Paths.GetUvxPath();
+ if (MCPServiceLocator.Paths.TryValidateUvExecutable(actualUvxPath, out string version))
{
- if (!string.IsNullOrEmpty(uvxPath) && File.Exists(uvxPath))
- {
- uvxPathStatus.AddToClassList("valid");
- }
- else
- {
- uvxPathStatus.AddToClassList("invalid");
- }
+ uvxPathStatus.AddToClassList("valid");
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Validation status for the uvx override no longer depends on `hasOverride`, which can misrepresent the UI state and leaves `hasOverride` unused.
The new logic always validates `GetUvxPath()` and no longer uses `hasOverride`, so the status can show `invalid` when the override is empty, or `valid` purely due to a system `uv`. If the goal is to reflect the override’s validity, consider validating `uvxPath` (and/or only validating when `hasOverride` is true), and either restore or remove `hasOverride` accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
Outdated
Show resolved
Hide resolved
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs
Outdated
Show resolved
Hide resolved
…s PATH augmentation Replace direct Process.Start calls with ExecPath.TryRun across all platform detectors. This change: - Fixes potential deadlocks by using async output reading - Adds proper timeout handling with process termination - Removes redundant fallback logic and simplifies version parsing - Adds Windows PATH augmentation with common uv, npm, and Python installation paths Co-Authored-By: Claude Opus 4.5 <[email protected]>
The version extraction logic now properly handles outputs like: - "uvx 0.9.18" -> "0.9.18" - "uvx 0.9.18 (hash date)" -> "0.9.18" - "uvx 0.9.18 extra info" -> "0.9.18" Uses Math.Min to find the first occurrence of either space or parenthesis. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
160-163: Minor: Redundant condition in version check.The condition
major > 3 || (major >= 3 && minor >= 10)works correctly but is slightly redundant. For Python 4+,major > 3is sufficient. The second part could bemajor == 3 && minor >= 10for clarity, though this is purely cosmetic.✨ Optional simplification
- return major > 3 || (major >= 3 && minor >= 10); + return major > 3 || (major == 3 && minor >= 10);MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
179-218: Consider extractingTryValidateUvWithPathto base class.This method is nearly identical to
MacOSPlatformDetector.TryValidateUvWithPath()(lines 176-215). The version parsing logic foruv/uvxoutput is duplicated across both platform detectors.Consider moving this to
PlatformDetectorBaseor a shared utility to reduce maintenance burden, since the version format is platform-agnostic.MCPForUnity/Editor/Services/PathResolverService.cs (1)
248-302: Core fix for the PR objective - validates UV executables properly.
TryValidateUvExecutable()is the key addition that addresses issue #538:
- Handles both absolute paths and bare command names
- Resolves bare commands via
FindUvxExecutableInPath()before execution- Uses
ExecPath.TryRunfor robust timeout-safe execution- Parses version from both stdout and stderr (some tools output to stderr)
The version parsing logic (lines 277-294) is duplicated with platform detectors - consider extracting to a shared helper.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Helpers/ExecPath.csMCPForUnity/Editor/Services/PathResolverService.cs
🧰 Additional context used
🧠 Learnings (5)
📚 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/WindowsPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.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/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Helpers/ExecPath.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.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/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
📚 Learning: 2025-08-13T21:00:17.228Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 209
File: UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:279-281
Timestamp: 2025-08-13T21:00:17.228Z
Learning: WinGet per-user Links directory is %LOCALAPPDATA%\Microsoft\WinGet\Links and machine Links directory is %ProgramFiles%\WinGet\Links based on official Microsoft documentation and winget --info command output.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.cs
🧬 Code graph analysis (4)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (3)
BuildAugmentedPath(220-224)TryValidatePython(137-177)GetPathAdditions(226-237)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
BuildAugmentedPath(217-222)TryValidatePython(135-174)GetPathAdditions(224-235)MCPForUnity/Editor/Helpers/ExecPath.cs (3)
ExecPath(12-297)TryRun(161-227)Where(265-295)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
BuildAugmentedPath(217-222)TryValidateUvWithPath(176-215)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
BuildAugmentedPath(220-224)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
BuildAugmentedPath(200-204)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)MCPForUnity/Editor/Services/IPathResolverService.cs (1)
TryValidateUvExecutable(70-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (12)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
242-261: LGTM! Async output handling prevents deadlocks.The refactored
Which()method correctly uses async output reading withBeginOutputReadLine()and the doubleWaitForExit()pattern to ensure buffers are flushed. The null check onProcess.Start()and timeout handling with process kill are good defensive additions.
275-295: LGTM! WindowsWhere()follows the same robust async pattern.Consistent with the
Which()implementation - proper async output collection, null check, timeout handling, and buffer flush.MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
109-132: LGTM! Python via UV detection now uses centralized execution.The refactored
TryFindPythonViaUv()properly usesExecPath.TryRunwith an augmented PATH, handles<download available>entries correctly, and validates discovered Python executables.
200-243: LGTM! Windows PATH augmentation is well-designed.
BuildAugmentedPath()correctly usesPath.PathSeparatorfor Windows compatibility, andGetPathAdditions()efficiently filters to existing directories. The comprehensive list of common install locations (uv, npm, Python versions, user scripts) ensures broad coverage.MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (2)
95-135: LGTM! UV detection now properly respects path overrides.The refactored
DetectUv()correctly:
- Delegates to base implementation first (which uses PathResolverService)
- Returns early if base found UV (respecting overrides)
- Checks
HasUvxPathOverrideto avoid redundant lookups when override is set- Clears
ErrorMessageon successThis directly addresses the PR objective of respecting UV path overrides.
153-177: LGTM! Python validation uses centralized execution.The refactored
TryValidatePython()properly usesExecPath.TryRunwith an augmented PATH, consistent with the other platform detectors.MCPForUnity/Editor/Services/PathResolverService.cs (3)
107-114: LGTM! Simplified Python detection.The refactored
IsPythonDetected()cleanly usesExecPath.TryRunwith a 2-second timeout, removing the need for manual process management. The platform-appropriate executable name selection is correct.
145-161: LGTM! PATH-based discovery is now prioritized correctly.The reordered
EnumerateUvxCandidates()sensibly prioritizes:
- User's PATH (respects environment configuration)
- User directories (
~/.local/bin,~/.cargo/bin)- System directories (platform-specific)
- Windows program directories
The Windows bare-name fallback on line 158 handles edge cases where PATH entries may reference the file directly.
304-333: LGTM! Bare command resolution via candidate enumeration.
FindUvxExecutableInPath()correctly resolves bare command names by iterating throughEnumerateUvxCandidates()and matching against the expected executable name with case-insensitive comparison for Windows compatibility.MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
93-133: LGTM! macOS UV detection properly respects path overrides.The refactored
DetectUv()mirrors the Linux implementation:
- Delegates to base implementation first
- Returns early if base found UV (respecting overrides)
- Checks
HasUvxPathOverridebefore fallback lookup- Clears
ErrorMessageon successThis ensures the UV path override in Advanced Settings is respected, addressing the PR objective.
150-174: LGTM! Python validation includes Apple Silicon Homebrew paths.The refactored
TryValidatePython()correctly includes/opt/homebrew/bin(Apple Silicon Homebrew location) in the augmented PATH, ensuring Python detection works on both Intel and Apple Silicon Macs.
252-270: LGTM! Path lookup uses centralized execution.The refactored
TryFindInPath()properly usesExecPath.TryRunwith/usr/bin/which, maintaining consistency with the Linux implementation while using macOS-appropriate paths.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
150-166: Check both stdout and stderr for Python version output. TheTryValidatePythonmethod should use the same fallback logic already implemented inTryValidateUvWithPath(line 188) to handle macOS Python shims/wrappers that output version information to stderr.Proposed fix
- string output = stdout.Trim(); + string output = string.IsNullOrWhiteSpace(stdout) ? stderr.Trim() : stdout.Trim(); if (output.StartsWith("Python ")) { version = output.Substring(7);MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
153-170: Check both stdout and stderr for Python version output.
While Python 3.10+ (the minimum required version) outputs--versionto stdout, older Python versions (pre-3.4) output to stderr. Although such old versions are rare in modern environments, adding a stderr fallback is defensive and improves robustness.Proposed fix
- string output = stdout.Trim(); + string output = !string.IsNullOrWhiteSpace(stdout) ? stdout.Trim() : stderr.Trim(); if (output.StartsWith("Python ")) { version = output.Substring(7);Note: The same issue exists in the Windows detector at the same logical location.
🤖 Fix all issues with AI agents
In @MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs:
- Around line 112-123: The code is passing a full PATH into
TryValidateUvWithPath/ExecPath.TryRun via BuildAugmentedPath which causes
double-appending of PATH; change the call so extraPathPrepend contains only the
directories you intend to prepend (not the entire PATH). Update
BuildAugmentedPath to return just the prepend fragment (or introduce a new
BuildPrependPath helper) and use that value when calling
TryValidateUvWithPath("uv", ...) and TryValidateUvWithPath("uvx", ...); ensure
ExecPath.TryRun is invoked with the prepend-only string (extraPathPrepend) so
the existing PATH is not appended twice.
In @MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs:
- Around line 110-121: The code is passing a full PATH string from
BuildAugmentedPath into TryValidateUvWithPath/ExecPath.TryRun; instead pass only
the directories that should be prepended (not the entire PATH) so
ExecPath.TryRun receives an extraPathPrepend segment, not a full PATH; update
the call sites in MacOSPlatformDetector (the TryValidateUvWithPath invocations)
to compute and pass a prepend-only value (e.g., the augmented directories list
or a single prepend string from BuildAugmentedPath), and if necessary adjust
TryValidateUvWithPath and its use of ExecPath.TryRun to accept and forward that
prepend-only value rather than treating it as a full PATH.
In
@MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs:
- Around line 200-204: BuildAugmentedPath currently prepends the PATH again
causing duplication and a possible leading empty entry; change it to only return
the joined GetPathAdditions (without appending the current PATH). Specifically,
update BuildAugmentedPath to compute var additions =
string.Join(Path.PathSeparator, GetPathAdditions()) and return additions == "" ?
string.Empty : additions (i.e., do not append currentPath or an extra
PathSeparator). Apply the same change to the related methods in the 206-243
range that build an extraPathPrepend so they don't embed the existing PATH.
In @MCPForUnity/Editor/Services/PathResolverService.cs:
- Around line 242-330: FindUvxExecutableInPath currently calls
EnumerateUvxCandidates() which only yields uvx paths so
TryValidateUvExecutable("uv", ...) fails for bare "uv"; change
EnumerateUvxCandidates to accept a commandName parameter (e.g.,
EnumerateUvxCandidates(string commandName)) and use that parameter to generate
both "uv" and "uvx" candidate file names (and append ".exe" when
RuntimeInformation.IsOSPlatform(OSPlatform.Windows) as needed). Then update
FindUvxExecutableInPath to call EnumerateUvxCandidates(commandName) instead of
the parameterless version and update ResolveUvxFromSystem to pass the correct
command ("uv" or "uvx") when enumerating; preserve existing File.Exists checks
and case-insensitive name comparisons (Path.GetFileName and Equals) and keep
exception swallowing behavior.
🧹 Nitpick comments (5)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
242-258: Good move to async output reading + null process guard; consider also draining stderr for completeness.
Which()only reads stdout; addingRedirectStandardError=true+BeginErrorReadLine()would make it matchTryRun()’s deadlock-safety profile (even ifwhichrarely writes much).
276-292: Same note forWhere(): stdout-only async read is better than sync, but stderr drain would make it more robust.
Especially on Windows,wherecan emit errors to stderr; draining it avoids edge-case blocking and can aid diagnostics.MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
191-205: Version parsing inTryValidateUvWithPath()should stop at first token (not whole remainder).
Right now, ifuv/uvx --versionever emits extra tokens not wrapped in parentheses,versionwill include them.MCPForUnity/Editor/Services/PathResolverService.cs (1)
107-113:IsPythonDetected()likely should trypythonas fallback on non-Windows.
Some Linux distros still expose onlypython(or users intentionally alias it).MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
188-201: Align uv/uvx version parsing with the stricter “first token” logic used in PathResolverService.
Keeps behavior consistent if--versionoutput grows additional fields.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Helpers/ExecPath.csMCPForUnity/Editor/Services/PathResolverService.cs
🧰 Additional context used
🧠 Learnings (4)
📚 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/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Helpers/ExecPath.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
📚 Learning: 2025-08-13T21:00:17.228Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 209
File: UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:279-281
Timestamp: 2025-08-13T21:00:17.228Z
Learning: WinGet per-user Links directory is %LOCALAPPDATA%\Microsoft\WinGet\Links and machine Links directory is %ProgramFiles%\WinGet\Links based on official Microsoft documentation and winget --info command output.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
BuildAugmentedPath(212-217)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
109-133:uv python listparsing is reasonable; watch for format drift.
Current logic assumes the last token is an absolute python path and filters<download available>lines; ifuvoutput format changes, this may silently regress.
149-165: No changes needed. The project explicitly requires Python 3.10 or newer (per README.md), which outputspython --versionto stdout. Python versions prior to 3.4 (which output to stderr) are outside the supported range and not a concern for this codebase.Likely an incorrect or invalid review comment.
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
97-108: DetectUv override/early-return flow looks right.
Returning the base result when an override exists matches the intended “override is source of truth” behavior.MCPForUnity/Editor/Services/PathResolverService.cs (1)
145-198: PATH-first candidate enumeration is a good change.
This matches typical install scripts and should reduce false “not found” in standard shells.MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
95-106: DetectUv override/early-return flow looks consistent with the intended behavior.
Base detection covers overrides; fallback only runs when no override is set.
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs
Show resolved
Hide resolved
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
Show resolved
Hide resolved
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
TryValidateUvExecutable, the expression!string.IsNullOrWhiteSpace(stdout) ? stdout.Trim() : stderr.Trim()can throw ifstderris null; consider using a null-coalescing fallback (e.g.stderr ?? string.Empty) before trimming to avoid a potentialNullReferenceExceptionwhen the tool emits no output. FindUvxExecutableInPathiteratesEnumerateUvxCandidates()(which only yields uvx locations) but is called with arbitrarycommandNamevalues such as"uv"; this means plainuvmay never resolve even when present, so it would be safer either to specialize this helper strictly for uvx or extend the candidates enumeration to also cover uv binaries consistently.- The uv version-string parsing logic is duplicated across
PathResolverService.TryValidateUvExecutableand theTryValidateUvWithPathmethods on macOS/Linux; extracting a shared helper for parsing would reduce repetition and ensure any future format tweaks are handled consistently on all platforms.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TryValidateUvExecutable`, the expression `!string.IsNullOrWhiteSpace(stdout) ? stdout.Trim() : stderr.Trim()` can throw if `stderr` is null; consider using a null-coalescing fallback (e.g. `stderr ?? string.Empty`) before trimming to avoid a potential `NullReferenceException` when the tool emits no output.
- `FindUvxExecutableInPath` iterates `EnumerateUvxCandidates()` (which only yields uvx locations) but is called with arbitrary `commandName` values such as `"uv"`; this means plain `uv` may never resolve even when present, so it would be safer either to specialize this helper strictly for uvx or extend the candidates enumeration to also cover uv binaries consistently.
- The uv version-string parsing logic is duplicated across `PathResolverService.TryValidateUvExecutable` and the `TryValidateUvWithPath` methods on macOS/Linux; extracting a shared helper for parsing would reduce repetition and ensure any future format tweaks are handled consistently on all platforms.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs:154-155` </location>
<code_context>
+ return false;
- if (process.ExitCode == 0 && output.StartsWith("Python "))
+ string output = stdout.Trim();
+ if (output.StartsWith("Python "))
{
- version = output.Substring(7); // Remove "Python " prefix
</code_context>
<issue_to_address>
**issue (bug_risk):** Python version detection ignores stderr, which is where many Python builds print `--version` output.
Because `python --version` commonly writes to stderr (e.g., CPython on Windows), using only `stdout` means `output` may be empty and version detection will fail even when Python is installed.
To make this robust and consistent with your other helpers, derive `output` from both streams, preferring stdout when present:
```csharp
string output = !string.IsNullOrWhiteSpace(stdout) ? stdout.Trim() : stderr.Trim();
if (output.StartsWith("Python "))
{
// existing logic
}
```
The same combined stdout/stderr handling should be applied in the macOS and Linux `TryValidatePython` methods as well.
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs:200-203` </location>
<code_context>
return false;
}
+
+ private string BuildAugmentedPath()
+ {
+ string currentPath = Environment.GetEnvironmentVariable("PATH") ?? "";
+ return string.Join(Path.PathSeparator, GetPathAdditions()) + Path.PathSeparator + currentPath;
+ }
+
</code_context>
<issue_to_address>
**nitpick:** Avoid leading path separator when there are no additions to prepend.
If `GetPathAdditions()` returns an empty array, this will produce a leading separator and an empty PATH entry (e.g., `";C:\Existing\Path"` on Windows). Consider only adding the separator when there are actual additions:
```csharp
var additions = string.Join(Path.PathSeparator, GetPathAdditions());
return string.IsNullOrEmpty(additions)
? currentPath
: additions + Path.PathSeparator + currentPath;
```
</issue_to_address>
### Comment 3
<location> `MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs:202-208` </location>
<code_context>
- if (hasOverride)
+
+ string actualUvxPath = MCPServiceLocator.Paths.GetUvxPath();
+ if (MCPServiceLocator.Paths.TryValidateUvExecutable(actualUvxPath, out string version))
{
- if (!string.IsNullOrEmpty(uvxPath) && File.Exists(uvxPath))
- {
- uvxPathStatus.AddToClassList("valid");
- }
- else
- {
- uvxPathStatus.AddToClassList("invalid");
- }
+ uvxPathStatus.AddToClassList("valid");
</code_context>
<issue_to_address>
**question (bug_risk):** Settings UI now marks uvx as invalid when auto-detection fails, changing behavior from previous implementation.
Previously, when `hasOverride == false`, `uvxPathStatus` was always marked "valid" regardless of whether uv/uvx was installed. Now it depends entirely on `TryValidateUvExecutable(actualUvxPath)`, so a project with no override and no uv will show as "invalid".
If this semantic change is intended (showing real availability), the update is fine. If you only meant to validate user-provided overrides and keep the non-override case neutral, consider restoring the old behavior for `!hasOverride` and using `TryValidateUvExecutable` only when an override is present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs:
- Around line 95-123: DetectUv currently builds an "augmentedPath" that already
appends the current PATH, then passes that to ExecPath.TryRun which further
prepends PATH, causing PATH duplication; fix by making DetectUv pass only the
additions to ExecPath.TryRun (either change BuildAugmentedPath to return
additions-only or add a new BuildPathAdditions helper) and use that
additions-only string as the extraPathPrepend argument in
TryValidateUvWithPath/ExecPath.TryRun so PATH is not duplicated; update
references to augmentedPath in DetectUv/TryValidateUvWithPath accordingly and
ensure status.Path/version logic remains unchanged.
In @MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs:
- Around line 93-122: DetectUv() is causing duplicate PATH entries because
BuildAugmentedPath() currently returns the full PATH plus prepended entries and
ExecPath.TryRun also appends PATH via its extraPathPrepend; change
BuildAugmentedPath() to return only the path-prefix to prepend (not include the
existing PATH), so TryValidateUvWithPath/ExecPath.TryRun receives a prepend-only
string and PATH won’t be duplicated; update BuildAugmentedPath() implementation
and verify callers like DetectUv() and TryValidateUvWithPath still treat its
result as an extraPathPrepend for ExecPath.TryRun.
In @MCPForUnity/Editor/Services/PathResolverService.cs:
- Around line 105-114: IsPythonDetected currently only tries "python.exe" on
Windows which misses installs accessible via "python" or the "py" launcher;
update IsPythonDetected to try a short sequence on Windows using ExecPath.TryRun
for "python.exe", then "python", and finally "py" with arguments "-3 --version"
(or "py -3") until one succeeds, returning true on the first success and false
only if all attempts fail; keep the same timeout and out parameters and preserve
non-Windows behavior of trying "python3".
- Around line 241-333: TryValidateUvExecutable claims to accept both "uv" and
"uvx" but FindUvxExecutableInPath only searches UVX-specific candidates
(EnumerateUvxCandidates), so a bare "uv" never resolves; update
FindUvxExecutableInPath to accept the requested commandName and locate it via
the PATH rather than only EnumerateUvxCandidates: if on Windows run "where
commandName" (fall back to appending ".exe" when appropriate) or on Unix run
"which commandName", or alternatively enumerate PATH entries and check for
commandName (and commandName + ".exe" on Windows) with File.Exists; keep
TryValidateUvExecutable and EnumerateUvxCandidates intact but use the new
PATH-based resolution as the primary lookup for bare commands to ensure
TryValidateUvExecutable works for both "uv" and "uvx".
🧹 Nitpick comments (5)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
141-198: Windows UVX candidate enumeration: add WinGet Links + prefer realistic extensions.
Path.Combine(dir, "uvx")is unlikely to exist on Windows; better to check.exe/.cmd/.ps1variants.- Consider adding
%LOCALAPPDATA%\Microsoft\WinGet\Linksand%ProgramFiles%\WinGet\Linksas fallbacks (per retrieved learnings). Based on learnings, WinGet commonly exposes shims there.Proposed tweak
private static IEnumerable<string> EnumerateUvxCandidates() { string exeName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "uvx.exe" : "uvx"; // Priority 1: User-configured PATH (most common scenario from official install scripts) string pathEnv = Environment.GetEnvironmentVariable("PATH"); if (!string.IsNullOrEmpty(pathEnv)) { foreach (string rawDir in pathEnv.Split(Path.PathSeparator)) { if (string.IsNullOrWhiteSpace(rawDir)) continue; string dir = rawDir.Trim(); - yield return Path.Combine(dir, exeName); + yield return Path.Combine(dir, exeName); if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { - // Some PATH entries may already contain the file without extension - yield return Path.Combine(dir, "uvx"); + yield return Path.Combine(dir, "uvx.cmd"); + yield return Path.Combine(dir, "uvx.ps1"); } } } @@ else if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { // Priority 4: Windows-specific program directories string localAppData = Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData); string programFiles = Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles); if (!string.IsNullOrEmpty(localAppData)) { yield return Path.Combine(localAppData, "Programs", "uv", exeName); + yield return Path.Combine(localAppData, "Microsoft", "WinGet", "Links", exeName); } if (!string.IsNullOrEmpty(programFiles)) { yield return Path.Combine(programFiles, "uv", exeName); + yield return Path.Combine(programFiles, "WinGet", "Links", exeName); } } }
19-20: Avoid passingnulldefaultValue intoEditorPrefs.GetStringunless confirmed supported.
Safer to usestring.Emptyas the default; avoids potential API edge cases and makes intent clearer.MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
137-177: TryValidatePython(): consider reading stderr when stdout is empty.
Makes detection resilient across environments where--versionends up on stderr.Proposed tweak
- string output = stdout.Trim(); + string output = string.IsNullOrWhiteSpace(stdout) ? stderr.Trim() : stdout.Trim();MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
135-174: TryValidatePython(): same stdout-only parsing; consider stderr fallback.
176-215: TryValidateUvWithPath(): same “Path isn’t a full path” concern; keep consistent with base resolver.
If feasible, consider reusingIPathResolverService.TryValidateUvExecutable(...)here (or at least renamefullPathtoinvocation/resolvedCommandto avoid implying it’s absolute).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.cs
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-13T21:00:17.228Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 209
File: UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:279-281
Timestamp: 2025-08-13T21:00:17.228Z
Learning: WinGet per-user Links directory is %LOCALAPPDATA%\Microsoft\WinGet\Links and machine Links directory is %ProgramFiles%\WinGet\Links based on official Microsoft documentation and winget --info command output.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (2)
BuildAugmentedPath(220-224)TryValidateUvWithPath(179-218)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
BuildAugmentedPath(200-204)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (4)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
BuildAugmentedPath(217-222)TryValidateUvWithPath(176-215)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
BuildAugmentedPath(200-204)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (1)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
179-218: No action required.DependencyStatus.Pathis display-only metadata and is never used for process execution. Actual execution usesPathResolverService, which properly resolves bare commands like"uv"/"uvx"to their full paths before launching processes.Likely an incorrect or invalid review comment.
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs
Show resolved
Hide resolved
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
Show resolved
Hide resolved
| public bool IsPythonDetected() | ||
| { | ||
| try | ||
| { | ||
| var psi = new ProcessStartInfo | ||
| { | ||
| FileName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "python.exe" : "python3", | ||
| Arguments = "--version", | ||
| UseShellExecute = false, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| CreateNoWindow = true | ||
| }; | ||
| using var p = Process.Start(psi); | ||
| p.WaitForExit(2000); | ||
| return p.ExitCode == 0; | ||
| } | ||
| catch | ||
| { | ||
| return false; | ||
| } | ||
| return ExecPath.TryRun( | ||
| RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "python.exe" : "python3", | ||
| "--version", | ||
| null, | ||
| out _, | ||
| out _, | ||
| 2000); | ||
| } |
There was a problem hiding this comment.
Python detection is a bit Windows-fragile (python.exe only).
On Windows, py launcher / alias-only installs can exist without python.exe being directly invokable. Consider trying python (and/or py -3) as a fallback to reduce false negatives.
Proposed tweak
public bool IsPythonDetected()
{
- return ExecPath.TryRun(
- RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "python.exe" : "python3",
- "--version",
- null,
- out _,
- out _,
- 2000);
+ if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+ {
+ return ExecPath.TryRun("python", "--version", null, out _, out _, 2000) ||
+ ExecPath.TryRun("python.exe", "--version", null, out _, out _, 2000) ||
+ ExecPath.TryRun("py", "-3 --version", null, out _, out _, 2000);
+ }
+
+ return ExecPath.TryRun("python3", "--version", null, out _, out _, 2000) ||
+ ExecPath.TryRun("python", "--version", null, out _, out _, 2000);
}🤖 Prompt for AI Agents
In @MCPForUnity/Editor/Services/PathResolverService.cs around lines 105 - 114,
IsPythonDetected currently only tries "python.exe" on Windows which misses
installs accessible via "python" or the "py" launcher; update IsPythonDetected
to try a short sequence on Windows using ExecPath.TryRun for "python.exe", then
"python", and finally "py" with arguments "-3 --version" (or "py -3") until one
succeeds, returning true on the first success and false only if all attempts
fail; keep the same timeout and out parameters and preserve non-Windows behavior
of trying "python3".
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
McpSettingsSection.UpdatePathOverrides, the new logic validatesGetUvxPath()regardless of whether an override is configured, which means the UV override field will show as invalid whenever UV is not installed even if no override is set—consider only runningTryValidateUvExecutablewhenHasUvxPathOverrideis true and treating the empty/unused override field as neutral. - The UV version parsing logic (e.g., in
PathResolverService.TryValidateUvExecutable,LinuxPlatformDetector.TryValidateUvWithPath, andMacOSPlatformDetector.TryValidateUvWithPath) is duplicated and slightly diverging; extracting a shared helper for parsinguv/uvx--versionoutput would reduce drift and make future changes safer. TryValidateUvExecutablecallsFindUvxExecutableInPath, which in turn uses bothEnumerateUvxCandidatesandEnumerateCommandCandidatesthat each searchPATHand common locations, leading to redundant filesystem checks—consider consolidating these candidate enumerations to avoid duplicate work on every validation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `McpSettingsSection.UpdatePathOverrides`, the new logic validates `GetUvxPath()` regardless of whether an override is configured, which means the UV override field will show as invalid whenever UV is not installed even if no override is set—consider only running `TryValidateUvExecutable` when `HasUvxPathOverride` is true and treating the empty/unused override field as neutral.
- The UV version parsing logic (e.g., in `PathResolverService.TryValidateUvExecutable`, `LinuxPlatformDetector.TryValidateUvWithPath`, and `MacOSPlatformDetector.TryValidateUvWithPath`) is duplicated and slightly diverging; extracting a shared helper for parsing `uv`/`uvx` `--version` output would reduce drift and make future changes safer.
- `TryValidateUvExecutable` calls `FindUvxExecutableInPath`, which in turn uses both `EnumerateUvxCandidates` and `EnumerateCommandCandidates` that each search `PATH` and common locations, leading to redundant filesystem checks—consider consolidating these candidate enumerations to avoid duplicate work on every validation.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/PathResolverService.cs:274` </location>
<code_context>
+ return false;
+
+ // Check stdout first, then stderr (some tools output to stderr)
+ string versionOutput = !string.IsNullOrWhiteSpace(stdout) ? stdout.Trim() : stderr.Trim();
+
+ // uvx outputs "uvx x.y.z" or "uv x.y.z", extract version number
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `stderr` being null before calling `.Trim()` to avoid a possible NullReferenceException.
Because `ExecPath.TryRun` may return `true` while leaving either `stdout` or `stderr` null (e.g., when the process produces no output or only one stream), calling `.Trim()` on `stderr` without a null check can throw. A safer approach is to fall back only on non-empty, non-null values, for example:
```csharp
tstring versionOutput =
!string.IsNullOrWhiteSpace(stdout) ? stdout.Trim() :
!string.IsNullOrWhiteSpace(stderr) ? stderr.Trim() :
string.Empty;
```
Please apply the same null-guarding pattern in the Linux/macOS/Windows `TryValidatePython` and `TryValidateUvWithPath` methods that use this `stdout`/`stderr` fallback logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
157-159: Minor: Simplify version check condition.The logic is correct, but
major >= 3in the second part is redundant sincemajor > 3in the first part already handles Python 4+.✨ Suggested simplification
- return major > 3 || (major >= 3 && minor >= 10); + return major > 3 || (major == 3 && minor >= 10);MCPForUnity/Editor/Services/PathResolverService.cs (1)
345-397: Consider refactoring to reduce duplication.
EnumerateCommandCandidatesshares significant logic withEnumerateUvxCandidates. While the current implementation works correctly, consider extracting shared path enumeration logic into a common helper to reduce maintenance burden.This is a minor improvement that could be deferred—the current implementation is correct and readable.
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
155-157: Minor: Simplify version check condition.Same as the Linux detector - the logic is correct but could be simplified.
✨ Suggested simplification
- return major > 3 || (major >= 3 && minor >= 10); + return major > 3 || (major == 3 && minor >= 10);
169-208: Consider extracting shared UV validation logic.
TryValidateUvWithPathis identical between Linux and macOS detectors. This could be moved toPlatformDetectorBaseto reduce duplication.The current implementation works correctly; this is a maintenance improvement that could be addressed in a follow-up.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
🧰 Additional context used
🧠 Learnings (6)
📚 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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.cs
📚 Learning: 2025-08-13T21:00:17.228Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 209
File: UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:279-281
Timestamp: 2025-08-13T21:00:17.228Z
Learning: WinGet per-user Links directory is %LOCALAPPDATA%\Microsoft\WinGet\Links and machine Links directory is %ProgramFiles%\WinGet\Links based on official Microsoft documentation and winget --info command output.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
BuildAugmentedPath(210-219)TryValidateUvWithPath(169-208)GetPathAdditions(221-232)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
BuildAugmentedPath(201-210)GetPathAdditions(212-249)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)MCPForUnity/Editor/Services/IPathResolverService.cs (1)
TryValidateUvExecutable(70-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (13)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (5)
5-8: LGTM!Import changes correctly add the required namespaces for
MCPServiceLocatorand constants used in the updated detection logic.
95-135: LGTM!The updated
DetectUv()flow correctly:
- Delegates to base implementation first (respects override path via
PathResolverService)- Returns early on success
- Preserves base result when override is configured (so validation errors are visible)
- Falls back to platform-specific augmented PATH detection
- Clears
ErrorMessageon successThis aligns with the PR objective to respect UV path overrides.
171-211: LGTM!The version parsing logic correctly handles various output formats:
- Simple:
"uvx 0.9.18"- With build info:
"uvx 0.9.18 (hash date)"- Extra tokens:
"uvx 0.9.18 extra"Using
ExecPath.TryRunensures proper timeout handling and avoids the previous deadlock potential.
213-235: LGTM!
BuildAugmentedPathcorrectly prepends platform-specific directories to the current PATH usingPath.PathSeparator. The additions cover standard Linux binary locations (/usr/local/bin,/usr/bin,/bin,/snap/bin) and user-local installations (~/.local/bin).
237-261: LGTM!The refactored
TryFindInPathcorrectly usesExecPath.TryRunwith the augmented PATH, ensuring consistent behavior with other detection methods and proper timeout handling.MCPForUnity/Editor/Services/PathResolverService.cs (4)
105-114: LGTM!Simplified
IsPythonDetected()usingExecPath.TryRuninstead of brittle try/catch with manual process handling. The 2-second timeout is reasonable for a quick check.
145-161: LGTM!Prioritizing PATH entries first correctly addresses the PR objective - paths added by official uv install scripts are now found before falling back to hardcoded locations. The Windows-specific handling for bare command names without
.exeextension is a nice touch for compatibility.
248-302: LGTM!The new
TryValidateUvExecutablemethod correctly:
- Handles both absolute paths and bare command names
- Resolves bare commands via
FindUvxExecutableInPathbefore execution- Uses
ExecPath.TryRunwith proper timeout handling- Parses version from both
uvanduvxoutput formatsThis enables both the Settings UI and platform detectors to validate UV executables consistently.
304-339: LGTM!The method correctly handles the special case for
"uvx"commands by using the specializedEnumerateUvxCandidateswhich includes uvx-specific installation paths, while falling back to the genericEnumerateCommandCandidatesfor other commands.MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (4)
5-8: LGTM!Import changes correctly add the required namespaces for the updated detection logic, consistent with the Linux detector.
93-133: LGTM!The
DetectUv()implementation correctly mirrors the Linux detector's pattern:
- Delegates to base implementation (respects override)
- Returns early if available
- Preserves base result when override is configured
- Falls back to macOS-specific augmented PATH detection
221-232: LGTM!
GetPathAdditionscorrectly includes macOS-specific paths:
/opt/homebrew/bin- Apple Silicon Homebrew/usr/local/bin- Intel Homebrew- Standard Unix paths and user-local
~/.local/binThis ensures UV detection works in GUI environments where shell profile PATH modifications aren't inherited.
234-258: LGTM!
TryFindInPathcorrectly usesExecPath.TryRunwith the augmented PATH, consistent with the Linux implementation.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The uv version parsing logic is now duplicated in
PathResolverService.TryValidateUvExecutableand the platform-specificTryValidateUvWithPathmethods; consider extracting a shared helper to keep behavior consistent and reduce maintenance overhead. EnumerateUvxCandidatesandEnumerateCommandCandidatesimplement very similar PATH and common-location search logic with slightly different priorities; it may be worth unifying these into a single, parameterized routine to avoid subtle divergence over time.- In
McpSettingsSection.UpdatePathOverrides, thehasOverrideflag is no longer used and the validation always runs throughGetUvxPath/TryValidateUvExecutable; if the UX should distinguish between "no override configured" and "configured but invalid", consider reflecting that difference in the status styling or messaging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The uv version parsing logic is now duplicated in `PathResolverService.TryValidateUvExecutable` and the platform-specific `TryValidateUvWithPath` methods; consider extracting a shared helper to keep behavior consistent and reduce maintenance overhead.
- `EnumerateUvxCandidates` and `EnumerateCommandCandidates` implement very similar PATH and common-location search logic with slightly different priorities; it may be worth unifying these into a single, parameterized routine to avoid subtle divergence over time.
- In `McpSettingsSection.UpdatePathOverrides`, the `hasOverride` flag is no longer used and the validation always runs through `GetUvxPath`/`TryValidateUvExecutable`; if the UX should distinguish between "no override configured" and "configured but invalid", consider reflecting that difference in the status styling or messaging.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Add absolute path resolution in TryValidatePython and TryValidateUvWithPath for better UI display - Fix BuildAugmentedPath to avoid PATH duplication - Add comprehensive comments for version parsing logic - Ensure cross-platform consistency across all three detectors - Fix override path validation logic with clear state handling - Fix platform detector path resolution and Python version detection - Use UserProfile consistently in GetClaudeCliPath instead of Personal - All platforms now use protected BuildAugmentedPath method This change improves user experience by displaying full paths in the UI while maintaining robust fallback behavior if path resolution fails. Co-Authored-By: GLM4.7 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
345-401: Consider consolidating withEnumerateUvxCandidates.
EnumerateCommandCandidatesandEnumerateUvxCandidatesshare ~80% identical logic. The only meaningful difference isEnumerateUvxCandidatesalso yields paths without.exeon Windows PATH entries.Not blocking, but unifying them would reduce maintenance overhead.
♻️ Possible consolidation approach
private static IEnumerable<string> EnumerateExecutableCandidates(string commandName, bool includeNoExtensionVariant = false) { string exeName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && !commandName.EndsWith(".exe") ? commandName + ".exe" : commandName; // Search PATH first string pathEnv = Environment.GetEnvironmentVariable("PATH"); if (!string.IsNullOrEmpty(pathEnv)) { foreach (string rawDir in pathEnv.Split(Path.PathSeparator)) { if (string.IsNullOrWhiteSpace(rawDir)) continue; string dir = rawDir.Trim(); yield return Path.Combine(dir, exeName); if (includeNoExtensionVariant && RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && exeName.EndsWith(".exe")) { yield return Path.Combine(dir, exeName.Replace(".exe", "")); } } } // ... rest of common logic }Then call with
EnumerateExecutableCandidates("uvx.exe", includeNoExtensionVariant: true)for uv discovery.
220-234: Consider usingTryValidateUvExecutablefor stronger validation.
SetUvxPathOverridevalidates only withFile.Exists, butGetUvxPathlater validates usingTryValidateUvExecutable. If the user selects a non-uv executable (e.g., any random.exe), the override is accepted here, butGetUvxPathreturnsnull— potentially causing confusing behavior.For consistency, consider validating with
TryValidateUvExecutableat set time, or clearly documenting that the override is only file-existence-checked here.♻️ Proposed stronger validation
public void SetUvxPathOverride(string path) { if (string.IsNullOrEmpty(path)) { ClearUvxPathOverride(); return; } - if (!File.Exists(path)) + if (!TryValidateUvExecutable(path, out _)) { - throw new ArgumentException("The selected uvx executable does not exist"); + throw new ArgumentException("The selected path is not a valid uv/uvx executable"); } EditorPrefs.SetString(EditorPrefKeys.UvxPathOverride, path); }MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
244-253: Hardcoded Python version paths may become stale.The paths
Python313,Python312,Python311,Python310are hardcoded. When Python 3.14+ is released, this list won't include it.Consider enumerating existing
Python3*directories dynamically, or noting this as a maintenance item.♻️ Dynamic Python directory enumeration
// Instead of hardcoded versions, enumerate existing directories if (!string.IsNullOrEmpty(programFiles) && Directory.Exists(programFiles)) { try { foreach (var dir in Directory.EnumerateDirectories(programFiles, "Python3*")) { additions.Add(dir); } } catch { /* Ignore enumeration errors */ } }MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
178-225: Version parsing duplicated acrossTryValidateUvWithPathimplementations.The version extraction logic (lines 199-216) is identical in
LinuxPlatformDetector,MacOSPlatformDetector, andPathResolverService.TryValidateUvExecutable. If the uv version output format changes, all three need updating.Consider extracting to a shared helper method.
♻️ Extract shared version parsing
// In a shared location (e.g., PlatformDetectorBase or a utility class) protected static bool TryParseUvVersion(string output, out string version) { version = null; if (!output.StartsWith("uv ") && !output.StartsWith("uvx ")) return false; int spaceIndex = output.IndexOf(' '); if (spaceIndex < 0) return false; var remainder = output.Substring(spaceIndex + 1).Trim(); int nextSpace = remainder.IndexOf(' '); int parenIndex = remainder.IndexOf('('); int endIndex = Math.Min( nextSpace >= 0 ? nextSpace : int.MaxValue, parenIndex >= 0 ? parenIndex : int.MaxValue ); version = endIndex < int.MaxValue ? remainder.Substring(0, endIndex).Trim() : remainder; return true; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
.gitignoreMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.csTestProjects/AssetStoreUploads/Assets/Readme.asset.metaTestProjects/AssetStoreUploads/Assets/Settings/SampleSceneProfile.asset.metaTestProjects/AssetStoreUploads/Assets/Settings/URP-Balanced-Renderer.asset.metaTestProjects/AssetStoreUploads/Assets/Settings/URP-Balanced.asset.metaTestProjects/AssetStoreUploads/Assets/Settings/URP-HighFidelity-Renderer.asset.metaTestProjects/AssetStoreUploads/Assets/Settings/URP-HighFidelity.asset.metaTestProjects/AssetStoreUploads/Assets/Settings/URP-Performant-Renderer.asset.metaTestProjects/AssetStoreUploads/Assets/Settings/URP-Performant.asset.metaTestProjects/AssetStoreUploads/Assets/UniversalRenderPipelineGlobalSettings.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Animation Clips.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Audio Clipping.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Colliders.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Compressed Files.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Empty Prefabs.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check File Menu Names.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check LODs.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Line Endings.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Mesh Prefabs.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Missing Components in Assets.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Missing Components in Scenes.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Model Import Logs.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Model Orientation.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Model Types.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Normal Map Textures.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Package Naming.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Particle Systems.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Path Lengths.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Prefab Transforms.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Script Compilation.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Shader Compilation.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Texture Dimensions.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Type Namespaces.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove Executable Files.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove JPG Files.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove JavaScript Files.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove Lossy Audio Files.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove Mixamo Files.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove SpeedTree Files.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove Video Files.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/UnityPackage/Check Demo Scenes.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/UnityPackage/Check Documentation.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/UnityPackage/Check Package Size.asset.metaTestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/UnityPackage/Check Project Template Assets.asset.meta
💤 Files with no reviewable changes (43)
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Package Naming.asset.meta
- TestProjects/AssetStoreUploads/Assets/Settings/URP-Performant-Renderer.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Shader Compilation.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Mesh Prefabs.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/UnityPackage/Check Package Size.asset.meta
- TestProjects/AssetStoreUploads/Assets/Settings/URP-Performant.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Model Orientation.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Path Lengths.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Model Import Logs.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Compressed Files.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/UnityPackage/Check Demo Scenes.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check File Menu Names.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Missing Components in Scenes.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check LODs.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove JPG Files.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Normal Map Textures.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Particle Systems.asset.meta
- TestProjects/AssetStoreUploads/Assets/Settings/SampleSceneProfile.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Script Compilation.asset.meta
- TestProjects/AssetStoreUploads/Assets/Readme.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/UnityPackage/Check Documentation.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove Executable Files.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Colliders.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove Video Files.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Line Endings.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Animation Clips.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Prefab Transforms.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Texture Dimensions.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove SpeedTree Files.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove JavaScript Files.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove Mixamo Files.asset.meta
- TestProjects/AssetStoreUploads/Assets/UniversalRenderPipelineGlobalSettings.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Empty Prefabs.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Audio Clipping.asset.meta
- TestProjects/AssetStoreUploads/Assets/Settings/URP-HighFidelity.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Missing Components in Assets.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Model Types.asset.meta
- TestProjects/AssetStoreUploads/Assets/Settings/URP-HighFidelity-Renderer.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Remove Lossy Audio Files.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/Generic/Check Type Namespaces.asset.meta
- TestProjects/AssetStoreUploads/Assets/Settings/URP-Balanced-Renderer.asset.meta
- TestProjects/AssetStoreUploads/Packages/com.unity.asset-store-tools/Editor/Validator/Tests/UnityPackage/Check Project Template Assets.asset.meta
- TestProjects/AssetStoreUploads/Assets/Settings/URP-Balanced.asset.meta
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/WindowsPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.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/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs
🧬 Code graph analysis (3)
MCPForUnity/Editor/Services/PathResolverService.cs (3)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)MCPForUnity/Editor/Services/IPathResolverService.cs (1)
TryValidateUvExecutable(70-70)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (4)
BuildAugmentedPath(227-234)TryValidatePython(137-176)TryFindInPath(249-273)GetPathAdditions(236-247)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (4)
BuildAugmentedPath(225-232)TryValidatePython(135-174)TryFindInPath(247-271)GetPathAdditions(234-245)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-297)TryRun(161-227)
🔇 Additional comments (17)
.gitignore (1)
62-62: Metadata file ignores are appropriate for Unity test assets.The addition of
*.metaand**/*.metapatterns aligns with standard Unity project practices and the PR's modifications to test assets. These patterns will prevent generated/modified.metafiles from being committed.Also applies to: 64-64
MCPForUnity/Editor/Services/PathResolverService.cs (4)
22-46: LGTM! Clear override priority logic.The three-state behavior is well-implemented:
- Valid override → use it
- Invalid override → return null (no silent fallback)
- No override → discovery with "uvx" bare command fallback
This ensures System Requirements respects Advanced Settings and prevents confusing UX where detection silently ignores a misconfigured override.
83-140: Well-structured candidate enumeration with correct priority.The priority order (PATH → user directories → system directories) aligns with the PR objectives. The Windows-specific handling for
.exeextensions is appropriate.
268-322: Solid validation implementation with robust version parsing.The approach of validating via
--versionexecution rather than justFile.Existsis the correct fix for the original issue. The version parsing handles common output formats likeuvx 0.9.18anduvx 0.9.18 (Homebrew ...).
204-213: LGTM! Simple detection check.Using
ExecPath.TryRunwith a short timeout (2000ms) for a boolean detection is appropriate. This aligns with the centralized execution pattern used elsewhere.MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (4)
1-5: Good documentation of design decision.The header comment clearly explains why Windows doesn't override
DetectUv()— the base class andPathResolverServicealready handle Windows-specific paths. This helps maintainability.
107-145: LGTM! Robust Python discovery via uv.The implementation correctly:
- Skips
<download available>lines fromuv python listoutput- Validates candidates with
TryValidatePythonbefore accepting- Handles errors gracefully when uv isn't installed
147-186: LGTM! Consistent with other platform detectors.The implementation mirrors
LinuxPlatformDetector.TryValidatePythonandMacOSPlatformDetector.TryValidatePython, ensuring cross-platform consistency. The version check correctly requires Python 3.10+.
188-212: LGTM! Correct Windows-specific path resolution.Using
wherecommand is the Windows equivalent of Unixwhich. The implementation correctly takes the first result and validates existence.MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (4)
95-135: LGTM! Correct override-respecting detection flow.The three-phase approach is well-structured:
- Delegate to base (handles override and cross-platform resolution)
- If override is set but base failed, return as-is (don't mask invalid override)
- Otherwise, attempt Linux-specific discovery
Setting
status.ErrorMessage = nullon success ensures clean status reporting.
137-176: LGTM! Consistent cross-platform Python validation.The implementation follows the same pattern as Windows and macOS detectors, ensuring maintainable cross-platform behavior.
236-247: LGTM! Appropriate Linux-specific paths.The inclusion of
/snap/binis a nice touch for Ubuntu/Snap users. The path list covers common installation locations.
249-273: LGTM! Safewhichinvocation.Using the absolute path
/usr/bin/whichavoids potential issues wherewhichitself might not be in PATH (edge case but possible in minimal environments).MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (4)
93-133: LGTM! Correct override-respecting detection flow.The implementation mirrors
LinuxPlatformDetector.DetectUvexactly, maintaining consistency across Unix-like platforms. The override handling correctly preserves base failure status when user has configured an invalid override.
135-174: LGTM! Consistent Python validation.The implementation is identical to
LinuxPlatformDetector.TryValidatePython, maintaining cross-platform consistency for Unix-like systems.
234-245: LGTM! Correct macOS-specific paths.The path order correctly prioritizes:
/opt/homebrew/bin— Apple Silicon Homebrew/usr/local/bin— Intel Mac Homebrew / manual installs- Standard Unix paths
This matches common macOS configurations and Homebrew installation patterns.
247-271: LGTM! Consistent with Linux implementation.The
TryFindInPathimplementation is identical toLinuxPlatformDetector.TryFindInPath. While this is code duplication, keeping platform-specific code in separate files aids maintainability and platform-specific adjustments.
whatevertogo
left a comment
whatevertogo
left a comment
There was a problem hiding this comment.
unity-mcp Recent Changes Summary
Timeline Overview
204e95a ─┐
25e5d05 ─┤
8d5fa2f ─┤ Series of fixes for platform detectors and path resolution
84cee8c ─┤
6476597 ─┤
42d5aab ─┤
154a8fe ─┘
Commit Details
1️⃣ 204e95a - Fix UV Path Override Detection
Issue: System Requirements panel showed "UV Package Manager: Not Found" even when a valid UV path override was configured.
Fix:
DetectUv() now uses PathResolverService.GetUvxPath() to check override first
Added TryValidateUvExecutable() to verify executables via --version
Fixed process output read order to prevent deadlocks
2️⃣ 25e5d05 - Improve UV/UVX Detection on macOS/Linux
Fix:
Read both stdout and stderr when validating uv/uvx
Respect WaitForExit timeout return value
Parse versions with extra tokens like (Homebrew 2025-01-01)
Resolve bare commands to absolute paths
3️⃣ 8d5fa2f - Refactor: Unify Process Execution
Refactor:
Replaced direct Process.Start with ExecPath.TryRun across all platform detectors
Added Windows PATH augmentation (common uv, npm, Python paths)
Fixed potential deadlocks with async output reading
Added proper timeout handling
4️⃣ 84cee8c - Improve Version Parsing
Fix: Handle multiple version output formats
uvx 0.9.18 → 0.9.18
uvx 0.9.18 (hash date) → 0.9.18
5️⃣ 6476597 - Fix Platform Detector Path Resolution
Fix:
Generalized FindUvxExecutableInPath to support any command name
Fixed BuildAugmentedPath PATH duplication issue
Python version detection now checks both stdout and stderr
Unified logic across all three platform detectors
6️⃣ 42d5aab - Fix Override Path Validation Logic
Fix: Clear behavior for three states
State | Behavior -- | -- No override | Use auto-discovery, UI shows neutral Valid override | Use override path, UI shows "Valid" Invalid override | Return null, UI shows "Invalid"
Key Improvements
Path Override Priority: User setting → System PATH → Auto-discovery
Cross-Platform Consistency: Unified detection logic for Windows/macOS/Linux
Version Parsing Robustness: Compatible with multiple output formats
Process Execution Safety: Deadlock prevention, timeout handling
|
@whatevertogo why are there so many deletions? Is it possible to create a more focused PR with just the relevant changes? |
- Rename TryValidateUvExecutable -> TryValidateUvxExecutable for consistency - Add cross-platform FindInPath() helper in ExecPath.cs - Remove platform-specific where/which implementations in favor of unified helper - Add Windows-specific DetectUv() override with enhanced uv/uvx detection - Add WinGet shim path support for Windows uvx installation - Update UI labels: "UV Path" -> "UVX Path" - Only show uvx path status when override is configured Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
McpSettingsSection.UpdatePathOverrides, the new logic ignoreshasOverrideand will mark the uvx status as invalid when no override is set but discovery fails; this changes the semantics from “override validity” to “overall uv availability” and may confuse users who haven’t configured an override—consider only validating/flagging whenhasOverrideis true. PathResolverService.GetClaudeCliPathstill only checksFile.Existsfor the override, while uv now usesTryValidateUvExecutable; for consistency and to catch broken or non-executable Claude CLIs, you might want a similar validation routine (e.g., runningclaude --version) instead of just existence.- The
BuildAugmentedPath/GetPathAdditionspatterns in the macOS, Linux, and Windows platform detectors are now quite similar but duplicated; consider extracting shared logic or a helper in a common place so PATH augmentation behavior stays consistent and easier to adjust across platforms.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `McpSettingsSection.UpdatePathOverrides`, the new logic ignores `hasOverride` and will mark the uvx status as invalid when no override is set but discovery fails; this changes the semantics from “override validity” to “overall uv availability” and may confuse users who haven’t configured an override—consider only validating/flagging when `hasOverride` is true.
- `PathResolverService.GetClaudeCliPath` still only checks `File.Exists` for the override, while uv now uses `TryValidateUvExecutable`; for consistency and to catch broken or non-executable Claude CLIs, you might want a similar validation routine (e.g., running `claude --version`) instead of just existence.
- The `BuildAugmentedPath`/`GetPathAdditions` patterns in the macOS, Linux, and Windows platform detectors are now quite similar but duplicated; consider extracting shared logic or a helper in a common place so PATH augmentation behavior stays consistent and easier to adjust across platforms.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Helpers/ExecPath.cs:240-241` </location>
<code_context>
};
string path = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
psi.EnvironmentVariables["PATH"] = string.IsNullOrEmpty(path) ? prependPath : (prependPath + Path.PathSeparator + path);
+
using var p = Process.Start(psi);
</code_context>
<issue_to_address>
**issue:** Guard against both PATH and prependPath being null or empty when setting the child process PATH.
In `Which`, if the current `PATH` is empty and `prependPath` is `null` or empty, this line will set `psi.EnvironmentVariables["PATH"]` to `null`, which can cause the process start to fail on some runtimes and is unnecessary. Instead, build an effective PATH only when you have a non-empty value, e.g.:
```csharp
var path = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
var effectivePrepend = string.IsNullOrEmpty(prependPath)
? string.Empty
: prependPath + Path.PathSeparator;
var effectivePath = string.IsNullOrEmpty(path)
? effectivePrepend.TrimEnd(Path.PathSeparator)
: effectivePrepend + path;
if (!string.IsNullOrEmpty(effectivePath))
psi.EnvironmentVariables["PATH"] = effectivePath;
```
The same consideration applies anywhere else `prependPath` is used to construct `PATH`.
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Services/PathResolverService.cs:268-277` </location>
<code_context>
+ /// <param name="uvPath">Absolute or relative path to the uv/uvx executable.</param>
+ /// <param name="version">Parsed version string if successful.</param>
+ /// <returns>True when the executable runs and returns a uv version string.</returns>
+ public bool TryValidateUvExecutable(string uvPath, out string version)
+ {
+ version = null;
+
+ if (string.IsNullOrEmpty(uvPath))
+ return false;
+
try
{
- var psi = new ProcessStartInfo
+ // Check if the path is just a command name (no directory separator)
+ bool isBareCommand = !uvPath.Contains('/') && !uvPath.Contains('\\');
+
+ if (isBareCommand)
</code_context>
<issue_to_address>
**suggestion:** Bare-command detection via path separator checks can misclassify some Windows-style paths.
Current detection treats anything without `/` or `\` as a bare command, which misclassifies valid Windows paths like `"C:uvx.exe"` (drive-relative) and any future path syntaxes that lack separators.
To harden this without much complexity, consider using `Path.IsPathRooted` plus a separator check instead:
```csharp
bool isBareCommand = !Path.IsPathRooted(uvPath) && uvPath.IndexOfAny(new[] { '/', '\' }) < 0;
```
This still keeps the logic simple while avoiding rooted / drive-qualified paths being treated as bare commands.
Suggested implementation:
```csharp
try
{
// Check if the path is just a command name (non-rooted and no directory separator)
bool isBareCommand = !Path.IsPathRooted(uvPath) && uvPath.IndexOfAny(new[] { '/', '\\' }) < 0;
if (isBareCommand)
{
```
1. Ensure the file has `using System.IO;` at the top with the other `using` directives so that `Path.IsPathRooted` resolves correctly.
2. If your project uses fully qualified names instead of `using System.IO;`, replace `Path.IsPathRooted(uvPath)` with `System.IO.Path.IsPathRooted(uvPath)`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| string path = Environment.GetEnvironmentVariable("PATH") ?? string.Empty; | ||
| psi.EnvironmentVariables["PATH"] = string.IsNullOrEmpty(path) ? prependPath : (prependPath + Path.PathSeparator + path); |
There was a problem hiding this comment.
issue: Guard against both PATH and prependPath being null or empty when setting the child process PATH.
In Which, if the current PATH is empty and prependPath is null or empty, this line will set psi.EnvironmentVariables["PATH"] to null, which can cause the process start to fail on some runtimes and is unnecessary. Instead, build an effective PATH only when you have a non-empty value, e.g.:
var path = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
var effectivePrepend = string.IsNullOrEmpty(prependPath)
? string.Empty
: prependPath + Path.PathSeparator;
var effectivePath = string.IsNullOrEmpty(path)
? effectivePrepend.TrimEnd(Path.PathSeparator)
: effectivePrepend + path;
if (!string.IsNullOrEmpty(effectivePath))
psi.EnvironmentVariables["PATH"] = effectivePath;The same consideration applies anywhere else prependPath is used to construct PATH.
…ides and system paths
whatevertogo
left a comment
whatevertogo
left a comment
There was a problem hiding this comment.
efactor: unify uv/uvx naming and path detection across platforms
Rename TryValidateUvExecutable -> TryValidateUvxExecutable for consistency
Add cross-platform FindInPath() helper in ExecPath.cs
Remove platform-specific where/which implementations in favor of unified helper
Add Windows-specific DetectUv() override with enhanced uv/uvx detection
Add WinGet shim path support for Windows uvx installation
Update UI labels: "UV Path" -> "UVX Path"
Only show uvx path status when override is configured
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- You now have uv version parsing logic duplicated in several places (PathResolverService.TryValidateUvxExecutable, Windows/Mac/Linux TryValidateUvWithPath); consider extracting a shared helper to avoid future divergence in how uv/uvx output is interpreted.
- BuildAugmentedPath/GetPathAdditions are implemented separately on Windows, macOS, and Linux with very similar patterns; factoring this into a common utility (with per-OS configuration) would reduce duplication and make PATH augmentation changes easier to maintain.
- In PathResolverService.TryValidateUvxExecutable the XML documentation parameter name (
uvPath) does not match the method parameter (uvxPath), which could be confusing when using or maintaining this API; aligning the naming would improve clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You now have uv version parsing logic duplicated in several places (PathResolverService.TryValidateUvxExecutable, Windows/Mac/Linux TryValidateUvWithPath); consider extracting a shared helper to avoid future divergence in how uv/uvx output is interpreted.
- BuildAugmentedPath/GetPathAdditions are implemented separately on Windows, macOS, and Linux with very similar patterns; factoring this into a common utility (with per-OS configuration) would reduce duplication and make PATH augmentation changes easier to maintain.
- In PathResolverService.TryValidateUvxExecutable the XML documentation parameter name (`uvPath`) does not match the method parameter (`uvxPath`), which could be confusing when using or maintaining this API; aligning the naming would improve clarity.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Helpers/ExecPath.cs:233-236` </location>
<code_context>
+ /// Cross-platform path lookup. Uses 'where' on Windows, 'which' on macOS/Linux.
+ /// Returns the full path if found, null otherwise.
+ /// </summary>
+ internal static string FindInPath(string executable, string extraPathPrepend = null)
+ {
+#if UNITY_EDITOR_WIN
+ return FindInPathWindows(executable);
+#elif UNITY_EDITOR_OSX || UNITY_EDITOR_LINUX
+ return Which(executable, extraPathPrepend ?? string.Empty);
</code_context>
<issue_to_address>
**issue (bug_risk):** On Windows, `extraPathPrepend` is ignored in `FindInPath`, so callers’ augmented PATH additions have no effect.
Callers like `WindowsPlatformDetector.TryFindInPath` pass `BuildAugmentedPath()` expecting those paths to be searched first, but on Windows `FindInPath` drops `extraPathPrepend` and just calls `FindInPathWindows(executable)`. If augmented PATH lookups should work on Windows too, either `FindInPathWindows` needs to accept and prepend `extraPathPrepend` (as `Which` does), or `FindInPath` should inject it into the environment before invoking `where`.
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Services/PathResolverService.cs:326-331` </location>
<code_context>
}
- private static string ResolveUvxFromSystem()
+ private string FindUvxExecutableInPath(string commandName)
{
try
{
- foreach (string candidate in EnumerateUvxCandidates())
+ // Generic search for any command in PATH and common locations
+ foreach (string candidate in EnumerateCommandCandidates(commandName))
{
if (!string.IsNullOrEmpty(candidate) && File.Exists(candidate))
</code_context>
<issue_to_address>
**suggestion:** Custom PATH scanning for `uvx` may miss valid Windows shim types (e.g. `.cmd`), leading to false negatives compared to `ExecPath.FindInPath`.
On Windows, `EnumerateCommandCandidates` only yields `.exe` variants and omits other common shim types like `.cmd`. As a result, `uvx.cmd` on PATH would be found by `ExecPath.FindInPath` but not by this logic, causing `TryValidateUvxExecutable("uvx")` to fail. To avoid these discrepancies, consider delegating to `ExecPath.FindInPath` (with the appropriate `prependPath`) instead of maintaining a separate candidate enumeration.
Suggested implementation:
```csharp
private string FindUvxExecutableInPath(string commandName)
{
try
{
// Delegate to ExecPath to ensure consistent PATH resolution
var resolvedPath = ExecPath.FindInPath(commandName);
if (!string.IsNullOrEmpty(resolvedPath) && File.Exists(resolvedPath))
{
return resolvedPath;
}
}
catch (Exception ex)
{
McpLog.Debug($"PathResolver error: {ex.Message}");
}
```
1. If your codebase uses an overload like `ExecPath.FindInPath(string commandName, string prependPath)`, adjust the call to pass the appropriate `prependPath` argument (e.g. `ExecPath.FindInPath(commandName, prependPath)`).
2. If `EnumerateCommandCandidates` is no longer used anywhere, consider removing it to keep the code clean.
3. Ensure the namespace containing `ExecPath` is imported at the top of this file (e.g. `using <YourNamespace>.Exec;`) if it isn’t already.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (1)
200-226: Good validation improvement, but potential path inconsistency.The validation logic correctly uses
TryValidateUvxExecutableto verify executables. However, there's a subtle inconsistency: the display value usespathService.GetUvxPath()(line 193) while validation uses the rawEditorPrefs.GetString()value (line 204). IfGetUvxPath()applies any normalization or processing, the display and validation could diverge.Consider using the same path source for both display and validation:
♻️ Suggested simplification
if (hasOverride) { // Override mode: validate the override path - string overridePath = EditorPrefs.GetString(EditorPrefKeys.UvxPathOverride, string.Empty); - if (pathService.TryValidateUvxExecutable(overridePath, out _)) + if (!string.IsNullOrEmpty(uvxPath) && pathService.TryValidateUvxExecutable(uvxPath, out _)) { uvxPathStatus.AddToClassList("valid"); }MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (2)
178-226: Code duplication:TryValidateUvWithPathis identical across all platform detectors.This method is copy-pasted in LinuxPlatformDetector, MacOSPlatformDetector, and WindowsPlatformDetector with identical logic. Consider extracting this to
PlatformDetectorBaseor a shared utility class to reduce maintenance burden and ensure consistent behavior across platforms.♻️ Suggested refactor: Move to base class
// In PlatformDetectorBase.cs protected bool TryValidateUvWithPath(string command, string augmentedPath, out string version, out string fullPath) { version = null; fullPath = null; try { string commandToRun = command; if (TryFindInPath(command, out string resolvedPath)) { commandToRun = resolvedPath; } if (!ExecPath.TryRun(commandToRun, "--version", null, out string stdout, out string stderr, 5000, augmentedPath)) return false; string output = string.IsNullOrWhiteSpace(stdout) ? stderr.Trim() : stdout.Trim(); if (output.StartsWith("uvx ") || output.StartsWith("uv ")) { int spaceIndex = output.IndexOf(' '); if (spaceIndex >= 0) { var remainder = output.Substring(spaceIndex + 1).Trim(); int nextSpace = remainder.IndexOf(' '); int parenIndex = remainder.IndexOf('('); int endIndex = Math.Min( nextSpace >= 0 ? nextSpace : int.MaxValue, parenIndex >= 0 ? parenIndex : int.MaxValue ); version = endIndex < int.MaxValue ? remainder.Substring(0, endIndex).Trim() : remainder; fullPath = commandToRun; return true; } } } catch { // Ignore validation errors } return false; } // Add abstract method for subclasses to implement protected abstract bool TryFindInPath(string executable, out string fullPath);
162-167: Python version check logic is slightly off.The condition
major > 3 || (major >= 3 && minor >= 10)has a redundant check. Ifmajor > 3, Python 4+ would pass regardless of minor version, which is correct. But the second condition(major >= 3 && minor >= 10)would incorrectly reject Python 3.10+ whenmajor == 3is already covered. The logic works correctly but is clearer as:return major > 3 || (major == 3 && minor >= 10);This is a minor readability improvement and doesn't affect correctness.
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
128-128: Non-English comment should be translated for consistency.Line 128 contains a Chinese comment
// 真实的路径反馈which means "real path feedback". For codebase consistency and maintainability, please translate to English.♻️ Suggested fix
- status.Details = $"Found uv {uvVersion} at {uvPath}"; // 真实的路径反馈 + status.Details = $"Found uv {uvVersion} at {uvPath}"; // Actual path feedback
324-328: Hardcoded Python version paths will need updates for future Python releases.The hardcoded Python version directories (Python313, Python312, etc.) will require manual updates as new Python versions are released. Consider a more dynamic approach if feasible.
♻️ Alternative: Dynamic version discovery
// Instead of hardcoded versions, enumerate existing directories if (!string.IsNullOrEmpty(programFiles)) { try { var pythonDirs = Directory.GetDirectories(programFiles, "Python3*") .OrderByDescending(d => d); // Newest first foreach (var dir in pythonDirs) { additions.Add(dir); } } catch { /* Ignore if directory doesn't exist */ } }This is optional since the current approach is simpler and covers the most common versions.
MCPForUnity/Editor/Services/PathResolverService.cs (3)
52-78:ResolveUvxFromSystemonly checks file existence, not executability.The method returns the first candidate that passes
File.Exists(), but doesn't validate that it's actually a working UV executable. If a user has a corrupted or incompatible file at an expected path, this would return it as valid.Consider using
TryValidateUvxExecutablefor discovered paths to ensure they're functional:♻️ Suggested improvement
foreach (string candidate in EnumerateUvxCandidates(commandName)) { - if (!string.IsNullOrEmpty(candidate) && File.Exists(candidate)) + if (!string.IsNullOrEmpty(candidate) && File.Exists(candidate) && + TryValidateUvxExecutable(candidate, out _)) { return candidate; } }Note: This adds overhead for each candidate check. An alternative is to validate only the first existing file found, returning null if validation fails.
83-142: Duplication betweenEnumerateUvxCandidatesandEnumerateCommandCandidates.These two methods have nearly identical logic for enumerating PATH, user directories, and platform-specific locations. The main difference is that
EnumerateUvxCandidateshandles Windows.exeextension removal at lines 97-101.Consider consolidating into a single generic enumeration method to reduce duplication.
Also applies to: 347-405
270-324: Version parsing logic is duplicated from platform detectors.The version extraction logic (lines 299-316) is identical to
TryValidateUvWithPathin the platform detectors. SincePathResolverService.TryValidateUvxExecutableis now the authoritative validation method, consider having the platform detectors call this instead of duplicating the parsing logic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Helpers/ExecPath.csMCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.csMCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Helpers/ExecPath.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Services/IPathResolverService.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Services/PathResolverService.csMCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.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/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.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/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.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/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.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/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
🧬 Code graph analysis (6)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (4)
BuildAugmentedPath(226-233)TryValidateUvWithPath(176-224)TryFindInPath(248-252)GetPathAdditions(235-246)MCPForUnity/Editor/Helpers/ExecPath.cs (3)
ExecPath(12-312)TryRun(161-227)FindInPath(233-242)
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
MCPForUnity/Editor/Services/PathResolverService.cs (1)
TryValidateUvxExecutable(270-324)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (4)
BuildAugmentedPath(228-235)TryValidateUvWithPath(178-226)TryFindInPath(250-254)GetPathAdditions(237-248)MCPForUnity/Editor/Helpers/ExecPath.cs (3)
ExecPath(12-312)TryRun(161-227)FindInPath(233-242)MCPForUnity/Editor/External/Tommy.cs (1)
Join(2000-2013)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (4)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
uvxPath(188-195)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/IPathResolverService.cs (2)
GetUvxPath(12-12)TryValidateUvxExecutable(70-70)MCPForUnity/Editor/Services/PathResolverService.cs (2)
GetUvxPath(22-46)TryValidateUvxExecutable(270-324)
MCPForUnity/Editor/Services/PathResolverService.cs (3)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)MCPForUnity/Editor/Services/IPathResolverService.cs (7)
TryValidateUvxExecutable(70-70)GetClaudeCliPath(18-18)IsClaudeCliDetected(30-30)SetUvxPathOverride(36-36)ClearUvxPathOverride(47-47)SetClaudeCliPathOverride(42-42)ClearClaudeCliPathOverride(52-52)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-312)TryRun(161-227)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (3)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)MCPForUnity/Editor/Services/IPathResolverService.cs (2)
TryValidateUvxExecutable(70-70)GetUvxPath(12-12)MCPForUnity/Editor/Services/PathResolverService.cs (2)
TryValidateUvxExecutable(270-324)GetUvxPath(22-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (13)
MCPForUnity/Editor/Helpers/ExecPath.cs (3)
229-242: LGTM! Good cross-platform abstraction.The
FindInPathmethod provides a clean unified API for path lookup across platforms. The conditional compilation correctly delegates to platform-specific implementations.
257-276: LGTM! Proper async output handling prevents deadlocks.The refactored
Whichmethod correctly usesBeginOutputReadLine()withOutputDataReceivedevents, avoiding the deadlock that can occur when readingStandardOutput.ReadToEnd()beforeWaitForExit(). The doubleWaitForExit()pattern (first with timeout, then parameterless) ensures async buffers are fully flushed.
279-310: LGTM! Consistent async pattern applied to Windows path lookup.The renamed
FindInPathWindowsmethod mirrors the async output handling pattern fromWhich, maintaining consistency across platforms.MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml (1)
23-23: LGTM! UI label aligns with terminology changes.The label rename from "UV Path:" to "UVX Path:" is consistent with the broader terminology updates across the codebase.
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
40-48: LGTM! Error message aligns with UVX terminology.The updated message correctly reflects the tool name and provides actionable guidance to users.
MCPForUnity/Editor/Services/IPathResolverService.cs (1)
63-70: LGTM! Well-designed interface addition.The
TryValidateUvxExecutablemethod follows .NET conventions (TryXxx pattern withoutparameter), includes comprehensive XML documentation, and enables centralized executable validation. The implementation (per relevant snippets) correctly handles both absolute paths and bare commands.MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (1)
29-45: Consider adding explicit null check foruvxPath.
GetUvxPath()can returnnullwhen an override is set but invalid (per PathResolverService lines 33-34). WhenuvxPathis null,TryValidateUvxExecutable(null, ...)will handle it gracefully (returns false per line 274-275 in PathResolverService), but thestatus.Pathassignment at line 37 would set it to null silently. The error messaging at lines 44-45 provides appropriate guidance, so this is acceptable behavior.The refactored implementation correctly addresses PR objective #538 by consulting
PathResolverService.GetUvxPath()which respects the override setting.MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
95-134: Well-structured delegation pattern for override handling.The implementation correctly:
- Delegates to
base.DetectUv()first to respect overrides- Returns early if base found UV or if an override is configured (even if invalid)
- Falls back to platform-specific PATH augmentation only when no override is set
- Clears
ErrorMessageon line 122 when discovery succeedsThis addresses the PR objective of ensuring System Requirements respects the UV path override.
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
103-151: Consistent implementation with Linux/macOS detectors.The Windows detector follows the same delegation pattern, correctly handling override paths and falling back to platform-specific discovery. The additional Windows-specific path augmentation for uv, npm, and Python locations is appropriate.
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
93-133: Consistent implementation across platforms.The macOS detector correctly follows the same delegation pattern as Linux and Windows:
- Delegates to
base.DetectUv()first- Respects override path settings
- Falls back to platform-specific discovery with augmented PATH
- Clears
ErrorMessageon successful discovery (line 120)The Homebrew path
/opt/homebrew/binis correctly prioritized for Apple Silicon Macs.
235-246: Path additions are appropriate for macOS.The path additions correctly cover:
/opt/homebrew/bin- Homebrew on Apple Silicon/usr/local/bin- Homebrew on Intel Macs and system binaries- Standard Unix paths
- User local bin directory
This aligns with the learning about macOS path handling in this project.
MCPForUnity/Editor/Services/PathResolverService.cs (2)
22-46: Good design: Override path takes precedence with no silent fallback.The implementation correctly ensures that when a user explicitly configures an override path:
- The override is validated first (line 29)
- If valid, it's returned immediately
- If invalid,
nullis returned (line 34) - no silent fallback to system discoveryThis behavior is intentional and correct per the PR objectives. It prevents confusion where a user configures an override but the system silently uses a different executable.
The fallback to bare
"uvx"at line 45 only happens when no override is set and system discovery fails.
222-262: Override setters correctly validate file existence.
SetUvxPathOverrideandSetClaudeCliPathOverrideproperly:
- Handle null/empty by clearing the override
- Validate file existence before saving
- Throw
ArgumentExceptionfor invalid pathsThis ensures UI validation feedback when users configure invalid paths in Advanced Settings.
…ethods across platform detectors
There was a problem hiding this comment.
Actionable comments posted: 0
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/WindowsPlatformDetector.cs (1)
154-192: Replaceuvx python listwithuv python list.Line 163 runs
uvx python list, but the correct command to list installed Python versions isuv python list. Theuvxtool is for running tools from packages, not foruvsubcommands.🐛 Proposed fix
- if (!ExecPath.TryRun("uvx", "python list", null, out string stdout, out string stderr, 5000, augmentedPath)) + if (!ExecPath.TryRun("uv", "python list", null, out string stdout, out string stderr, 5000, augmentedPath))
🧹 Nitpick comments (3)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
160-174: Consider aligning validation withTryValidateUvxExecutable.
SetUvxPathOverrideonly checksFile.Exists(path)before saving, whileGetUvxPathusesTryValidateUvxExecutablewhich runs--version. This inconsistency means a user could set an override to a non-executable file (e.g., a text file), and it would be accepted but fail validation later.Consider using
TryValidateUvxExecutablefor consistency, or at minimum document this design choice.♻️ Optional: Validate executable before saving
public void SetUvxPathOverride(string path) { if (string.IsNullOrEmpty(path)) { ClearUvxPathOverride(); return; } - if (!File.Exists(path)) + if (!TryValidateUvxExecutable(path, out _)) { - throw new ArgumentException("The selected uvx executable does not exist"); + throw new ArgumentException("The selected path is not a valid uv/uvx executable"); } EditorPrefs.SetString(EditorPrefKeys.UvxPathOverride, path); }
208-262: Version parsing logic is duplicated.The version parsing logic (lines 236-254) is nearly identical to
PlatformDetectorBase.TryValidateUvWithPath(lines 96-111 in PlatformDetectorBase.cs). Consider extracting this to a shared helper to reduce duplication.MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (1)
76-120: Version parsing duplicated withPathResolverService.TryValidateUvxExecutable.This method and
PathResolverService.TryValidateUvxExecutable(lines 236-254) share nearly identical version extraction logic. The duplication could lead to divergent behavior if one is updated without the other.Consider extracting the version parsing to a shared utility method.
♻️ Example shared helper approach
Create a static helper in
ExecPathor a new utility class:// In ExecPath.cs or a new UvVersionParser utility internal static bool TryParseUvVersion(string output, out string version) { version = null; if (string.IsNullOrWhiteSpace(output)) return false; string trimmed = output.Trim(); if (!trimmed.StartsWith("uvx ") && !trimmed.StartsWith("uv ")) return false; int spaceIndex = trimmed.IndexOf(' '); if (spaceIndex < 0) return false; var remainder = trimmed.Substring(spaceIndex + 1).Trim(); int nextSpace = remainder.IndexOf(' '); int parenIndex = remainder.IndexOf('('); int endIndex = Math.Min( nextSpace >= 0 ? nextSpace : int.MaxValue, parenIndex >= 0 ? parenIndex : int.MaxValue ); version = endIndex < int.MaxValue ? remainder.Substring(0, endIndex).Trim() : remainder; return true; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.cs
🧰 Additional context used
🧠 Learnings (5)
📚 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/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.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/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.csMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.csMCPForUnity/Editor/Services/PathResolverService.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/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.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/Dependencies/PlatformDetectors/LinuxPlatformDetector.csMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (6)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/IPathResolverService.cs (2)
GetUvxPath(12-12)TryValidateUvxExecutable(70-70)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
TryFindInPath(200-204)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
TryFindInPath(198-202)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
TryFindInPath(235-239)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-312)TryRun(161-227)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (3)
BuildAugmentedPath(178-185)TryFindInPath(200-204)GetPathAdditions(187-198)MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (2)
TryValidateUvWithPath(77-120)TryFindInPath(124-124)MCPForUnity/Editor/Helpers/ExecPath.cs (3)
ExecPath(12-312)TryRun(161-227)FindInPath(233-242)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (15)
MCPForUnity/Editor/Services/PathResolverService.cs (3)
22-46: LGTM - Override-first path resolution with intentional no-fallback behavior.The logic correctly prioritizes the override path and returns
nullwhen an invalid override is set, rather than silently falling back to discovery. This aligns with the PR objective to make System Requirements respect the configured override.
52-78: LGTM - System discovery for uv/uvx.The method correctly tries
uvxbeforeuvand uses centralized candidate enumeration. The exception handling with debug logging is appropriate.
289-343: LGTM - Comprehensive candidate enumeration.The method covers PATH, user-local directories, and platform-specific install locations including WinGet shims. The platform-conditional logic is clean.
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (3)
93-133: LGTM - Clean delegation pattern for UV detection.The implementation correctly:
- Delegates to base for override handling
- Returns early if override is configured (valid or invalid)
- Uses augmented PATH for platform-specific discovery
- Clears
ErrorMessageon success to override any base error
135-174: LGTM - Python validation with proper version checking.The method correctly validates Python 3.10+ requirement and handles both stdout/stderr output locations.
176-202: LGTM - Path augmentation and lookup.
BuildAugmentedPathandTryFindInPathare consistent with the Linux detector implementation and correctly useExecPathhelpers.MCPForUnity/Editor/Dependencies/PlatformDetectors/PlatformDetectorBase.cs (3)
21-54: Core fix: UV detection now respects configured overrides.This implementation correctly uses
MCPServiceLocator.Paths.GetUvxPath()which checks the override first, addressing the root cause of issue #538. The status details appropriately distinguish between override and system paths for user clarity.
56-75: LGTM - Clean version parsing helper.Simple and effective parsing with proper error handling.
121-124: LGTM - Abstract method enables platform-specific path resolution.Clean design allowing each platform detector to implement its own PATH augmentation strategy.
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (3)
103-151: LGTM - Windows UV detection follows consistent pattern.The implementation correctly delegates to base for override handling and uses Windows-specific path augmentation for fallback discovery.
194-233: LGTM - Python validation with proper version checking.Correctly validates Python 3.10+ and handles output from both stdout and stderr.
250-294: LGTM - Comprehensive Windows path additions.Good coverage including:
- UV install locations (LocalAppData, ProgramFiles)
- npm global paths
- Python directories (dynamically enumerated)
- User scripts directory
The dynamic enumeration of Python directories (lines 279-284) is a nice touch to avoid hardcoding versions.
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (3)
95-135: LGTM - Linux UV detection follows consistent pattern.The implementation correctly:
- Delegates to base for override handling
- Returns early if override is configured
- Uses Linux-specific augmented PATH
- Clears
ErrorMessageon success
137-176: LGTM - Python validation consistent with other platforms.Same pattern as macOS and Windows detectors with proper Python 3.10+ validation.
178-204: LGTM - Linux-specific path handling.
GetPathAdditionscorrectly includes/snap/binfor snap-installed packages, and theTryFindInPathoverride delegates toExecPath.FindInPathconsistently with other platforms.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
80-84: Consider case-insensitive comparison and stdio mode consistency.Two potential issues:
Case sensitivity: This uses
==for string comparison, but line 193 usesStringComparison.OrdinalIgnoreCasefor a similar client name check. Consider using case-insensitive comparison for consistency and robustness.Stdio mode: If Claude Code requires the
typefield for HTTP transport, does it also need it for stdio transport? Lines 112-115 only settypefor VSCode clients in stdio mode, potentially leaving Claude Code without thetypefield when using stdio.♻️ Suggested fix for case-insensitive comparison
- else if (client?.name == "Claude Code") + else if (string.Equals(client?.name, "Claude Code", StringComparison.OrdinalIgnoreCase))Consider also adding Claude Code handling in the stdio branch (around line 115) if the
typefield is required for both transport modes.
118-122: Use case-insensitive comparison for consistency.Same issue as above: use case-insensitive comparison to match the pattern used elsewhere in the codebase.
♻️ Suggested fix
- if (!isVSCode && client?.name != "Claude Code" && unity["type"] != null) + if (!isVSCode && !string.Equals(client?.name, "Claude Code", StringComparison.OrdinalIgnoreCase) && unity["type"] != null)Additionally, consider extracting
"Claude Code"to a constant (similar toClaudeDesktopConfigurator.ClientNameat line 193) to avoid magic strings and reduce the risk of typos across multiple comparison sites.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
… WindowsPlatformDetector
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs:
- Around line 163-164: In TryFindPythonViaUv, the ExecPath.TryRun call uses "uv"
which won't match the other checks; update the call to use "uv.exe" (so change
the ExecPath.TryRun invocation that currently passes "uv" to pass "uv.exe"
instead) to match the existing uses of "uv.exe"/"uvx.exe" and ensure
ExecPath.TryRun can locate the executable on Windows.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
289-291: Unix-style path in Windows detector.
~/.local/binis a Unix/macOS convention that typically doesn't exist on Windows. This is harmless but unnecessary in a Windows-specific detector.Consider removing or replacing with Windows equivalent
- // User scripts - if (!string.IsNullOrEmpty(homeDir)) - additions.Add(Path.Combine(homeDir, ".local", "bin")); + // User scripts (Windows typically uses AppData paths, already covered above)Or if you want to keep a user scripts path, consider:
- // User scripts - if (!string.IsNullOrEmpty(homeDir)) - additions.Add(Path.Combine(homeDir, ".local", "bin")); + // User scripts + if (!string.IsNullOrEmpty(localAppData)) + additions.Add(Path.Combine(localAppData, "Microsoft", "WindowsApps"));
277-287: Lexicographic ordering may not reflect actual version order.
OrderByDescending(d => d)uses lexicographic ordering, so "Python39" would sort after "Python310" (since '9' > '1'). However, since the minimum Python requirement is 3.10+, this only affects the search order for valid versions (310, 311, 312, 313), where lexicographic ordering happens to work correctly.If future-proofing is desired:
Extract version number for proper sorting
var pythonDirs = Directory.GetDirectories(programFiles, "Python3*") - .OrderByDescending(d => d); // Newest first + .OrderByDescending(d => + { + var name = Path.GetFileName(d); + if (name.StartsWith("Python") && int.TryParse(name.Substring(6), out int ver)) + return ver; + return 0; + }); // Newest first by version number
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
🧰 Additional context used
🧠 Learnings (3)
📚 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/WindowsPlatformDetector.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/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (5)
1-11: LGTM!The new imports for
Collections.Generic,Linq,Constants, andServicesare appropriate for the added path enumeration and service locator functionality.
23-76: LGTM!The Python detection flow with fallback chain (direct validation → PATH lookup → uv-based discovery) is well-structured and handles errors appropriately.
103-151: Override handling correctly implemented.The method properly delegates to
base.DetectUv()first to respect configured overrides viaPathResolverService, and only falls back to Windows-specific augmented PATH detection when no override is set. This aligns with the PR objective of ensuring System Requirements respects the UV Path override.
194-233: LGTM!The Python validation correctly handles version output from both stdout and stderr, and the version requirement check (
major > 3 || (major == 3 && minor >= 10)) accurately enforces Python 3.10+.
235-248: LGTM!The
TryFindInPathoverride correctly delegates toExecPath.FindInPath, andBuildAugmentedPathproperly returns only the additions for ExecPath to prepend to the existing PATH.
| if (!ExecPath.TryRun("uv", "python list", null, out string stdout, out string stderr, 5000, augmentedPath)) | ||
| return false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ExecPath.cs file
find . -name "ExecPath.cs" -type f
# Search for TryRun method implementation
rg -n "public static bool TryRun" --type cs -A 20 -B 2Repository: CoplayDev/unity-mcp
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Read ExecPath.cs to see TryRun implementation
cat ./MCPForUnity/Editor/Helpers/ExecPath.csRepository: CoplayDev/unity-mcp
Length of output: 12440
🏁 Script executed:
#!/bin/bash
# Check WindowsPlatformDetector.cs around the mentioned lines
sed -n '115,200p' ./MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 3775
Fix executable name inconsistency: use "uv.exe" on line 163.
Line 163 uses "uv" without the .exe extension, but lines 123 and 133 explicitly use "uv.exe" and "uvx.exe". Since ExecPath.TryRun doesn't auto-append the extension on Windows, this will cause TryFindPythonViaUv to fail to locate the uv executable. Change line 163 from ExecPath.TryRun("uv", ...) to ExecPath.TryRun("uv.exe", ...) to be consistent with the other uv lookups.
🤖 Prompt for AI Agents
In @MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
around lines 163 - 164, In TryFindPythonViaUv, the ExecPath.TryRun call uses
"uv" which won't match the other checks; update the call to use "uv.exe" (so
change the ExecPath.TryRun invocation that currently passes "uv" to pass
"uv.exe" instead) to match the existing uses of "uv.exe"/"uvx.exe" and ensure
ExecPath.TryRun can locate the executable on Windows.
There was a problem hiding this comment.
both work in windows i think
There was a problem hiding this comment.
@whatevertogo You're correct! Windows Command Prompt uses the PATHEXT environment variable to determine which extensions to look for when running a command without the extension, so both "uv" and "uv.exe" will work through the "where" command used by ExecPath.FindInPath.
The inconsistency is stylistic rather than functional. Since both work correctly on Windows, feel free to keep it as-is. Thanks for clarifying!
whatevertogo
changed the title
i have one question.Now i change the logic like this. When override path is invalid, it no longer falls back to auto-detection in path until you clear it.Is that right? |
3 participants
Summary
Fixes #538 - The System Requirements panel showed "UV Package Manager: Not Found" even when a valid UV path override was configured in Advanced Settings.
Root Cause
PlatformDetectorBase.DetectUv()only searched PATH with bare command names ("uvx","uv") and never consultedPathResolverServicewhich respects the user's override setting.Changes
Refactor
DetectUv()to usePathResolverService.GetUvxPath()which checks:"uvx"commandAdd
TryValidateUvExecutable()- verifies executables by running--versioninstead of just checkingFile.ExistsPrioritize PATH environment variable in
EnumerateUvxCandidates()for better compatibility with official uv install scriptsFix process output read order -
ReadToEndbeforeWaitForExitto prevent potential deadlocksFix version parsing - correctly handle both
uvanduvxversion output formats across all platformsTesting
uv x.y.zanduvx x.y.zformatsCo-Authored-By: ChatGLM 4.7 [email protected]
Summary by Sourcery
Improve uv/uvx detection and validation across platforms and settings, ensuring overrides are honored and PATH discovery is robust.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Bug Fixes
Refactor
UI Updates
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.