Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 46 additions & 9 deletions MCPForUnity/Editor/Helpers/CodexConfigHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -123,24 +123,61 @@ private static TomlTable TryParseToml(string toml)
}
}

/// <summary>
/// Checks if we should use remote uvx (Asset Store install without embedded server)
/// </summary>
private static bool ShouldUseRemoteUvx(string serverSrc)
{
// If no serverSrc provided, use remote
if (string.IsNullOrEmpty(serverSrc))
return true;

// If no embedded server exists, use remote
if (!ServerInstaller.HasEmbeddedServer())
return true;

return false;
}

/// <summary>
/// Creates a TomlTable for the unityMCP server configuration
/// </summary>
/// <param name="uvPath">Path to uv executable</param>
/// <param name="serverSrc">Path to server source directory</param>
/// <param name="uvPath">Path to uv executable (can be null for remote uvx)</param>
/// <param name="serverSrc">Path to server source directory (can be null for remote uvx)</param>
private static TomlTable CreateUnityMcpTable(string uvPath, string serverSrc)
{
var unityMCP = new TomlTable();
unityMCP["command"] = new TomlString { Value = uvPath };
bool useRemote = ShouldUseRemoteUvx(serverSrc);

var argsArray = new TomlArray();
argsArray.Add(new TomlString { Value = "run" });
argsArray.Add(new TomlString { Value = "--directory" });
argsArray.Add(new TomlString { Value = serverSrc });
argsArray.Add(new TomlString { Value = "server.py" });
unityMCP["args"] = argsArray;
if (useRemote)
{
// Asset Store install - use remote uvx
string version = AssetPathUtility.GetPackageVersion();
string remoteUrl = $"git+https://github.com/CoplayDev/unity-mcp@v{version}#subdirectory=MCPForUnity/UnityMcpServer~/src";

unityMCP["command"] = new TomlString { Value = "uvx" };

var argsArray = new TomlArray();
argsArray.Add(new TomlString { Value = "--from" });
argsArray.Add(new TomlString { Value = remoteUrl });
argsArray.Add(new TomlString { Value = "mcp-for-unity" });
unityMCP["args"] = argsArray;
}
else
{
// Git/embedded install - use local path
unityMCP["command"] = new TomlString { Value = uvPath };

var argsArray = new TomlArray();
argsArray.Add(new TomlString { Value = "run" });
argsArray.Add(new TomlString { Value = "--directory" });
argsArray.Add(new TomlString { Value = serverSrc });
argsArray.Add(new TomlString { Value = "server.py" });
unityMCP["args"] = argsArray;
}
Comment on lines +150 to +177
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Force remote when uvPath is null to avoid invalid TOML.

Mirror the JSON fix so command is never null.

Apply this diff:

-            bool useRemote = ShouldUseRemoteUvx(serverSrc);
+            bool useRemote = string.IsNullOrEmpty(uvPath) || ShouldUseRemoteUvx(serverSrc);
📝 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.

Suggested change
bool useRemote = ShouldUseRemoteUvx(serverSrc);
var argsArray = new TomlArray();
argsArray.Add(new TomlString { Value = "run" });
argsArray.Add(new TomlString { Value = "--directory" });
argsArray.Add(new TomlString { Value = serverSrc });
argsArray.Add(new TomlString { Value = "server.py" });
unityMCP["args"] = argsArray;
if (useRemote)
{
// Asset Store install - use remote uvx
string version = AssetPathUtility.GetPackageVersion();
string remoteUrl = $"git+https://github.com/CoplayDev/unity-mcp@v{version}#subdirectory=MCPForUnity/UnityMcpServer~/src";
unityMCP["command"] = new TomlString { Value = "uvx" };
var argsArray = new TomlArray();
argsArray.Add(new TomlString { Value = "--from" });
argsArray.Add(new TomlString { Value = remoteUrl });
argsArray.Add(new TomlString { Value = "mcp-for-unity" });
unityMCP["args"] = argsArray;
}
else
{
// Git/embedded install - use local path
unityMCP["command"] = new TomlString { Value = uvPath };
var argsArray = new TomlArray();
argsArray.Add(new TomlString { Value = "run" });
argsArray.Add(new TomlString { Value = "--directory" });
argsArray.Add(new TomlString { Value = serverSrc });
argsArray.Add(new TomlString { Value = "server.py" });
unityMCP["args"] = argsArray;
}
bool useRemote = string.IsNullOrEmpty(uvPath) || ShouldUseRemoteUvx(serverSrc);
if (useRemote)
{
// Asset Store install - use remote uvx
string version = AssetPathUtility.GetPackageVersion();
string remoteUrl = $"git+https://github.com/CoplayDev/unity-mcp@v{version}#subdirectory=MCPForUnity/UnityMcpServer~/src";
unityMCP["command"] = new TomlString { Value = "uvx" };
var argsArray = new TomlArray();
argsArray.Add(new TomlString { Value = "--from" });
argsArray.Add(new TomlString { Value = remoteUrl });
argsArray.Add(new TomlString { Value = "mcp-for-unity" });
unityMCP["args"] = argsArray;
}
else
{
// Git/embedded install - use local path
unityMCP["command"] = new TomlString { Value = uvPath };
var argsArray = new TomlArray();
argsArray.Add(new TomlString { Value = "run" });
argsArray.Add(new TomlString { Value = "--directory" });
argsArray.Add(new TomlString { Value = serverSrc });
argsArray.Add(new TomlString { Value = "server.py" });
unityMCP["args"] = argsArray;
}
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Helpers/CodexConfigHelper.cs around lines 150-177, ensure
unityMCP["command"] is never set to null by forcing the remote branch when
uvPath is null: after computing bool useRemote = ShouldUseRemoteUvx(serverSrc),
set useRemote = true if string.IsNullOrEmpty(uvPath) (or fold into the initial
condition), so the code executes the remote branch which assigns
unityMCP["command"] = new TomlString { Value = "uvx" }; and avoid leaving
command null for the TOML output.


// Add Windows-specific environment configuration, see: https://github.com/CoplayDev/unity-mcp/issues/315
// Always include for Windows, even with remote uvx
var platformService = MCPServiceLocator.Platform;
if (platformService.IsWindows())
{
Expand Down
42 changes: 37 additions & 5 deletions MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,50 @@ public static JObject ApplyUnityServerToExistingConfig(JObject root, string uvPa
return root;
}

/// <summary>
/// Checks if we should use remote uvx (Asset Store install without embedded server)
/// </summary>
private static bool ShouldUseRemoteUvx(string directory)
{
// If no directory provided, use remote
if (string.IsNullOrEmpty(directory))
return true;

// If no embedded server exists, use remote
if (!ServerInstaller.HasEmbeddedServer())
return true;

return false;
}

/// <summary>
/// Centralized builder that applies all caveats consistently.
/// - Sets command/args with provided directory
/// - Sets command/args with provided directory OR remote uvx for Asset Store
/// - Ensures env exists
/// - Adds type:"stdio" for VSCode
/// - Adds disabled:false for Windsurf/Kiro only when missing
/// </summary>
private static void PopulateUnityNode(JObject unity, string uvPath, string directory, McpClient client, bool isVSCode)
{
unity["command"] = uvPath;
// Check if we should use remote uvx (Asset Store without embedded server)
bool useRemote = ShouldUseRemoteUvx(directory);

// For Cursor (non-VSCode) on macOS, prefer a no-spaces symlink path to avoid arg parsing issues in some runners
string effectiveDir = directory;
if (useRemote)
{
// Asset Store install - use remote uvx
string version = AssetPathUtility.GetPackageVersion();
string remoteUrl = $"git+https://github.com/CoplayDev/unity-mcp@v{version}#subdirectory=MCPForUnity/UnityMcpServer~/src";

unity["command"] = "uvx";
unity["args"] = JArray.FromObject(new[] { "--from", remoteUrl, "mcp-for-unity" });
}
Comment on lines +68 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Prevent null command: force remote when uvPath is null.

If pythonDir is empty but an embedded server exists, builder may choose local while uvPath is null → writes command = null. Make useRemote depend on uvPath.

Apply this diff:

-            // Check if we should use remote uvx (Asset Store without embedded server)
-            bool useRemote = ShouldUseRemoteUvx(directory);
+            // Check if we should use remote uvx (also force remote if uvPath is null)
+            bool useRemote = string.IsNullOrEmpty(uvPath) || ShouldUseRemoteUvx(directory);
📝 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.

Suggested change
// Check if we should use remote uvx (Asset Store without embedded server)
bool useRemote = ShouldUseRemoteUvx(directory);
// For Cursor (non-VSCode) on macOS, prefer a no-spaces symlink path to avoid arg parsing issues in some runners
string effectiveDir = directory;
if (useRemote)
{
// Asset Store install - use remote uvx
string version = AssetPathUtility.GetPackageVersion();
string remoteUrl = $"git+https://github.com/CoplayDev/unity-mcp@v{version}#subdirectory=MCPForUnity/UnityMcpServer~/src";
unity["command"] = "uvx";
unity["args"] = JArray.FromObject(new[] { "--from", remoteUrl, "mcp-for-unity" });
}
// Check if we should use remote uvx (also force remote if uvPath is null)
bool useRemote = string.IsNullOrEmpty(uvPath) || ShouldUseRemoteUvx(directory);
if (useRemote)
{
// Asset Store install - use remote uvx
string version = AssetPathUtility.GetPackageVersion();
string remoteUrl = $"git+https://github.com/CoplayDev/unity-mcp@v{version}#subdirectory=MCPForUnity/UnityMcpServer~/src";
unity["command"] = "uvx";
unity["args"] = JArray.FromObject(new[] { "--from", remoteUrl, "mcp-for-unity" });
}
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs around lines 68 to 79, the
code can set unity["command"] = null when uvPath is empty because useRemote is
determined only by ShouldUseRemoteUvx(directory); change the useRemote
calculation to also force remote when the local uvx path (uvPath) is null or
empty (e.g., useRemote = ShouldUseRemoteUvx(directory) ||
string.IsNullOrEmpty(uvPath)), so the Asset Store branch is taken and a valid
command/args are written whenever uvPath is missing.

Comment on lines +71 to +79
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add Windows env parity (SystemRoot) for JSON configs (remote and local).

TOML adds SystemRoot for Windows; JSON path does not. This breaks some Windows clients. Add env consistently.

Apply this diff (after setting command/args, before type handling):

+            // Windows env parity with TOML (issue #315): always include SystemRoot
+            try
+            {
+                var platform = MCPForUnity.Editor.Services.MCPServiceLocator.Platform;
+                if (platform.IsWindows())
+                {
+                    var env = unity["env"] as JObject ?? new JObject();
+                    env["SystemRoot"] = platform.GetSystemRoot();
+                    unity["env"] = env;
+                }
+            }
+            catch { /* best-effort */ }

And add the import at top:

+using MCPForUnity.Editor.Services;

Also applies to: 126-127

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only needed for Codex, a bug on Codex requires this env var. Other clients, so far, do not need it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msanatan, thank you for the clarification! I understand now — the SystemRoot environment variable is a Codex-specific requirement due to a bug in that client, and since Codex uses TOML configuration (handled separately in CodexConfigHelper.cs), the JSON configuration path in ConfigJsonBuilder.cs doesn't need it. I'll skip this suggestion for the JSON configs.


✏️ Learnings added
Learnt from: msanatan
PR: CoplayDev/unity-mcp#348
File: MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs:71-79
Timestamp: 2025-10-24T14:09:08.594Z
Learning: The SystemRoot environment variable on Windows is only required for Codex MCP client configurations due to a Codex bug. Other MCP clients (VSCode, Cursor, Windsurf, Kiro) do not need this environment variable. Codex configurations use TOML format (CodexConfigHelper.cs), while other clients use JSON format (ConfigJsonBuilder.cs).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

else
{
// Git/embedded install - use local path
unity["command"] = uvPath;

// For Cursor (non-VSCode) 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 != McpTypes.VSCode);
if (isCursor && !string.IsNullOrEmpty(directory))
Expand Down Expand Up @@ -92,7 +123,8 @@ private static void PopulateUnityNode(JObject unity, string uvPath, string direc
}
#endif

unity["args"] = JArray.FromObject(new[] { "run", "--directory", effectiveDir, "server.py" });
unity["args"] = JArray.FromObject(new[] { "run", "--directory", effectiveDir, "server.py" });
}

if (isVSCode)
{
Expand Down
54 changes: 35 additions & 19 deletions MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,19 +93,27 @@ public static string WriteMcpConfiguration(string pythonDir, string configPath,
}
catch { }

// 1) Start from existing, only fill gaps (prefer trusted resolver)
string uvPath = ServerInstaller.FindUvPath();
// Optionally trust existingCommand if it looks like uv/uv.exe
try
// 1) Check if we should use remote uvx (Asset Store without embedded server)
bool useRemote = !ServerInstaller.HasEmbeddedServer() || string.IsNullOrEmpty(pythonDir);

string uvPath = null;
if (!useRemote)
{
var name = Path.GetFileName((existingCommand ?? string.Empty).Trim()).ToLowerInvariant();
if ((name == "uv" || name == "uv.exe") && IsValidUvBinary(existingCommand))
// Git/embedded install - need UV path
uvPath = ServerInstaller.FindUvPath();
// Optionally trust existingCommand if it looks like uv/uv.exe
try
{
uvPath = existingCommand;
var name = Path.GetFileName((existingCommand ?? string.Empty).Trim()).ToLowerInvariant();
if ((name == "uv" || name == "uv.exe") && IsValidUvBinary(existingCommand))
{
uvPath = existingCommand;
}
}
catch { }
if (uvPath == null) return "UV package manager not found. Please install UV first.";
}
Comment on lines +96 to 115
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Align remote/local decision with builders to avoid null-command configs.

When useRemote is true, pass null serverSrc to the builder so it cannot pick local while uvPath is null.

Apply this diff:

@@
-            string uvPath = null;
+            string uvPath = null;
             if (!useRemote)
             {
@@
-            string serverSrc = ResolveServerDirectory(pythonDir, existingArgs);
+            string serverSrc = ResolveServerDirectory(pythonDir, existingArgs);
+            // Force remote downstream if requested
+            string serverDirForConfig = useRemote ? null : serverSrc;
@@
-            existingRoot = ConfigJsonBuilder.ApplyUnityServerToExistingConfig(existingRoot, uvPath, serverSrc, mcpClient);
+            existingRoot = ConfigJsonBuilder.ApplyUnityServerToExistingConfig(existingRoot, uvPath, serverDirForConfig, mcpClient);

Also applies to: 117-128

🤖 Prompt for AI Agents
In MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs around lines 96-115 and
similarly 117-128, the logic sets useRemote=true when no embedded server or
pythonDir is empty but still allows builders to infer a local server by passing
a non-null serverSrc; update the builder calls so that when useRemote is true
you explicitly pass serverSrc as null (instead of leaving it as an empty or
existing path) to prevent the builder from selecting a local UV installation
when uvPath is null; ensure this change is applied to both code blocks mentioned
so remote mode always supplies null serverSrc and avoids producing
null-command/local-selection configurations.

catch { }
if (uvPath == null) return "UV package manager not found. Please install UV first.";

string serverSrc = ResolveServerDirectory(pythonDir, existingArgs);

// Ensure containers exist and write back configuration
Expand Down Expand Up @@ -165,20 +173,28 @@ public static string ConfigureCodexClient(string pythonDir, string configPath, M
CodexConfigHelper.TryParseCodexServer(existingToml, out existingCommand, out existingArgs);
}

string uvPath = ServerInstaller.FindUvPath();
try
// Check if we should use remote uvx (Asset Store without embedded server)
bool useRemote = !ServerInstaller.HasEmbeddedServer() || string.IsNullOrEmpty(pythonDir);

string uvPath = null;
if (!useRemote)
{
var name = Path.GetFileName((existingCommand ?? string.Empty).Trim()).ToLowerInvariant();
if ((name == "uv" || name == "uv.exe") && IsValidUvBinary(existingCommand))
// Git/embedded install - need UV path
uvPath = ServerInstaller.FindUvPath();
try
{
uvPath = existingCommand;
var name = Path.GetFileName((existingCommand ?? string.Empty).Trim()).ToLowerInvariant();
if ((name == "uv" || name == "uv.exe") && IsValidUvBinary(existingCommand))
{
uvPath = existingCommand;
}
}
}
catch { }
catch { }

if (uvPath == null)
{
return "UV package manager not found. Please install UV first.";
if (uvPath == null)
{
return "UV package manager not found. Please install UV first.";
}
}
Comment on lines +176 to 198
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Same fix for Codex path: pass null serverSrc when remote.

Mirrors the JSON path; prevents TOML with command=null if builder picks local.

Apply this diff:

@@
-            string uvPath = null;
+            string uvPath = null;
             if (!useRemote)
             {
@@
-            string serverSrc = ResolveServerDirectory(pythonDir, existingArgs);
+            string serverSrc = ResolveServerDirectory(pythonDir, existingArgs);
+            string serverDirForToml = useRemote ? null : serverSrc;
@@
-            string updatedToml = CodexConfigHelper.UpsertCodexServerBlock(existingToml, uvPath, serverSrc);
+            string updatedToml = CodexConfigHelper.UpsertCodexServerBlock(existingToml, uvPath, serverDirForToml);

Also applies to: 200-215

🤖 Prompt for AI Agents
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs around lines 176-198 (and
also apply same change to 200-215): when useRemote is true you must pass a null
serverSrc for the Codex/TOML path generation the same way the JSON path does to
avoid producing TOML with command=null when the builder later selects a local
server; update the code that constructs the Codex path/builder call to pass
serverSrc = null when useRemote (mirror the JSON handling), and make the
identical change in the block at lines ~200-215.


string serverSrc = ResolveServerDirectory(pythonDir, existingArgs);
Expand Down
97 changes: 0 additions & 97 deletions MCPForUnity/Editor/Helpers/ServerInstaller.cs
Original file line number Diff line number Diff line change
Expand Up @@ -866,103 +866,6 @@ private static bool ValidateUvBinary(string uvPath)
return false;
}

/// <summary>
/// Download and install server from GitHub release (Asset Store workflow)
/// </summary>
public static bool DownloadAndInstallServer()
{
string packageVersion = AssetPathUtility.GetPackageVersion();
if (packageVersion == "unknown")
{
McpLog.Error("Cannot determine package version for download.");
return false;
}

string downloadUrl = $"https://github.com/CoplayDev/unity-mcp/releases/download/v{packageVersion}/mcp-for-unity-server-v{packageVersion}.zip";
string tempZip = Path.Combine(Path.GetTempPath(), $"mcp-server-v{packageVersion}.zip");
string destRoot = Path.Combine(GetSaveLocation(), ServerFolder);

try
{
EditorUtility.DisplayProgressBar("MCP for Unity", "Downloading server...", 0.3f);

// Download
using (var client = new WebClient())
{
client.DownloadFile(downloadUrl, tempZip);
}

EditorUtility.DisplayProgressBar("MCP for Unity", "Extracting server...", 0.7f);

// Kill any running UV processes
string destSrc = Path.Combine(destRoot, "src");
TryKillUvForPath(destSrc);

// Delete old installation
if (Directory.Exists(destRoot))
{
if (!DeleteDirectoryWithRetry(destRoot, maxRetries: 5, delayMs: 1000))
{
McpLog.Warn($"Could not fully delete old server (files may be in use)");
}
}

// Extract to temp location first
string tempExtractDir = Path.Combine(Path.GetTempPath(), $"mcp-server-extract-{Guid.NewGuid()}");
Directory.CreateDirectory(tempExtractDir);

try
{
ZipFile.ExtractToDirectory(tempZip, tempExtractDir);

// The ZIP contains UnityMcpServer~ folder, find it and move its contents
string extractedServerFolder = Path.Combine(tempExtractDir, "UnityMcpServer~");
Directory.CreateDirectory(destRoot);
CopyDirectoryRecursive(extractedServerFolder, destRoot);
}
finally
{
// Cleanup temp extraction directory
try
{
if (Directory.Exists(tempExtractDir))
{
Directory.Delete(tempExtractDir, recursive: true);
}
}
catch (Exception ex)
{
McpLog.Warn($"Could not fully delete temp extraction directory: {ex.Message}");
}
}

EditorUtility.ClearProgressBar();
McpLog.Info($"Server v{packageVersion} downloaded and installed successfully!");
return true;
}
catch (Exception ex)
{
EditorUtility.ClearProgressBar();
McpLog.Error($"Failed to download server: {ex.Message}");
EditorUtility.DisplayDialog(
"Download Failed",
$"Could not download server from GitHub.\n\n{ex.Message}\n\nPlease check your internet connection or try again later.",
"OK"
);
return false;
}
finally
{
try
{
if (File.Exists(tempZip)) File.Delete(tempZip);
}
catch (Exception ex)
{
McpLog.Warn($"Could not delete temp zip file: {ex.Message}");
}
}
}

/// <summary>
/// Check if the package has an embedded server (Git install vs Asset Store)
Expand Down
Loading