Patch-Fix-04-26#1084
Conversation
1. fix(CoplayDev#1068): block NuGet System.CodeDom conflict via overrideReferences 2. fix(CoplayDev#1062): add reflection branch for Unity 6.6 InstanceIDToObject deprecation 3. refactor: merge EditorObjectResolver into UnityObjectIdCompat (could squash into prev) 4. fix(CoplayDev#1065): cache McpLogRecord.IsEnabled to allow ThreadPool reads 5. fix(CoplayDev#1070): emit stdio entry from OpenCodeConfigurator 6. fix: rewrite localhost→127.0.0.1 in NormalizeBaseUrl (supersedes CoplayDev#1057)
📝 WalkthroughWalkthroughThese changes refactor MCP server configuration detection to support multiple transport types (stdio/local and remote HTTP), consolidate instance ID compatibility helpers into a shared utility class, optimize repeated preference lookups with caching, normalize localhost addresses to IPv4, and adjust assembly reference overrides. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Clients/Configurators/OpenCodeConfigurator.cs (1)
153-160:⚠️ Potential issue | 🟡 Minor
GetManualSnippetwill surface rawInvalidOperationExceptionto UI.
BuildServerEntry()now throws whenuvxisn't found, butGetManualSnippet()has no try/catch. PerMcpClientConfigSection.UpdateManualConfiguration(), this call site has no error handling either, so a missing uvx in stdio mode will throw straight into the UI when the user opens the manual config panel — producing a red-error popup instead of a usable message in the snippet field.Configure()already handles this by settingMcpStatus.Error; the snippet path needs the same defensiveness.🛡️ Suggested fix
public override string GetManualSnippet() { - var snippet = new JObject - { - ["mcp"] = new JObject { [ServerName] = BuildServerEntry() } - }; - return JsonConvert.SerializeObject(snippet, Formatting.Indented); + try + { + var snippet = new JObject + { + ["mcp"] = new JObject { [ServerName] = BuildServerEntry() } + }; + return JsonConvert.SerializeObject(snippet, Formatting.Indented); + } + catch (InvalidOperationException ex) + { + return $"// Unable to build snippet: {ex.Message}"; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Clients/Configurators/OpenCodeConfigurator.cs` around lines 153 - 160, GetManualSnippet should not let BuildServerEntry's InvalidOperationException bubble to the UI; wrap the call to BuildServerEntry() in a try/catch for InvalidOperationException and, on catch, return a serialized snippet JSON that contains a clear error placeholder (e.g. ["mcp"][ServerName]["error"] = ex.Message) instead of rethrowing; this mirrors how Configure() sets McpStatus.Error and prevents the raw exception from producing a red popup in the manual config panel.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Clients/Configurators/OpenCodeConfigurator.cs (1)
209-222: Consider validating stdio command contents, not just shape.For remote entries you compare the URL to the expected
GetMcpRpcUrl(), but for stdio you only check thatcommandis aJArray. If the configuredcommandarray is stale (e.g. wrong uvx path, missing--transport stdio, different package name),CheckStatuswill reportConfiguredand skip the auto-rewrite branch. A lightweight check (e.g. comparing element[0] to the resolved uvx path or asserting--transport/stdioare present) would keep the symmetry with the remote path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Clients/Configurators/OpenCodeConfigurator.cs` around lines 209 - 222, EntryMatchesCurrentTransport currently only checks that entry["command"] is a JArray for stdio transports; extend this to validate the command contents to avoid stale configs: when expected == ConfiguredTransport.Stdio, ensure entry["command"] is a JArray with at least one element where element[0] equals the resolved uvx executable path (use your existing helper that resolves the uvx executable) and that the array contains the transport flags (e.g. a "--transport" arg followed by "stdio" or a single "--transport=stdio" token); keep the other return semantics and still use LocalType for the type check and UrlsEqual for the remote case.MCPForUnity/Runtime/Helpers/UnityObjectIdCompat.cs (1)
53-69: Reflection fallback design is intentional; consider a Debug.LogWarning for diagnostics (optional).The
InstanceIDToObject(int)API is documented as remaining at runtime on 6.6+ (only obsolete-as-error at compile time), and the reflection approach is explicitly designed to bypass the compile-time check. The null-safe?.Invokealready handles the case where the method is not found, and bothGameObjectLookup.ResolveInstanceIDandUnityTypeConvertersinclude their own null-result handling.If you add a one-time
Debug.LogWarningwhen_instanceIdToObject == null(as shown in the original diff), it would help distinguish "API removed from runtime" from "instance ID stale" on future Unity versions, though both currently produce the same null result by design.♻️ Suggested fix (optional)
`#if` UNITY_6000_6_OR_NEWER if (!_instanceIdToObjectInitialized) { _instanceIdToObject = typeof(EditorUtility).GetMethod( "InstanceIDToObject", BindingFlags.Public | BindingFlags.Static, null, new[] { typeof(int) }, null); _instanceIdToObjectInitialized = true; + if (_instanceIdToObject == null) + { + Debug.LogWarning("[UnityObjectIdCompat] EditorUtility.InstanceIDToObject(int) not found via reflection; instance-ID resolution will return null on this Unity version."); + } } return _instanceIdToObject?.Invoke(null, new object[] { instanceId }) as Object;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Helpers/UnityObjectIdCompat.cs` around lines 53 - 69, In UnityObjectIdCompat, when using reflection to find EditorUtility.InstanceIDToObject (check _instanceIdToObject and _instanceIdToObjectInitialized), add a one-time Debug.LogWarning if the reflected method (_instanceIdToObject) is null after initialization so callers can distinguish "method missing at runtime" from a stale instance ID; implement this by detecting _instanceIdToObject == null immediately after the GetMethod call and logging a clear one-time warning (use the same _instanceIdToObjectInitialized flag to ensure it only logs once) while leaving the existing null-safe ?.Invoke behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs`:
- Around line 339-348: The current localhost branch replaces the host by
building a new Uri and then calls builder.Uri.GetLeftPart(UriPartial.Authority),
which drops any path/query (so "http://localhost:8080/api?x=1" becomes
"http://127.0.0.1:8080"); instead, after creating the UriBuilder (and keeping
the remoteScope check and Uri.TryCreate logic), preserve the original
parsed.PathAndQuery when composing trimmed — e.g. use the authority from
builder.Uri combined with parsed.PathAndQuery (or otherwise append the original
path/query) so only the host is swapped and any path/query remains intact;
update the assignment to trimmed accordingly and ensure it still participates in
the existing NormalizeBaseUrl trailing-slash/path trimming logic.
---
Outside diff comments:
In `@MCPForUnity/Editor/Clients/Configurators/OpenCodeConfigurator.cs`:
- Around line 153-160: GetManualSnippet should not let BuildServerEntry's
InvalidOperationException bubble to the UI; wrap the call to BuildServerEntry()
in a try/catch for InvalidOperationException and, on catch, return a serialized
snippet JSON that contains a clear error placeholder (e.g.
["mcp"][ServerName]["error"] = ex.Message) instead of rethrowing; this mirrors
how Configure() sets McpStatus.Error and prevents the raw exception from
producing a red popup in the manual config panel.
---
Nitpick comments:
In `@MCPForUnity/Editor/Clients/Configurators/OpenCodeConfigurator.cs`:
- Around line 209-222: EntryMatchesCurrentTransport currently only checks that
entry["command"] is a JArray for stdio transports; extend this to validate the
command contents to avoid stale configs: when expected ==
ConfiguredTransport.Stdio, ensure entry["command"] is a JArray with at least one
element where element[0] equals the resolved uvx executable path (use your
existing helper that resolves the uvx executable) and that the array contains
the transport flags (e.g. a "--transport" arg followed by "stdio" or a single
"--transport=stdio" token); keep the other return semantics and still use
LocalType for the type check and UrlsEqual for the remote case.
In `@MCPForUnity/Runtime/Helpers/UnityObjectIdCompat.cs`:
- Around line 53-69: In UnityObjectIdCompat, when using reflection to find
EditorUtility.InstanceIDToObject (check _instanceIdToObject and
_instanceIdToObjectInitialized), add a one-time Debug.LogWarning if the
reflected method (_instanceIdToObject) is null after initialization so callers
can distinguish "method missing at runtime" from a stale instance ID; implement
this by detecting _instanceIdToObject == null immediately after the GetMethod
call and logging a clear one-time warning (use the same
_instanceIdToObjectInitialized flag to ensure it only logs once) while leaving
the existing null-safe ?.Invoke behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 462996db-1960-45b6-93d9-9eafced4df0f
📒 Files selected for processing (10)
MCPForUnity/Editor/Clients/Configurators/OpenCodeConfigurator.csMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Helpers/HttpEndpointUtility.csMCPForUnity/Editor/Helpers/McpLogRecord.csMCPForUnity/Editor/MCPForUnity.Editor.asmdefMCPForUnity/Runtime/Helpers/UnityObjectIdCompat.csMCPForUnity/Runtime/Helpers/UnityObjectIdCompat.cs.metaMCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.csMCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs.metaMCPForUnity/Runtime/Serialization/UnityTypeConverters.cs
💤 Files with no reviewable changes (2)
- MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs.meta
- MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs
| // For local scope, force 127.0.0.1 over "localhost". Windows getaddrinfo returns ::1 | ||
| // first; clients without Happy Eyeballs (e.g., Codex/reqwest) hit the v6 socket while | ||
| // our server binds v4-only, breaking the handshake. The default server bind is | ||
| // 127.0.0.1, so emitting the literal v4 keeps every client unambiguous. | ||
| if (!remoteScope && Uri.TryCreate(trimmed, UriKind.Absolute, out Uri parsed) | ||
| && string.Equals(parsed.Host, "localhost", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| var builder = new UriBuilder(parsed) { Host = "127.0.0.1" }; | ||
| trimmed = builder.Uri.GetLeftPart(UriPartial.Authority); | ||
| } |
There was a problem hiding this comment.
Path/query silently dropped when host is localhost.
GetLeftPart(UriPartial.Authority) strips everything after the authority, so a stored value like http://localhost:8080/api would be rewritten to http://127.0.0.1:8080, while the same URL with a non-localhost host on lines 319-337 keeps its path intact. This is an inconsistent normalization. Since NormalizeBaseUrl is designed for base URLs (and already strips /mcp/trailing slashes), this is likely fine in practice, but consider preserving any remaining path for symmetry, e.g. only swapping the host:
♻️ Suggested fix to preserve path/query during host swap
- if (!remoteScope && Uri.TryCreate(trimmed, UriKind.Absolute, out Uri parsed)
- && string.Equals(parsed.Host, "localhost", StringComparison.OrdinalIgnoreCase))
- {
- var builder = new UriBuilder(parsed) { Host = "127.0.0.1" };
- trimmed = builder.Uri.GetLeftPart(UriPartial.Authority);
- }
+ if (!remoteScope && Uri.TryCreate(trimmed, UriKind.Absolute, out Uri parsed)
+ && string.Equals(parsed.Host, "localhost", StringComparison.OrdinalIgnoreCase))
+ {
+ var builder = new UriBuilder(parsed) { Host = "127.0.0.1" };
+ trimmed = builder.Uri.AbsoluteUri.TrimEnd('/');
+ }📝 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.
| // For local scope, force 127.0.0.1 over "localhost". Windows getaddrinfo returns ::1 | |
| // first; clients without Happy Eyeballs (e.g., Codex/reqwest) hit the v6 socket while | |
| // our server binds v4-only, breaking the handshake. The default server bind is | |
| // 127.0.0.1, so emitting the literal v4 keeps every client unambiguous. | |
| if (!remoteScope && Uri.TryCreate(trimmed, UriKind.Absolute, out Uri parsed) | |
| && string.Equals(parsed.Host, "localhost", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| var builder = new UriBuilder(parsed) { Host = "127.0.0.1" }; | |
| trimmed = builder.Uri.GetLeftPart(UriPartial.Authority); | |
| } | |
| // For local scope, force 127.0.0.1 over "localhost". Windows getaddrinfo returns ::1 | |
| // first; clients without Happy Eyeballs (e.g., Codex/reqwest) hit the v6 socket while | |
| // our server binds v4-only, breaking the handshake. The default server bind is | |
| // 127.0.0.1, so emitting the literal v4 keeps every client unambiguous. | |
| if (!remoteScope && Uri.TryCreate(trimmed, UriKind.Absolute, out Uri parsed) | |
| && string.Equals(parsed.Host, "localhost", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| var builder = new UriBuilder(parsed) { Host = "127.0.0.1" }; | |
| trimmed = builder.Uri.AbsoluteUri.TrimEnd('/'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs` around lines 339 - 348,
The current localhost branch replaces the host by building a new Uri and then
calls builder.Uri.GetLeftPart(UriPartial.Authority), which drops any path/query
(so "http://localhost:8080/api?x=1" becomes "http://127.0.0.1:8080"); instead,
after creating the UriBuilder (and keeping the remoteScope check and
Uri.TryCreate logic), preserve the original parsed.PathAndQuery when composing
trimmed — e.g. use the authority from builder.Uri combined with
parsed.PathAndQuery (or otherwise append the original path/query) so only the
host is swapped and any path/query remains intact; update the assignment to
trimmed accordingly and ensure it still participates in the existing
NormalizeBaseUrl trailing-slash/path trimming logic.
There was a problem hiding this comment.
Pull request overview
This PR delivers a set of compatibility and configuration patches across the Unity package, addressing Unity 6.6 API deprecations, editor assembly reference conflicts, logging thread-safety, and client configuration generation.
Changes:
- Add a unified InstanceID/EntityId compatibility helper (including a Unity 6.6 reflection path) and route editor-side instance resolution through it.
- Update editor compilation behavior to avoid NuGet
System.CodeDomtype conflicts and make log-record enablement safe to read from ThreadPool continuations. - Improve client/config endpoint generation by normalizing local
localhostto127.0.0.1and emitting an OpenCode stdio configuration entry when the server transport is stdio.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | Switches instanceID deserialization lookup to UnityObjectIdCompat.InstanceIDToObjectCompat. |
| MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs.meta | Removes meta for the old compat extension file (part of rename/move). |
| MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs | Removes the old compat extension implementation (replaced by new unified helper). |
| MCPForUnity/Runtime/Helpers/UnityObjectIdCompat.cs.meta | Adds meta for the new unified compat helper (retaining GUID). |
| MCPForUnity/Runtime/Helpers/UnityObjectIdCompat.cs | Introduces GetInstanceIDCompat and editor-only InstanceIDToObjectCompat with Unity 6.6 reflection fallback. |
| MCPForUnity/Editor/MCPForUnity.Editor.asmdef | Enables overrideReferences to avoid reference collisions (e.g., System.CodeDom). |
| MCPForUnity/Editor/Helpers/McpLogRecord.cs | Caches IsEnabled via an editor-load refresh to avoid ThreadPool EditorPrefs reads. |
| MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs | Normalizes local base URLs by rewriting localhost → 127.0.0.1. |
| MCPForUnity/Editor/Helpers/GameObjectLookup.cs | Uses UnityObjectIdCompat.InstanceIDToObjectCompat for instance ID resolution. |
| MCPForUnity/Editor/Clients/Configurators/OpenCodeConfigurator.cs | Emits a stdio-capable OpenCode server entry and validates config against the current transport. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| && string.Equals(parsed.Host, "localhost", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| var builder = new UriBuilder(parsed) { Host = "127.0.0.1" }; | ||
| trimmed = builder.Uri.GetLeftPart(UriPartial.Authority); |
There was a problem hiding this comment.
NormalizeBaseUrl currently rewrites localhost by rebuilding the URI and then assigning trimmed = builder.Uri.GetLeftPart(UriPartial.Authority). This drops any existing path/query/fragment, which is inconsistent with the rest of the function (it only strips a trailing /mcp) and can change behavior only for localhost inputs (e.g., http://localhost:8080/foo becomes http://127.0.0.1:8080). Consider normalizing to the authority for all parsed URLs (to match the method’s doc comment), or preserve the original path when rewriting the host so the transformation is host-only.
| trimmed = builder.Uri.GetLeftPart(UriPartial.Authority); | |
| trimmed = builder.Uri.ToString().TrimEnd('/'); |
| /// <summary> | ||
| /// Resolves an int instance ID handle back to a UnityEngine.Object. | ||
| /// Pre-6.0 : EditorUtility.InstanceIDToObject(int) | ||
| /// 6.0–6.5 : EditorUtility.EntityIdToObject(int) (implicit int→EntityId cast) | ||
| /// 6.6+ : reflection on InstanceIDToObject(int) — the API still exists at runtime | ||
| /// but is obsolete-as-error; reflection bypasses CS0619 until the public | ||
| /// EntityId(int) ctor stabilizes. | ||
| /// </summary> |
There was a problem hiding this comment.
The version notes in this XML doc don’t match the actual compile-time gates below: the EntityIdToObject(int) path is behind UNITY_6000_3_OR_NEWER, so Unity 6.0–6.2 would still use InstanceIDToObject(int). Please update the comment to reflect the real version ranges used by the #if blocks so future changes don’t rely on incorrect assumptions.
Patch for several issues mentioned in the repo:
overrideReferences
deprecation
Replace localhost with 127.0.0.1 for correct Codex handshake #1057)
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Chores