Fix/auto port fallback #101
Conversation
…play - StdioBridgeHost.cs: Disable ReuseAddress on macOS to force AddressAlreadyInUse exception on conflict. - PortManager.cs: Add connection check safety net for macOS. - McpConnectionSection.cs: Ensure UI displays the actual live port instead of just the requested one.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a macOS TCP connect re-check for port availability; centralizes TcpListener construction and socket options with persisted port fallback; tightens PluginHub exception handling; adds Windows UV-based Python detection; restructures macOS Python detection messaging; minor UI and comment tweaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Greptile OverviewGreptile SummaryThis PR fixes a critical macOS port conflict issue where Key Changes:
Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Unity as Unity Editor
participant Port as PortManager
participant Bridge as StdioBridgeHost
participant OS as Operating System
participant Server as MCP Server
Note over Unity,Server: Port Allocation & Fallback Flow
Unity->>Port: GetPortWithFallback()
Port->>Port: Check stored config
alt Stored port exists for project
Port-->>Unity: Return stored port
else No stored port
Port-->>Unity: Return default (6400)
end
Unity->>Bridge: Start() with port
Bridge->>Port: IsPortAvailable(port)?
alt macOS Platform
Port->>OS: Try TcpListener.Start()
alt Bind succeeds
OS-->>Port: Bind OK
Port->>OS: Try connect to port
alt Connection succeeds
OS-->>Port: Connected (port in use!)
Port-->>Bridge: false (not available)
else Connection fails
OS-->>Port: Connection failed
Port-->>Bridge: true (available)
end
else Bind fails
OS-->>Port: SocketException
Port-->>Bridge: false (not available)
end
else Windows/Linux
Port->>OS: Try TcpListener.Start() with SO_REUSEADDR
OS-->>Port: Result
Port-->>Bridge: true/false
end
alt Port available
Bridge->>OS: Start TcpListener
Note over Bridge: SO_REUSEADDR disabled on macOS
OS-->>Bridge: Listener started
Bridge-->>Unity: Success
else Port in use (after retries)
Bridge->>Port: DiscoverNewPort()
Port->>Port: FindAvailablePort()
loop Search 6401 to 6500
Port->>Port: IsPortAvailable(port)?
alt Found available
Port->>Port: SavePort(newPort)
Port-->>Bridge: newPort
end
end
Bridge->>OS: Start on newPort
OS-->>Bridge: Listener started
Bridge-->>Unity: Success with new port
end
Unity->>Server: Connect to Unity port
Server->>Unity: Handshake
Note over Unity,Server: Connection established
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| using var client = new TcpClient(); | ||
| var connectTask = client.ConnectAsync(IPAddress.Loopback, port); | ||
| // If we connect successfully, someone is listening -> Not available | ||
| if (connectTask.Wait(50) && client.Connected) | ||
| { | ||
| if (IsDebugEnabled()) McpLog.Info($"[PortManager] Port {port} bind succeeded but connection also succeeded -> Not available (Conflict)."); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
style: Resource leak: TcpClient is created without using statement, but client.ConnectAsync() returns a Task that's awaited synchronously with .Wait(50). If connection times out or succeeds, the TcpClient may not be properly disposed before entering the catch block or returning.
| using var client = new TcpClient(); | |
| var connectTask = client.ConnectAsync(IPAddress.Loopback, port); | |
| // If we connect successfully, someone is listening -> Not available | |
| if (connectTask.Wait(50) && client.Connected) | |
| { | |
| if (IsDebugEnabled()) McpLog.Info($"[PortManager] Port {port} bind succeeded but connection also succeeded -> Not available (Conflict)."); | |
| return false; | |
| } | |
| } | |
| try | |
| { | |
| using var client = new TcpClient(); | |
| var connectTask = client.ConnectAsync(IPAddress.Loopback, port); | |
| // If we connect successfully, someone is listening -> Not available | |
| if (connectTask.Wait(50) && client.Connected) | |
| { | |
| if (IsDebugEnabled()) McpLog.Info($"[PortManager] Port {port} bind succeeded but connection also succeeded -> Not available (Conflict)."); | |
| return false; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: MCPForUnity/Editor/Helpers/PortManager.cs
Line: 140:148
Comment:
**style:** Resource leak: `TcpClient` is created without using statement, but `client.ConnectAsync()` returns a `Task` that's awaited synchronously with `.Wait(50)`. If connection times out or succeeds, the `TcpClient` may not be properly disposed before entering the `catch` block or returning.
```suggestion
try
{
using var client = new TcpClient();
var connectTask = client.ConnectAsync(IPAddress.Loopback, port);
// If we connect successfully, someone is listening -> Not available
if (connectTask.Wait(50) && client.Connected)
{
if (IsDebugEnabled()) McpLog.Info($"[PortManager] Port {port} bind succeeded but connection also succeeded -> Not available (Conflict).");
return false;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.![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
🧹 Nitpick comments (4)
Server/src/services/tools/debug_request_context.py (1)
64-66: Consider documenting that these debug fields expose internal implementation details.The new fields are useful for debugging, but consumers should be aware that
all_keys_in_storeandmiddleware_idreflect internal state that may change between versions.Server/src/transport/unity_instance_middleware.py (1)
106-117: Consider narrowing the exception type for better diagnostics.The broad
Exceptioncatch is flagged by static analysis (BLE001). While the intent is resilience, catching specific exceptions would help distinguish between expected failures (e.g., connection issues) and unexpected bugs.- except Exception as exc: + except (ConnectionError, TimeoutError, ValueError, KeyError) as exc:Alternatively, if you want to keep the broad catch for maximum resilience, you could at minimum re-raise if it's a
SystemExitorKeyboardInterrupt:- except Exception as exc: + except BaseException as exc: + if isinstance(exc, (SystemExit, KeyboardInterrupt)): + raiseMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
170-197: UI now correctly reflects live port while running; consider UX and persistence edge casesThe new logic to always set
unityPortField.valuefrombridgeService.CurrentPortwhen a session is active ensures the UI matches the actual bound port, which is important now that the bridge can change ports dynamically. Likewise, only falling back toCurrentPortwhenEditorPrefs.GetInt(EditorPrefKeys.UnitySocketPort, 0)is0avoids clobbering an explicitly stored user preference when the bridge is idle. This looks sound.Two follow‑ups worth considering:
- While
IsRunning,UpdateConnectionStatusis called every editor update and now overwritesunityPortField.valueeach time. That effectively prevents editing the port while a session is active and may cause visible flicker if a user tries. If you intend port edits to be “stop, edit, start”, you might want to disable the field whenisRunningis true instead of constantly resetting its text.- Auto‑fallback in
StdioBridgeHost.Startnow goes throughPortManager.DiscoverNewPort(), which persists to PortManager’s registry but does not updateEditorPrefs.UnitySocketPort. In the non‑running branch here you only sync fromCurrentPortwhen the saved EditorPrefs value is0, so a manually set (non‑zero) preference could diverge from the auto‑discovered port used and stored by PortManager. If the intent is that auto‑fallback should become the new canonical project port, you might want to also updateEditorPrefs(either inDiscoverNewPortor in the fallback path) so the idle UI doesn’t drift from the actual stored port.MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
340-381: Auto‑fallback now uses DiscoverNewPort; be aware of rare failure and persistence semanticsSwitching the AddressAlreadyInUse fallback from whatever was used before to:
currentUnityPort = PortManager.DiscoverNewPort(); ... listener = new TcpListener(IPAddress.Loopback, currentUnityPort); #if !UNITY_EDITOR_OSX listener.Server.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); #endif ... listener.Start();makes the behavior line up with
PortManager’s new discovery path and ensures the chosen port is persisted viaSavePort. The debug logging that distinguishes “became available” vs “switching to port X” is also helpful.Two small considerations:
- If, despite
IsPortAvailable,DiscoverNewPort()returned a port that is still in use, thelistener.Start()inside this catch block will throw anotherSocketException. That one will escape theforloop and be caught only by the outercatch (SocketException ex)at the end ofStart(), resulting in a logged error and no further retry attempts. This is a very low‑probability corner case, but it’s worth knowing that there is no second fallback chain.- The listener initialization sequence (
TcpListenerconstruction, optionalReuseAddress,ExclusiveAddressUsetweak, andLingerState) is now duplicated in both the initialtryand the fallback catch. If you end up touching this again, factoring it into a small helper (e.g.,CreateConfiguredListener(int port)) would reduce the chance of the two code paths diverging over time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
MCPForUnity/Editor/Helpers/PortManager.cs(1 hunks)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs(3 hunks)MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs(2 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(1 hunks)MCPForUnity/package.json(1 hunks)Server/README.md(2 hunks)Server/pyproject.toml(1 hunks)Server/src/main.py(3 hunks)Server/src/services/resources/unity_instances.py(4 hunks)Server/src/services/tools/debug_request_context.py(3 hunks)Server/src/services/tools/set_active_instance.py(3 hunks)Server/src/transport/legacy/unity_connection.py(1 hunks)Server/src/transport/unity_instance_middleware.py(3 hunks)Server/src/transport/unity_transport.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:19:59.258Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 85
File: tests/test_telemetry_queue_worker.py:33-37
Timestamp: 2025-10-21T05:19:59.258Z
Learning: In the unity-mcp repository, pyproject.toml is located in MCPForUnity/UnityMcpServer~/src/, not at the repository root. When testing code that imports telemetry.py, the working directory should be changed to SRC (MCPForUnity/UnityMcpServer~/src/) so that telemetry.py's get_package_version() can find pyproject.toml via relative path.
Applied to files:
Server/pyproject.tomlServer/README.md
🧬 Code graph analysis (7)
Server/src/services/tools/debug_request_context.py (1)
Server/src/transport/unity_instance_middleware.py (2)
get_session_key(53-66)get_active_instance(74-78)
Server/src/services/resources/unity_instances.py (1)
Server/src/transport/unity_transport.py (1)
_current_transport(22-24)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)
Server/src/services/tools/set_active_instance.py (2)
Server/src/transport/unity_transport.py (1)
_current_transport(22-24)Server/src/transport/unity_instance_middleware.py (3)
get_unity_instance_middleware(22-31)set_active_instance(68-72)get_session_key(53-66)
Server/src/transport/unity_instance_middleware.py (1)
Server/src/services/tools/set_active_instance.py (1)
set_active_instance(15-112)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
MCPForUnity/Editor/Helpers/PortManager.cs (2)
PortManager(18-344)DiscoverNewPort(59-65)
Server/src/main.py (1)
Server/src/transport/unity_instance_middleware.py (2)
UnityInstanceMiddleware(40-122)get_unity_instance_middleware(22-31)
🪛 Ruff (0.14.7)
Server/src/transport/unity_instance_middleware.py
106-106: Do not catch blind exception: Exception
(BLE001)
Server/src/main.py
57-57: Do not catch blind exception: Exception
(BLE001)
60-60: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (21)
Server/pyproject.toml (1)
3-3: Version bump consistent across repository.Version update from 8.1.4 to 8.1.5 aligns with the coordinated version increment visible in MCPForUnity/package.json. Dependency constraints remain appropriate and unchanged.
MCPForUnity/package.json (1)
3-3: Version bump aligned with server package.Version update from 8.1.4 to 8.1.5 maintains consistency with the Server/pyproject.toml version increment.
Server/README.md (1)
26-26: Documentation version references updated consistently.All three git URL references (HTTP uvx example, Stdio uvx example, and MCP client stdio configuration) have been updated from v8.1.4 to v8.1.5 as expected.
Ensure that a git tag
v8.1.5is created at the time of release so that the documentation examples continue to work. Users following the uvx examples will depend on this tag being available.Also applies to: 30-30, 55-55
Server/src/transport/legacy/unity_connection.py (1)
63-64: LGTM!The comment clearly documents the current trust assumption for
config.unity_hostand notes a potential future improvement regarding OS DNS resolver behavior. This is helpful context for future maintainers.Server/src/services/tools/debug_request_context.py (1)
39-47: LGTM!Good use of the new public
get_session_key(ctx)API. Accessing middleware internals (_lock,_active_by_key) is acceptable here since this is explicitly a debugging tool. The additional diagnostic fields (all_keys_in_store,plugin_hub_configured,middleware_id) will be helpful for troubleshooting session state issues.Server/src/transport/unity_instance_middleware.py (3)
22-31: LGTM - Proper double-checked locking for lazy singleton initialization.The pattern is correctly implemented with the lock acquired only when needed, and the second check inside the lock prevents race conditions.
53-66: Good API improvement.Renaming from
_get_session_keytoget_session_keycorrectly exposes this as public API. The updated logic prioritizingclient_idoversession_idwith a fallback to"global"is a sensible approach for local dev stability.
92-105: Clear and helpful inline documentation.The comments explaining the transport-mode considerations and the rationale for being permissive are valuable for future maintainers.
Server/src/transport/unity_transport.py (1)
22-25: LGTM!Clean helper function that provides a consistent string-based transport mode identifier. This is better than having each caller convert the boolean to a string.
Server/src/services/resources/unity_instances.py (2)
39-40: LGTM!Good refactor to use
_current_transport()for cleaner transport mode detection. The string comparison is explicit and readable.
75-75: Good use of dynamic transport value.Using the
transportvariable in the response payload ensures consistency between the detection logic and the reported value.Server/src/main.py (3)
25-28: LGTM!Import change correctly includes
get_unity_instance_middlewareto support the new singleton accessor pattern.
50-62: Good logging improvements.Setting
propagate = Falseprevents duplicate log entries when using custom handlers. The change from silentpasstologger.debug(..., exc_info=exc)provides visibility into logging setup issues without affecting startup. The broadExceptioncatches are appropriate here since logging configuration failures should never crash the server.
267-269: LGTM - Correct use of singleton accessor.Using
get_unity_instance_middleware()ensures the same middleware instance is used throughout the application, which aligns with the lazy singleton pattern introduced inunity_instance_middleware.py.Server/src/services/tools/set_active_instance.py (4)
19-22: LGTM!Consistent with the refactored transport detection pattern used across the codebase.
47-47: Good defensive check.Filtering out instances without an
idattribute prevents potentialKeyErrororAttributeErrorwhen building the lookup dictionary.
65-89: Well-implemented hash prefix matching.The case-insensitive prefix matching is user-friendly, and the handling of ambiguous matches with clear error messages helps users disambiguate. Good UX improvement.
99-111: LGTM!Including
session_keyin the response is helpful for debugging session state issues, and aligns with the debugging capabilities added todebug_request_context.py.MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
298-317: ReuseAddress addition on non‑OSX looks correct for fast restartsAdding
listener.Server.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true)under#if !UNITY_EDITOR_OSXis a reasonable way to make editor restarts and quick rebinds more reliable on Windows/Linux while avoiding the known quirks on macOS. Combined with keepingExclusiveAddressUse = falseon Windows and aLingerStateof(true, 0), this should help smooth out domain‑reload cases without changing externally observable behavior.No correctness issues stand out here.
MCPForUnity/Editor/Helpers/PortManager.cs (1)
120-157: macOS double‑check for port availability is reasonable; note the small blocking costThe updated
IsPortAvailable:
- Keeps the original quick “bind to loopback with
TcpListener.Start/Stop” check.- On
UNITY_EDITOR_OSX, adds a follow‑up probe that attempts aTcpClient.ConnectAsyncto the same port and, if it can connect within 50 ms, treats the port as not available despite the successful bind.This design makes sense to mitigate macOS’s more permissive
SO_REUSEADDRbehavior where a bind can succeed even if another process is already listening. Treating “can connect” as authoritative for “in use” and “connect throws or times out” as “likely available” is a pragmatic balance, especially with debug logging to help diagnose edge cases.The only trade‑off to be aware of is that
connectTask.Wait(50)will block the calling thread for up to 50 ms on each check under macOS. SinceIsPortAvailableis only used in port discovery/persistence code paths (not hot paths per frame), this is probably fine, but it’s something to keep in mind if you ever call it in tighter loops.MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
213-232: Health check scheduling is now safely guarded against teardown race conditionsThe updated
ScheduleHealthCheck:
- Verifies
this != null(Unity “fake null” semantics for destroyed windows) andconnectionSection != nullinside the delayed callback before doing any work.- Wraps
await connectionSection.VerifyBridgeConnectionAsync()in a try/catch and logs viaMcpLog.Warninstead of letting async exceptions escape.This effectively prevents cleanup‑time/teardown races from throwing null‑reference or unobserved async exceptions while still giving you useful diagnostics. The remaining theoretical race where the window is closed after the guard but before the awaited call completes is harmless, since it only leaves the verification operating on a detached section object.
Looks good.
…play - StdioBridgeHost.cs: Disable ReuseAddress on macOS to force AddressAlreadyInUse exception on conflict. - PortManager.cs: Add connection check safety net for macOS. - McpConnectionSection.cs: Ensure UI displays the actual live port instead of just the requested one.
dsarno
force-pushed
the
fix/auto-port-fallback
branch
from
cd8256f to
5c83dec
Compare
- McpConnectionSection.cs: Disable port field when session is running to prevent editing conflicts - StdioBridgeHost.cs: Refactor listener creation into helper method and update EditorPrefs on port fallback - unity_instance_middleware.py: Narrow exception handling and preserve SystemExit/KeyboardInterrupt - debug_request_context.py: Document that debug fields expose internal implementation details
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Server/src/transport/unity_instance_middleware.py (1)
106-127: Narrowed exception handling improves robustness.The change to catch specific exceptions (ConnectionError, ValueError, KeyError, TimeoutError) for expected failures is a good practice. However, the
isinstance(exc, (SystemExit, KeyboardInterrupt))check on line 120 is dead code—in Python 3,SystemExitandKeyboardInterruptinherit directly fromBaseException, notException, so they will never be caught byexcept Exception.You can safely remove the dead code:
except Exception as exc: - # Re-raise unexpected system exceptions to avoid swallowing critical failures - if isinstance(exc, (SystemExit, KeyboardInterrupt)): - raise logger.error( "Unexpected error during PluginHub session resolution for %s: %s", active_instance, exc, exc_info=True )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs(4 hunks)MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs(1 hunks)Server/src/services/tools/debug_request_context.py(1 hunks)Server/src/transport/unity_instance_middleware.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Server/src/services/tools/debug_request_context.py
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (2)
MCPForUnity/Editor/Helpers/PortManager.cs (2)
PortManager(18-344)DiscoverNewPort(59-65)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-40)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-40)
🔇 Additional comments (5)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (2)
176-179: LGTM!Good UX improvement: when a session is active, the port field correctly reflects the actual port in use and is disabled to prevent confusing edits.
187-199: LGTM!The logic correctly handles the fallback case when no port has been persisted (
savedPort == 0), ensuring the UI always displays a valid port value.MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (3)
309-311: LGTM!Centralizing listener creation improves maintainability and ensures consistent socket configuration across all code paths.
339-346: Port persistence ensures consistent behavior across restarts.The change from
GetPortWithFallback()toDiscoverNewPort()combined withEditorPrefspersistence ensures the fallback port is reused, preventing port churn. The empty catch on line 346 is acceptable for robustness, though you might consider adding a debug log for troubleshooting.
388-413: Well-structured platform-specific socket configuration.The helper correctly centralizes socket options with appropriate platform guards:
- Excluding
ReuseAddresson macOS addresses the port availability detection issues specific to that platform.ExclusiveAddressUse = falseon Windows allows faster port reuse after restart.LingerStatewith immediate close prevents socket hang in TIME_WAIT.The empty catch blocks are acceptable here since these socket options may not be supported on all platforms/runtimes, and failure to set them shouldn't prevent startup.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (2)
309-334: Unresolved merge conflict markers - code will not compile.This file contains unresolved Git merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>>) that must be resolved before the code can compile. The intended change appears to replace the inline socket configuration with a call toCreateConfiguredListener(currentUnityPort).Resolve the merge conflict by keeping only the refactored approach:
try { -<<<<<<< HEAD - listener = new TcpListener(IPAddress.Loopback, currentUnityPort); -#if !UNITY_EDITOR_OSX - listener.Server.SetSocketOption( - SocketOptionLevel.Socket, - SocketOptionName.ReuseAddress, - true - ); -#endif -#if UNITY_EDITOR_WIN - try - { - listener.ExclusiveAddressUse = false; - } - catch { } -#endif - try - { - listener.Server.LingerState = new LingerOption(true, 0); - } - catch (Exception) - { - } -======= listener = CreateConfiguredListener(currentUnityPort); ->>>>>>> b693edcaa9cc51bfec59d00595db0e96c49129eb listener.Start(); break; }
385-410: Second unresolved merge conflict - same resolution needed.This is the same merge conflict pattern as above. Resolve by keeping only the
CreateConfiguredListenercall.-<<<<<<< HEAD - listener = new TcpListener(IPAddress.Loopback, currentUnityPort); -#if !UNITY_EDITOR_OSX - listener.Server.SetSocketOption( - SocketOptionLevel.Socket, - SocketOptionName.ReuseAddress, - true - ); -#endif -#if UNITY_EDITOR_WIN - try - { - listener.ExclusiveAddressUse = false; - } - catch { } -#endif - try - { - listener.Server.LingerState = new LingerOption(true, 0); - } - catch (Exception) - { - } -======= listener = CreateConfiguredListener(currentUnityPort); ->>>>>>> b693edcaa9cc51bfec59d00595db0e96c49129eb listener.Start();
🧹 Nitpick comments (1)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
138-138: Consider case-insensitive extension check.On Windows, file extensions are case-insensitive. While unlikely in practice, the current check could miss executables named
Python.exeorPYTHON.EXE.Apply this diff for a case-insensitive check:
- if (File.Exists(potentialPath) && - (potentialPath.EndsWith("python.exe") || potentialPath.EndsWith("python3.exe"))) + if (File.Exists(potentialPath) && + (potentialPath.EndsWith("python.exe", StringComparison.OrdinalIgnoreCase) || + potentialPath.EndsWith("python3.exe", StringComparison.OrdinalIgnoreCase)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs(2 hunks)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs(6 hunks)MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-40)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (2)
MCPForUnity/Editor/Helpers/PortManager.cs (2)
PortManager(18-344)DiscoverNewPort(59-65)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-40)
🔇 Additional comments (4)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
176-179: LGTM - Port field synchronization when session is active.The UI correctly reflects the actual port in use and disables editing during an active session, preventing user confusion about which port is being used.
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (2)
364-371: Port persistence after fallback discovery looks good.Persisting the newly discovered port to
EditorPrefsensures consistency across restarts when the preferred port was unavailable. This aligns with the UI reading from the same key.
438-463: Good refactor - centralized listener configuration.The
CreateConfiguredListenerhelper properly encapsulates platform-specific socket options:
- Skips
ReuseAddresson macOS (aligns with the macOS port-availability connect check inPortManager)- Sets
ExclusiveAddressUse = falseon Windows- Configures
LingerStatewith proper exception handlingThis eliminates duplication and makes the socket setup consistent across both initial bind and fallback paths.
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
53-61: LGTM! UV fallback integration is well-structured.The UV-based fallback is properly integrated as a third detection tier after PATH and where-based checks. The status updates and early return pattern are consistent with the existing fallbacks.
| process.WaitForExit(5000); | ||
|
|
||
| if (process.ExitCode == 0 && !string.IsNullOrEmpty(output)) |
There was a problem hiding this comment.
Check WaitForExit return value to handle timeouts.
The WaitForExit(5000) call returns a boolean indicating whether the process exited within the timeout, but this return value is not checked. If the process times out, ExitCode may not be valid.
Apply this diff to handle the timeout case:
string output = process.StandardOutput.ReadToEnd();
- process.WaitForExit(5000);
+ if (!process.WaitForExit(5000))
+ {
+ return false; // Process timed out
+ }
if (process.ExitCode == 0 && !string.IsNullOrEmpty(output))🤖 Prompt for AI Agents
In MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs
around lines 120-122, the code calls process.WaitForExit(5000) but does not
check its boolean return; before reading process.ExitCode you must verify the
process actually exited in time. Change the logic to capture the
WaitForExit(5000) result into a variable, handle the false (timeout) case by
killing/disposing the process or logging/returning an error/false, and only read
process.ExitCode and inspect output if the wait returned true.
| var lines = output.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries); | ||
| foreach (var line in lines) | ||
| { | ||
| // Look for installed python paths | ||
| // Format is typically: <version> <path> | ||
| // Skip lines with "<download available>" | ||
| if (line.Contains("<download available>")) continue; | ||
|
|
||
| // The path is typically the last part of the line | ||
| var parts = line.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries); | ||
| if (parts.Length >= 2) | ||
| { | ||
| string potentialPath = parts[parts.Length - 1]; | ||
| if (File.Exists(potentialPath) && | ||
| (potentialPath.EndsWith("python.exe") || potentialPath.EndsWith("python3.exe"))) | ||
| { | ||
| if (TryValidatePython(potentialPath, out version, out fullPath)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
uv python list output format Windows paths with spaces
💡 Result:
Short answer: uv's CLI doesn't provide a JSON/output-format for "uv python list", so don't rely on its human-readable lines for machine parsing. Instead either use uv python find to get a single interpreter path (safe for programmatic use), or parse a command that supports structured output (e.g. uv pip list --format json) or call the discovery logic from Python. See uv docs. [1][2]
Concrete options:
- Get a single interpreter path (no ambiguous columns/spacing): uv python find. [2]
- For package listings use structured JSON: uv pip list --format json. [3]
- If you must parse uv python list output, handle Windows paths with spaces by expecting quoted paths or by using robust parsing (split only on known separators, or match drive-letter + :\... with regex) rather than naive whitespace splitting. See discussion of quoting/escaping on Windows. [4][5]
Sources:
- uv CLI reference (python list description). [2]
- uv python find / python versions docs. [2]
- uv pip list --format json (structured output). [3]
- Windows/CMD/PowerShell quoting and backslash issues (Stack Overflow). [6]
- Python path-string handling (raw strings, backslashes, repr). [4]
Use uv python find for robust single interpreter discovery instead of parsing uv python list output.
The uv python list command produces human-readable output not designed for machine parsing. The current logic splits lines by spaces and takes the last token as the path, which will fail on Windows paths with spaces (e.g., C:\Program Files\Python\python.exe). The uv tool provides uv python find specifically for programmatic interpreter discovery; use that instead. If uv python list parsing is necessary, implement regex-based extraction (e.g., match drive letters and colons on Windows) rather than naive whitespace splitting.
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (2)
296-365: Port retry/fallback logic looks sound and matches the new PortManager behaviorThe
AddressAlreadyInUseretry loop plus the short grace period when the port appears to belong to this process, followed byPortManager.DiscoverNewPort()and a second bind on the new port, is coherent and reads correctly. PersistingcurrentUnityPortviaEditorPrefs.SetIntin the fallback path also makes sense so subsequent sessions start from the discovered port.If you ever extend the fallback to handle more error types, you might consider logging failures of
EditorPrefs.SetIntwhen debug logging is enabled instead of silently swallowing them, but the current guarded write is fine as-is.
388-413: Centralized listener configuration keeps platform-specific socket options consistent
CreateConfiguredListenernicely centralizesTcpListenercreation and re-applies the same socket options for all call sites. SkippingReuseAddresson macOS while keeping it (andExclusiveAddressUse = false) elsewhere aligns with typical Unity/Mono behavior, and wrappingExclusiveAddressUse/LingerStateintry/catchavoids hard failures on older runtimes.Optional: consider adding a brief comment above the
#if !UNITY_EDITOR_OSXblock to document whyReuseAddressis intentionally disabled on macOS, so future tweaks don’t accidentally reintroduce it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
28-38: Two‑phase detection looks good; consider de‑duplicating PATH augmentationThe new “which → validate” phase is a nice improvement and aligns with the Linux detector, and the updated detail string including the resolved path is helpful. However, the logic that builds the augmented PATH is now effectively duplicated between
TryValidatePython,TryFindInPath, andGetPathAdditions(used viaBuildAugmentedPathfor uv).To keep macOS detection consistent and easier to maintain, consider reusing
GetPathAdditions()(orBuildAugmentedPath()) for Python as well, so all tools share the same PATH construction.- // Set PATH to include common locations - var homeDir = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); - var pathAdditions = new[] - { - "/opt/homebrew/bin", - "/usr/local/bin", - "/usr/bin", - Path.Combine(homeDir, ".local", "bin") - }; - - string currentPath = Environment.GetEnvironmentVariable("PATH") ?? ""; - psi.EnvironmentVariables["PATH"] = string.Join(":", pathAdditions) + ":" + currentPath; + // Set PATH to include common locations (reuse same additions as uv/which) + psi.EnvironmentVariables["PATH"] = BuildAugmentedPath();You could make a similar change in
TryFindInPathto callBuildAugmentedPath()instead of locally rebuilding the same list.Also applies to: 153-165, 241-250, 269-281
42-54: Consider adding stderr fallback toTryValidatePythonfor robustnessThe current implementation reads only
StandardOutputwhen checkingpython --version. While Python 3.4+ (including 3.10+) outputs version to stdout, some edge-case builds or system configurations may write to stderr. Since stderr is already being redirected, you can cheaply add a fallback:- string output = process.StandardOutput.ReadToEnd().Trim(); - process.WaitForExit(5000); - - if (process.ExitCode == 0 && output.StartsWith("Python ")) + process.WaitForExit(5000); + + // Python sometimes writes version info to stderr; fall back if stdout is empty + string output = process.StandardOutput.ReadToEnd().Trim(); + if (string.IsNullOrEmpty(output)) + { + output = process.StandardError.ReadToEnd().Trim(); + } + + if (process.ExitCode == 0 && output.StartsWith("Python ")) { version = output.Substring(7); // Remove "Python " prefix fullPath = pythonPath;This applies to both the
DetectPython()fallback (lines 42–54) and theTryValidatePythonmethod itself (lines 136–190). Optionally, you could also distinguish "Python found but version < 3.10" from "not found" to provide clearer upgrade guidance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-40)
🔇 Additional comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
170-199: Port field now correctly reflects runtime vs preferred portThe updated
UpdateConnectionStatuslogic looks solid: when the bridge is running you forceunityPortFieldto the actualbridgeService.CurrentPortand disable editing, and when not running you re-enable the field and repopulate it fromEditorPrefs(falling back toCurrentPortif no preference is stored). This keeps the UI consistent with the new persisted/auto‑fallback port behavior and avoids user confusion about which port is actually in use, with no obvious edge‑case regressions.
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.