Fix/port discovery missing imports #398
Conversation
Problem: The port discovery mechanism was failing to detect running Unity instances because the _try_probe_unity_mcp() function was using struct.pack() and struct.unpack() without importing the struct module, and other functions were using os.path.basename() without importing the os module. This caused NameError exceptions that were silently caught, preventing Unity instances from being discovered. Root Cause: - struct.pack() used at line 80 to create message headers - struct.unpack() used at line 98 to parse response headers - os.path.basename() used at lines 217 and 247 to extract filenames - Neither os nor struct modules were imported Impact: - Unity instances running on port 6400 were not discoverable - The MCP server would fail to connect even when Unity was running - The probe function would fail silently due to exception handling Solution: Added missing imports: - import os (for os.path.basename) - import struct (for struct.pack/unpack in the framed protocol) Verification: - Test script confirmed Unity is responding correctly on port 6400 - The framed protocol (8-byte header + payload) works correctly - Once the MCP server restarts with these imports, Unity discovery will work
Claude Desktop on macOS does not inherit the user's full PATH, causing 'spawn uvx ENOENT' errors when it tries to run the server. This change adds auto-discovery logic to find 'uvx' in common locations (like ~/.local/bin, ~/.cargo/bin, /opt/homebrew/bin) and writes the absolute path to the config. Also updated README with troubleshooting steps.
WalkthroughThis PR adds automatic cross-platform discovery for the uvx executable path. A new Changes
Sequence DiagramsequenceDiagram
participant PR as PathResolverService
participant EP as ExecPath
participant OS as Operating System
PR->>PR: Check for uvx path override
alt Override exists
PR-->>PR: Return override path
else No override
PR->>EP: ResolveUvx()
rect rgb(200, 220, 255)
note over EP: Platform-specific lookup
alt macOS/Linux
EP->>OS: Which("uvx")
else Windows
EP->>OS: Check appdata candidates
EP->>OS: Where("uvx.exe"/"uvx")
end
end
alt Path found
EP-->>PR: Return absolute path
else Not found
EP-->>PR: Return null
end
alt Discovered path available
PR-->>PR: Return resolved path
else Discovery failed
PR-->>PR: Return "uvx" (literal)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ExecPath.cs (1)
135-183: Clarify the comment to match implementation.The comment states "Pref → env → common locations → PATH" but the implementation only checks common locations and PATH. EditorPrefs (pref) is handled by the caller (
PathResolverService.GetUvxPath()), and no environment variable is checked (unlikeResolveClaude()which checksCLAUDE_CLI).Consider either:
- Updating the comment to reflect the actual flow:
// Resolve uvx absolute path. Common locations -> PATH.- Adding an environment variable check (e.g.,
UVX_PATH) for consistency withResolveClaude().Apply this diff to update the comment:
- // Resolve uvx absolute path. Pref -> env -> common locations -> PATH. + // Resolve uvx absolute path. Common locations -> PATH.MCPForUnity/README.md (1)
85-88: Clear troubleshooting guidance.The documentation accurately describes the new auto-discovery behavior and provides helpful remediation steps for users encountering PATH issues with Claude Desktop on macOS.
Optionally, you can refine line 87 to avoid the split infinitive:
- - The MCP for Unity plugin attempts to automatically resolve the absolute path to `uvx`. + - The MCP for Unity plugin automatically attempts to resolve the absolute path to `uvx`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Helpers/ExecPath.cs(1 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(1 hunks)MCPForUnity/README.md(1 hunks)Server/src/transport/legacy/port_discovery.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/README.md
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/README.md
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Helpers/ExecPath.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Services/PathResolverService.cs (1)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-322)ResolveUvx(136-183)
🪛 LanguageTool
MCPForUnity/README.md
[style] ~87-~87: Try moving the adverb to make the sentence clearer.
Context: ...ll's PATH. - The MCP for Unity plugin attempts to automatically resolve the absolute path to uvx. - If this fails, use the "C...
(SPLIT_INFINITIVE)
🔇 Additional comments (2)
Server/src/transport/legacy/port_discovery.py (1)
17-18: LGTM! Critical bug fix.These imports are required by the existing code:
structis used for binary framing protocol operations (lines 80, 98) andosis used for path operations (lines 217, 247). Without these imports, the module would fail at runtime.MCPForUnity/Editor/Services/PathResolverService.cs (1)
37-42: LGTM! Clean auto-discovery integration.The fallback chain (override → auto-discovery → default) follows good defensive programming practices and mirrors the pattern used in
GetClaudeCliPath(). This should resolve PATH inheritance issues on macOS while maintaining backward compatibility.
1 participant
This is partial and not windows tested. It worked for claude and claude code on mac for stdio.
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.