Fix: Python Detection, Port Conflicts, and Script Creation Reliability #428
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.
…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.
- 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
…/auto-port-fallback
|
Caution Review failedThe pull request is closed. WalkthroughAdds a uv-based Python discovery fallback on Windows, tightens macOS port availability checks with an extra TCP connect test, centralizes TcpListener creation and persists discovered ports, disables port editing during active sessions, and refines a few server comments and exception handlers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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
🧹 Nitpick comments (4)
Server/src/transport/unity_instance_middleware.py (1)
106-127: Clarify generic exception handler vs system-exception intentCatching
Exceptionhere meansSystemExitandKeyboardInterruptare never intercepted, so theisinstance(exc, (SystemExit, KeyboardInterrupt))branch is effectively dead code and the comment about “re‑raise unexpected system exceptions” is misleading.Two clearer options:
- If you just want to log unexpected non-system errors (current effective behavior), simplify to:
except Exception as exc: logger.error( "Unexpected error during PluginHub session resolution for %s: %s", active_instance, exc, exc_info=True, )
- If you truly want to special‑case system-level exceptions, switch to
except BaseException as exc:so they can be re‑raised explicitly:except BaseException as exc: if isinstance(exc, (SystemExit, KeyboardInterrupt)): raise logger.error( "Unexpected error during PluginHub session resolution for %s: %s", active_instance, exc, exc_info=True, )MCPForUnity/Editor/Helpers/PortManager.cs (1)
122-156: macOS port availability check looks good, but consider the timeout duration.The TCP connection check to supplement the bind test on macOS is a reasonable approach to work around
SO_REUSEADDRbehavior. The logic correctly identifies that a successful connection means the port is in use.However, the 50ms timeout at line 143 may be too short in some edge cases (e.g., system under load), potentially causing false positives where the port is reported as available when the connection simply didn't complete in time. Consider increasing to 100-200ms for more reliable detection on loopback.
- if (connectTask.Wait(50) && client.Connected) + if (connectTask.Wait(150) && client.Connected)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
149-152: Optionally narrow exception handling.While the broad
catchblock is consistent with other methods in this file (e.g.,TryValidatePythonat Lines 192-195), consider catching specific exceptions likeFileNotFoundExceptionorWin32Exceptionto avoid suppressing unexpected errors during development.} - catch + catch (Exception ex) when ( + ex is FileNotFoundException || + ex is Win32Exception || + ex is InvalidOperationException) { // Ignore errors if uv is not installed or fails }
124-146: The parsing logic is workable for the currentuv python listoutput format, but consider a more explicit approach.While the current logic (split on spaces, take last token) does work with
uv's actual Windows output format where paths are single whitespace-separated tokens without embedded spaces, the approach is implicitly fragile. If the output format evolves—for example, if additional metadata columns are added or column separators change—this parsing would break silently.A more explicit parsing strategy would be:
- Use a regex pattern to match full Windows Python executable paths (e.g.,
[A-Za-z]:\\.*\\python3?\.exe)- Check if
uvsupports JSON output mode for structured data parsing (e.g.,uv python list --output jsonif available)- Document the assumed output format as a dependency
The existing
File.Exists()and.EndsWith()checks provide good validation coverage, so the current implementation is currently safe, but adding one of the suggested approaches would make the code more maintainable for future changes touv's output format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs(2 hunks)MCPForUnity/Editor/Helpers/PortManager.cs(1 hunks)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/services/tools/manage_script.py(1 hunks)Server/src/transport/unity_instance_middleware.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-40)
MCPForUnity/Editor/Helpers/PortManager.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
IsDebugEnabled(16-16)McpLog(7-52)Info(37-41)
MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
TryValidatePython(138-193)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
TryValidatePython(136-190)
🔇 Additional comments (8)
Server/src/services/tools/manage_script.py (1)
374-374: Documentation improvement prevents double-encoding issues.The updated annotation correctly clarifies that callers should pass plain text C# code, as the function handles Base64 encoding internally (lines 402-403). This prevents LLMs from double-encoding the contents, which was causing compilation errors.
Server/src/services/tools/debug_request_context.py (1)
43-43: Good explicit disclaimer around internal middleware fieldsThe added note clearly marks
_lockand_active_by_keyas internal and potentially unstable, which is appropriate for a debug-only tool touching private state. No issues from a concurrency or reliability perspective.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
176-179: Good UX improvement.Forcing the UI to reflect the actual active port and disabling edits during an active session prevents confusion and accidental misconfiguration.
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (3)
388-413: Well-structured listener configuration helper.The
CreateConfiguredListenermethod properly centralizes socket configuration with appropriate platform-specific behavior:
- macOS: Correctly excludes
SO_REUSEADDRto ensureAddressAlreadyInUseis raised when ports are busy (aligns with the PR objective).- Windows: Sets
ExclusiveAddressUse = falsefor flexibility.LingerState(true, 0)enables immediate socket close.The empty catch blocks at lines 403 and 409-411 are acceptable here since these are best-effort socket options that may not be available on all platforms.
309-311: Good refactor to use centralized listener configuration.The listener creation is now consistently delegated to
CreateConfiguredListener, ensuring platform-specific socket options are applied uniformly across all code paths.
339-346: Verify if port persistence is redundant.
PortManager.DiscoverNewPort()(line 339) may already persist the port internally. The additionalEditorPrefs.SetInt()at line 344 writes to a separate storage location (EditorPrefs vs. config file). If both storage mechanisms are intentional for redundancy or different purposes, consider adding a comment explaining why dual persistence is needed. If not intentional, consolidate to a single persistence mechanism.MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
53-61: LGTM: Clean integration of uv fallback.The integration of the uv-based Python detection follows the existing fallback pattern and properly updates the status object with version, path, and details.
99-155: Verify 0-byte App Execution Alias detection.The code only checks
File.Exists(potentialPath)at line 137, which doesn't filter out 0-byte placeholder files. WhileTryValidatePythonmay implicitly reject these by failing to execute them, an explicit file size check would clarify intent. If 0-byte App Execution Aliases are a stated concern for this PR, add:if (parts.Length >= 2) { string potentialPath = parts[parts.Length - 1]; - if (File.Exists(potentialPath) && + var fileInfo = new FileInfo(potentialPath); + if (fileInfo.Exists && fileInfo.Length > 0 && (potentialPath.EndsWith("python.exe") || potentialPath.EndsWith("python3.exe"))) {
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
28-40: Path‑first detection looks good; consider reusing shared PATH augmentation helpersThe new
TryFindInPath("python3"/"python")flow with an augmented PATH and then validating that concrete path matches the issue goals (prefer Homebrew locations and surface the actual binary path instatus.Details). That part looks solid.There is some duplication in how PATH is built for Python vs. uv though (Lines 153‑165, 270‑282, and
GetPathAdditionsat Lines 240‑250). To avoid future drift between Python and uv detection, consider reusingGetPathAdditions()(or a sharedBuildAugmentedPath()) from here and fromTryValidatePython, instead of inlining the arrays again.
42-51: Alignstatus.Path/Details between primary and fallback branchesFunctionally this fallback is fine, but there’s a small consistency gap: in the primary branch (Lines 32‑37)
status.Pathis an absolute path (fullPath), while here it will typically just be"python3"or"python"sinceTryValidatePythonsetsfullPath = pythonPath.If downstream code or UX treats
status.Pathas “the resolved interpreter path”, it may be less clear when it sometimes shows a bare command name. Optional improvement:
- Either resolve the absolute path in the fallback (e.g., via
TryFindInPathbefore settingstatus.Path/status.Details), or- Teach
TryValidatePythonto compute and return the resolved path it actually used whenpythonPathis not rooted.Example sketch (inside this fallback block):
- status.Path = p; - status.Details = $"Found Python {v} in PATH"; + var displayPath = p; + if (!Path.IsPathRooted(displayPath) && TryFindInPath(displayPath, out var resolvedPath)) + { + displayPath = resolvedPath; + } + status.Path = displayPath; + status.Details = $"Found Python {v} at {displayPath}";This keeps the behavior consistent with the primary detection path.
📜 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 (3)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (2)
176-179: LGTM! Clear improvement to show the active port.Disabling the port field during an active session and forcing it to display the actual port in use is good UX and aligns with the PR objective to display the active port rather than only the requested port.
187-188: The duplicateSetEnabled(true)issue from the previous review appears resolved.The past review flagged duplicate
SetEnabled(true)calls, but the current code only contains a single call on line 188.MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
53-54: Clearer failure messaging is helpful and aligns with the version requirementThe updated error and details strings are much clearer: they correctly treat “no suitable Python ≥3.10 found” as “not found” and directly steer users toward
brew install python3with a 3.10+ requirement, which matches the detector’s version check logic.
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
Outdated
Show resolved
Hide resolved
1 participant
This PR addresses several stability and reliability issues across Windows and macOS, mainly related to multiple instances, and the uv/uvx detection and user experience.
Key Changes:
python.exe(Microsoft Store placeholders), ensuring the bridge finds a valid Python interpreter.SO_REUSEADDRon macOSStdioBridgeHostto ensureAddressAlreadyInUseexceptions are thrown correctly when a port is busy, preventing silent failures.McpConnectionSectionto display the actual active port rather than just the requested one.create_scriptTool: Updated the tool description to explicitly state that the server handles Base64 encoding. This prevents LLMs (like Claude/Sonnet) from double-encoding content, which was causing "Expected expression" compilation errors in generated scripts.Summary by CodeRabbit
Bug Fixes
New Features
UI/UX
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.