Replace localhost with 127.0.0.1 for correct Codex handshake#1057
Closed
hvesuk wants to merge 2 commits into
Closed
Replace localhost with 127.0.0.1 for correct Codex handshake#1057hvesuk wants to merge 2 commits into
hvesuk wants to merge 2 commits into
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts Codex-specific MCP HTTP URL generation to use 127.0.0.1 instead of localhost while keeping shared HTTP endpoint logic unchanged for other clients. Class diagram for updated CodexConfigHelper HTTP URL generationclassDiagram
class CodexConfigHelper {
+string BuildCodexServerBlock(string uvPath)
-TomlTable CreateUnityMcpTable(string uvPath)
-string GetCodexHttpUrl()
}
class HttpEndpointUtility {
+string GetMcpRpcUrl()
}
class TomlTable
class TomlString {
+string Value
}
CodexConfigHelper --> HttpEndpointUtility : uses
CodexConfigHelper --> TomlTable : creates
CodexConfigHelper --> TomlString : sets_url_value
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a private helper Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- GetCodexHttpUrl relies on a string.Replace of the hard-coded prefix "http://localhost:"; consider parsing the URI (e.g., via Uri) and forcing the host to 127.0.0.1 so you correctly handle https, alternate schemes, or any future change to the base URL format.
- If Codex has the same handshake issue over https, you may want GetCodexHttpUrl to normalize any
localhosthost (regardless of scheme) to 127.0.0.1 rather than only matching the specifichttp://localhost:prefix.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- GetCodexHttpUrl relies on a string.Replace of the hard-coded prefix "http://localhost:"; consider parsing the URI (e.g., via Uri) and forcing the host to 127.0.0.1 so you correctly handle https, alternate schemes, or any future change to the base URL format.
- If Codex has the same handshake issue over https, you may want GetCodexHttpUrl to normalize any `localhost` host (regardless of scheme) to 127.0.0.1 rather than only matching the specific `http://localhost:` prefix.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Helpers/CodexConfigHelper.cs" line_range="232-235" />
<code_context>
return unityMCP;
}
+ private static string GetCodexHttpUrl()
+ {
+ return HttpEndpointUtility.GetMcpRpcUrl()
+ .Replace("http://localhost:", "http://127.0.0.1:");
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Make the localhost-to-127.0.0.1 conversion more robust (https, non-prefix matches, future changes).
This relies on `HttpEndpointUtility.GetMcpRpcUrl()` always returning `http://localhost:<port>`. If it ever returns `https://localhost:...` or `localhost` appears elsewhere in the URL, this string replace may fail or corrupt the URL. Instead, parse the URL with `Uri`, check that `Host == "localhost"`, and rebuild it with `Host = "127.0.0.1"` while preserving the original scheme, port, path, and query to avoid these assumptions.
Suggested implementation:
```csharp
private static string GetCodexHttpUrl()
{
var rawUrl = HttpEndpointUtility.GetMcpRpcUrl();
if (string.IsNullOrWhiteSpace(rawUrl))
{
return rawUrl;
}
if (!Uri.TryCreate(rawUrl, UriKind.Absolute, out var uri))
{
// If parsing fails, don't try to be clever – just return the original value
return rawUrl;
}
if (!string.Equals(uri.Host, "localhost", StringComparison.OrdinalIgnoreCase))
{
// Only rewrite when the host is actually localhost
return rawUrl;
}
var builder = new UriBuilder(uri)
{
Host = "127.0.0.1"
};
return builder.Uri.ToString();
}
```
If `System` is not already imported at the top of `CodexConfigHelper.cs`, add `using System;` so that `Uri`, `UriBuilder`, and `StringComparison` resolve correctly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+232
to
+235
| private static string GetCodexHttpUrl() | ||
| { | ||
| return HttpEndpointUtility.GetMcpRpcUrl() | ||
| .Replace("http://localhost:", "http://127.0.0.1:"); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Make the localhost-to-127.0.0.1 conversion more robust (https, non-prefix matches, future changes).
This relies on HttpEndpointUtility.GetMcpRpcUrl() always returning http://localhost:<port>. If it ever returns https://localhost:... or localhost appears elsewhere in the URL, this string replace may fail or corrupt the URL. Instead, parse the URL with Uri, check that Host == "localhost", and rebuild it with Host = "127.0.0.1" while preserving the original scheme, port, path, and query to avoid these assumptions.
Suggested implementation:
private static string GetCodexHttpUrl()
{
var rawUrl = HttpEndpointUtility.GetMcpRpcUrl();
if (string.IsNullOrWhiteSpace(rawUrl))
{
return rawUrl;
}
if (!Uri.TryCreate(rawUrl, UriKind.Absolute, out var uri))
{
// If parsing fails, don't try to be clever – just return the original value
return rawUrl;
}
if (!string.Equals(uri.Host, "localhost", StringComparison.OrdinalIgnoreCase))
{
// Only rewrite when the host is actually localhost
return rawUrl;
}
var builder = new UriBuilder(uri)
{
Host = "127.0.0.1"
};
return builder.Uri.ToString();
}If System is not already imported at the top of CodexConfigHelper.cs, add using System; so that Uri, UriBuilder, and StringComparison resolve correctly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/CodexConfigHelper.cs`:
- Around line 232-236: GetCodexHttpUrl currently uses string.Replace on the raw
URL from HttpEndpointUtility.GetMcpRpcUrl(), which fails when the scheme changes
(e.g., https) or the host appears in different forms; instead parse the value
returned by HttpEndpointUtility.GetMcpRpcUrl() into a System.Uri (or
UriBuilder), detect if the host is "localhost" or IPLoopback, and if so set the
host to "127.0.0.1" while preserving scheme, port, path and query, then return
the rebuilt URL string; update GetCodexHttpUrl to use Uri/UriBuilder logic and
handle exceptions/fallbacks to the original string from
HttpEndpointUtility.GetMcpRpcUrl().
🪄 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: 281bc350-08c6-4e43-a6ee-3d095a854a10
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs
Comment on lines
+232
to
+236
| private static string GetCodexHttpUrl() | ||
| { | ||
| return HttpEndpointUtility.GetMcpRpcUrl() | ||
| .Replace("http://localhost:", "http://127.0.0.1:"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate HttpEndpointUtility and inspect URL construction =="
fd -a "HttpEndpointUtility.cs" | while read -r f; do
echo "--- $f ---"
rg -n -C3 'GetBaseUrl|GetMcpRpcUrl|localhost|https|Uri|port' "$f"
done
echo
echo "== Find localhost URL patterns and transport-related usage across C# files =="
rg -n -C2 --type=cs 'http://localhost|https://localhost|GetMcpRpcUrl\(|UseHttpTransport'Repository: CoplayDev/unity-mcp
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect CodexConfigHelper.cs GetCodexHttpUrl method ==="
fd -a "CodexConfigHelper.cs" | while read -r f; do
echo "--- $f (lines 230-250) ---"
sed -n '230,250p' "$f"
done
echo
echo "=== Inspect HttpEndpointUtility GetLocalBaseUrl and GetRemoteBaseUrl ==="
fd -a "HttpEndpointUtility.cs" | while read -r f; do
echo "--- GetLocalBaseUrl method ---"
sed -n '33,90p' "$f"
echo
echo "--- GetRemoteBaseUrl method ---"
sed -n '103,130p' "$f" 2>/dev/null || echo "(not in expected range)"
done
echo
echo "=== Check for HTTPS usage with localhost ==="
rg -n 'https.*localhost|localhost.*https' --type=cs -i
echo
echo "=== Check what AppendPathSegment does ==="
rg -n -A10 'AppendPathSegment' --type=cs | head -30Repository: CoplayDev/unity-mcp
Length of output: 4312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find DefaultLocalBaseUrl and DefaultRemoteBaseUrl constants ==="
rg -n 'DefaultLocalBaseUrl|DefaultRemoteBaseUrl|LocalPrefKey|RemotePrefKey' --type=cs | head -20
echo
echo "=== Find NormalizeBaseUrl implementation ==="
rg -n -A30 'private static string NormalizeBaseUrl' --type=cs
echo
echo "=== Check what schemes are assigned in the normalization process ==="
rg -n 'http:|https:' --type=cs -C2 | grep -A2 -B2 'scheme\|http://' | head -40Repository: CoplayDev/unity-mcp
Length of output: 10211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for GetCodexHttpUrl usage ==="
rg -n 'GetCodexHttpUrl' --type=cs
echo
echo "=== Context around CodexConfigHelper GetCodexHttpUrl and its callers ==="
fd -a "CodexConfigHelper.cs" | while read -r f; do
echo "--- Full context (lines 220-250) ---"
sed -n '220,250p' "$f"
done
echo
echo "=== Check if localhost ever appears in remote scope settings ==="
rg -n 'HttpRemoteBaseUrl|RemoteBaseUrl.*localhost' --type=cs
echo
echo "=== Find any tests for CodexConfigHelper ==="
fd -a "*CodexConfigHelper*Test*" --type fRepository: CoplayDev/unity-mcp
Length of output: 4263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Context of GetCodexHttpUrl calls at lines 39 and 194 ==="
fd -a "CodexConfigHelper.cs" | while read -r f; do
echo "--- Lines 30-50 (first call at 39) ---"
sed -n '30,50p' "$f"
echo
echo "--- Lines 185-205 (second call at 194) ---"
sed -n '185,205p' "$f"
done
echo
echo "=== Check if GetCodexHttpUrl result is only used for local scope ==="
fd -a "CodexConfigHelper.cs" | while read -r f; do
echo "--- Full method context around GetCodexHttpUrl calls ---"
sed -n '25,75p' "$f"
echo
sed -n '175,220p' "$f"
done
echo
echo "=== Look for tests of CodexConfigHelper with HTTP URLs ==="
fd -a "CodexConfigHelperTests.cs" | head -1 | xargs -I {} sed -n '460,540p' {}Repository: CoplayDev/unity-mcp
Length of output: 10227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if IsRemoteScope can be true when useHttpTransport=true ==="
rg -n -B5 -A5 'GetCodexHttpUrl\|GetMcpRpcUrl\|IsRemoteScope' MCPForUnity/Editor/Helpers/CodexConfigHelper.cs | head -30
echo
echo "=== Verify remote scope can use localhost ==="
rg -n 'HttpRemoteBaseUrl|RemoteBaseUrl|IsRemoteScope' MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs | head -20
echo
echo "=== Check actual URL construction for remote scope ==="
fd -a "HttpEndpointUtility.cs" | while read -r f; do
echo "--- NormalizeBaseUrl with remoteScope examples ---"
sed -n '312,340p' "$f"
doneRepository: CoplayDev/unity-mcp
Length of output: 132
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if IsRemoteScope can be true and what URLs it produces ==="
fd -a "HttpEndpointUtility.cs" | while read -r f; do
echo "--- GetBaseUrl and IsRemoteScope logic ---"
sed -n '29,32p' "$f"
echo
echo "--- NormalizeBaseUrl remoteScope=true branch (lines 312-340) ---"
sed -n '312,340p' "$f"
done
echo
echo "=== Check if GetRemoteBaseUrl can return a localhost URL ==="
rg -n 'GetRemoteBaseUrl|DefaultRemoteBaseUrl|RemotePrefKey' MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs | head -15
echo
echo "=== Verify: Can a user configure RemoteBaseUrl with localhost? ==="
rg -n -C3 'SaveRemoteBaseUrl|SetHttpRemoteBaseUrl' --type=cs | head -40
echo
echo "=== Check validation of remote URLs ==="
fd -a "HttpEndpointUtility.cs" | while read -r f; do
echo "--- ValidateRemoteBaseUrl method ---"
sed -n '255,290p' "$f"
doneRepository: CoplayDev/unity-mcp
Length of output: 7806
Rewrite localhost URL handling to use URI-based host replacement instead of string replacement.
The current implementation only matches http://localhost: exactly. When the HTTP Remote scope is active and configured with a localhost URL (e.g., localhost:9000), it normalizes to https://localhost:..., which the .Replace() call silently misses.
Suggested fix
private static string GetCodexHttpUrl()
{
- return HttpEndpointUtility.GetMcpRpcUrl()
- .Replace("http://localhost:", "http://127.0.0.1:");
+ string rawUrl = HttpEndpointUtility.GetMcpRpcUrl();
+ if (!Uri.TryCreate(rawUrl, UriKind.Absolute, out Uri uri))
+ {
+ return rawUrl;
+ }
+
+ if (!string.Equals(uri.Host, "localhost", StringComparison.OrdinalIgnoreCase))
+ {
+ return rawUrl;
+ }
+
+ var builder = new UriBuilder(uri)
+ {
+ Host = "127.0.0.1"
+ };
+
+ return builder.Uri.ToString();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/CodexConfigHelper.cs` around lines 232 - 236,
GetCodexHttpUrl currently uses string.Replace on the raw URL from
HttpEndpointUtility.GetMcpRpcUrl(), which fails when the scheme changes (e.g.,
https) or the host appears in different forms; instead parse the value returned
by HttpEndpointUtility.GetMcpRpcUrl() into a System.Uri (or UriBuilder), detect
if the host is "localhost" or IPLoopback, and if so set the host to "127.0.0.1"
while preserving scheme, port, path and query, then return the rebuilt URL
string; update GetCodexHttpUrl to use Uri/UriBuilder logic and handle
exceptions/fallbacks to the original string from
HttpEndpointUtility.GetMcpRpcUrl().
Collaborator
|
Hi Alex, thanks for the fix on the Codex issue. Can you add comments on this hard-coded fix, in case Codex in the future route it correctly, and this hard-coded fix becomes an issue? |
Contributor
Author
|
@Scriptwonder I added a comment. Is that enough? |
Merged
Scriptwonder
added a commit
that referenced
this pull request
Apr 27, 2026
1. fix(#1068): block NuGet System.CodeDom conflict via overrideReferences 2. fix(#1062): add reflection branch for Unity 6.6 InstanceIDToObject deprecation 3. refactor: merge EditorObjectResolver into UnityObjectIdCompat (could squash into prev) 4. fix(#1065): cache McpLogRecord.IsEnabled to allow ThreadPool reads 5. fix(#1070): emit stdio entry from OpenCodeConfigurator 6. fix: rewrite localhost→127.0.0.1 in NormalizeBaseUrl (supersedes #1057)
Collaborator
|
I looked into it more closely and implemented a more generalized solution in #1084; this way, when Codex and other similar clients that are running IPv6 are running local HTTP, it will transform local to 127.0.0.1, all in one place (tested with Windows Codex App). Thanks for submitting and putting in the good work! |
Contributor
Author
|
@Scriptwonder Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
127.0.0.1instead oflocalhostWhy
In practice, Unity MCP was reachable and responded correctly, but Codex could fail during the MCP initialize/handshake step when configured with
http://localhost:8080/mcp.Using
http://127.0.0.1:8080/mcpfixes the issue consistently after restarting Codex, which suggests the problem is client-side loopback/hostname resolution behavior in Codex rather than a Unity MCP server issue.Type of Change
Save your change type
Changes Made
127.0.0.1instead oflocalhostCodexConfigHelperonlyTesting/Screenshots/Recordings
enable HTTP transport for Codex config generation
verify generated TOML contains:
[mcp_servers.unityMCP]url = "http://127.0.0.1:8080/mcp"restart Codex
confirm Codex connects to Unity MCP successfully
confirm no behavior changes for non-Codex clients
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Summary by Sourcery
Update Codex HTTP MCP configuration generation to use a loopback IP address that ensures reliable Codex handshakes.
Bug Fixes:
Enhancements:
Summary by CodeRabbit