Improve Windsurf MCP Config #231
Conversation
If it ain't broke with the other clients, don't fix...
WalkthroughAdds a ConfigJsonBuilder helper to build/merge Unity MCP JSON, refactors MCPForUnityEditorWindow to use it (centralized merge and UTF‑8 no‑BOM writes), adds Windows EditMode tests and related .meta files, adjusts a test namespace, and updates several file writes to use UTF‑8 without BOM. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Editor User
participant Win as MCPForUnityEditorWindow
participant FS as File System
participant CJ as ConfigJsonBuilder
note over Win,CJ: WriteToConfig (merge → write UTF‑8 no BOM)
User->>Win: Click "Write to Config"
Win->>FS: Read existing config (if present)
Win->>CJ: ApplyUnityServerToExistingConfig(root, uvPath, serverSrc, client)
CJ-->>Win: Updated JObject root
Win->>FS: Serialize (UTF‑8 no BOM) → write temp file → replace target
FS-->>User: Config file updated
sequenceDiagram
autonumber
actor User as Editor User
participant Win as MCPForUnityEditorWindow
participant CJ as ConfigJsonBuilder
note over Win,CJ: Manual JSON generation
User->>Win: Open Manual Instructions
Win->>CJ: BuildManualConfigJson(uvPath, pythonDir, client)
CJ-->>Win: Indented JSON string
Win-->>User: Display manual JSON
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
🪧 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 (
|
|
This will close #228 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)
1079-1086: Potential null-dereference and invalid write when serverSrc cannot be resolved.serverSrc can be null if ResolveServerSrc() returns null, which will:
- Throw on Line 1081: serverSrc.IndexOf(...) on Windows.
- Produce args containing a null directory path.
Guard serverSrc before the Windows PackageCache check and abort early if server.py cannot be found.
Apply this patch near the existing logic:
- string serverSrc = ExtractDirectoryArg(existingArgs); - bool serverValid = !string.IsNullOrEmpty(serverSrc) - && System.IO.File.Exists(System.IO.Path.Combine(serverSrc, "server.py")); - if (!serverValid) - { - serverSrc = ResolveServerSrc(); - } + string serverSrc = ExtractDirectoryArg(existingArgs); + bool serverValid = !string.IsNullOrEmpty(serverSrc) + && System.IO.File.Exists(System.IO.Path.Combine(serverSrc, "server.py")); + if (!serverValid) + { + serverSrc = ResolveServerSrc(); + } + // Abort early if still unresolved + if (string.IsNullOrEmpty(serverSrc) || !System.IO.File.Exists(System.IO.Path.Combine(serverSrc, "server.py"))) + { + return "Python server not found. Please install or select the server directory."; + } - // Hard-block PackageCache on Windows unless dev override is set - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) - && serverSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 + // Hard-block PackageCache on Windows unless dev override is set + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) + && serverSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 && !UnityEditor.EditorPrefs.GetBool("MCPForUnity.UseEmbeddedServer", false)) { serverSrc = ServerInstaller.GetServerPath(); + if (string.IsNullOrEmpty(serverSrc) || !System.IO.File.Exists(System.IO.Path.Combine(serverSrc, "server.py"))) + { + return "Installed server not found. Please install or select the server directory."; + } }Additionally, consider normalizing serverSrc with Path.GetFullPath() before writing args to avoid later mismatches.
882-909: VSCode Manual Setup bypasses ConfigJsonBuilder and uses outdated schema (mcp.servers) without type=stdio.This path reintroduces the inconsistency that the PR is fixing. Use ConfigJsonBuilder.BuildManualConfigJson for VSCode as well so we emit top-level "servers" and include type:"stdio".
Apply this diff:
if (mcpClient.mcpType == McpTypes.VSCode) { - string pythonDir = FindPackagePythonDirectory(); - string uvPath = FindUvPath(); - if (uvPath == null) - { - UnityEngine.Debug.LogError("UV package manager not found. Cannot configure VSCode."); - return; - } - - var vscodeConfig = new - { - mcp = 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); - VSCodeManualSetupWindow.ShowWindow(configPath, manualConfigJson); + string pythonDir = FindPackagePythonDirectory(); + string uvPath = FindUvPath(); + if (uvPath == null) + { + UnityEngine.Debug.LogError("UV package manager not found. Cannot configure VSCode."); + return; + } + // Reuse centralized builder to ensure correct schema and fields + string manualConfigJson = ConfigJsonBuilder.BuildManualConfigJson(uvPath, pythonDir, mcpClient); + VSCodeManualSetupWindow.ShowWindow(configPath, manualConfigJson); }
🧹 Nitpick comments (13)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs (7)
35-39: Fake uv shim is sufficient; minor hardening optional.Your stub prints a valid version string regardless of args, which is fine. If you want stricter behavior, you could parse $1 and only print on --version.
55-60: Clean up UvPath EditorPrefs in TearDown to prevent cross-test leakage.WriteToConfig sets MCPForUnity.UvPath on success. Delete it in TearDown so this test suite doesn’t influence others.
Apply this diff:
// Clean up editor preferences set during SetUp EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); EditorPrefs.DeleteKey("MCPForUnity.LockCursorConfig"); + EditorPrefs.DeleteKey("MCPForUnity.UvPath");
64-79: Assert command and args normalization as part of the contract.After WriteToConfig, we expect:
- command == _fakeUvPath (validated and reused)
- args == ["run","--directory", _serverSrcDir, "server.py"]
Add these assertions to make regressions visible.
Apply this diff:
var root = JObject.Parse(File.ReadAllText(configPath)); var unity = (JObject)root.SelectToken("mcpServers.unityMCP"); Assert.NotNull(unity, "Expected mcpServers.unityMCP node"); Assert.NotNull(unity["env"], "env should be present for all clients"); Assert.IsTrue(unity["env"]!.Type == JTokenType.Object, "env should be an object"); Assert.AreEqual(false, (bool)unity["disabled"], "disabled:false should be set for Windsurf when missing"); + Assert.AreEqual(_fakeUvPath, (string)unity["command"], "command should be set to validated uv path"); + CollectionAssert.AreEqual( + new[] { "run", "--directory", _serverSrcDir, "server.py" }, + unity["args"]!.ToObject<string[]>(), + "args should be canonicalized to the resolved server src");
81-96: Mirror the command/args assertions for Kiro.Apply this diff:
var root = JObject.Parse(File.ReadAllText(configPath)); var unity = (JObject)root.SelectToken("mcpServers.unityMCP"); Assert.NotNull(unity, "Expected mcpServers.unityMCP node"); Assert.NotNull(unity["env"], "env should be present for all clients"); Assert.IsTrue(unity["env"]!.Type == JTokenType.Object, "env should be an object"); Assert.AreEqual(false, (bool)unity["disabled"], "disabled:false should be set for Kiro when missing"); + Assert.AreEqual(_fakeUvPath, (string)unity["command"]); + CollectionAssert.AreEqual( + new[] { "run", "--directory", _serverSrcDir, "server.py" }, + unity["args"]!.ToObject<string[]>());
98-112: Also assert args for Cursor to ensure path migration happened.Apply this diff:
var root = JObject.Parse(File.ReadAllText(configPath)); var unity = (JObject)root.SelectToken("mcpServers.unityMCP"); Assert.NotNull(unity, "Expected mcpServers.unityMCP node"); Assert.IsNull(unity["env"], "env should not be added for non-Windsurf/Kiro clients"); Assert.IsNull(unity["disabled"], "disabled should not be added for non-Windsurf/Kiro clients"); + CollectionAssert.AreEqual( + new[] { "run", "--directory", _serverSrcDir, "server.py" }, + unity["args"]!.ToObject<string[]>(), + "args should be canonicalized to the resolved server src for Cursor as well");
114-129: And for VSCode, assert command/args (plus existing type check).Apply this diff:
var root = JObject.Parse(File.ReadAllText(configPath)); var unity = (JObject)root.SelectToken("servers.unityMCP"); Assert.NotNull(unity, "Expected servers.unityMCP node"); Assert.IsNull(unity["env"], "env should not be added for VSCode client"); Assert.IsNull(unity["disabled"], "disabled should not be added for VSCode client"); Assert.AreEqual("stdio", (string)unity["type"], "VSCode entry should include type=stdio"); + Assert.AreEqual(_fakeUvPath, (string)unity["command"]); + CollectionAssert.AreEqual( + new[] { "run", "--directory", _serverSrcDir, "server.py" }, + unity["args"]!.ToObject<string[]>());
131-160: Great preservation test; add one more assert to confirm args were updated.Confirm that the directory migrated from /old/path to _serverSrcDir while env/disabled remained intact.
Apply this diff:
var root = JObject.Parse(File.ReadAllText(configPath)); var unity = (JObject)root.SelectToken("mcpServers.unityMCP"); Assert.NotNull(unity, "Expected mcpServers.unityMCP node"); Assert.AreEqual("bar", (string)unity["env"]!["FOO"], "Existing env should be preserved"); Assert.AreEqual(true, (bool)unity["disabled"], "Existing disabled value should be preserved"); + CollectionAssert.AreEqual( + new[] { "run", "--directory", _serverSrcDir, "server.py" }, + unity["args"]!.ToObject<string[]>(), + "args should be updated to the new server src while preserving other fields");UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)
1025-1066: Dynamic JSON access is brittle; consider uniform JObject traversal.existingConfig as dynamic plus try/catch is workable but fragile. Using JObject.Parse with SelectToken("servers.unityMCP.command")/Value() avoids binder issues and allocations from JObject.FromObject later.
If you choose to refactor, a minimal pattern:
var root = string.IsNullOrWhiteSpace(existingJson) ? new JObject() : JObject.Parse(existingJson); var unity = (JObject)(isVSCode ? root.SelectToken("servers.unityMCP") : root.SelectToken("mcpServers.unityMCP")); string existingCommand = unity?["command"]?.Value<string>(); string[] existingArgs = unity?["args"]?.ToObject<string[]>();
1701-1779: Consolidate UV discovery logic into a single implementationTo avoid drift and maintenance overhead, delegate all UV‐path resolution in MCPForUnityEditorWindow to the existing ServerInstaller.FindUvPath (exposed via ExecPath.ResolveUv) instead of duplicating it.
• Remove the private FindUvPath method in UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (lines 1701–1779).
• Replace each call to FindUvPath() (in the early UV check at line 724; VS Code setup at 885; quick‐info visibility at 923; existing‐command fallback at 1068; manual JSON builder at 1140; and manual setup at 1346) with:string uvPath = ExecPath.ResolveUv() ?? (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "uv.exe" : "uv");• Keep the existing IsValidUvInstallation(...) helper in MCPForUnityEditorWindow for fast UI validation feedback where needed.
This centralizes UV detection and ensures all callers use the same logic and fallback behavior.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs (3)
26-38: Add explicit command/args assertions for Windsurf.This complements the env/disabled checks and catches regressions in argument ordering.
Apply this diff:
var root = JObject.Parse(json); var unity = (JObject)root.SelectToken("mcpServers.unityMCP"); Assert.NotNull(unity, "Expected mcpServers.unityMCP node"); Assert.NotNull(unity["env"], "env should be included"); Assert.AreEqual(false, (bool)unity["disabled"], "disabled:false should be added for Windsurf"); Assert.IsNull(unity["type"], "type should not be added for non-VSCode clients"); + Assert.AreEqual("/usr/bin/uv", (string)unity["command"]); + CollectionAssert.AreEqual( + new[] { "run", "--directory", "/path/to/server", "server.py" }, + unity["args"]!.ToObject<string[]>());
40-52: Ditto for Cursor: assert command/args too.Ensures manual JSON is end-to-end correct.
Apply this diff:
var root = JObject.Parse(json); var unity = (JObject)root.SelectToken("mcpServers.unityMCP"); Assert.NotNull(unity, "Expected mcpServers.unityMCP node"); Assert.IsNull(unity["env"], "env should not be added for Cursor"); Assert.IsNull(unity["disabled"], "disabled should not be added for Cursor"); Assert.IsNull(unity["type"], "type should not be added for non-VSCode clients"); + Assert.AreEqual("/usr/bin/uv", (string)unity["command"]); + CollectionAssert.AreEqual( + new[] { "run", "--directory", "/path/to/server", "server.py" }, + unity["args"]!.ToObject<string[]>());
6-9: Consider adding a Kiro case to mirror Windsurf expectations.Same behavior as Windsurf (env present, disabled=false). Increases confidence across supported clients.
Apply this diff to add a new test:
public class ManualConfigJsonBuilderTests { + [Test] + public void Kiro_ManualJson_HasMcpServersEnv_DisabledFalse() + { + var client = new McpClient { name = "Kiro", mcpType = McpTypes.Kiro }; + string json = ConfigJsonBuilder.BuildManualConfigJson("/usr/bin/uv", "/path/to/server", client); + + var root = JObject.Parse(json); + var unity = (JObject)root.SelectToken("mcpServers.unityMCP"); + Assert.NotNull(unity, "Expected mcpServers.unityMCP node"); + Assert.NotNull(unity["env"], "env should be included"); + Assert.AreEqual(false, (bool)unity["disabled"], "disabled:false should be added for Kiro"); + Assert.IsNull(unity["type"], "type should not be added for non-VSCode clients"); + Assert.AreEqual("/usr/bin/uv", (string)unity["command"]); + CollectionAssert.AreEqual( + new[] { "run", "--directory", "/path/to/server", "server.py" }, + unity["args"]!.ToObject<string[]>()); + }UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (1)
43-51: Docstring clarity: env is ensured only for Windsurf/Kiro.The summary “Ensures env exists” is a bit ambiguous. Tighten wording to reflect the current conditional behavior.
Apply this diff:
- /// Centralized builder that applies all caveats consistently. - /// - Sets command/args with provided directory - /// - Ensures env exists - /// - Adds type:"stdio" for VSCode - /// - Adds disabled:false for Windsurf/Kiro only when missing + /// Centralized builder that applies all caveats consistently. + /// - Sets command/args with provided directory + /// - Ensures env exists only for Windsurf/Kiro + /// - Adds type:"stdio" for VSCode + /// - Adds disabled:false for Windsurf/Kiro only when missing
📜 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 (10)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs(1 hunks)UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs.meta(1 hunks)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs (3)
UnityMcpBridge/Editor/Models/McpClient.cs (1)
McpClient(3-45)UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (2)
JObject(31-41)JObject(79-85)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
MCPForUnityEditorWindow(21-2238)
UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (1)
UnityMcpBridge/Editor/Models/McpClient.cs (1)
McpClient(3-45)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs (5)
Test(64-79)Test(81-96)Test(98-112)Test(114-129)Test(131-160)UnityMcpBridge/Editor/Models/McpClient.cs (1)
McpClient(3-45)UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (4)
ConfigJsonBuilder(7-86)BuildManualConfigJson(9-29)JObject(31-41)JObject(79-85)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)
UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (4)
JObject(31-41)JObject(79-85)ConfigJsonBuilder(7-86)BuildManualConfigJson(9-29)UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
FindUvPath(259-422)
🔇 Additional comments (14)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools.meta (1)
1-8: LGTM — correct Unity folder metaThe folder asset meta looks standard (DefaultImporter with empty fields). This should stabilize GUID-based references for collaborators and CI. No action required.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows.meta (1)
1-8: LGTM — Windows tests folder meta addedConsistent with Unity’s folder meta schema. This avoids GUID churn and keeps the new Windows EditMode tests discoverable in the project. No action required.
UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs.meta (1)
1-11: LGTM — script meta for Editor helperMonoImporter settings look standard, and the file resides under an Editor path, so it will compile into the Editor assembly as intended. No action required.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs.meta (1)
1-11: LGTM — test script metaAppropriate MonoImporter settings for a C# test under EditMode. No changes needed.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs (1)
6-6: Namespace alignment verifiedAll symbols resolve correctly under the MCPForUnity.Editor.Tools namespace:
- UnityMcpBridge/Editor/Tools/CommandRegistry.cs defines
namespace MCPForUnity.Editor.Tools { … public static class CommandRegistry }- UnityMcpBridge/Editor/Tools/ManageGameObject.cs defines
namespace MCPForUnity.Editor.Tools { … public static class ManageGameObject }
and exposes
public static object HandleCommand(JObject @params)- The test file includes
using MCPForUnity.Editor.Tools;and will correctly bind to both types.No further action is needed.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs.meta (1)
1-11: Unity meta file looks correct.Standard MonoImporter metadata; nothing actionable.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs (1)
25-31: Pragmatic Windows skip to avoid ProcessStartInfo pitfalls.Good call to skip on Windows, given cmd/bat + UseShellExecute=false issues. Keeps CI stable.
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3)
12-12: JObject usage import is appropriate.Required for the new centralized config update path.
1099-1107: Good: centralize merge via ConfigJsonBuilder and serialize once.Switching to ApplyUnityServerToExistingConfig reduces branching and keeps VSCode/non-VSCode parity in one place.
1139-1148: Nice: manual instructions now source JSON from the builder.This keeps VSCode/other clients consistent when invoked via ShowManualInstructionsWindow.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs (1)
10-24: VSCode path validation is solid.Covers container key, command, args, and type=stdio; also asserts env/disabled absence.
UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (3)
9-29: Manual JSON builder reads clean and centralized.Correctly chooses servers vs. mcpServers and uses PopulateUnityNode to avoid drift.
31-41: Apply to existing config path is safe and idempotent.Ensures container exists and preserves preexisting unityMCP subfields not touched by PopulateUnityNode.
65-76: Non-destructive defaults are handled well.Only adding env/disabled when missing avoids clobbering user overrides.
|
@Scriptwonder @dsarno can one of you check this out whenever you got the time? I couldn't connect using Windsurf (before version 3, it just stopped working) but Kiro was working correctly. I much rather use Windsurf than Kiro, when trying to debug I noticed that Kiro added more fields to its JSON. I copied it over to Windsurf and tinkered to get the minimal config that worked consistently for Windsurf. The logic to add was simple, but after having to update 4 different functions in the code for consistency, I centralized it a bit. The tests ensure that other IDEs are unaffected. And Windsurf has been working really well for me since |
|
I will do it this weekend. I am not a Windsurf user so it might take a bit of time to set it up. |
|
Tested on Mac OS / Apple Silicon, and found the same issue we've been seeing setting up with Claude Desktop lately: Windsurf doesn't like when the mcp_config.json is encoded 
|
|
Good catch, when I'm back home I'll check it out (I got a hunch)
*Marcus Sanatan (he/him)*
Co-founder @ Coplay <https://coplay.dev> | AI Copilot for Unity
LinkedIn <https://www.linkedin.com/in/msanatan/> | GitHub
<https://github.com/msanatan> | Website <https://gameboymarcus.com/>
…On Thu, Aug 21, 2025, 8:24 PM dsarno ***@***.***> wrote:
*dsarno* left a comment (CoplayDev/unity-mcp#231)
<#231 (comment)>
Tested on Mac OS / Apple Silicon, and found the same issue we've been
seeing setting up with Claude Desktop lately: Windsurf doesn't like when
the mcp_config.json is encoded UTF-8 with BOM. If the file is re-encoded
without BOM (just UTF-8), then it works. (see bottom screenshot).
Recommend double checking this in the code and we can re-test on Mac.
image.png (view on web)
<https://github.com/user-attachments/assets/d5bcc140-06fc-418d-85fe-6761d50e3870> image.png
(view on web)
<https://github.com/user-attachments/assets/40358887-84ea-481c-a8b8-31debd5a3522>
—
Reply to this email directly, view it on GitHub
<#231 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQFNUS3ARPQFE2PL4AUDHD3OZPLTAVCNFSM6AAAAACENCHMRWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEMJSGU2TINRWHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
| string mergedJson = JsonConvert.SerializeObject(existingConfig, jsonSettings); | ||
| string mergedJson = JsonConvert.SerializeObject(existingRoot, jsonSettings); | ||
| string tmp = configPath + ".tmp"; | ||
| System.IO.File.WriteAllText(tmp, mergedJson, System.Text.Encoding.UTF8); |
There was a problem hiding this comment.
@msanatan is this the fix?
- System.IO.File.WriteAllText(tmp, mergedJson, System.Text.Encoding.UTF8);
+ System.IO.File.WriteAllText(tmp, mergedJson, new System.Text.UTF8Encoding(false));
There was a problem hiding this comment.
Yep, it's the false flag
There was a problem hiding this comment.
Just pushed it up!
There was a problem hiding this comment.
We call WriteAllText in a few other files, should I add this parameter to those calls as well?
There was a problem hiding this comment.
@dsarno where did you see those Windsurf errors btw? What flagged the file encoding?
There was a problem hiding this comment.
That fixed it for me, nice!
I'm no BOM expert, but from what I understand now, it's mainly a legacy thing and the fact that it trips up multiple clients is a sign that we should err on the side of not using it.
There was a problem hiding this comment.
@msanatan I'm not sure if this answers your question, but -- there was no specific error, it was that Windsurf (on Mac / Silicon) was silently failing to process the mcp config JSON -- no MCP servers appeared in its dialog, even when the config was correct. That's the same thing that was happening with Claude to me and a few other people in recent days. Somehow we were using the BOM encoding to write the config files, may or may not have been a change I made when updating config stuff last week / week before.
So I asked Windsurf to look at its own config file, and say why it wasn't working, and it confirmed that was the issue.
There was a problem hiding this comment.
@msanatan Oh I got you -- the one in the screenshot. That was in the Windsurf developer console (where Windsurf told me to look):
(Windsurf) 2025-08-21 17:13:40 [ERROR]: Error refreshing MCP servers [unknown] failed to extract specs from json: failed to parse JSON config: hujson: line 1, column 1: invalid character '\ufeff' at start of value: hujson: line 1, column 1: invalid character '\ufeff' at start of value
workbench.desktop.main.js:3585 Extension Host
workbench.desktop.main.js:3585 (Windsurf) 2025-08-21 17:13:40 [ERROR]: Error refreshing MCP servers [unknown] failed to extract specs from json: failed to parse JSON config: hujson: line 1, column 1: invalid character '\ufeff' at start of value: hujson: line 1, column 1: invalid character '\ufeff' at start of value
There was a problem hiding this comment.
Thanks for sharing! It's always good to learn how we find the problems, and solve them 🙂
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
234-255: Ensure explicit UTF-8 no-BOM in all File.WriteAllText callsThe sweep uncovered four occurrences of
File.WriteAllTextin your test suite that don’t specifyUTF8Encoding(false), which means they’ll use the platform default (potentially UTF-8 with BOM). To enforce consistency and avoid BOM-related issues, please update these calls to include the no-BOM encoding overload.Locations to update:
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs:37
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs:43
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs:150
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs:216
Suggested change for each:
- File.WriteAllText(path, contents); + File.WriteAllText(path, contents, new System.Text.UTF8Encoding(false));
🧹 Nitpick comments (6)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (2)
706-724: Make heartbeat writes atomic to avoid partial reads by clientsWindsurf/Kiro may read this file while it’s being updated. Use a temp file + replace to ensure readers never see truncated JSON.
Apply this diff in WriteHeartbeat:
- File.WriteAllText(filePath, JsonConvert.SerializeObject(payload), new System.Text.UTF8Encoding(false)); + var json = JsonConvert.SerializeObject(payload); + var enc = new System.Text.UTF8Encoding(false); + var tmp = filePath + ".tmp"; + File.WriteAllText(tmp, json, enc); + try + { + // Atomic on most platforms + File.Replace(tmp, filePath, null); + } + catch + { + // Fallback if Replace is not supported + File.Copy(tmp, filePath, true); + File.Delete(tmp); + }
328-347: Consider signaling “stopped” in final heartbeatStop() currently calls WriteHeartbeat(false) which records reason “ready”. That’s ambiguous for consumers. Prefer a terminal state.
- // Mark heartbeat one last time before stopping - WriteHeartbeat(false); + // Mark heartbeat one last time before stopping + WriteHeartbeat(false, "stopped");UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
220-223: BOM‑free UTF‑8 for script writes — approved; centralize encoding constantThe change to new UTF8Encoding(false) is correct and consistent with the repo’s policy. To avoid repetition and mistakes, centralize the encoding (single source of truth) and reuse.
Apply this minimal diff here:
- File.WriteAllText(fullPath, contents, new System.Text.UTF8Encoding(false)); + File.WriteAllText(fullPath, contents, MCPForUnity.Editor.Helpers.TextEncodings.Utf8NoBom);- File.WriteAllText(fullPath, contents, new System.Text.UTF8Encoding(false)); + File.WriteAllText(fullPath, contents, MCPForUnity.Editor.Helpers.TextEncodings.Utf8NoBom);Add this tiny helper once (new file or existing helpers namespace):
// UnityMcpBridge/Editor/Helpers/TextEncodings.cs namespace MCPForUnity.Editor.Helpers { public static class TextEncodings { public static readonly System.Text.Encoding Utf8NoBom = new System.Text.UTF8Encoding(false); } }Also applies to: 301-304
218-227: Optional: atomic writes for create/update to prevent transient reads of partial filesUnity imports right after the write, but on slower disks or external tooling, atomic replacement can still help avoid transient partial reads.
Example tweak (pattern — same idea as in heartbeat):
- File.WriteAllText(fullPath, contents, MCPForUnity.Editor.Helpers.TextEncodings.Utf8NoBom); + var tmp = fullPath + ".tmp"; + File.WriteAllText(tmp, contents, MCPForUnity.Editor.Helpers.TextEncodings.Utf8NoBom); + try { System.IO.File.Replace(tmp, fullPath, null); } + catch { System.IO.File.Copy(tmp, fullPath, true); System.IO.File.Delete(tmp); }Also applies to: 299-308
UnityMcpBridge/Editor/Tools/ManageShader.cs (2)
174-177: BOM‑free UTF‑8 for shader writes — approved; reuse a shared encoding constantMatches the project standard and prevents BOM‑related parsing surprises in toolchains. Recommend reusing a centralized encoding constant for consistency.
Apply this small diff:
- File.WriteAllText(fullPath, contents, new System.Text.UTF8Encoding(false)); + File.WriteAllText(fullPath, contents, MCPForUnity.Editor.Helpers.TextEncodings.Utf8NoBom);- File.WriteAllText(fullPath, contents, new System.Text.UTF8Encoding(false)); + File.WriteAllText(fullPath, contents, MCPForUnity.Editor.Helpers.TextEncodings.Utf8NoBom);Also applies to: 242-249
172-181: Optional: atomic shader writesSame rationale as scripts: reduce the chance of partial reads if external tooling scans shaders immediately after creation/update.
Pattern:
- File.WriteAllText(fullPath, contents, MCPForUnity.Editor.Helpers.TextEncodings.Utf8NoBom); + var tmp = fullPath + ".tmp"; + File.WriteAllText(tmp, contents, MCPForUnity.Editor.Helpers.TextEncodings.Utf8NoBom); + try { System.IO.File.Replace(tmp, fullPath, null); } + catch { System.IO.File.Copy(tmp, fullPath, true); System.IO.File.Delete(tmp); }Also applies to: 240-249
📜 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 (4)
UnityMcpBridge/Editor/Helpers/PortManager.cs(1 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(1 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs(2 hunks)UnityMcpBridge/Editor/Tools/ManageShader.cs(2 hunks)
🔇 Additional comments (2)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
706-724: Explicit UTF‑8 (no BOM) write for heartbeat JSON looks goodThis directly addresses the macOS/Windsurf “UTF‑8 with BOM” pitfall and aligns with the PR goal of BOM‑free JSON.
UnityMcpBridge/Editor/Helpers/PortManager.cs (1)
206-214: BOM‑free UTF‑8 for registry files — approvedThis aligns with the Windsurf/Kiro expectations and dsarno’s macOS BOM finding. Good move.
|
Ok, first round of feedback was addressed (I think lol). Let round 2 commence 😀 |
|
@msanatan I did another test run -- uninstalled the server from my local dev directory (which you also have to do or current version won't install latest), and also made sure the .config server was clear. And it worked like a charm. I'd say merge it and we can fix if we hear any issues. Thanks for doing this! |
…x/installer-cleanup-v2 * 'main' of https://github.com/CoplayDev/unity-mcp: Improve Windsurf MCP Config (CoplayDev#231)
…ing-testes * fix/installer-cleanup-v2: Server: set server_version.txt to 3.0.1 MCP: Fix macOS paths and VSCode manual setup refactor(installer): revert to lean installer logic from ee23346; fix macOS path via Application.platform; escape pgrep pattern feat(installer): rewire known configs (EditorPrefs, Cursor mcp.json) to canonical path; then remove legacy if unreferenced chore(version): bump server_version.txt to 3.0.5 fix(installer): use Application.platform for OS detection; add canonical root logs; fallback to RuntimeInformation chore(logging): include OS and server version in MCP bridge start log chore(server): bump server_version.txt to 3.0.4 bump server version sec(installer): escape server path in pgrep pattern to prevent injection/regex issues chore(editor): WriteToConfig honors pythonDir; robust config match; remove unused helpers fix(installer): skip legacy deletion when still referenced in prefs or Cursor config Improve Windsurf MCP Config (CoplayDev#231) feat: installer cleanup, auto-migration, logging normalization chore: bump version to 3.0.0 Don't ignore the package's README (CoplayDev#230) Rename namespace and public facing plugin output from "Unity MCP" to "MCP for Unity" (CoplayDev#225) # Conflicts: # UnityMcpBridge/Editor/Tools/ManageScript.cs # UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs # UnityMcpBridge/UnityMcpServer~/src/server.py # UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py # UnityMcpBridge/UnityMcpServer~/src/uv.lock
3 participants
I had some issues with Windsurf not detecting the MCP servers. Kiro continued to work fine. When I checked Kiro's config, the editor added more fields. When I used Kiro's config in Windsurf, it's working pretty consistently again... I'm over the IDE, but this should help other Windsurf users.
To update the auto-config and the manual config, I created a helper function
Summary by CodeRabbit
New Features
Refactor
Tests
Chores