feat: installer cleanup, auto-migration of legacy server location to canonical, server logging normalization #239
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an internal Unity Editor logger, a per-package-version package detector that schedules server installation, a version-aware ServerInstaller with legacy detection/cleanup and GetServerPath(), safer prefs/config reads and atomic writes, macOS/Linux path normalizations and client path fields, VSCode mcp.json UI updates, package bumped to 3.0.1, and embedded server_version.txt. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Editor as Unity Editor (startup)
participant PD as PackageDetector
participant SI as ServerInstaller
participant Log as McpLog
Editor->>PD: Initialize ([InitializeOnLoad])
PD->>PD: ReadPackageVersionOrFallback()
PD->>PD: Evaluate shouldRun (per-version flag, legacy roots, missing server.py)
alt should run
PD-->>Editor: Schedule delayCall(EnsureServerInstalled)
Editor->>SI: EnsureServerInstalled()
SI->>SI: Read embedded `server_version.txt`
SI->>SI: Read installed version (if present)
SI->>SI: Compare versions -> decide overwrite
alt overwrite needed
SI->>SI: Copy embedded server -> install dir
SI->>SI: Write version file
SI->>Log: Info/Warn
else up-to-date
SI->>Log: Info (no action)
end
SI->>SI: Detect legacy roots/processes
SI->>SI: TryKillUvForPath + delete legacy dirs
SI->>Log: Warn on failures
PD->>PD: Set per-version flag
else skip
PD->>Log: Info (no-op)
end
sequenceDiagram
autonumber
participant EW as MCPForUnityEditorWindow
participant Pref as EditorPrefs
participant FS as FileSystem
participant Log as McpLog
EW->>Pref: SafeGetPrefBool("MCPForUnity.DebugLogs")
alt debug enabled
EW->>FS: ReadEmbeddedVersionOrFallback()
EW->>Pref: SafeGetPrefString(...) x N
EW->>Log: Info("MCP Debug Prefs: ...")
end
EW->>FS: WriteToConfig(...)
alt merge/update config
EW->>Pref: Update UV path / ServerSrc
EW->>Log: Info/Warn on result
else error
EW->>Log: Warn
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements auto-migration from legacy server installations to canonical locations, along with logging improvements and installer cleanup. The purpose is to provide zero-touch upgrades that automatically heal server and client configurations when moving between package versions.
Key changes:
- Adds automatic detection of legacy server installations with migration to canonical user directories
- Introduces version-aware server management with cleanup of outdated installations
- Implements centralized logging with colored output and debug-level controls
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Version bump from 3.0.0 to 3.0.1 |
| server_version.txt | New version file containing 3.0.1 |
| MCPForUnityEditorWindow.cs | Enhanced debug logging and preference state reporting |
| ServerInstaller.cs | Major refactor with legacy detection, canonical installation, and version management |
| PackageDetector.cs | New auto-detection system that runs on package load/update |
| McpLog.cs | New centralized logging helper with colored output |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@msanatan This is a better, human-guided rework of the PR that Copilot tried to write the other night, which didn't work. This one should work. LMK what you think. Good test is installing package from upstream/main, to installing the package from this branch -- see how it clears out the legacy server location and rewrites the JSON mcp config files to point to the new server. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3)
1076-1202: WriteToConfig should honor the pythonDir parameter and be null-safe for serverSrc.
- The pythonDir argument is currently unused when existing args are invalid. Prefer using the supplied pythonDir if it points to a valid server.py before falling back to ResolveServerSrc().
- Minor: guard the PackageCache check against potential nulls.
Apply this focused diff inside WriteToConfig:
- string serverSrc = ExtractDirectoryArg(existingArgs); + string serverSrc = ExtractDirectoryArg(existingArgs); bool serverValid = !string.IsNullOrEmpty(serverSrc) && System.IO.File.Exists(System.IO.Path.Combine(serverSrc, "server.py")); if (!serverValid) { - serverSrc = ResolveServerSrc(); + // Prefer the provided pythonDir if valid; fall back to resolver + if (!string.IsNullOrEmpty(pythonDir) && System.IO.File.Exists(System.IO.Path.Combine(pythonDir, "server.py"))) + { + serverSrc = pythonDir; + } + else + { + serverSrc = ResolveServerSrc(); + } } // Hard-block PackageCache on Windows unless dev override is set - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) - && serverSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) + && !string.IsNullOrEmpty(serverSrc) + && serverSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 && !UnityEditor.EditorPrefs.GetBool("MCPForUnity.UseEmbeddedServer", false)) { serverSrc = ServerInstaller.GetServerPath(); }
1616-1651: Robust config match: parse --directory and compare normalized paths.Current check uses substring Contains, which is fragile across path normalization, separators, or trailing slashes. You already have ExtractDirectoryArg(...) and PathsEqual(...). Use them here.
- bool matches = pythonDir != null && Array.Exists(args, arg => arg.Contains(pythonDir, StringComparison.Ordinal)); + string configuredDir = ExtractDirectoryArg(args); + bool matches = !string.IsNullOrEmpty(configuredDir) && PathsEqual(configuredDir, pythonDir);This reduces false positives/negatives and aligns with your write logic (which canonicalizes the --directory arg).
1802-1826: Remove unused helper methods to reduce dead code
BothParseTextOutputandFindClaudeCommandare defined but never invoked anywhere in the repository. Keeping them increases maintenance burden and risks drifting from the actual CLI behavior. Please remove these methods:
- UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (lines 1802–1826):
private bool ParseTextOutput(...)- UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (lines 2108–2193):
private string FindClaudeCommand()
🧹 Nitpick comments (13)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs.meta (1)
1-2: .meta appears incomplete for a C# script; align with other script metas to avoid Unity churn.Unlike McpLog.cs.meta, this file lacks the MonoImporter block. Unity may regenerate it, causing noisy diffs. Suggest adding the standard block (keep the same GUID).
Proposed completion:
fileFormatVersion: 2 guid: b82eaef548d164ca095f17db64d15af8 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant:UnityMcpBridge/Editor/Helpers/McpLog.cs (3)
10-13: Extract the EditorPrefs key into a named constant.Avoids magic strings and helps maintain consistency where the key is used elsewhere.
Apply:
internal static class McpLog { - private const string Prefix = "<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>:"; + private const string Prefix = "<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>:"; + private const string DebugPrefsKey = "MCPForUnity.DebugLogs"; private static bool IsDebugEnabled() { - try { return EditorPrefs.GetBool("MCPForUnity.DebugLogs", false); } catch { return false; } + try { return EditorPrefs.GetBool(DebugPrefsKey, false); } catch { return false; } }
21-29: Add overloads for exceptions and Unity object context to improve diagnostics.Overloads help preserve exception details and let Console highlight/ping related assets.
Suggested additions:
// Add inside McpLog public static void Warn(string message, Object context) { Debug.LogWarning($"<color=#cc7a00>{Prefix} {message}</color>", context); } public static void Error(string message, System.Exception ex) { Debug.LogError($"{Prefix} {message}\n{ex}", null); } public static void Error(System.Exception ex, Object context = null) { Debug.LogError($"{Prefix} {ex}", context); }
15-19: Split Info and Verbose logs; migrate debug-only callsTo improve clarity and avoid the implicit
alwaysflag, add an explicitVerbosemethod for debug-only logging, deprecate thealwaysparameter onInfo, and update all existingalways: falseusages:• UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:206
McpLog.Info("Detected legacy install without version file at: " + legacyRoot, always: false);
• UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:210
McpLog.Info("Detected legacy install path: " + legacyRoot, always: false);
• UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:215
McpLog.Info($"Legacy install version {legacyVer} is older than embedded {embeddedVer}", always: false);
• UnityMcpBridge/Editor/Helpers/ServerInstaller.cs:229
McpLog.Info($"Canonical install version {installedVer} is older than embedded {embeddedVer}", always: false);Proposed changes in
UnityMcpBridge/Editor/Helpers/McpLog.cs:- public static void Info(string message, bool always = true) + [Obsolete("Use Info for always-on messages or Verbose for debug-only logging.")] + public static void Info(string message, bool always = true) { if (!always && !IsDebugEnabled()) return; Debug.Log($"{Prefix} {message}"); } + /// <summary>Logs only when debug is enabled.</summary> + public static void Verbose(string message) + { + if (!IsDebugEnabled()) return; + Debug.Log($"{Prefix} {message}"); + }Then, update each call site:
- McpLog.Info("…", always: false); + McpLog.Verbose("…");UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (4)
257-321: Unify detection-key version source and de-duplicate helpers.
- The debug log uses detectKey = "MCPForUnity.LegacyDetectLogged:{embeddedVer}", while PackageDetector uses the package version for the same key. This will look inconsistent in logs when embedded and package versions diverge.
- SafeGetPrefString/Bool and ReadEmbeddedVersionOrFallback are duplicated here and in helpers. Consider centralizing (e.g., EditorPrefsUtil + VersionUtil) to avoid drift.
Action:
- Prefer a single version basis for the detect key (embedded version is fine since it directly gates server install behavior) and share the implementation with PackageDetector.
To align PackageDetector with this file’s embedded-version keying, apply the diff in PackageDetector (see my comment there).
580-581: Logging normalization mostly applied; finish converting remaining Debug.Log calls.*You standardized most warnings to McpLog.Warn (good). There are still scattered UnityEngine.Debug.* calls elsewhere in this class (e.g., Line 669, 960, 1249, 1352, 1456, 1670, 1703, 1713, 1786, 1791, 1898, etc.). Convert these to McpLog.Info/Warn/Error for consistency and easier filtering.
For example (outside the current hunk), change:
- UnityEngine.Debug.LogWarning($"Setup client '{client.name}' failed: {ex.Message}"); + MCPForUnity.Editor.Helpers.McpLog.Warn($"Setup client '{client.name}' failed: {ex.Message}");Also applies to: 597-598, 609-609
1730-1773: Unregister flow: unify candidate arrays and standardize logs.
- You build two nearly identical lists of candidate names (candidateNamesForGet and possibleNames). Keep a single array and reuse it for both “get” and “remove”.
- Convert UnityEngine.Debug.* calls to McpLog.* for consistency.
- string[] candidateNamesForGet = { "UnityMCP", "unityMCP", "unity-mcp", "UnityMcpServer" }; + string[] unityMcpNames = { "UnityMCP", "unityMCP", "unity-mcp", "UnityMcpServer" }; List<string> existingNames = new List<string>(); - foreach (var candidate in candidateNamesForGet) + foreach (var candidate in unityMcpNames) { ... } ... - string[] possibleNames = { "UnityMCP", "unityMCP", "unity-mcp", "UnityMcpServer" }; - bool success = false; - foreach (string serverName in possibleNames) + bool success = false; + foreach (string serverName in unityMcpNames) { if (ExecPath.TryRun(claudePath, $"mcp remove {serverName}", projectDir, out var stdout, out var stderr, 10000, pathPrepend)) { success = true; - UnityEngine.Debug.Log($"MCP for Unity: Successfully removed MCP server: {serverName}"); + MCPForUnity.Editor.Helpers.McpLog.Info($"Successfully removed MCP server: {serverName}"); break; } else if (!string.IsNullOrEmpty(stderr) && !stderr.Contains("No MCP server found", StringComparison.OrdinalIgnoreCase)) { - UnityEngine.Debug.LogWarning($"Error removing {serverName}: {stderr}"); + MCPForUnity.Editor.Helpers.McpLog.Warn($"Error removing {serverName}: {stderr}"); break; } }
1828-2106: Deduplicate uv discovery: call ServerInstaller.FindUvPath() or ExecPath.ResolveUv().This file ships a large duplicate of uv lookup logic (plus a Windows-specific variant). A single source of truth avoids bugs and version drift.
- Option A: Replace FindUvPath() body with a call to ServerInstaller.FindUvPath() (internal) if both are in the same assembly.
- Option B: Wire everything through ExecPath.ResolveUv() and ensure it delegates to the same resolver.
Minimal change if Option A is feasible:
- private string FindUvPath() - { - string uvPath = null; - ... - return uvPath; - } + private string FindUvPath() => MCPForUnity.Editor.Helpers.ServerInstaller.FindUvPath();If compilation boundaries prevent this, consider extracting a shared UvResolver under MCPForUnity.Editor.Helpers and have both classes depend on it.
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (3)
19-26: Use embedded server version for the detect-once key to match editor logs.The editor window logs the key as LegacyDetectLogged:{embeddedVer}. Here you scope by package version. If they ever diverge, users will see “DetectOnceKey” not matching the actual guard, which is confusing.
- string pkgVer = ReadPackageVersionOrFallback(); - string key = DetectOnceFlagKeyPrefix + pkgVer; + // Align with editor window debug keying – use embedded server version + string embeddedVer = ReadEmbeddedVersionOrFallback(); + string key = DetectOnceFlagKeyPrefix + embeddedVer;If you prefer package-scoped gating, then adjust the editor window to log the same (but choose one consistently across the codebase).
36-37: Standardize logging on McpLog.Warn.Keep the normalized logging story intact by using McpLog here too.
- Debug.LogWarning("MCP for Unity: Auto-detect on load failed: " + ex.Message); + McpLog.Warn("Auto-detect on load failed: " + ex.Message);
75-92: LegacyRootsExist should also cover Windows roaming paths (and reuse shared logic).Right now you only scan two Unix-style roots. ServerInstaller’s GetLegacyRootsForDetection covers a Windows roaming example too. Either:
- Make ServerInstaller’s root provider public/internal and call it here, or
- Mirror the Windows roaming path here.
private static bool LegacyRootsExist() { try { string home = System.Environment.GetFolderPath(System.Environment.SpecialFolder.UserProfile) ?? string.Empty; string[] roots = { System.IO.Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer", "src"), - System.IO.Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer", "src") + System.IO.Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer", "src"), + // Windows roaming AppData (legacy example) + System.IO.Path.Combine(System.Environment.GetFolderPath(System.Environment.SpecialFolder.ApplicationData) ?? string.Empty, + "UnityMCP", "UnityMcpServer", "src") }; foreach (var r in roots) { try { if (System.IO.File.Exists(System.IO.Path.Combine(r, "server.py"))) return true; } catch { } } }Even better: expose ServerInstaller.GetLegacyRootsForDetection() (internal/public) and iterate those to avoid duplication.
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
324-341: Semver comparator is tolerant but simplistic; consider pre-release awareness (optional).If you ever ship tags like 3.1.0-beta.1, the current parse will treat “beta.1” as 0. That’s usually fine (forces an update), but if you need precise ordering, expand parsing to handle pre-release/build metadata.
I can provide a tiny semver parser (no new deps) if you want strict ordering.
152-174: Remove unused helper methods in ServerInstaller.csBoth
IsDirectoryWritableandIsServerInstalledare private static methods with no call sites in the repository—dead code that increases maintenance and test surface. They can be safely removed unless you intend to use them imminently.• Location:
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs(lines 152–174)
• Verification: no references found for either method in the codebaseProposed diff to remove these methods:
--- a/UnityMcpBridge/Editor/Helpers/ServerInstaller.cs +++ b/UnityMcpBridge/Editor/Helpers/ServerInstaller.cs @@ -150,25 +150,6 @@ namespace UnityMcpBridge.Editor.Helpers } } - private static bool IsDirectoryWritable(string path) - { - try - { - File.Create(Path.Combine(path, "test.txt")).Dispose(); - File.Delete(Path.Combine(path, "test.txt")); - return true; - } - catch - { - return false; - } - } - - /// <summary> - /// Checks if the server is installed at the specified location. - /// </summary> - private static bool IsServerInstalled(string location) - { - return Directory.Exists(location) - && File.Exists(Path.Combine(location, ServerFolder, "src", "server.py")); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
UnityMcpBridge/Editor/Helpers/McpLog.cs(1 hunks)UnityMcpBridge/Editor/Helpers/McpLog.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/PackageDetector.cs(1 hunks)UnityMcpBridge/Editor/Helpers/PackageDetector.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/ServerInstaller.cs(7 hunks)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(6 hunks)UnityMcpBridge/UnityMcpServer~/src/server_version.txt(1 hunks)UnityMcpBridge/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (3)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (3)
ServerInstaller(11-670)GetServerPath(110-113)EnsureServerInstalled(21-108)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
ReadEmbeddedVersionOrFallback(304-320)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (4)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (1)
ReadEmbeddedVersionOrFallback(48-61)UnityMcpBridge/Editor/Helpers/McpLog.cs (3)
McpLog(6-30)Info(15-19)Warn(21-24)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)UnityMcpBridge/Editor/Models/McpClient.cs (1)
McpClient(3-45)
🔇 Additional comments (7)
UnityMcpBridge/package.json (1)
3-3: Version bump verification successful — changes approved
- Confirmed
UnityMcpBridge/package.jsonversion (3.0.1) matchesUnityMcpBridge/UnityMcpServer~/src/server_version.txt(3.0.1).- No discrepancies detected; ready to merge.
UnityMcpBridge/UnityMcpServer~/src/server_version.txt (1)
1-2: Version file whitespace is already trimmed on readThe
server_version.txtis consumed withFile.ReadAllText(...).Trim()in both the Editor window and PackageDetector/ServerInstaller helpers, ensuring any trailing newline or extra whitespace is removed before parsing. Keeping the blank second line is purely cosmetic and will not cause parse mismatches.– No changes required.
UnityMcpBridge/Editor/Helpers/McpLog.cs.meta (1)
1-11: McpLog.cs.meta looks correct and consistent — LGTM.Contains the expected MonoImporter block with sane defaults; GUID presence is good.
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)
61-64: Nice touch: debug prefs snapshot on first load.Logging the debug prefs state only when enabled is a good balance between signal and noise.
249-252: Good UX: echo debug prefs when the toggle is turned on.Immediate visibility into effective prefs helps diagnose misconfigurations quickly.
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
96-107: Great fallback behavior on install failure.Falling back to warn (not fail hard) when an installed or embedded server exists keeps the editor usable.
120-148: Canonical save locations align with PR goals.
- Windows: LocalApplicationData
- macOS: ~/Library/Application Support
- Linux: XDG_DATA_HOME or ~/.local/share
Looks good and matches the PR’s canonical paths.
|
@greptile review |
…emove unused helpers
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
371-379: Command injection vulnerability in pgrep argument.The
serverSrcPathis being used directly in the shell command arguments without proper escaping, which could lead to command injection if the path contains special characters or malicious input.Apply this diff to properly escape the path:
+ // Escape serverSrcPath for safe use in shell arguments + string safePath = serverSrcPath.Replace("\\", "\\\\") + .Replace("\"", "\\\"") + .Replace("$", "\\$") + .Replace("`", "\\`"); var psi = new System.Diagnostics.ProcessStartInfo { FileName = "/usr/bin/pgrep", - Arguments = $"-f \"uv .*--directory {serverSrcPath}\"", + Arguments = $"-f \"uv .*--directory {safePath}\"",
🧹 Nitpick comments (3)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (3)
303-344: Consider adding checks for other client configurations.While the current implementation checks Cursor config, consider extending this to check other mentioned client configs (Claude Desktop, VSCode, Windsurf, Kiro) to ensure comprehensive reference checking before legacy path deletion.
Example additional checks to add after the Cursor config check:
} catch { } } + + // VSCode config (~/.vscode/mcp.json or similar) + string vscodeCfg = Path.Combine(user, ".vscode", "mcp.json"); + if (File.Exists(vscodeCfg)) + { + try + { + string json = File.ReadAllText(vscodeCfg); + string dir = ExtractDirectoryArgFromJson(json); + if (!string.IsNullOrEmpty(dir) && PathsEqualSafe(dir, serverSrcPath)) return true; + } + catch { } + } + + // Claude Desktop config (check typical location) + // Add similar checks for other clients as needed + catch { } return false;
409-425: Semver comparison could fail on non-numeric version components.The
CompareSemverSafemethod only compares numeric parts and defaults non-numeric parts to 0. This could incorrectly compare versions with pre-release tags (e.g., "3.0.1-beta" vs "3.0.1").Consider handling pre-release/metadata components more robustly:
private static int CompareSemverSafe(string a, string b) { try { if (string.IsNullOrEmpty(a) || string.IsNullOrEmpty(b)) return 0; + + // Strip pre-release and metadata suffixes for simplified comparison + int dashA = a.IndexOf('-'); + int dashB = b.IndexOf('-'); + if (dashA > 0) a = a.Substring(0, dashA); + if (dashB > 0) b = b.Substring(0, dashB); + var ap = a.Split('.'); var bp = b.Split('.');
92-97: Consider making EditorPrefs cleanup more selective.The code unconditionally deletes the EditorPrefs keys for overrides. Consider only clearing them if they point to legacy locations, allowing users to maintain valid custom overrides.
// Clear overrides that might point at legacy locations try { - EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); - EditorPrefs.DeleteKey("MCPForUnity.PythonDirOverride"); + string serverSrcPref = EditorPrefs.GetString("MCPForUnity.ServerSrc", string.Empty); + string pythonDirPref = EditorPrefs.GetString("MCPForUnity.PythonDirOverride", string.Empty); + + // Only clear if pointing to legacy locations + foreach (var legacyRoot in GetLegacyRootsForDetection()) + { + string legacySrc = Path.Combine(legacyRoot, "src"); + if (PathsEqualSafe(serverSrcPref, legacySrc)) + { + EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); + break; + } + } + + foreach (var legacyRoot in GetLegacyRootsForDetection()) + { + string legacySrc = Path.Combine(legacyRoot, "src"); + if (PathsEqualSafe(pythonDirPref, legacySrc)) + { + EditorPrefs.DeleteKey("MCPForUnity.PythonDirOverride"); + break; + } + } } catch { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
UnityMcpBridge/Editor/Helpers/McpLog.cs (4)
McpLog(6-30)Info(15-19)Warn(21-24)Error(26-29)
🔇 Additional comments (4)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (4)
71-75: Good addition of reference check before deletion.The code now properly checks whether the legacy path is still referenced by EditorPrefs or client configs before deletion, addressing the concern from the previous review. This prevents breaking existing client configurations.
267-300: LGTM! Safe implementation of reference checking.The
IsPathPossiblyReferencedByPrefsOrKnownConfigsmethod correctly checks EditorPrefs and Cursor config file for references to legacy paths before deletion. The JSON extraction logic is robust with proper error handling.
129-131: Windows path resolution looks good.The use of
LocalApplicationDatawith proper fallback for the canonical install location on Windows is appropriate and follows platform conventions.
145-153: macOS path resolution correctly uses Application Support.The implementation properly uses
LocalApplicationData(which maps to~/Library/Application Supporton macOS) with an appropriate fallback construction.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
371-381: pgrep hardening looks good; add a tiny resilience fallback.Nice job addressing the past injection concern: UseShellExecute=false, quoting the pattern, and EscapeForPgrep() mitigate regex tricks. One nit: /usr/bin/pgrep may not exist on some distros; consider falling back to “pgrep” on PATH if launching the absolute path fails.
- var psi = new System.Diagnostics.ProcessStartInfo + var psi = new System.Diagnostics.ProcessStartInfo { - FileName = "/usr/bin/pgrep", + FileName = File.Exists("/usr/bin/pgrep") ? "/usr/bin/pgrep" : "pgrep", Arguments = $"-f \"uv .*--directory {safePath}\"", UseShellExecute = false, RedirectStandardOutput = true, RedirectStandardError = true, CreateNoWindow = true };Also applies to: 383-397
🧹 Nitpick comments (6)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (3)
346-362: Separate canonical from legacy roots at the source.Beyond the runtime guard, prevent the canonical root from being yielded as “legacy” on Linux. This makes detection/cleanup simpler and reduces future regressions.
private static IEnumerable<string> GetLegacyRootsForDetection() { var roots = new System.Collections.Generic.List<string>(); string home = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) ?? string.Empty; - // macOS/Linux legacy - roots.Add(Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer")); - roots.Add(Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer")); + // macOS/Linux: treat ~/.config as legacy; do NOT include canonical ~/.local/share on Linux + roots.Add(Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer")); + // NOTE: ~/.local/share/UnityMCP/UnityMcpServer is the canonical path on Linux and should not be considered legacy. // Windows roaming example try { string roaming = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData) ?? string.Empty; if (!string.IsNullOrEmpty(roaming)) roots.Add(Path.Combine(roaming, "UnityMCP", "UnityMcpServer")); } catch { } return roots; }Would you like me to propagate this adjustment everywhere we reference legacy roots? I can also add a small helper like IsCanonicalRoot(root) to centralize the rule.
430-446: Semver parsing fails on pre-release/build tags.CompareSemverSafe treats “3.0.1-beta.1” or “3.0.1+build” components as 0 due to int parsing, which could misorder versions and skip needed upgrades. Strip non-digit suffixes per segment, or implement a minimal semver parser (ignoring prerelease precedence would still be an improvement over collapsing to 0).
- var ap = a.Split('.'); - var bp = b.Split('.'); + var ap = a.Split('.'); + var bp = b.Split('.'); for (int i = 0; i < Math.Max(ap.Length, bp.Length); i++) { - int ai = (i < ap.Length && int.TryParse(ap[i], out var t1)) ? t1 : 0; - int bi = (i < bp.Length && int.TryParse(bp[i], out var t2)) ? t2 : 0; + // parse numeric prefix of each segment (e.g., "1-beta" -> 1) + int ai = 0, bi = 0; + if (i < ap.Length) + { + string seg = ap[i]; + int j = 0; while (j < seg.Length && char.IsDigit(seg[j])) j++; + _ = int.TryParse(j > 0 ? seg.Substring(0, j) : "0", out ai); + } + if (i < bp.Length) + { + string seg = bp[i]; + int j = 0; while (j < seg.Length && char.IsDigit(seg[j])) j++; + _ = int.TryParse(j > 0 ? seg.Substring(0, j) : "0", out bi); + } if (ai != bi) return ai.CompareTo(bi); } return 0;If server_version.txt will always be pure “MAJOR.MINOR.PATCH”, we can keep current logic—please confirm the expected format in this package.
91-98: Consider clearing only when the key points to legacy paths.Unconditionally deleting MCPForUnity.ServerSrc and MCPForUnity.PythonDirOverride is okay, but slightly heavy-handed. Optionally, only delete when they don’t match the canonical GetServerPath() to preserve user overrides in advanced setups.
- EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); - EditorPrefs.DeleteKey("MCPForUnity.PythonDirOverride"); + var canon = GetServerPath(); + var curSrc = EditorPrefs.GetString("MCPForUnity.ServerSrc", string.Empty); + if (!PathsEqualSafe(curSrc, canon)) EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); + var curOverride = EditorPrefs.GetString("MCPForUnity.PythonDirOverride", string.Empty); + if (!string.IsNullOrEmpty(curOverride) && !PathsEqualSafe(curOverride, canon)) + EditorPrefs.DeleteKey("MCPForUnity.PythonDirOverride");UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3)
1120-1138: Config merge/write flow is careful and minimal—one nit on duplication.The WriteToConfig flow (preserve JSON, only change unityMCP, atomic replace) is solid. You also gate PackageCache on Windows unless explicitly overridden—good. However, this class re-implements FindUvPath/validation logic that already exists (and is more exhaustive) in ServerInstaller.FindUvPath/ValidateUvBinary(). Prefer calling the shared helper to prevent drift.
- string uvPath = (ValidateUvBinarySafe(existingCommand) ? existingCommand : FindUvPath()); + string uvPath = (ValidateUvBinarySafe(existingCommand) ? existingCommand + : MCPForUnity.Editor.Helpers.ServerInstaller.FindUvPath());And remove this class’s private FindUvPath/IsValidUvInstallation/FindWindowsUvPath if you switch callers. I can send a follow-up diff to consolidate and adjust call sites (DrawClientConfigurationCompact, Manual config, etc.).
Also applies to: 1148-1156, 1160-1167, 1178-1211
1628-1661: Auto-rewrite on mismatch is helpful; consider a directory existence check before rewrite.You already test File.Exists(configPath) earlier. As a belt-and-suspenders, optionally ensure Directory.CreateDirectory(Path.GetDirectoryName(configPath)) before writing in WriteToConfig() when invoked from this path (where config file may be missing if users deleted it mid-session).
- string rewriteResult = WriteToConfig(pythonDir, configPath, mcpClient); + Directory.CreateDirectory(Path.GetDirectoryName(configPath)); + string rewriteResult = WriteToConfig(pythonDir, configPath, mcpClient);
1812-1814: Local UV discovery duplicates ServerInstaller logic; unify to a single source of truth.The editor window’s FindUvPath/IsValidUvInstallation/FindWindowsUvPath largely replicate ServerInstaller.FindUvPath/ValidateUvBinary. Prefer calling ServerInstaller.FindUvPath to avoid future inconsistencies (e.g., Homebrew/WinGet paths). This also shortens this class.
If you’d like, I can prepare a follow-up diff that:
- Removes private FindUvPath/IsValidUvInstallation/FindWindowsUvPath here,
- Replaces all call sites with ServerInstaller.FindUvPath,
- Uses ServerInstaller.ValidateUvBinary (or exposes a shared validator) as needed.
Also applies to: 1894-2092
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs(7 hunks)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
UnityMcpBridge/Editor/Helpers/McpLog.cs (4)
McpLog(6-30)Info(15-19)Warn(21-24)Error(26-29)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (4)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (1)
ReadEmbeddedVersionOrFallback(48-61)UnityMcpBridge/Editor/Helpers/McpLog.cs (3)
McpLog(6-30)Info(15-19)Warn(21-24)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)UnityMcpBridge/Editor/Models/McpClient.cs (1)
McpClient(3-45)
🔇 Additional comments (7)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
67-76: Deletion guard for referenced legacy installs is a solid improvement.The IsPathPossiblyReferencedByPrefsOrKnownConfigs() check before deleting legacy roots addresses the earlier race conditions. Good call.
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (6)
61-64: Debug logging on enable—nice touch.Calling LogDebugPrefsState() only when the toggle is enabled keeps noise down but makes first-run diagnostics easy. LGTM.
249-253: Immediate debug state dump after toggling is useful.Dumping prefs after the user enables “Show Debug Logs” provides fast feedback and helps support. Looks good.
1287-1314: ResolveServerSrc fallbacks look correct for canonical-first behavior.EnsureServerInstalled() → installed path, then embedded if UseEmbeddedServer is set. This aligns with the PR’s canonical install goal. LGTM.
1690-1701: Quoting in Claude registration command is correct.Embedding quotes around uvPath and srcDir protects spaces; combined with ExecPath.TryRun (no shell) this is safe. LGTM.
257-292: Robust debug-state logger.SafeGetPrefString/Bool wrappers and the version-scoped key make debugging migrations straightforward. Nice.
688-715: Cursor config parsing is pragmatic; path-equality helper prevents false negatives.ExtractDirectoryArg + PathsEqual avoids substring pitfalls. Looks good.
| // Cleanup legacy installs that are missing version or older than embedded | ||
| foreach (var legacyRoot in GetLegacyRootsForDetection()) | ||
| { | ||
| try | ||
| { | ||
| string legacySrc = Path.Combine(legacyRoot, "src"); | ||
| if (!File.Exists(Path.Combine(legacySrc, "server.py"))) continue; | ||
| string legacyVer = ReadVersionFile(Path.Combine(legacySrc, VersionFileName)); | ||
| bool legacyOlder = string.IsNullOrEmpty(legacyVer) | ||
| || (!string.IsNullOrEmpty(embeddedVer) && CompareSemverSafe(legacyVer, embeddedVer) < 0); | ||
| if (legacyOlder) | ||
| { | ||
| // Skip deletion if this path is still referenced by prefs or known client configs | ||
| if (IsPathPossiblyReferencedByPrefsOrKnownConfigs(legacySrc)) | ||
| { | ||
| McpLog.Info($"Skipping removal of legacy server at '{legacyRoot}' (still referenced).", always: false); | ||
| continue; | ||
| } | ||
| TryKillUvForPath(legacySrc); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against deleting the canonical install (Linux canonical path is in legacy list).
GetLegacyRootsForDetection returns ~/.local/share/UnityMCP/UnityMcpServer, which is also the canonical Linux destRoot. The cleanup loop doesn’t exclude destRoot, so in edge cases (e.g., partial copy, missing/empty version file, or failed write), the canonical install could be treated as “legacy” and removed.
Add an explicit skip for destRoot before evaluating legacyOlder.
- foreach (var legacyRoot in GetLegacyRootsForDetection())
+ foreach (var legacyRoot in GetLegacyRootsForDetection())
{
try
{
+ // Never consider the canonical install root for deletion
+ if (PathsEqualSafe(legacyRoot, destRoot))
+ continue;
+
string legacySrc = Path.Combine(legacyRoot, "src");
if (!File.Exists(Path.Combine(legacySrc, "server.py"))) continue;
string legacyVer = ReadVersionFile(Path.Combine(legacySrc, VersionFileName));
bool legacyOlder = string.IsNullOrEmpty(legacyVer)
|| (!string.IsNullOrEmpty(embeddedVer) && CompareSemverSafe(legacyVer, embeddedVer) < 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Cleanup legacy installs that are missing version or older than embedded | |
| foreach (var legacyRoot in GetLegacyRootsForDetection()) | |
| { | |
| try | |
| { | |
| string legacySrc = Path.Combine(legacyRoot, "src"); | |
| if (!File.Exists(Path.Combine(legacySrc, "server.py"))) continue; | |
| string legacyVer = ReadVersionFile(Path.Combine(legacySrc, VersionFileName)); | |
| bool legacyOlder = string.IsNullOrEmpty(legacyVer) | |
| || (!string.IsNullOrEmpty(embeddedVer) && CompareSemverSafe(legacyVer, embeddedVer) < 0); | |
| if (legacyOlder) | |
| { | |
| // Skip deletion if this path is still referenced by prefs or known client configs | |
| if (IsPathPossiblyReferencedByPrefsOrKnownConfigs(legacySrc)) | |
| { | |
| McpLog.Info($"Skipping removal of legacy server at '{legacyRoot}' (still referenced).", always: false); | |
| continue; | |
| } | |
| TryKillUvForPath(legacySrc); | |
| // Cleanup legacy installs that are missing version or older than embedded | |
| foreach (var legacyRoot in GetLegacyRootsForDetection()) | |
| { | |
| try | |
| { | |
| // Never consider the canonical install root for deletion | |
| if (PathsEqualSafe(legacyRoot, destRoot)) | |
| continue; | |
| string legacySrc = Path.Combine(legacyRoot, "src"); | |
| if (!File.Exists(Path.Combine(legacySrc, "server.py"))) continue; | |
| string legacyVer = ReadVersionFile(Path.Combine(legacySrc, VersionFileName)); | |
| bool legacyOlder = string.IsNullOrEmpty(legacyVer) | |
| || (!string.IsNullOrEmpty(embeddedVer) && CompareSemverSafe(legacyVer, embeddedVer) < 0); | |
| if (legacyOlder) | |
| { | |
| // Skip deletion if this path is still referenced by prefs or known client configs | |
| if (IsPathPossiblyReferencedByPrefsOrKnownConfigs(legacySrc)) | |
| { | |
| McpLog.Info($"Skipping removal of legacy server at '{legacyRoot}' (still referenced).", always: false); | |
| continue; | |
| } | |
| TryKillUvForPath(legacySrc); |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Helpers/ServerInstaller.cs around lines 58-76, the
cleanup loop may delete the canonical destRoot because the legacy roots list
contains the canonical Linux path; add an explicit skip for the canonical
destRoot before evaluating legacyOlder by resolving and normalizing both the
legacy path and the destRoot (use Path.GetFullPath and a case-insensitive
comparison on Windows) and if they refer to the same directory continue the
loop, placing this guard immediately after computing legacySrc and before
reading the legacy version or deciding to delete.
…x/installer-cleanup-v2 * 'main' of https://github.com/CoplayDev/unity-mcp: Improve Windsurf MCP Config (CoplayDev#231)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (6)
258-294: Standardize logging in LogDebugPrefsState and avoid fully-qualified loggerUse McpLog everywhere for consistency with the new logging helpers; also drop the fully-qualified type name where the using already exists.
Apply this diff:
- MCPForUnity.Editor.Helpers.McpLog.Info( + McpLog.Info( "MCP Debug Prefs:\n" + $" DebugLogs: {debugLogsEnabled}\n" + $" PythonDirOverride: '{pythonDirOverridePref}'\n" + $" UvPath: '{uvPathPref}'\n" + $" ServerSrc: '{serverSrcPref}'\n" + $" UseEmbeddedServer: {useEmbedded}\n" + $" DetectOnceKey: '{detectKey}' => {detectLogged}\n" + $" AutoRegisteredKey: '{autoKey}' => {autoRegistered}", always: false ); ... - UnityEngine.Debug.LogWarning($"MCP Debug Prefs logging failed: {ex.Message}"); + McpLog.Warn($"MCP Debug Prefs logging failed: {ex.Message}");
305-321: Avoid duplication of ReadEmbeddedVersionOrFallback across helpersThis logic exists in PackageDetector.ReadEmbeddedVersionOrFallback() as well. Consider moving it to a single shared helper (e.g., ServerPathResolver or a small EmbeddedVersion utility) and reusing it here to keep behavior in lockstep.
1077-1100: Config merge approach is sound; align read-error warning with McpLogMerging into existing JSON and writing minimal changes is the right trade-off. Small consistency nit: route the read error to McpLog.
Apply this diff:
- UnityEngine.Debug.LogWarning($"Error reading existing config: {e.Message}."); + McpLog.Warn($"Error reading existing config: {e.Message}.");
268-286: Minor privacy note: printed prefs may include user pathsThe debug snapshot prints absolute paths (PythonDirOverride, UvPath, ServerSrc). It’s gated by DebugLogs, but consider redacting the home directory portion to reduce accidental path leakage in shared logs.
1831-1834: Update Windows Installation Guidance for uvThe official Astral uv docs recommend using the standalone PowerShell installer or a native Windows package manager rather than a bare
pip install uv. Consider updating the error message to match upstream guidance:• Standalone installer via PowerShell script
PS> powershell -ExecutionPolicy ByPass -c "irm https://astral.sh/uv/install.ps1 | iex" ``` ([docs.astral.sh](https://docs.astral.sh/uv/getting-started/installation/?utm_source=chatgpt.com)) • WinGet ```powershell winget install --id=astral-sh.uv -e ``` ([docs.astral.sh](https://docs.astral.sh/uv/getting-started/installation/?utm_source=chatgpt.com)) • Scoop ```powershell scoop install main/uv ``` ([docs.astral.sh](https://docs.astral.sh/uv/getting-started/installation/?utm_source=chatgpt.com)) • pipx (recommended for isolated environments) ```bash pipx install uv ``` ([docs.astral.sh](https://docs.astral.sh/uv/getting-started/installation/?utm_source=chatgpt.com)) Suggested diff (UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs lines 1831–1834): ```diff - UnityEngine.Debug.LogError("UV package manager not found! Please install UV first:\n" + - "• macOS/Linux: curl -LsSf https://astral.sh/uv/install.sh | sh\n" + - "• Windows: pip install uv\n" + - "• Or visit: https://docs.astral.sh/uv/getting-started/installation"); + UnityEngine.Debug.LogError("UV package manager not found! Please install UV first:\n" + + "• macOS/Linux: curl -LsSf https://astral.sh/uv/install.sh | sh\n" + + "• Windows (PowerShell): powershell -ExecutionPolicy ByPass -c \"irm https://astral.sh/uv/install.ps1 | iex\"\n" + + " Or via WinGet: winget install --id=astral-sh.uv -e\n" + + " Or via Scoop: scoop install main/uv\n" + + " Or via pipx: pipx install uv\n" + + "• See: https://docs.astral.sh/uv/getting-started/installation");
291-291: Finish logging normalization in MCPForUnityEditorWindow.csA number of direct
UnityEngine.Debug.*calls remain in this file. To ensure consistent formatting and respect the DebugLogs gate, please replace every direct:
UnityEngine.Debug.Log(…)→McpLog.Info(…)UnityEngine.Debug.LogWarning(…)→McpLog.Warn(…)UnityEngine.Debug.LogError(…)→McpLog.Error(…)Instances found at the following lines (per
rg -n -C1 'UnityEngine\.Debug\.(Log|LogWarning|LogError)\b'):• LogWarning: 291, 670, 1094, 1116, 1308, 1605, 1727, 2063, 2114
• LogError: 961, 1226, 1314, 1411, 1432, 1627, 1661, 1677, 1831
• Log (Info): 1285, 1591, 1656, 1669, 1705, 1720, 1742, 1748, 2058After refactoring, re-run the above
rgcommand to confirm no directUnityEngine.Debug.*calls remain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (1)
ReadEmbeddedVersionOrFallback(48-61)UnityMcpBridge/Editor/Helpers/McpLog.cs (3)
McpLog(6-30)Info(15-19)Warn(21-24)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)
🔇 Additional comments (7)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (7)
62-65: Good call: emit a single debug snapshot on enableOnly logs when DebugLogs is true, so normal sessions stay quiet. This will help diagnose prefs state post-upgrade without noise.
250-254: Nice UX: snapshot prefs immediately after toggling DebugLogsEmitting a consolidated block right after enabling the toggle is helpful. No further action.
295-304: Defensive EditorPrefs accessors look goodTry/catch + sane defaults removes a common source of editor exceptions. No changes requested.
581-582: Consistent warn-path via McpLogSwitching catch-path warnings in AutoFirstRunSetup to McpLog matches the PR’s logging normalization goal. Looks good.
Also applies to: 598-599, 609-610
1575-1577: Correct comparison: extract --directory and use PathsEqualThis fixes false positives that substring checks can cause across similar paths. Good change.
1759-1760: Dead code removal acknowledgedReference to removed ParseTextOutput is fine; the method is gone. Nothing else to do.
2041-2042: Dead code removal acknowledgedReference to removed FindClaudeCommand is fine.
…cal root logs; fallback to RuntimeInformation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (2)
313-315: Switch this startup banner to McpLog for consistency with the new logging normalizationGiven the PR’s goal to centralize logging, emit this message via McpLog instead of Debug.Log so it adheres to the Info/Warn/Error pipeline and the DebugLogs toggle behavior.
Apply:
- string platform = Application.platform.ToString(); - string serverVer = ReadInstalledServerVersionSafe(); - Debug.Log($"<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>: MCPForUnityBridge started on port {currentUnityPort}. (OS={platform}, server={serverVer})"); + string platform = Application.platform.ToString(); + string serverVer = ReadInstalledServerVersionSafe(); + McpLog.Info($"MCPForUnityBridge started on port {currentUnityPort}. (OS={platform}, server={serverVer})");
732-747: Deduplicate version reading logic; expose a single source of truth in ServerInstallerThis helper reimplements reading server_version.txt even though ServerInstaller already owns server layout concerns. Centralize this in ServerInstaller to avoid drift if the file name or location changes and to reuse its existing safe I/O patterns.
Option A (preferred): add a public helper in ServerInstaller and call it here:
// In ServerInstaller.cs (new method) public static string ReadInstalledServerVersionSafe() { try { var verPath = Path.Combine(GetServerPath(), "server_version.txt"); return ReadVersionFile(verPath) ?? "unknown"; } catch { return "unknown"; } }Then replace the body here with:
- private static string ReadInstalledServerVersionSafe() - { - try - { - string serverSrc = ServerInstaller.GetServerPath(); - string verFile = Path.Combine(serverSrc, "server_version.txt"); - if (File.Exists(verFile)) - { - string v = File.ReadAllText(verFile)?.Trim(); - if (!string.IsNullOrEmpty(v)) return v; - } - } - catch { } - return "unknown"; - } + private static string ReadInstalledServerVersionSafe() + { + return ServerInstaller.ReadInstalledServerVersionSafe(); + }Option B (if you don’t want to change ServerInstaller right now): minimally reuse its logic by making ServerInstaller.ReadVersionFile(...) internal/public and calling it here.
Optional: If canonical install isn’t present yet, consider falling back to the packaged server_version.txt (the embedded UnityMcpServer~/src) so the banner can still surface the intended version-to-install. I can wire that up via ServerInstaller if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
ServerInstaller(11-776)GetServerPath(116-119)
…to canonical path; then remove legacy if unreferenced
… macOS path via Application.platform; escape pgrep pattern
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (3)
298-347: Good hardening: escaped, non-shell pgrep usage mitigates injection riskThe switch to UseShellExecute=false and EscapeForPgrep() addresses the earlier command-injection concern while retaining the intended matching behavior.
58-84: Critical: cleanup loop can delete the canonical install; add explicit destRoot guardGetLegacyRootsForDetection includes the Linux canonical root (~/.local/share/UnityMCP/UnityMcpServer). Without an explicit skip, the loop can misidentify the canonical install as “legacy” (e.g., missing/empty version file) and delete it. Add a hard guard to skip destRoot before any deletion logic.
Apply this diff inside the foreach, before computing legacySrc:
foreach (var legacyRoot in GetLegacyRootsForDetection()) { try { + // Never consider the canonical install root for deletion + if (PathsEqualSafe(legacyRoot, destRoot)) + continue; + string legacySrc = Path.Combine(legacyRoot, "src"); if (!File.Exists(Path.Combine(legacySrc, "server.py"))) continue; string legacyVer = ReadVersionFile(Path.Combine(legacySrc, VersionFileName)); bool legacyOlder = string.IsNullOrEmpty(legacyVer) || (!string.IsNullOrEmpty(embeddedVer) && CompareSemverSafe(legacyVer, embeddedVer) < 0); if (legacyOlder) { TryKillUvForPath(legacySrc); try { Directory.Delete(legacyRoot, recursive: true); McpLog.Info($"Removed legacy server at '{legacyRoot}'."); } catch (Exception ex) { McpLog.Warn($"Failed to remove legacy server at '{legacyRoot}': {ex.Message}"); } } } catch { } }
58-84: Delete legacy installs only when unreferenced by prefs or known client configsAs written, any “older” legacy path is deleted immediately. This risks breaking clients if config rewrites didn’t take effect yet. Add a conservative guard that skips deletion when the legacy src is still referenced by EditorPrefs or known client configs (e.g., Cursor), and proceed only when unreferenced.
Apply this diff right before TryKillUvForPath:
bool legacyOlder = string.IsNullOrEmpty(legacyVer) || (!string.IsNullOrEmpty(embeddedVer) && CompareSemverSafe(legacyVer, embeddedVer) < 0); if (legacyOlder) { + // Skip deletion if this legacy src is still referenced anywhere we know about + if (IsPathPossiblyReferencedByPrefsOrKnownConfigs(legacySrc)) + { + McpLog.Info($"Skipping removal of legacy server at '{legacyRoot}' (still referenced).", always: false); + continue; + } TryKillUvForPath(legacySrc); try { Directory.Delete(legacyRoot, recursive: true); McpLog.Info($"Removed legacy server at '{legacyRoot}'."); }Add these helpers (place them after PathsEqualSafe or near other helpers):
private static bool IsPathPossiblyReferencedByPrefsOrKnownConfigs(string serverSrcPath) { try { if (string.IsNullOrEmpty(serverSrcPath)) return false; // EditorPrefs overrides try { string prefServerSrc = EditorPrefs.GetString("MCPForUnity.ServerSrc", string.Empty) ?? string.Empty; if (PathsEqualSafe(prefServerSrc, serverSrcPath)) return true; string prefOverride = EditorPrefs.GetString("MCPForUnity.PythonDirOverride", string.Empty) ?? string.Empty; if (PathsEqualSafe(prefOverride, serverSrcPath)) return true; } catch { } // Cursor config (~/.cursor/mcp.json) string user = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) ?? string.Empty; string cursorCfg = Path.Combine(user, ".cursor", "mcp.json"); if (File.Exists(cursorCfg)) { var json = File.ReadAllText(cursorCfg); var dir = ExtractDirectoryArgFromJson(json); if (!string.IsNullOrEmpty(dir) && PathsEqualSafe(dir, serverSrcPath)) return true; } } catch { } return false; } // Minimal JSON scan for ["args": ["uv", "...", "--directory", "<path>", ...]] private static string ExtractDirectoryArgFromJson(string json) { try { int argsIdx = json.IndexOf("\"args\"", StringComparison.OrdinalIgnoreCase); if (argsIdx < 0) return null; int arrStart = json.IndexOf('[', argsIdx); int arrEnd = json.IndexOf(']', arrStart + 1); if (arrStart < 0 || arrEnd <= arrStart) return null; string arr = json.Substring(arrStart + 1, arrEnd - arrStart - 1); var parts = arr.Split(new[] {','}, StringSplitOptions.RemoveEmptyEntries); for (int i = 0; i < parts.Length - 1; i++) { string token = parts[i].Trim().Trim('"'); if (string.Equals(token, "--directory", StringComparison.OrdinalIgnoreCase)) return parts[i + 1].Trim().Trim('"'); } } catch { } return null; }
🧹 Nitpick comments (5)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (5)
85-92: Don’t clear EditorPrefs overrides unconditionally; only remove if they point to legacy pathsCurrent code deletes MCPForUnity.ServerSrc and MCPForUnity.PythonDirOverride even if a user intentionally set a non-legacy override (e.g., dev checkout). Restrict deletion to values that match known-legacy roots; keep canonical or custom overrides intact.
Apply this focused diff:
-// Clear overrides that might point at legacy locations +// Clear overrides only if they point at legacy locations try { - EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); - EditorPrefs.DeleteKey("MCPForUnity.PythonDirOverride"); + string canonicalSrc = destSrc; // Path.Combine(destRoot, "src") + string prefServerSrc = EditorPrefs.GetString("MCPForUnity.ServerSrc", string.Empty) ?? string.Empty; + if (!string.IsNullOrEmpty(prefServerSrc)) + { + bool pointsToLegacy = false; + foreach (var lr in GetLegacyRootsForDetection()) + { + if (PathsEqualSafe(prefServerSrc, Path.Combine(lr, "src"))) { pointsToLegacy = true; break; } + } + if (pointsToLegacy && !PathsEqualSafe(prefServerSrc, canonicalSrc)) + EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); + } + + string prefPyOverride = EditorPrefs.GetString("MCPForUnity.PythonDirOverride", string.Empty) ?? string.Empty; + if (!string.IsNullOrEmpty(prefPyOverride)) + { + bool pointsToLegacy = false; + foreach (var lr in GetLegacyRootsForDetection()) + { + if (PathsEqualSafe(prefPyOverride, Path.Combine(lr, "src"))) { pointsToLegacy = true; break; } + } + if (pointsToLegacy && !PathsEqualSafe(prefPyOverride, canonicalSrc)) + EditorPrefs.DeleteKey("MCPForUnity.PythonDirOverride"); + } } catch { }Note: If you intend to always clear these keys, please confirm; as written, the PR description claims we only clear overrides that point to legacy paths.
298-331: Add fallback when /usr/bin/pgrep is unavailable; prefer resolving from PATHSome distros or constrained environments may not have pgrep at /usr/bin/pgrep. Fall back to plain “pgrep” to allow PATH resolution.
Apply this minimal change:
- var psi = new System.Diagnostics.ProcessStartInfo + string pgrepBin = File.Exists("/usr/bin/pgrep") ? "/usr/bin/pgrep" : "pgrep"; + var psi = new System.Diagnostics.ProcessStartInfo { - FileName = "/usr/bin/pgrep", + FileName = pgrepBin, Arguments = $"-f \"uv .*--directory {safePath}\"", UseShellExecute = false, RedirectStandardOutput = true, RedirectStandardError = true, CreateNoWindow = true };
194-256: Avoid reading embedded version when embeddedSrc is unknownIf TryGetEmbeddedServerSource fails, ReadVersionFile(Path.Combine("", VersionFileName)) probes the current working dir unintentionally. Treat embeddedVer as null in that case.
Apply this diff:
- string embeddedVer = ReadVersionFile(Path.Combine(embeddedSrc ?? string.Empty, VersionFileName)); + string embeddedVer = null; + if (!string.IsNullOrEmpty(embeddedSrc)) + { + embeddedVer = ReadVersionFile(Path.Combine(embeddedSrc, VersionFileName)); + }
280-296: Exclude the canonical root from legacy roots at the sourceYou already guard in logging; doing the filter in GetLegacyRootsForDetection avoids accidental inclusion elsewhere and reduces duplication.
Apply this diff:
- private static IEnumerable<string> GetLegacyRootsForDetection() + private static IEnumerable<string> GetLegacyRootsForDetection() { - var roots = new System.Collections.Generic.List<string>(); + var roots = new System.Collections.Generic.List<string>(); + string canonicalRoot = null; + try { canonicalRoot = Path.Combine(GetSaveLocation(), ServerFolder); } catch { } string home = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) ?? string.Empty; // macOS/Linux legacy - roots.Add(Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer")); - roots.Add(Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer")); + var legacy1 = Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer"); + var legacy2 = Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer"); + if (canonicalRoot == null || !PathsEqualSafe(canonicalRoot, legacy1)) roots.Add(legacy1); + if (canonicalRoot == null || !PathsEqualSafe(canonicalRoot, legacy2)) roots.Add(legacy2); // Windows roaming example try { string roaming = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData) ?? string.Empty; if (!string.IsNullOrEmpty(roaming)) - roots.Add(Path.Combine(roaming, "UnityMCP", "UnityMcpServer")); + { + var legacyWin = Path.Combine(roaming, "UnityMCP", "UnityMcpServer"); + if (canonicalRoot == null || !PathsEqualSafe(canonicalRoot, legacyWin)) roots.Add(legacyWin); + } } catch { } return roots; }
436-513: Normalize RepairPythonEnvironment logging to McpLog for consistencyThis method still uses Debug.Log/Warning/Error. Switch to McpLog for consistent formatting and DebugLogs gating with the rest of the editor-side logging.
Apply this focused diff:
- Debug.LogWarning("RepairPythonEnvironment: server.py not found; ensure server is installed first."); + McpLog.Warn("RepairPythonEnvironment: server.py not found; ensure server is installed first."); return false; } } catch (Exception ex) { - Debug.LogError($"UV not found. Please install uv (https://docs.astral.sh/uv/)." ); + McpLog.Error("UV not found. Please install uv (https://docs.astral.sh/uv/)." ); return false; } @@ - if (!proc.Start()) + if (!proc.Start()) { - Debug.LogError("Failed to start uv process."); + McpLog.Error("Failed to start uv process."); return false; } @@ - Debug.LogError("uv sync timed out."); + McpLog.Error("uv sync timed out."); return false; } @@ - if (proc.ExitCode != 0) + if (proc.ExitCode != 0) { - Debug.LogError($"uv sync failed: {stderr}\n{stdout}"); + McpLog.Error($"uv sync failed: {stderr}\n{stdout}"); return false; } - Debug.Log("<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>: Python environment repaired successfully."); + McpLog.Info("Python environment repaired successfully."); return true; } catch (Exception ex) { - Debug.LogError($"RepairPythonEnvironment failed: {ex.Message}"); + McpLog.Error($"RepairPythonEnvironment failed: {ex.Message}"); return false; }Optionally also convert inline warnings above:
- try { Directory.Delete(venvPath, recursive: true); } catch (Exception ex) { Debug.LogWarning($"Failed to delete .venv: {ex.Message}"); } + try { Directory.Delete(venvPath, recursive: true); } catch (Exception ex) { McpLog.Warn($"Failed to delete .venv: {ex.Message}"); } @@ - try { File.Delete(pyPin); } catch (Exception ex) { Debug.LogWarning($"Failed to delete .python-version: {ex.Message}"); } + try { File.Delete(pyPin); } catch (Exception ex) { McpLog.Warn($"Failed to delete .python-version: {ex.Message}"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
UnityMcpBridge/Editor/Helpers/McpLog.cs (4)
McpLog(6-30)Info(15-19)Warn(21-24)Error(26-29)
🔇 Additional comments (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
32-56: Install/overwrite flow looks solid; version file write is resilientGood handling of missing installed version, semver compare against embedded, and writing/refreshing server_version.txt with try/catch keeps the installer robust across partial updates.
Normalize macOS to Application Support; use AppSupport symlink for Cursor args Map XDG (~/.local/share, ~/.config) to Application Support VSCode manual: show mcp.json path; use top-level servers JSON VSCode macOS path: ~/Library/Application Support/Code/User/mcp.json
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 (2)
UnityMcpBridge/Editor/Windows/VSCodeManualSetupWindow.cs (1)
111-134: Linux path not handled; non-Windows branch hardcodes macOS location.displayPath uses the macOS path for all non-Windows OSes. On Linux it should be ~/.config/Code/User/mcp.json. Match the OS-aware logic used in McpClients.cs.
- else - { - displayPath = System.IO.Path.Combine( - System.Environment.GetFolderPath(System.Environment.SpecialFolder.UserProfile), - "Library", - "Application Support", - "Code", - "User", - "mcp.json" - ); - } + else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) + { + displayPath = System.IO.Path.Combine( + System.Environment.GetFolderPath(System.Environment.SpecialFolder.UserProfile), + "Library", "Application Support", "Code", "User", "mcp.json"); + } + else + { + displayPath = System.IO.Path.Combine( + System.Environment.GetFolderPath(System.Environment.SpecialFolder.UserProfile), + ".config", "Code", "User", "mcp.json"); + }UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
964-979: Manual VSCode JSON should include type:"stdio"; prefer central builder for consistency.The manual JSON omits type = "stdio" required by VSCode MCP. To avoid schema drift, use the shared ConfigJsonBuilder.BuildManualConfigJson(...) (which adds type for VSCode) instead of hand-rolling the object here.
- var vscodeConfig = new - { - servers = new - { - unityMCP = new - { - command = uvPath, - args = new[] { "run", "--directory", pythonDir, "server.py" } - } - } - }; - JsonSerializerSettings jsonSettings = new() { Formatting = Formatting.Indented }; - string manualConfigJson = JsonConvert.SerializeObject(vscodeConfig, jsonSettings); + string manualConfigJson = ConfigJsonBuilder.BuildManualConfigJson(uvPath, pythonDir, mcpClient); VSCodeManualSetupWindow.ShowWindow(configPath, manualConfigJson);
♻️ Duplicate comments (3)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
321-353: Escape regex for pgrep to avoid over-matching or unintended kills.The pgrep -f pattern treats serverSrcPath as a regex fragment. Unescaped metacharacters (e.g., . * + ? [ ] ( ) { } | ^ $) in paths can over-match and kill unrelated processes. Escape regex characters before embedding.
- var psi = new System.Diagnostics.ProcessStartInfo + string safePath = EscapeForPgrep(serverSrcPath); + var psi = new System.Diagnostics.ProcessStartInfo { FileName = "/usr/bin/pgrep", - Arguments = $"-f \"uv .*--directory {serverSrcPath}\"", + Arguments = $"-f \"uv .*--directory {safePath}\"", UseShellExecute = false, RedirectStandardOutput = true, RedirectStandardError = true, CreateNoWindow = true };Add this helper near other utilities:
+ private static string EscapeForPgrep(string input) + { + if (string.IsNullOrEmpty(input)) return input; + // Escape POSIX ERE metacharacters used by pgrep -f + var sb = new StringBuilder(input.Length * 2); + foreach (char c in input) + { + switch (c) + { + case '.': case '*': case '+': case '?': case '[': case ']': + case '(': case ')': case '{': case '}': case '|': case '^': case '$': case '\\': + sb.Append('\\').Append(c); + break; + default: + sb.Append(c); + break; + } + } + return sb.ToString(); + }
59-84: Guard against deleting the canonical install (destRoot) during legacy cleanup.GetLegacyRootsForDetection() includes ~/.local/share/UnityMCP/UnityMcpServer, which is also the canonical Linux destRoot. If the version file is missing or unreadable, the cleanup can erroneously delete the canonical install. Add an explicit skip for destRoot before evaluating legacyOlder.
- foreach (var legacyRoot in GetLegacyRootsForDetection()) + foreach (var legacyRoot in GetLegacyRootsForDetection()) { try { + // Never consider the canonical install root for deletion + if (PathsEqualSafe(legacyRoot, destRoot)) + continue; + string legacySrc = Path.Combine(legacyRoot, "src"); if (!File.Exists(Path.Combine(legacySrc, "server.py"))) continue; string legacyVer = ReadVersionFile(Path.Combine(legacySrc, VersionFileName)); bool legacyOlder = string.IsNullOrEmpty(legacyVer) || (!string.IsNullOrEmpty(embeddedVer) && CompareSemverSafe(legacyVer, embeddedVer) < 0); if (legacyOlder) { TryKillUvForPath(legacySrc); try { Directory.Delete(legacyRoot, recursive: true); McpLog.Info($"Removed legacy server at '{legacyRoot}'."); }UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
1157-1174: Validate serverSrc before writing; avoid emitting broken configs.After resolving/normalizing serverSrc, guard against null/missing server.py. Also, when blocking PackageCache on Windows, switch to the installed path only if it actually contains server.py. This prevents writing unusable entries.
if (!serverValid) { // Prefer the provided pythonDir if valid; fall back to resolver if (!string.IsNullOrEmpty(pythonDir) && System.IO.File.Exists(System.IO.Path.Combine(pythonDir, "server.py"))) { serverSrc = pythonDir; } else { serverSrc = ResolveServerSrc(); } } // macOS normalization... try { if (System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.OSX) && !string.IsNullOrEmpty(serverSrc)) { ... } } catch { } // Hard-block PackageCache on Windows unless dev override is set if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && !string.IsNullOrEmpty(serverSrc) && serverSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 && !UnityEditor.EditorPrefs.GetBool("MCPForUnity.UseEmbeddedServer", false)) { - serverSrc = ServerInstaller.GetServerPath(); + var installed = ServerInstaller.GetServerPath(); + if (!string.IsNullOrEmpty(installed) && File.Exists(Path.Combine(installed, "server.py"))) + { + serverSrc = installed; + } } + + // Final validation to avoid writing a broken config + if (string.IsNullOrEmpty(serverSrc) || !File.Exists(Path.Combine(serverSrc, "server.py"))) + { + return "UnityMCP server source not found (server.py). Install/update the server first."; + }Also applies to: 1175-1182
🧹 Nitpick comments (8)
UnityMcpBridge/Editor/Data/McpClients.cs (1)
86-111: VSCode mcp.json path: good OS-aware logic, but centralize to avoid drift.The runtime switch to map macOS to ~/Library/Application Support/Code/User/mcp.json and Linux to ~/.config/Code/User/mcp.json looks correct. However, this logic is duplicated in VSCodeManualSetupWindow and currently diverges (Linux not handled there). Consider extracting a small helper (e.g., ConfigPaths.GetVSCodeMcpConfigPath()) so both UI and data sources stay in sync. Also, the field name linuxConfigPath now stores macOS paths as well; add a short code comment clarifying “non-Windows” semantics to reduce confusion.
+// Note: linuxConfigPath is used for both Linux and macOS (non-Windows). linuxConfigPath = RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "Library", "Application Support", "Code", "User", "mcp.json" ) : Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".config", "Code", "User", "mcp.json" ),UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (1)
54-85: Scope of macOS “Cursor-only” path tweak is broader than intended.The comment says this AppSupport symlink mapping is for Cursor on macOS, but the condition currently applies to all non-VSCode clients (Claude Desktop, Windsurf, Kiro). Tighten the check to Cursor explicitly to avoid unexpected path rewrites for other clients.
-// For Cursor (non-VSCode) on macOS, prefer a no-spaces symlink path to avoid arg parsing issues in some runners -string effectiveDir = directory; +// For Cursor on macOS, prefer a no-spaces symlink path to avoid arg parsing issues in some runners +string effectiveDir = directory; #if UNITY_EDITOR_OSX || UNITY_STANDALONE_OSX - bool isCursor = !isVSCode && (client == null || client.mcpType != Models.McpTypes.VSCode); + bool isCursor = !isVSCode && client != null && client.mcpType == McpTypes.Cursor; if (isCursor && !string.IsNullOrEmpty(directory)) { const string canonical = "/Library/Application Support/"; const string symlinkSeg = "/Library/AppSupport/"; try { if (directory.Contains(canonical)) { effectiveDir = directory.Replace(canonical, symlinkSeg); } else { // If installer returned XDG-style on macOS, map to canonical symlink string norm = directory.Replace('\\', '/'); int idx = norm.IndexOf("/.local/share/UnityMCP/", System.StringComparison.Ordinal); if (idx >= 0) { string home = System.Environment.GetFolderPath(System.Environment.SpecialFolder.Personal) ?? string.Empty; string suffix = norm.Substring(idx + "/.local/share/".Length); // UnityMCP/... effectiveDir = System.IO.Path.Combine(home, "Library", "AppSupport", suffix).Replace('\\', '/'); } } } catch { /* fallback to original directory on any error */ } } #endifUnityMcpBridge/Editor/Helpers/ServerInstaller.cs (3)
86-93: Clear overrides only when they point to legacy or invalid paths.Unconditionally removing MCPForUnity.ServerSrc and MCPForUnity.PythonDirOverride can drop a valid user override. Check whether they point to known-legacy roots or to a non-existent server.py before deletion.
- try - { - EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); - EditorPrefs.DeleteKey("MCPForUnity.PythonDirOverride"); - } + try + { + string prefServer = EditorPrefs.GetString("MCPForUnity.ServerSrc", string.Empty) ?? string.Empty; + string prefOverride = EditorPrefs.GetString("MCPForUnity.PythonDirOverride", string.Empty) ?? string.Empty; + bool IsLegacy(string p) => !string.IsNullOrEmpty(p) + && (GetLegacyRootsForDetection().Any(r => PathsEqualSafe(p, Path.Combine(r, "src"))) + || !File.Exists(Path.Combine(p, "server.py"))); + if (IsLegacy(prefServer)) EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); + if (IsLegacy(prefOverride)) EditorPrefs.DeleteKey("MCPForUnity.PythonDirOverride"); + } catch { }
156-193: Symlink creation is reasonable; add minimal logging on success/failure for diagnostics.Best-effort ln -s is fine. Consider logging once (debug level) when created to aid troubleshooting of path issues.
523-686: Duplicate UV discovery logic across the codebase; prefer single source.This robust FindUvPath is duplicated by another implementation in MCPForUnityEditorWindow. To reduce drift, expose this one and reuse it in UI, or centralize in a shared ExecPath helper.
UnityMcpBridge/Editor/Windows/VSCodeManualSetupWindow.cs (1)
173-190: “Open File” UX can fail when file doesn’t exist; prefer opening the folder.Opening the file directly will error if mcp.json hasn’t been created yet. Consider an “Open Folder” action that opens the containing directory, or create an empty file if missing.
- "Open File", + "Open Folder", ... - // Open the file using the system's default application - System.Diagnostics.Process.Start( - new System.Diagnostics.ProcessStartInfo - { - FileName = displayPath, - UseShellExecute = true, - } - ); + var folder = System.IO.Path.GetDirectoryName(displayPath); + System.IO.Directory.CreateDirectory(folder); + System.Diagnostics.Process.Start(new System.Diagnostics.ProcessStartInfo + { + FileName = folder, + UseShellExecute = true, + });UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)
176-206: UV discovery/search duplicated across classes; consolidate.FindUvPath/IsValidUvInstallation logic appears here and within ServerInstaller. Consider reusing ServerInstaller.FindUvPath() (make it public or route via a shared ExecPath API) to keep behavior consistent.
Also applies to: 523-686, 1778-1854, 1892-2054
1590-1592: Auto-rewrite-on-mismatch is good; add minimal debug on no-op.The rewrite attempt is solid. For debug mode, consider logging when nothing changed (ArgsEqual, command match) but the status remains IncorrectPath due to external factors; it helps diagnose stale UI vs. real mismatch.
Also applies to: 1601-1623
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
UnityMcpBridge/Editor/Data/McpClients.cs(2 hunks)UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs(1 hunks)UnityMcpBridge/Editor/Helpers/ServerInstaller.cs(7 hunks)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(11 hunks)UnityMcpBridge/Editor/Windows/VSCodeManualSetupWindow.cs(4 hunks)UnityMcpBridge/UnityMcpServer~/src/server_version.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- UnityMcpBridge/UnityMcpServer~/src/server_version.txt
🧰 Additional context used
🧬 Code graph analysis (2)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (4)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (1)
ReadEmbeddedVersionOrFallback(48-61)UnityMcpBridge/Editor/Helpers/McpLog.cs (3)
McpLog(6-30)Info(15-19)Warn(21-24)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)UnityMcpBridge/Editor/Models/McpClient.cs (1)
McpClient(3-45)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
UnityMcpBridge/Editor/Helpers/McpLog.cs (4)
McpLog(6-30)Info(15-19)Warn(21-24)Error(26-29)
🔇 Additional comments (5)
UnityMcpBridge/Editor/Data/McpClients.cs (1)
4-4: OS detection import is appropriate.Bringing in RuntimeInformation here is justified for per-OS config path resolution. No issues.
UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (1)
87-88: Args construction is consistent with the rest of the codebase.Using ["run", "--directory", , "server.py"] matches the normalized pattern elsewhere. Looks good.
UnityMcpBridge/Editor/Windows/VSCodeManualSetupWindow.cs (1)
93-103: VSCode instructions updated to mcp.json — looks good.Text changes are clear and match the new config schema.
Also applies to: 203-206
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)
62-66: On-demand debug prefs dump is a nice touch.Calling LogDebugPrefsState() on enable and when toggling improves visibility without spamming logs.
Also applies to: 250-254
258-321: Defensive EditorPrefs reads and consolidated debug block — good.SafeGetPref* helpers and scoped keys (DetectOnceKey, AutoRegisteredKey) are clean and robust.
|
@msanatan All right I put this through its paces even more, made some more fixes and tightens, tested on all the IDEs on Mac and Windows. Feels like it should be pretty good. If you have a minute to look and/or test, go for it. Thanks! |
|
@greptileai review |
- Fix macOS path for Claude Desktop config: use ~/Library/Application Support/Claude/ instead of ~/.config/Claude/ - Improve atomic write pattern with backup/restore safety - Replace File.Replace() with File.Move() for better macOS compatibility - Add proper error handling and cleanup for file operations - Resolves issue where installer couldn't find Claude Desktop config on macOS
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
1146-1155: Prevent writing a broken config when serverSrc is invalid (reapply final guard)serverSrc may still be null/invalid after resolution and Windows PackageCache override. Without a last-mile validation, WriteToConfig can emit configs pointing to a non-existent server.py. This mirrors a prior review note; it still needs the final guard and a safe override on Windows.
Apply this diff:
// Hard-block PackageCache on Windows unless dev override is set if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && !string.IsNullOrEmpty(serverSrc) && serverSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 && !UnityEditor.EditorPrefs.GetBool("MCPForUnity.UseEmbeddedServer", false)) { - serverSrc = ServerInstaller.GetServerPath(); + var installed = ServerInstaller.GetServerPath(); + if (!string.IsNullOrEmpty(installed) && File.Exists(Path.Combine(installed, "server.py"))) + { + serverSrc = installed; + } } + // Final validation to avoid writing a broken config + if (string.IsNullOrEmpty(serverSrc) || !File.Exists(Path.Combine(serverSrc, "server.py"))) + { + return "UnityMCP server source not found (server.py). Install/update the server first."; + } + // 2) Canonical args order var newArgs = new[] { "run", "--directory", serverSrc, "server.py" };Also applies to: 1157-1174, 1176-1182, 1184-1186
🧹 Nitpick comments (9)
UnityMcpBridge/Editor/Data/McpClients.cs (3)
73-88: Prefer SpecialFolder.ApplicationData on macOS and honor XDG_CONFIG_HOME on Linux (Claude Desktop).The current construction hardcodes "
/Library/Application Support" for macOS and "/.config" for Linux. Using SpecialFolder.ApplicationData improves portability on macOS, and honoring XDG_CONFIG_HOME avoids missing configs on systems that override ~/.config.Apply this diff within the current block:
- // For macOS, Claude Desktop stores config under ~/Library/Application Support/Claude - // For Linux, it remains under ~/.config/Claude + // For macOS, Claude Desktop stores config under ~/Library/Application Support/Claude + // For Linux, prefer XDG_CONFIG_HOME (fallback ~/.config)/Claude linuxConfigPath = RuntimeInformation.IsOSPlatform(OSPlatform.OSX) - ? Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - "Library", - "Application Support", - "Claude", - "claude_desktop_config.json" - ) - : Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - ".config", - "Claude", - "claude_desktop_config.json" - ), + ? Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), + "Claude", + "claude_desktop_config.json" + ) + : Path.Combine( + GetXdgConfigHome() ?? Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".config"), + "Claude", + "claude_desktop_config.json" + ),Add this helper inside the class (outside the changed hunk):
private static string? GetXdgConfigHome() { try { var xdg = Environment.GetEnvironmentVariable("XDG_CONFIG_HOME"); return string.IsNullOrWhiteSpace(xdg) ? null : xdg; } catch { return null; } }Optional: since linuxConfigPath is being used as a “non-Windows” path, consider renaming to nonWindowsConfigPath in a follow-up to reduce ambiguity. I’m not asking for it in this PR.
96-96: Nit: keep Windows env var casing consistent in comments.Use %APPDATA% (all caps) to match common Windows docs and the earlier comment style. No code changes needed.
103-120: Use SpecialFolder.ApplicationData on macOS and XDG_CONFIG_HOME on Linux (VSCode GitHub Copilot).Same portability concern as above. Also future-proofs against users with a custom XDG layout.
Apply this diff within the current block:
- // For macOS, VSCode stores user config under ~/Library/Application Support/Code/User - // For Linux, it remains under ~/.config/Code/User + // For macOS, VSCode stores user config under ~/Library/Application Support/Code/User + // For Linux, prefer XDG_CONFIG_HOME (fallback ~/.config)/Code/User linuxConfigPath = RuntimeInformation.IsOSPlatform(OSPlatform.OSX) - ? Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - "Library", - "Application Support", - "Code", - "User", - "mcp.json" - ) - : Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - ".config", - "Code", - "User", - "mcp.json" - ), + ? Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), + "Code", + "User", + "mcp.json" + ) + : Path.Combine( + GetXdgConfigHome() ?? Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".config"), + "Code", + "User", + "mcp.json" + ),Optional (future): if you plan to support VSCode Insiders/VSCodium, we can extend the consumer logic to probe alternative directories like "Code - Insiders" or "VSCodium" before writing. Not necessary for this PR.
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (6)
258-293: Unify logging to McpLog in the catch for consistencyThe catch currently uses UnityEngine.Debug.LogWarning; prefer McpLog.Warn for normalized output.
Apply this diff:
- catch (Exception ex) - { - UnityEngine.Debug.LogWarning($"MCP Debug Prefs logging failed: {ex.Message}"); - } + catch (Exception ex) + { + MCPForUnity.Editor.Helpers.McpLog.Warn($"MCP Debug Prefs logging failed: {ex.Message}"); + }
305-321: Avoid duplicating embedded-version probe across classesThis helper duplicates logic in PackageDetector.ReadEmbeddedVersionOrFallback (see related snippet). Consider centralizing in a single shared helper (e.g., ServerPathResolver or a small VersionInfo utility) to avoid drift.
1157-1174: macOS XDG→Application Support remap: add existence check after rewritingAfter remapping ~/.local/share → ~/Library/Application Support, verify the new path actually contains server.py before proceeding; otherwise you may regress a valid XDG path into an invalid canonical path if migration hasn’t happened yet.
Example follow-up (adjusted to your flow):
- string norm = serverSrc.Replace('\\', '/'); + string norm = serverSrc.Replace('\\', '/'); int idx = norm.IndexOf("/.local/share/UnityMCP/", StringComparison.Ordinal); if (idx >= 0) { string home = Environment.GetFolderPath(Environment.SpecialFolder.Personal) ?? string.Empty; string suffix = norm.Substring(idx + "/.local/share/".Length); // UnityMCP/... - serverSrc = System.IO.Path.Combine(home, "Library", "Application Support", suffix); + var remapped = System.IO.Path.Combine(home, "Library", "Application Support", suffix); + if (File.Exists(Path.Combine(remapped, "server.py"))) serverSrc = remapped; }
1206-1250: Robust atomic write is a solid improvement; minor OS-specific refinementThe tmp+backup+move flow is safe. On Windows you could use File.Replace(temp, target, backup, ignoreMetadataErrors: true) for atomic replacement without a delete window. Not required—current approach is fine cross-platform.
1356-1365: Normalize logging via McpLogThis warning uses Debug.LogWarning; consider McpLog.Warn for consistency with the rest of the window.
Apply this diff:
- UnityEngine.Debug.LogWarning("Could not find Python directory with server.py; falling back to installed path"); + MCPForUnity.Editor.Helpers.McpLog.Warn("Could not find Python directory with server.py; falling back to installed path");
991-1001: Avoid noisy logs in OnGUI: don’t call FindUvPath() just to decide UI visibilityFindUvPath() logs an error when uv isn’t found; calling it on every repaint can spam the Console. Use a quiet check (e.g., ExecPath.ResolveUv()) for gating UI.
Apply this diff:
- bool hideConfigInfo = - (mcpClient.mcpType == McpTypes.ClaudeCode && string.IsNullOrEmpty(ExecPath.ResolveClaude())) - || ((mcpClient.mcpType != McpTypes.ClaudeCode) && string.IsNullOrEmpty(FindUvPath())); + bool hideConfigInfo = + (mcpClient.mcpType == McpTypes.ClaudeCode && string.IsNullOrEmpty(ExecPath.ResolveClaude())) + || ((mcpClient.mcpType != McpTypes.ClaudeCode) && string.IsNullOrEmpty(ExecPath.ResolveUv()));If you prefer to keep FindUvPath as the source of truth, consider adding an optional parameter to suppress error logging during detection in paint paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
UnityMcpBridge/Editor/Data/McpClients.cs(2 hunks)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (1)
ReadEmbeddedVersionOrFallback(48-61)UnityMcpBridge/Editor/Helpers/McpLog.cs (3)
McpLog(6-30)Info(15-19)Warn(21-24)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)
🔇 Additional comments (8)
UnityMcpBridge/Editor/Data/McpClients.cs (2)
4-4: Good call adding RuntimeInformation for OS detection.This is Unity/.NET Standard friendly and keeps platform checks explicit. No issues here.
73-88: linuxConfigPath correctly covers both macOS and LinuxI audited all references to
linuxConfigPathand confirmed that it’s never gated by a strict Linux-only check. Instead, every usage either:
- Uses
RuntimeInformation.IsOSPlatform(OSPlatform.OSX) || RuntimeInformation.IsOSPlatform(OSPlatform.Linux)to guard access tolinuxConfigPath(e.g. in ManualConfigEditorWindow.cs lines 119–124, MCPForUnityEditorWindow.cs lines 1415–1419 and 1457–1461), or- Falls back from a Windows check—
RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? windowsConfigPath : linuxConfigPath—which inherently covers both macOS and Linux (e.g. MCPForUnityEditorWindow.cs line 953).No instances of
linuxConfigPathbeing used under an explicit Linux-only condition were found. The current implementation correctly handles macOS and Linux without further changes.UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (6)
62-65: Good call: surface prefs only when debugging is enabledConditionally logging the consolidated prefs on enable is helpful and keeps normal sessions quiet.
250-253: Nice touch: immediate debug snapshot after togglingTriggering LogDebugPrefsState() on enablement gives users instant visibility into the effective prefs.
295-304: Safe EditorPrefs wrappers look goodThe defensive get-with-fallback helpers avoid exceptions during domain reloads or corrupted prefs.
581-611: Great: route noisy auto-setup failures through McpLog.WarnStandardized warnings make logs easier to scan and respect the DebugLogs toggle for info messages elsewhere.
1629-1631: Path comparison for configuredDir vs pythonDir is correctExtracting --directory and comparing with PathsEqual avoids false negatives due to case or separators. LGTM.
965-977: Additional context needed to verify VSCode MCP schemaI need the official VSCode MCP extension documentation or repository link to confirm the expected structure. Could you please provide:
- The GitHub (or other) repo for the VSCode “MCP” extension you’re targeting
- A link to the JSON schema or docs that define the
serversobject- (If available) An example config file from the official docs
Once I have that, I can confirm whether the top-level
servers.unityMCP.commandandservers.unityMCP.argsshape matches the documented schema.
…de Desktop, etc.). Fallback to linuxConfigPath only if mac path missing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
1176-1182: Add a final guard before writing to avoid broken configs (serverSrc may still be invalid)Past feedback flagged this exact risk. After Windows PackageCache override or any resolution failure,
serverSrccan still be null/invalid, yet we proceed to buildnewArgsand write. Short-circuit with a user-friendly message instead of emitting a bad config.if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && !string.IsNullOrEmpty(serverSrc) && serverSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 && !UnityEditor.EditorPrefs.GetBool("MCPForUnity.UseEmbeddedServer", false)) { serverSrc = ServerInstaller.GetServerPath(); } + // Final validation to avoid writing a broken config + if (string.IsNullOrEmpty(serverSrc) || !File.Exists(Path.Combine(serverSrc, "server.py"))) + { + return "UnityMCP server source not found (server.py). Install/update the server first."; + } + // 2) Canonical args order var newArgs = new[] { "run", "--directory", serverSrc, "server.py" };Also applies to: 1184-1193
🧹 Nitpick comments (5)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (5)
258-294: Use McpLog consistently in the catch; consider light redaction of PII
- The catch logs via UnityEngine.Debug.LogWarning; prefer McpLog.Warn for consistency with centralized formatting/toggle.
- Optional: these debug lines can include full absolute paths (user home). Given they’re gated behind DebugLogs, that’s fine, but consider minimal redaction (e.g., replace home path with ~) to reduce PII in shared logs.
Apply this small consistency fix:
- UnityEngine.Debug.LogWarning($"MCP Debug Prefs logging failed: {ex.Message}"); + MCPForUnity.Editor.Helpers.McpLog.Warn($"MCP Debug Prefs logging failed: {ex.Message}");
305-322: Avoid repeated disk reads for embedded version (micro-optimization)If LogDebugPrefsState is called frequently, cache the embedded version string (static lazy) to avoid re-reading server_version.txt.
1158-1174: macOS XDG → Application Support remap should validate destination before usingIf the remapped target doesn’t exist, we risk writing a config pointing to a non-existent path. Only remap when the target exists with server.py.
if (System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.OSX) && !string.IsNullOrEmpty(serverSrc)) { string norm = serverSrc.Replace('\\', '/'); int idx = norm.IndexOf("/.local/share/UnityMCP/", StringComparison.Ordinal); if (idx >= 0) { string home = Environment.GetFolderPath(Environment.SpecialFolder.Personal) ?? string.Empty; string suffix = norm.Substring(idx + "/.local/share/".Length); // UnityMCP/... - serverSrc = System.IO.Path.Combine(home, "Library", "Application Support", suffix); + var remapped = System.IO.Path.Combine(home, "Library", "Application Support", suffix); + if (Directory.Exists(remapped) && File.Exists(Path.Combine(remapped, "server.py"))) + { + serverSrc = remapped; + } } }
1210-1250: Atomic write: prefer File.Replace on Windows; keep rename on UnixCurrent delete-then-move creates a small window where the file is missing and doesn’t preserve metadata/ACLs. Use:
- Windows: File.Replace(tmp, configPath, backup, ignoreMetadataErrors: true)
- macOS/Linux: atomic rename via Move without deleting first (tmp must be in same directory — which it is)
You already create tmp alongside the final file, so this is safe.
- // Atomic move operation (more reliable than Replace on macOS) - if (System.IO.File.Exists(configPath)) - { - System.IO.File.Delete(configPath); - } - System.IO.File.Move(tmp, configPath); - - // Clean up backup - if (System.IO.File.Exists(backup)) - { - System.IO.File.Delete(backup); - } + if (System.IO.File.Exists(configPath)) + { + if (System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.Windows)) + { + // Replace preserves attributes/ACLs and is atomic on NTFS + System.IO.File.Replace(tmp, configPath, backup, ignoreMetadataErrors: true); + } + else + { + // POSIX rename is atomic when same dir/device; tmp is beside configPath + System.IO.File.Move(tmp, configPath); + if (System.IO.File.Exists(backup)) System.IO.File.Delete(backup); + } + } + else + { + System.IO.File.Move(tmp, configPath); + }
1906-1910: Installation hint: update Windows guidance for uvDocs now recommend the official installer over
pip install uv. Suggest:
- Windows: PowerShell install script from Astral or
winget installwhen available.- UnityEngine.Debug.LogError("UV package manager not found! Please install UV first:\n" + - "• macOS/Linux: curl -LsSf https://astral.sh/uv/install.sh | sh\n" + - "• Windows: pip install uv\n" + - "• Or visit: https://docs.astral.sh/uv/getting-started/installation"); + UnityEngine.Debug.LogError("UV package manager not found! Please install UV first:\n" + + "• macOS/Linux: curl -LsSf https://astral.sh/uv/install.sh | sh\n" + + "• Windows (PowerShell): iwr https://astral.sh/uv/install.ps1 -UseB | iex\n" + + "• Or visit: https://docs.astral.sh/uv/getting-started/installation");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3)
UnityMcpBridge/Editor/Helpers/McpLog.cs (3)
McpLog(6-30)Info(15-19)Warn(21-24)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)UnityMcpBridge/Editor/Models/McpClient.cs (1)
McpClient(3-45)
🔇 Additional comments (9)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (9)
62-65: Good call: log debug state as soon as the window enablesHelps diagnose first-load behavior without extra user action.
250-253: Toggling Show Debug Logs immediately emits state — niceImmediate feedback is useful; no extra repaint logic needed.
295-304: Safe EditorPrefs accessors look solidThe try/catch wrappers prevent UI noise during domain reloads.
579-583: Nice: migrated noisy warnings to McpLog.WarnThis aligns with the PR’s logging normalization goals.
Also applies to: 596-600, 607-611
964-976: VSCode manual JSON switched to top-level “servers” — verify schema and back-compatYou already handle “servers” vs legacy in readers elsewhere. For manual UI, confirm VSCode’s MCP extension currently reads top-level
servers.unityMCPand that users on older versions won’t paste an incompatible block.Would you like a conditional example that shows both “servers” and legacy “mcp.servers” with comments so users can pick the right one?
1591-1592: OS branching reads clearlyThe three-way OS split is straightforward; no changes needed.
1650-1652: Correctly extracts and compares configuredDirUsing ExtractDirectoryArg + PathsEqual avoids false negatives due to separators/casing.
1834-1834: Dead code removal acknowledgedThe comment about removing unused ParseTextOutput is fine; keeps the file lean.
2116-2116: Dead code removal acknowledged (FindClaudeCommand)Reduced surface area is good.
| RuntimeInformation.IsOSPlatform(OSPlatform.OSX) | ||
| || RuntimeInformation.IsOSPlatform(OSPlatform.Linux) | ||
| ) | ||
| { | ||
| configPath = string.IsNullOrEmpty(mcpClient.macConfigPath) | ||
| ? mcpClient.linuxConfigPath | ||
| : mcpClient.macConfigPath; | ||
| } | ||
| else if ( | ||
| RuntimeInformation.IsOSPlatform(OSPlatform.Linux) | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
macConfigPath usage — verify the field exists on McpClient and is populated
The referenced McpClient in the shared snippet has only windowsConfigPath/linuxConfigPath. If macConfigPath wasn’t added in this PR, this won’t compile. If it was added, ensure it’s set for each client and falls back cleanly to the Linux path as you coded.
Run to confirm symbol presence and call-sites:
Also applies to: 1464-1473
🏁 Script executed:
#!/bin/bash
# Verify McpClient defines macConfigPath and where it's set
rg -nP -C2 'class\s+McpClient\b'
rg -n 'macConfigPath' -C3Length of output: 3259
Missing macConfigPath on McpClient – will not compile
I ran the searches and didn’t find any definition of macConfigPath in UnityMcpBridge/Editor/Models/McpClient.cs, yet it’s referenced in MCPForUnityEditorWindow.cs at lines 1415-1424, 1464-1473, and 1586-1591. This will cause a compile error.
Please add and populate the field on McpClient, and ensure it falls back to linuxConfigPath when unset:
• Define the field in UnityMcpBridge/Editor/Models/McpClient.cs alongside the existing paths:
public class McpClient
{
public string windowsConfigPath;
public string linuxConfigPath;
+ public string macConfigPath;
// …
}• Populate macConfigPath wherever McpClient instances are created or deserialized (e.g., in your JSON/XML loader or constructor), defaulting to linuxConfigPath if it’s null or empty.
• Verify the fallback logic in MCPForUnityEditorWindow.cs (around lines 1415, 1464, 1586) now points to the correct field.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RuntimeInformation.IsOSPlatform(OSPlatform.OSX) | |
| || RuntimeInformation.IsOSPlatform(OSPlatform.Linux) | |
| ) | |
| { | |
| configPath = string.IsNullOrEmpty(mcpClient.macConfigPath) | |
| ? mcpClient.linuxConfigPath | |
| : mcpClient.macConfigPath; | |
| } | |
| else if ( | |
| RuntimeInformation.IsOSPlatform(OSPlatform.Linux) | |
| ) | |
| // File: UnityMcpBridge/Editor/Models/McpClient.cs | |
| public class McpClient | |
| { | |
| public string windowsConfigPath; | |
| public string linuxConfigPath; | |
| public string macConfigPath; | |
| // … | |
| } |
…n (fixes CS1061 in editor window).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
UnityMcpBridge/Editor/Models/McpClient.cs (2)
8-8: Nit: Clarify fallback semantics in the inline comment.Make it explicit that consumers should fall back to linuxConfigPath when macConfigPath is empty, which mirrors existing behavior elsewhere in the PR.
- public string macConfigPath; // optional macOS-specific config path + public string macConfigPath; // Optional macOS-specific config path; if null/empty, consumers should fall back to linuxConfigPath.
5-12: Centralize OS-specific path selection to reduce call-site drift.Multiple call sites choosing between windows/linux/mac paths is error-prone. Add a small helper here to return the effective path for the current editor OS with sane fallbacks (mac → macConfigPath ?? linuxConfigPath; linux → linuxConfigPath; windows → windowsConfigPath). This keeps future client additions consistent and makes migrations safer.
Example (to be added within this class; requires
using UnityEngine;):public string GetConfigPathForCurrentOS() { // Editor-only: restrict to Editor platforms for clarity switch (Application.platform) { case RuntimePlatform.OSXEditor: if (!string.IsNullOrEmpty(macConfigPath)) return macConfigPath; if (!string.IsNullOrEmpty(linuxConfigPath)) return linuxConfigPath; return windowsConfigPath; // last-resort fallback case RuntimePlatform.LinuxEditor: return linuxConfigPath; case RuntimePlatform.WindowsEditor: return windowsConfigPath; default: // Preserve existing signaling if needed by callers return null; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Models/McpClient.cs(1 hunks)
🔇 Additional comments (1)
UnityMcpBridge/Editor/Models/McpClient.cs (1)
8-8: LGTM: Adding macOS-specific config path aligns with the PR’s OS-aware migration.This complements the canonical macOS location work and avoids overloading Linux paths for Darwin. No API breakage expected since this is an additive field.
| public string name; | ||
| public string windowsConfigPath; | ||
| public string linuxConfigPath; | ||
| public string macConfigPath; // optional macOS-specific config path |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify consumers prefer macConfigPath on macOS and maintain the Linux fallback.
Given the new field, ensure downstream selection logic (e.g., config writers/migrators) was updated to use it on macOS and to fall back to linuxConfigPath when unset. This guards against regressions where mac still reads Linux-only paths.
Run from repo root:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Usages of macConfigPath (expect: readers/writers/migrators reference it):"
rg -n -C2 '\bmacConfigPath\b' --hidden
echo
echo "Call sites that branch on macOS (ensure they use macConfigPath or the new helper):"
rg -n -C3 -P '(OSX|macOS|darwin|RuntimePlatform\.OSXEditor|Application\.platform\s*==\s*RuntimePlatform\.OSXEditor)' --hidden
echo
echo "Potential legacy assumptions mapping mac -> linux (review and adjust if necessary):"
rg -n -C2 -P '\b(linuxConfigPath)\b' --hidden | rg -n -C1 -P '(OSX|macOS|darwin|apple|cupertino|homebrew|Library/Application Support)' --hidden || trueLength of output: 32619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- OSPlatform.OSX branches:"
rg -n -P 'RuntimeInformation\.IsOSPlatform\(OSPlatform\.OSX\)' --hidden
echo
echo "--- Application.platform == RuntimePlatform.OSXEditor branches:"
rg -n -P 'Application\.platform\s*==\s*RuntimePlatform\.OSXEditor' --hidden
echo
echo "--- File.WriteAllText in MCPForUnityEditorWindow:"
rg -n -C2 'File\.WriteAllText' -g 'UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs' --hidden || true
echo
echo "--- WriteToConfig occurrences:"
rg -n 'WriteToConfig' -g 'UnityMcpBridge/**/*' --hidden || trueLength of output: 4079
ManualConfigEditorWindow must also prefer macConfigPath on macOS
The primary MCP editor window (MCPForUnityEditorWindow.cs) correctly chooses
configPath = string.IsNullOrEmpty(mcpClient.macConfigPath)
? mcpClient.linuxConfigPath
: mcpClient.macConfigPath;on every macOS branch (e.g. lines 1418–1420, 1467–1469, 1586–1588).
However, in UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs (around line 119) the code still does:
else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)
|| RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
displayPath = mcpClient.linuxConfigPath;
}This means macOS users never see or can override a custom macConfigPath.
Please update that branch to mirror the main-window logic, for example:
- else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)
- || RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
- {
- displayPath = mcpClient.linuxConfigPath;
- }
+ else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
+ {
+ displayPath = string.IsNullOrEmpty(mcpClient.macConfigPath)
+ ? mcpClient.linuxConfigPath
+ : mcpClient.macConfigPath;
+ }
+ else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
+ {
+ displayPath = mcpClient.linuxConfigPath;
+ }This ensures that on macOS you get the new macConfigPath when set, with a fallback to linuxConfigPath as intended.
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs around line 119,
the OS branch unconditionally uses mcpClient.linuxConfigPath for both macOS and
Linux; change it so that on macOS you prefer mcpClient.macConfigPath when it is
not null/empty and fall back to mcpClient.linuxConfigPath, while leaving Linux
to use linuxConfigPath; implement this by checking
RuntimeInformation.IsOSPlatform(OSPlatform.OSX) and selecting
string.IsNullOrEmpty(mcpClient.macConfigPath) ? mcpClient.linuxConfigPath :
mcpClient.macConfigPath, otherwise keep the existing linux path for Linux.
…allback to linux path); Linux/Windows unchanged.
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)
UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs (1)
171-188: “Open File” should validate path and handle errors
Process.Startwill throw ifdisplayPathis null/empty or the file doesn’t exist. Guard and show a friendly dialog instead of crashing the Editor.Apply this diff:
if ( GUILayout.Button( "Open File", copyButtonStyle, GUILayout.Height(25), GUILayout.Width(100) ) ) { - // Open the file using the system's default application - System.Diagnostics.Process.Start( - new System.Diagnostics.ProcessStartInfo - { - FileName = displayPath, - UseShellExecute = true, - } - ); + // Open the file using the system's default application + try + { + if (string.IsNullOrWhiteSpace(displayPath)) + { + EditorUtility.DisplayDialog("Path unavailable", "No configuration path is available to open.", "OK"); + } + else if (!System.IO.File.Exists(displayPath)) + { + EditorUtility.DisplayDialog("File not found", $"No file exists at:\n{displayPath}", "OK"); + } + else + { + System.Diagnostics.Process.Start( + new System.Diagnostics.ProcessStartInfo + { + FileName = displayPath, + UseShellExecute = true, + } + ); + } + } + catch (System.Exception ex) + { + Debug.LogError($"Failed to open config file at '{displayPath}': {ex.Message}"); + EditorUtility.DisplayDialog("Error", "Failed to open the configuration file. See Console for details.", "OK"); + } }
🧹 Nitpick comments (3)
UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs (3)
119-126: Harden macOS fallback: ensure a non-empty path (mac → linux → provided config) and avoid nullsIf both
macConfigPathandlinuxConfigPathare empty,displayPathbecomes null, which can cause errors later (e.g., when clicking “Open File”). PreferIsNullOrWhiteSpaceand coalesce toconfigPath.Apply this diff:
- else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) - { - displayPath = string.IsNullOrEmpty(mcpClient.macConfigPath) - ? mcpClient.linuxConfigPath - : mcpClient.macConfigPath; - } + else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) + { + var macPath = mcpClient.macConfigPath; + var linuxPath = mcpClient.linuxConfigPath; + displayPath = !string.IsNullOrWhiteSpace(macPath) + ? macPath + : (!string.IsNullOrWhiteSpace(linuxPath) ? linuxPath : (configPath ?? string.Empty)); + }
139-146: TextField won’t wrap; use SelectableLabel to avoid truncating long paths
EditorGUILayout.TextFieldis single-line;wordWrapon the style doesn’t wrap. UseSelectableLabelfor read-only, selectable text so long paths remain visible.Apply this diff:
- EditorGUILayout.TextField( - displayPath, - pathStyle, - GUILayout.Height(EditorGUIUtility.singleLineHeight) - ); + EditorGUILayout.SelectableLabel( + displayPath ?? string.Empty, + pathStyle, + GUILayout.Height(EditorGUIUtility.singleLineHeight * 2) + );
112-133: Centralize OS-specific path selection to keep fallbacks consistentThis OS-switch appears in multiple places across the PR. Encapsulating the logic reduces drift and ensures the mac → linux → provided-config fallback stays uniform.
Consider adding a helper within this window (or on
McpClient) and using it here:// Example helper (outside the selected lines; place it inside the class) private static string GetPreferredConfigPath(McpClient client, string fallback) { if (client == null) return fallback ?? string.Empty; if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) return !string.IsNullOrWhiteSpace(client.windowsConfigPath) ? client.windowsConfigPath : (fallback ?? string.Empty); if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) return !string.IsNullOrWhiteSpace(client.macConfigPath) ? client.macConfigPath : (!string.IsNullOrWhiteSpace(client.linuxConfigPath) ? client.linuxConfigPath : (fallback ?? string.Empty)); if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) return !string.IsNullOrWhiteSpace(client.linuxConfigPath) ? client.linuxConfigPath : (fallback ?? string.Empty); return fallback ?? string.Empty; } // Usage inside OnGUI: var displayPath = GetPreferredConfigPath(mcpClient, configPath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs(1 hunks)
|
Gonna merge in order to move forward with upcoming PRs. |
MCP for Unity — Installer auto‑migration and logging cleanup
Problem
~/.config/UnityMCP/...) and client configs like Cursor, Claude kept stale paths, so clients were talking to old servers ==> broken protocol.Why the PR helps
What’s in this PR
PackageDetectorruns once per package version (and force‑runs if legacy paths exist or the canonical install is missing).ServerInstallernow~/Library/Application Support/UnityMCP/UnityMcpServer/src; Linux:~/.local/share/UnityMCP/UnityMcpServer/src; Windows:%LOCALAPPDATA%/UnityMCP/UnityMcpServer/src),server_version.txt,Library/PackageCachepaths; auto‑updates Cursor/Claude Desktop/VSCode/Windsurf/Kiro configs.McpLoghelper with colored, consistent Info/Warn/Error; routed key logs through it and hid noisy diagnostics behind the DebugLogs toggle.Upgrade/migration behavior
Testing done
~/.config) to this version:Risk/guardrails
Summary by CodeRabbit
New Features
Bug Fixes
Chores