-
Notifications
You must be signed in to change notification settings - Fork 507
Harden MCP Bridge: reliable reloads, sticky port, embedded server, cleaner Auto‑Setup UI #192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…stop on domain reload; start/stop locking; per-project sticky ports + brief release wait; Python discovery scans hashed+legacy files and probes; editor window live status refresh.
…ename auto-run toggle to client section ("Auto-connect to MCP Clients"); rename button to "Run Client Setup"; fix dev-mode status by using FindPackagePythonDirectory() for Claude/Desktop path checks
…nstaller - Switch ServerInstaller to embedded copy-only (no network) - Simplify Editor UI server status to 'Installed (Embedded)' - Vendor UnityMcpServer/src into UnityMcpBridge/UnityMcpServer/src for UPM distribution - Keep bridge recompile robustness (heartbeat + sticky port)
…6400 blip after UPM import
…ckage; remove dev-mode logs under UPM
…paths case-insensitively to prevent sticky-port drift across reloads
… port and let bind micro-retry handle release to avoid port swapping on recompiles
…rt changes during client setup
…etup with Connected ✓ state; add Debug Logs toggle and gate verbose logs fix(bridge): reuse stored port in StartAutoConnect; guard listener stop to avoid ObjectDisposedException chore(clients): reorder dropdown to Cursor, Claude Code, Windsurf, Claude Desktop, VSCode
…opies embedded server, adds RepairPythonEnvironment() (deletes .venv, runs 'uv sync'); robust uv path discovery; macOS install path -> Application Support\n- UI: Server Status shows Installed(Embedded); Python missing warning with install link; Repair button tooltip; header Show Debug Logs; cleaned layout\n- Python: unpin .python-version; set requires-python >=3.10 in both pyprojects\n- Dev: improved package/dev path resolution
…hind Debug Logs toggle; improve Python and UV detection on Windows (flex versions, where.exe/Path scan); tidy installer messages
|
From GPT 5 Pro: Here’s a focused review of PR #192 (“Unity MCP: bridge stability + embedded server”), based on the PR thread and the key commits/diffs. I’ve grouped notes into what looks great, risks/edge cases, and actionable suggestions, then closed with a quick test plan you can run before merge. Note from PR author @dsarno : Great feedback, not sure it's all necessary for a merge, but I can work on some of it for a follow-up PR unless anyone feels strongly. Edit: Ended up hardening the reload and retry/debounce functionality, much better than in the original commit, see comment below. What’s solid
Risks & edge cases to consider
Actionable suggestions (fast wins)
Targeted code nits
Quick test plan (manual)Run these in both macOS and Windows:
Overall takeThe direction is right: stickiness across reloads, deterministic installs, and practical recovery tools. Before merge, I’d address the main‑thread waits, add a positive MCP handshake, and make the bind‑retry behavior explicit and well‑logged. With those tightened, this should noticeably improve day‑to‑day reliability. If you want, I can draft inline GitHub review comments you can paste onto the PR (one per file/region) focusing on the specific lines in |
|
Hey @dsarno, thanks for this! I haven't fully checked out everything, realistically by tomorrow I can be more thorough. That said, make a few adjustments?
|
…ents This merge combines upstream's organizational rebrand and updates with our comprehensive bridge stability improvements: **From Upstream:** - CoplayDev organizational rebrand (README, LICENSE, documentation) - Updated logo and deployment scripts - Python version pinning (.python-version file) **From Our Branch (Preserved):** - Comprehensive bridge stability improvements (threading, heartbeat, retries) - Enhanced debugging and diagnostic features - Embedded server installation approach (more reliable than git-based) - Broader Python compatibility (>=3.10 vs >=3.12) - Advanced port management with per-project persistence - Auto-setup and connection reliability features - Robust error handling and recovery mechanisms **Key Technical Decisions:** - Used our comprehensive UnityMcpBridge.cs (625 lines vs 473) with all stability features - Maintained embedded server approach over upstream's git-based installer - Preserved broader Python compatibility (>=3.10) for better accessibility - Used our optimized connection settings and retry logic - Kept our user-centric server installation approach (on-demand vs automatic) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…iles; delete old root UnityMcpServer; update editor lookup for tilde path; adjust deploy/restore scripts; remove orphan meta
…ty-mcp into feat/bridge-stability * 'feat/bridge-stability' of https://github.com/dsarno/unity-mcp: Bridge logs: add bold blue UNITY-MCP prefix; gate PortManager logs behind Debug Logs toggle; improve Python and UV detection on Windows (flex versions, where.exe/Path scan); tidy installer messages
|
Adding the fixes from @msanatan, as well as a couple others, now. Please wait to review until fixes are in, I will ping. |
…binds.
Editor: auto-rewrite MCP client config when package path changes.
Server: heartbeat-aware retries, structured {state: reloading, retry_after_ms}, single auto-retry across tools; guard empty calls.
Repo: remove global *~ ignore (was hiding UnityMcpServer~), track tilde server folder (Unity still excludes it from assemblies).
|
Per @msanatan 's requests
Other screw-tightening on domain reloading:
Note: Observed one transient Ready for review @msanatan @Scriptwonder . |
Added new image of MCP Editor window to README
|
I'm successfully on this branch now after experiencing the issues discussed here: #195. Just noting here for posterity. |
msanatan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a couple of questions, one is a minor edit suggestion and the other is just so I understand things better.
It was a lot to go through but overall looks good
| { | ||
| Debug.Log($"Using stored port {storedPort}"); | ||
| return storedPort; | ||
| if (IsDebugEnabled()) Debug.Log($"<b><color=#2EA3FF>UNITY-MCP</color></b>: Using stored port {storedConfig.unity_port} for current project"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self for later, set up a structured logger with levels and the like
| } | ||
| } | ||
|
|
||
| private bool IsPythonDetected() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self for later, check if there's a more generic way to do this
| What changed and why: | ||
| - Unity now writes a per-project port file named like | ||
| `~/.unity-mcp/unity-mcp-port-<hash>.json` to avoid projects overwriting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this solution
| # Use the get_unity_connection function to retrieve the active connection instance | ||
| # Changed "MANAGE_GAMEOBJECT" to "manage_gameobject" to potentially match Unity expectation | ||
| response = get_unity_connection().send_command("manage_gameobject", params) | ||
| if isinstance(response, dict) and not response.get("success", True) and response.get("state") == "reloading": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any non-connection reason why response['success'] can be False? Also, why does it default to True?
To be clear I'm not asking for it to be changed, just asking for my knowledge.
Non critical, future task could be to use a wrapper function around get_unity_connection().send_command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any non-connection reason why response['success'] can be False?
Yes, two cases:
(1) "Preflight" call finds Unity is reloading, not ready, so retry...
{"success": false, "state": "reloading", "retry_after_ms": 250}
(2) Tool ran but bad params or other type of fail
{"success": false, "error": "GameObject 'Foo' not found"}
(Preflight returns before the tool executes ... tool‑level errors only appear after preflight passes.)
why does it default to True?
Overcautious and no longer strictly necessary, but ... the live ("old") version of the endpoint has no success field. So I kept a default to true in case we hit an older version of endpoint as we transition. In which case the missing success isn't treated as a fail, more like "this is the old version so go ahead"
Since server is now embedded, mixing old/new is unlikely; we could drop the default and do:
if isinstance(response, dict) and response.get("state") == "reloading" and response.get("success") is False:
return response
But since people have been testing it and it seems to be working, maybe we slate that for a cleanup later with some of your other suggestions above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation!
| return { | ||
| "success": False, | ||
| "state": "reloading", | ||
| "retry_after_ms": int(250), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the value from config.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msanatan Good call -- While addressing this, I found another issue with individual tools not retrying when they hit a recompile/reload gap. Will work on it tomorrow and hopefully that can be the last bit for now. Other fixes can go in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msanatan Ok re your above note, I replaced the magic 250 with a single-sourced value in config. Also realized individual tools weren't capable of retries after/during Unity reload, so added centralized reload-aware retries that every tool can use, so they retry instead of reporting fail and moving on. You can see this behavior if you ask e.g. Cursor to run all 8 tool calls in a row, starting with a manage_script to force a recompile.
Tools now:
- detect reload, sleep reload_retry_ms (default 250ms), retry up to reload_max_retries (default 40 ≈ 10s)
- preserve structured reloading failures if Unity takes longer
- use shared helpers, avoiding per-tool duplication
Follow-up possibilities (future PR)
- Add env overrides for reload_retry_ms and reload_max_retries
- Include a retry counter and total waited ms in timeout responses/logs
- Optionally expand reload detection to cover socket drops/exception paths
- Revisit defaults if we see reloads >10s in CI or large projects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @dsarno, great catch and good remedy
…_ms via config; increase default retry window (40 x 250ms); preserve structured reloading failures
msanatan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!

Overview
This PR hardens the Unity MCP Bridge and streamlines the end‑to‑end setup. It removes the old Git‑based server installer in favor of an embedded Python server, reduces perceived downtime across domain reloads, and improves the Unity Editor UX for configuration and troubleshooting.
Behavior Changes (Before → After)
Why
Key Changes
Bridge lifecycle:
UnityMcpBridge/Editor/UnityMcpBridge.csunity_portandreloadingstateAddressAlreadyInUseReuseAddressandLinger(0)to reduce TIME_WAIT binds~/.unity-mcp/unity-mcp-status-<hash>.jsonPort management:
UnityMcpBridge/Editor/Helpers/PortManager.csEmbedded server installer:
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs~/Library/Application Support/UnityMCP), no Git requiredRepairPythonEnvironment()deletes.venvand runsuv syncto rebuild a clean envwhich uv), plusEditorPrefsoverride (UnityMCP.UvPath)Editor UI:
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.csFindPackagePythonDirectory()so dev clones don’t appear misconfigured (in other words, all you have to do to swap to a dev clone is change themanifest.jsonfile in Unity)Python server (vendored)
UnityMcpBridge/UnityMcpServer/src.python-versionpin; setrequires-python = ">=3.10"inpyproject.tomluv.lockupdatedStability/UX Improvements
Compatibility
Risks & Mitigations
whichscan +EditorPrefsoverrideHow to Verify (Quick)
read_consoleacross reloads → remains responsiveFollow‑ups (Optional)
.venvimport errors are detected at runtimeLinks