Merge pull request #3 from dsarno/codex/refactor-mcp-editing-tools-with-guardrails #4
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
…ev casing and main branch
…v.unity-mcp to comply with Unity package naming standards
…-and-docs Update docs and package
…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
… NVM version parse robustness; help labels [HELP]
…tion-ui UX and Fix: Claude Code detection + UV gating (Cursor/Windsurf) + VSCode mcp.json schema
… command during config writes
…nt so button toggles and status updates
…ere.exe); Unregister UX: use 'claude mcp get' exit codes; stop PATH prepend on Windows; safer detection when unregistered
…pstream/remote/local)
…argparse version (auto-detect manifest/repo; supports interactive/non-interactive)
…rve existing uv command; Claude unregister UI fix; .ps1 handling; add generic mcp_source.py
Windows UV stability: prefer WinGet Links, preserve pinned uv path; Claude Code unregister UI; add generic mcp_source.py
…add ResolveServerSrc; block PackageCache unless dev override; canonical uv args
…etection; atomic writes; uv validation; WinGet Links ordering
…p config-only changes)
…de) after LOCALAPPDATA; drop ProgramData; update comment
Config stability: preserve user configs, prefer installed server, safer uv resolution, XDG paths (Linux)
slight cleanup
Update README.md
…ove blob stream tools; simplify tool registration - Python server: always consume handshake and negotiate framing on reconnects (prevents invalid framed length).\n- C#: strict FRAMING=1 handshake and NoDelay; debounce AssetDatabase/compilation.\n- Tools: keep manage_script + script edits; remove manage_script_stream and test tools from default registration.\n- Editor window: guard against auto retargeting IDE config.
…Exact, safe write framing; remove unused vars
… MCP edit ops; mitigate false 'no opening brace' errors and allow relaxed validation for text edits
…ority and server apply_text_edits/validate; add resources list/read; deprecate manage_script read/update/edit; remove stdout prints; tweak connection handshake logging
WalkthroughRebrands to Coplay, embeds the Python server within the Unity package, adds robust auto-setup and tooling, introduces framed TCP I/O, per-project port/state files, client config remapping, and extensive Windows/macOS path resolution. Updates docs, scripts, and tests. Replaces legacy Git-based install with local copy workflow and retry-capable command routing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UnityEditor
participant PackageInstaller
participant ServerInstaller
participant PythonServer
participant MCPClient
User->>UnityEditor: Open project / Enable package
UnityEditor->>PackageInstaller: InitializeOnLoad
PackageInstaller->>ServerInstaller: EnsureServerInstalled()
ServerInstaller-->>PackageInstaller: Embedded copy ready
UnityEditor->>UnityEditor: Start UnityMcpBridge (framed TCP)
MCPClient->>PythonServer: Launch (stdio MCP)
PythonServer->>UnityEditor: Discover port (status/registry) + connect
PythonServer-->>MCPClient: MCP ready
sequenceDiagram
participant MCPClient
participant PythonServer
participant UnityBridge
MCPClient->>PythonServer: Tool request (e.g., manage_script)
PythonServer->>UnityBridge: send_command_with_retry(...)
UnityBridge-->>PythonServer: Framed response JSON
PythonServer-->>MCPClient: Normalized dict result
sequenceDiagram
participant UnityBridge
participant FS
participant Status
UnityBridge->>FS: Read/write per-project port file
UnityBridge->>Status: Write heartbeat unity-mcp-status-<hash>.json
Note right of Status: Includes port, state, timestamps
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (2)
UnityMcpBridge/Editor/Tools/ReadConsole.cs (1)
62-69: Add fallback for older Unity versions: also reflect StopGettingEntriesSome Unity versions expose StopGettingEntries instead of EndGettingEntries. Currently, reflection throws if EndGettingEntries is missing, breaking console reads. Try both and only fail if neither exists.
- // Try reflecting EndGettingEntries based on warning message - _endGettingEntriesMethod = logEntriesType.GetMethod( - "EndGettingEntries", - staticFlags - ); - if (_endGettingEntriesMethod == null) - throw new Exception("Failed to reflect LogEntries.EndGettingEntries"); + // Try reflecting EndGettingEntries; fall back to StopGettingEntries for older Unity + _endGettingEntriesMethod = logEntriesType.GetMethod("EndGettingEntries", staticFlags); + if (_endGettingEntriesMethod == null) + { + _endGettingEntriesMethod = logEntriesType.GetMethod("StopGettingEntries", staticFlags); + } + if (_endGettingEntriesMethod == null) + throw new Exception("Failed to reflect LogEntries.EndGettingEntries/StopGettingEntries");UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
1839-1873: Extract duplicate UV validation logicThe
IsValidUvInstallationmethod duplicates UV validation logic that appears in multiple places. This should use a shared implementation, potentially fromServerInstaller.ValidateUvBinary.private bool IsValidUvInstallation(string uvPath) { - try - { - var psi = new ProcessStartInfo - { - FileName = uvPath, - Arguments = "--version", - UseShellExecute = false, - RedirectStandardOutput = true, - RedirectStandardError = true, - CreateNoWindow = true - }; - - using var process = Process.Start(psi); - process.WaitForExit(5000); // 5 second timeout - - if (process.ExitCode == 0) - { - string output = process.StandardOutput.ReadToEnd().Trim(); - // Basic validation - just check if it responds with version info - // UV typically outputs "uv 0.x.x" format - if (output.StartsWith("uv ") && output.Contains(".")) - { - return true; - } - } - - return false; - } - catch - { - return false; - } + // Delegate to the shared validation method in ServerInstaller + return ServerInstaller.ValidateUvBinary(uvPath); }Also applies to: 1875-2037
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
539-557: Extract SHA1 computation to shared utilityThis
ComputeSha1method duplicates the hash computation logic found inUnityMcpBridge.csandPortManager.cs. It should use the shared utility suggested earlier.-private static string ComputeSha1(string input) -{ - try - { - using SHA1 sha1 = SHA1.Create(); - byte[] bytes = Encoding.UTF8.GetBytes(input ?? string.Empty); - byte[] hash = sha1.ComputeHash(bytes); - StringBuilder sb = new StringBuilder(hash.Length * 2); - foreach (byte b in hash) - { - sb.Append(b.ToString("x2")); - } - return sb.ToString(); - } - catch - { - return ""; - } -} // Update the call site: -string key = $"UnityMCP.AutoRegistered.{ComputeSha1(projectPath)}"; +string key = $"UnityMCP.AutoRegistered.{HashUtility.ComputeProjectHash(projectPath)}";
🧹 Nitpick comments (85)
package.json (1)
1-1: Avoid accidental publish/tooling confusion by adding minimal metadata (or remove the file if unnecessary).An empty root package.json can:
- Cause npm/yarn/pnpm to treat the repo root as a Node package unintentionally.
- Risk accidental
npm publish.- Interfere with tooling that auto-detects workspaces or expects fields like “name”/“license”.
If you need this manifest, set it to a safe minimal config. Otherwise, consider removing it.
Apply this diff for a safe minimal manifest:
-{} +{ + "name": "unity-mcp", + "private": true, + "license": "MIT" +}UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json (1)
1-4: reportMissingImports: "none" hides real problems; point Pyright at your venv instead and surface warnings.Given this repo now embeds the Python server and has auto-setup, it’s better to let Pyright locate your venv and warn on missing imports rather than suppressing them entirely.
Consider this tweak (assuming a venv at UnityMcpBridge/UnityMcpServer~/.venv):
{ "typeCheckingMode": "basic", - "reportMissingImports": "none" + "reportMissingImports": "warning", + "venvPath": "..", + "venv": ".venv" }If the venv path differs, adjust venvPath/venv accordingly. You can keep “basic” mode for now and tighten later.
restore-dev.bat (3)
22-22: Avoid hardcoding a specific version in the example pathUsing a placeholder reduces churn and confusion when versions update.
Apply this diff:
-echo Example: X:\UnityProject\Library\PackageCache\com.coplaydev.unity-mcp@1.0.0 +echo Example: X:\UnityProject\Library\PackageCache\com.coplaydev.unity-mcp@<version>
66-70: Guard against non-numeric input for backup selectionIf a user types non-numeric input, the numeric comparisons can error out. Add a quick numeric regex check before the lss/gtr comparisons.
Apply this diff:
:: Validate choice -if "%choice%"=="" goto :invalid_choice -if %choice% lss 1 goto :invalid_choice -if %choice% gtr %counter% goto :invalid_choice +if "%choice%"=="" goto :invalid_choice +rem Ensure choice is numeric +echo %choice% | findstr /r "^[0-9][0-9]*$" >nul || goto :invalid_choice +if %choice% lss 1 goto :invalid_choice +if %choice% gtr %counter% goto :invalid_choice
120-147: Consider using robocopy for more robust copies and clearer exit codesxcopy exit codes can be ambiguous. robocopy is more resilient (preserves directory trees, better error handling). Not required, but improves reliability.
Example refactor for both copy sections (success is ERRORLEVEL < 8):
- rd /s /q "%PACKAGE_CACHE_PATH%\Editor" 2>nul - xcopy "%SELECTED_BACKUP%\UnityBridge\Editor\*" "%PACKAGE_CACHE_PATH%\Editor\" /E /I /Y > nul - if !errorlevel! neq 0 ( - echo Error: Failed to restore Unity Bridge files - pause - exit /b 1 - ) + rd /s /q "%PACKAGE_CACHE_PATH%\Editor" 2>nul + robocopy "%SELECTED_BACKUP%\UnityBridge\Editor" "%PACKAGE_CACHE_PATH%\Editor" /MIR /COPY:DAT /R:1 /W:1 /NFL /NDL /NP >nul + if !errorlevel! geq 8 ( + echo Error: Failed to restore Unity Bridge files + pause + exit /b 1 + ) @@ - rd /s /q "%SERVER_PATH%" 2>nul - mkdir "%SERVER_PATH%" - xcopy "%SELECTED_BACKUP%\PythonServer\*" "%SERVER_PATH%\" /E /I /Y > nul - if !errorlevel! neq 0 ( - echo Error: Failed to restore Python Server files - pause - exit /b 1 - ) + rd /s /q "%SERVER_PATH%" 2>nul + robocopy "%SELECTED_BACKUP%\PythonServer" "%SERVER_PATH%" /MIR /COPY:DAT /R:1 /W:1 /NFL /NDL /NP >nul + if !errorlevel! geq 8 ( + echo Error: Failed to restore Python Server files + pause + exit /b 1 + )Note: robocopy is available on modern Windows by default; adjust if you need compatibility with very old environments.
UnityMcpBridge/Editor/Models/MCPConfigServer.cs (1)
15-17: Sensible addition; consider documenting allowed values or constraining via enumAdding an optional transport “type” is fine and backwards-compatible with NullValueHandling.Ignore. For resilience, consider documenting allowed values (e.g., stdio/framed/etc.) and/or introducing an enum + converter to prevent typos from propagating into configs. Also double-check downstream readers that may switch on this field.
Would you like me to sketch an enum-based approach using StringEnumConverter while keeping the serialized name as "type"?
claude-chunk.md (1)
35-36: Add an actual link target for “More details”“See: Troubleshooting Unity MCP and Claude Code” lacks a hyperlink. Link to the canonical doc (README section or a dedicated troubleshooting page) so users can follow through.
Point me to the intended doc and I’ll wire up the exact link.
deploy-dev.bat (2)
12-12: DRY: Build SERVER_SOURCE from BRIDGE_SOURCEThis avoids repeating the package root and stays correct if BRIDGE_SOURCE ever changes.
Apply this diff:
-set "SERVER_SOURCE=%SCRIPT_DIR%UnityMcpBridge\UnityMcpServer~\src" +set "SERVER_SOURCE=%BRIDGE_SOURCE%\UnityMcpServer~\src"
21-23: Update example to current package version or generalizeThe example shows @1.0.0 but package.json declares 2.1.1. Update to reduce confusion (or use a placeholder).
Apply this diff:
-echo Example: X:\UnityProject\Library\PackageCache\com.coplaydev.unity-mcp@1.0.0 +echo Example: X:\UnityProject\Library\PackageCache\com.coplaydev.unity-mcp@2.1.1Optionally: print “com.coplaydev.unity-mcp@”.
UnityMcpBridge/package.json (1)
2-3: Package rename requires consumer migration planRenaming to com.coplaydev.unity-mcp is fine but breaks projects pinned to the old name. Ensure README/upgrade notes cover how to switch package names in existing projects (and consider leaving a deprecated stub package if feasible).
I can draft a short “Upgrading from com.justinpbarnett.unity-mcp” section with steps and gotchas.
UnityMcpBridge/UnityMcpServer~/src/config.py (1)
26-32: Mixed time units can be confusing (seconds vs milliseconds)retry_delay is in seconds while reload_retry_ms is in milliseconds. Consider standardizing units (prefer seconds) or suffixing all fields explicitly with their units to avoid misuse.
Apply one of:
- Rename reload_retry_ms to reload_retry_s (float) and convert callers.
- Or add docstrings for each field explicitly stating units and intended usage in client/server handshake responses.
UnityMcpBridge/Editor/Tools/ReadConsole.cs (3)
175-179: “all” should explicitly include assert/exception typesYou later special-case Exception/Assert during filtering, but expanding "all" to include them makes behavior clearer and avoids relying on side-effects of “error” coverage.
- if (types.Contains("all")) - { - types = new List<string> { "error", "warning", "log" }; // Expand 'all' - } + if (types.Contains("all")) + { + types = new List<string> { "error", "warning", "log", "exception", "assert" }; + }
268-275: Severity inference from message is heuristic; prefer mode bits when ambiguousRelying on substrings like "Exception" can misclassify logs that merely mention exceptions. Recommend flipping the precedence: start with GetLogTypeFromMode(mode), then escalate based on explicit Debug:LogError/LogWarning markers or clear stack-trace signals.
Consider:
- First compute type = GetLogTypeFromMode(mode).
- If IsExplicitDebugLog(message) or strong heuristics indicate higher severity, override.
- Keep current behavior behind a feature flag if cross-version stability is a concern.
451-468: Remove unused remapping helperGetRemappedTypeForFiltering is not used anymore and can be removed to reduce confusion and maintenance.
- /// <summary> - /// Applies the "one level lower" remapping for filtering, like the old version. - /// This ensures compatibility with the filtering logic that expects remapped types. - /// </summary> - private static LogType GetRemappedTypeForFiltering(LogType unityType) - { - switch (unityType) - { - case LogType.Error: - return LogType.Warning; // Error becomes Warning - case LogType.Warning: - return LogType.Log; // Warning becomes Log - case LogType.Assert: - return LogType.Assert; // Assert remains Assert - case LogType.Log: - return LogType.Log; // Log remains Log - case LogType.Exception: - return LogType.Warning; // Exception becomes Warning - default: - return LogType.Log; // Default fallback - } - }test_unity_socket_framing.py (3)
30-46: Legacy JSON reader: consider boundary detection to avoid O(N^2) concatenationsJoining bytes on each recv scales poorly for large payloads. Use a BytesIO buffer or extend chunks only and validate the last few bytes for JSON termination when possible. Not critical for a harness but helps with very large filterText.
47-58: Large filter payload: guard against excessive memory useFILL * (SIZE_MB * 1024 * 1024) in Python builds a multi-MB string, then JSON-encodes it. For stress tests this is fine; consider a lower default or an environment/arg guard to prevent accidental 100s of MB allocations.
61-73: Handshake robustnessGood handling of optional greeting and framing detection. As a minor enhancement, also tolerate lowercased tokens and trailing metadata (e.g., "FRAMING=1;...").
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (3)
15-21: Remove unused import 'os'os is imported but not used. Dropping it keeps the module clean and resolves Ruff F401.
-import os import logging
40-55: Be resilient to filesystem races when sorting candidatesp.stat().st_mtime can raise if a file disappears between glob and stat, causing the whole sort to fail. Consider wrapping the stat in a try/except and skipping problematic entries.
I can send a small patch to collect (mtime, path) with guarded stat then sort, if you want.
56-75: Probe heuristic: validate beyond 'pong' if feasibleThe ping→"pong" check is lightweight and OK. If you want higher confidence it’s our MCP listener (and not an unrelated local service), consider a stricter marker (e.g., presence of a known key like protocol or product in the JSON) while keeping the short timeout.
UnityMcpBridge/Editor/Helpers/PortManager.cs (3)
44-65: Stored-port reuse logic: refine fallback when busy by non-MCP processIf the stored port is busy and not used by Unity MCP, you currently return the same port, pushing the bind failure to the caller. Consider falling back to FindAvailablePort in that case to reduce churn on the caller side.
- // Prefer sticking to the same port; let the caller handle bind retries/fallbacks - return storedConfig.unity_port; + // If not used by our MCP, prefer finding a new free port to avoid immediate bind failure + if (!IsPortUsedByUnityMcp(storedConfig.unity_port)) + return FindAvailablePort(); + return storedConfig.unity_port;
139-157: Heuristic 'used by Unity MCP' check: optionally probe a signatureConnectAsync().Wait(100) only confirms something is listening. If misidentification is an issue, consider a quick signature probe (send b"ping", expect known greeting/pong) behind a short timeout, similar to the Python PortDiscovery._try_probe_unity_mcp.
291-317: Duplicate hashing logic exists elsewhere; consider centralizingComputeProjectHash duplicates UnityMcpBridge.Editor.UnityMcpBridge.ComputeProjectHash. Extracting a shared helper (e.g., in a small internal utility class) would avoid drift if the algorithm changes.
mcp_source.py (4)
19-21: Remove unused imports to satisfy Ruff F401 and keep the module lean
osandreare not used.-import os +import os # unused import pathlib -import re +import re # unusedIf truly unused, please delete:
- import os - import re
94-109: Ensure consistent Unity Git URL scheme and correct file: URI form
- Option [1] uses
git+https://...but option [2] may be plainhttps://.... Unity typically acceptshttps://..., but mixing schemes can be brittle. Prefer consistentgit+https://for both.- Local
file:URI should usefile:///absolute/path(triple slash) for correctness across platforms.def build_options(repo_root: pathlib.Path, branch: str, origin_https: str): - upstream = "git+https://github.com/CoplayDev/unity-mcp.git?path=/UnityMcpBridge" + upstream = "git+https://github.com/CoplayDev/unity-mcp.git?path=/UnityMcpBridge" # Ensure origin is https origin = origin_https # If origin is a local file path or non-https, try to coerce to https github if possible if origin.startswith("file:"): # Not meaningful for remote option; keep upstream origin_remote = upstream else: - origin_remote = origin + # Prefer git+https for consistency with Unity Git resolver + origin_remote = origin.replace("https://", "git+https://", 1) if origin.startswith("https://") else origin - return [ + # file: URI should be file:///absolute/path + local_abs = (repo_root / BRIDGE_SUBPATH).resolve() + local_uri = f"file:///{local_abs.as_posix().lstrip('/')}" + return [ ("[1] Upstream main", upstream), ("[2] Remote current branch", f"{origin_remote}?path=/{BRIDGE_SUBPATH}#{branch}"), - ("[3] Local workspace", f"file:{(repo_root / BRIDGE_SUBPATH).as_posix()}"), + ("[3] Local workspace", local_uri), ]Verification suggestions:
- Confirm Unity accepts
git+https://...for both upstream and origin forms in your environment.- If Unity rejects
git+https://for origin remotes, keephttps://but make schemes consistent (i.e., removegit+from upstream).
141-144: Inline the choice selection with an or-expression (Ruff SIM108)Minor simplification.
- if args.choice: - choice = args.choice - else: - choice = input("Enter 1-3: ").strip() + choice = args.choice or input("Enter 1-3: ").strip()
153-165: Add a recovery path for missing dependency and improve UXCurrently we error if
com.coplaydev.unity-mcpisn’t present. Many users will have the legacy package id. Consider:
- If legacy id is present, migrate it in-place.
- Otherwise, offer
--force-addto add the dependency.I can draft a small migration block that:
- Detects
com.justinbarnett.unity-mcpand renames the key.- Or adds a new entry when
--force-addis set.
Would you like me to add that?README-DEV.md (2)
51-68: Clarify Windows path examples and code-fence languages for readability
- Consider tagging the fenced blocks with a language for syntax highlighting (e.g.,
textfor raw paths).- For the “Example (hash)” block, keep the closing backticks aligned with the content to avoid breaking paragraph flow.
-``` +```text X:\UnityProject\Library\PackageCache\com.coplaydev.unity-mcp@<version-or-hash>-Example (hash):
-+Example (hash): +text
X:\UnityProject\Library\PackageCache\com.coplaydev.unity-mcp@272123cfd97e
112-113: Tighten the uv guidance wordingSmall tone and clarity tweak to make it directive and consistent with other bullets.
-### Windows uv path issues -- On Windows, when testing GUI clients, prefer the WinGet Links `uv.exe`; if multiple `uv.exe` exist, use "Choose UV Install Location" to pin the Links shim. +### Windows uv path issues +- On Windows, when testing GUI clients, prefer the WinGet Links `uv.exe`. If multiple `uv.exe` are present, use “Choose UV Install Location” to pin the WinGet Links shim.README.md (2)
108-111: Specify a fenced code language for the Git URL block (MD040)Use
textfor non-code URLs to quiet markdownlint and improve consistency.- ``` - https://github.com/CoplayDev/unity-mcp.git?path=/UnityMcpBridge - ``` + ```text + https://github.com/CoplayDev/unity-mcp.git?path=/UnityMcpBridge + ```
320-323: Minor: standardize PR guidance wordingSmall consistency tweak: “to the main branch” reads more naturally.
-6. **Open a Pull Request** against the main branch. +6. **Open a Pull Request** to the main branch.UnityMcpBridge/Editor/Tools/ManageScript.cs (3)
230-234: Update the “Unknown action” message to include new actionsList currently valid actions to reduce confusion now that read/update/edit are deprecated in favor of
apply_text_editsandvalidate.- return Response.Error( - $"Unknown action: '{action}'. Valid actions are: create, read, update, delete." - ); + return Response.Error( + $"Unknown action: '{action}'. Valid actions: create, delete, apply_text_edits, validate." + );
267-271: Align error guidance with current API (update is deprecated)The message suggests using
update, butupdatenow returns a deprecation error. Point users toapply_text_edits.- return Response.Error( - $"Script already exists at '{relativePath}'. Use 'update' action to modify." - ); + return Response.Error( + $"Script already exists at '{relativePath}'. Use 'apply_text_edits' to modify the file." + );
571-589: O(n*m) line/col indexing is fine for small edits; consider precomputing line starts for larger filesFor many edits on large files, repeatedly scanning the whole string per edit is costly. Precompute line start offsets once and translate line/col to index in O(1).
I can provide a drop-in helper that builds an int[] of line starts and a fast translator that respects CRLF and LF.
UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs.meta (1)
1-11: Meta file LGTM; consider adding a trailing newlineUnity doesn’t require it, but some linters prefer a newline at EOF.
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (3)
5-5: Remove unused imports and redundant connection retrieval.
timeandconfigare unused (Ruff F401).get_unity_connection()is called to setbridgebutbridgeis never used.send_command_with_retryinternally obtains the connection, so this is redundant.Apply this diff:
-from typing import List, Dict, Any -import time +from typing import List, Dict, Any from mcp.server.fastmcp import FastMCP, Context -from unity_connection import get_unity_connection, send_command_with_retry -from config import config +from unity_connection import send_command_with_retry @@ - # Get the connection instance - bridge = get_unity_connection() + # Connection is resolved internally by send_command_with_retryAlso applies to: 7-8, 40-41
67-69: Remove dead branch re-adding 'count'.
countis always present inparams_dict(kept by the comprehension allowingk == 'count'), so this check never triggers.- # Add count back if it was None, explicitly sending null might be important for C# logic - if 'count' not in params_dict: - params_dict['count'] = None
21-22: Parameter name 'format' shadows Python’s built-in.Not harmful, but consider
output_formatin future revisions to avoid shadowing if you plan further processing. Double-check external API compatibility before renaming.UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (3)
3-5: Prune unused imports.
get_unity_connection,config, andtimeare not used (Ruff F401). Keep onlysend_command_with_retry.-from unity_connection import get_unity_connection, send_command_with_retry -from config import config -import time +from unity_connection import send_command_with_retry
31-37: Normalize action for consistency.Other tools (e.g., manage_asset) lowercase
action. Do the same here to avoid case-sensitivity bugs.- try: - params = { + try: + action = action.lower() + params = { "action": action, "name": name, "path": path, "buildIndex": build_index }
11-17: Function signature doesn’t match the docstring; make non-essential args optional.Doc says
pathdefaults to "Assets/" andbuild_indexis action-specific. Consider optional defaults to align behavior and avoid forced arguments for unrelated actions.- def manage_scene( + def manage_scene( ctx: Context, action: str, name: str, - path: str, - build_index: int, + path: str = "Assets/", + build_index: int | None = None, ) -> Dict[str, Any]:Would changing the signature affect any existing clients invoking this tool positionally? If so, we can instead default inside the function and keep the signature unchanged.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (4)
9-10: Remove unused imports (config,time).Both are unused (Ruff F401).
-from config import config -import time
4-4: Drop unnecessary loop/connection handling; let the helper manage it.
async_send_command_with_retrygets the running loop internally whenloop=None.- Pre-fetching a connection is redundant; the retry helper resolves it.
This simplifies code and reduces noise.
-import asyncio # Added: Import asyncio for running sync code in async @@ -from unity_connection import get_unity_connection, async_send_command_with_retry # Use centralized retry helper +from unity_connection import async_send_command_with_retry # Use centralized retry helper @@ - # Get the current asyncio event loop - loop = asyncio.get_running_loop() - # Get the Unity connection instance - connection = get_unity_connection() - - # Use centralized async retry helper to avoid blocking the event loop - result = await async_send_command_with_retry("manage_asset", params_dict, loop=loop) + # Use centralized async retry helper; it obtains the loop and connection internally + result = await async_send_command_with_retry("manage_asset", params_dict) # Return the result obtained from Unity return result if isinstance(result, dict) else {"success": False, "message": str(result)}Also applies to: 8-8, 71-79
46-48: Docstring return fields don’t match actual response shape.You return
{"success": False, "message": ...}on failure, noterror. Align the docstring for clarity.- Returns: - A dictionary with operation results ('success', 'data', 'error'). + Returns: + A dictionary with operation results ('success', 'data', 'message').
7-7: Clean up commented-out import.The commented import is legacy noise. Consider removing it to keep the file tidy.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (1)
2-5: Remove unused imports flagged by Rufftime, get_unity_connection, and config are unused here.
Apply:
-import time -from unity_connection import get_unity_connection, send_command_with_retry -from config import config +from unity_connection import send_command_with_retryUnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (2)
123-125: Key mismatch: prefab_folder vs prefabFolderYou build params with "prefabFolder" (camelCase) but pop "prefab_folder" (snake_case), so the field is never removed. If the intent is to avoid sending prefabFolder when prefabPath is present, pop the correct key.
- params.pop("prefab_folder", None) + params.pop("prefabFolder", None)
3-5: Remove unused imports flagged by Ruffget_unity_connection, config, and time are not used in this module.
-from unity_connection import get_unity_connection, send_command_with_retry -from config import config -import time +from unity_connection import send_command_with_retryUnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py (2)
6-9: Remove unused imports flagged by Ruffget_unity_connection, config, and time are unused here.
-from unity_connection import get_unity_connection, send_command_with_retry # Import retry helper -from config import config -import time +from unity_connection import send_command_with_retry # Import retry helper
47-49: Optional: Normalize success like other tools for consistencyOther tools unwrap successful dicts into {"success": True, "message", "data"}. Consider aligning for uniformity.
- resp = send_command_with_retry("execute_menu_item", params_dict) - return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} + resp = send_command_with_retry("execute_menu_item", params_dict) + if isinstance(resp, dict) and resp.get("success"): + return {"success": True, "message": resp.get("message", "Menu item executed."), "data": resp.get("data")} + return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (3)
3-5: Remove unused imports flagged by Ruffget_unity_connection, config, and time are unused in this file.
-from unity_connection import get_unity_connection, send_command_with_retry -from config import config -import time +from unity_connection import send_command_with_retry
14-19: Make contents optional for non-write actionscontents is required in the signature, but read/delete don’t need it. Making it optional simplifies callers.
- contents: str, + contents: str | None = None,
55-64: Guard base64 decode to avoid KeyError when fields are absentIf contentsEncoded is True but encodedContents is missing, current code will KeyError. Guard and use pop to clean up.
- if isinstance(response, dict) and response.get("success"): - # If the response contains base64 encoded content, decode it - if response.get("data", {}).get("contentsEncoded"): - decoded_contents = base64.b64decode(response["data"]["encodedContents"]).decode('utf-8') - response["data"]["contents"] = decoded_contents - del response["data"]["encodedContents"] - del response["data"]["contentsEncoded"] + if isinstance(response, dict) and response.get("success"): + data = response.get("data") or {} + if data.get("contentsEncoded"): + encoded = data.get("encodedContents") + if encoded is not None: + decoded_contents = base64.b64decode(encoded).decode('utf-8') + data["contents"] = decoded_contents + data.pop("encodedContents", None) + data.pop("contentsEncoded", None) - return {"success": True, "message": response.get("message", "Operation successful."), "data": response.get("data")} + return {"success": True, "message": response.get("message", "Operation successful."), "data": response.get("data")}UnityMcpBridge/Editor/Helpers/PackageInstaller.cs (4)
14-22: Gate auto-install in batch mode and confirm EditorPrefs scope is appropriate
- Consider skipping the delayed install when running in batch mode (CI/builds) to avoid side effects during non-interactive sessions.
- Verify that using EditorPrefs (user-wide) is intended vs. a project-scoped sentinel. If install state should be per project/version, EditorPrefs may be too coarse.
Apply this minimal guard to avoid running in batch mode:
static PackageInstaller() { // Check if this is the first time the package is loaded - if (!EditorPrefs.GetBool(InstallationFlagKey, false)) + if (Application.isBatchMode) + { + // Skip auto-install on build/CI + return; + } + if (!EditorPrefs.GetBool(InstallationFlagKey, false)) { // Schedule the installation for after Unity is fully loaded EditorApplication.delayCall += InstallServerOnFirstLoad; } }
12-12: Make the install sentinel version-aware to support future upgradesIf the embedded server changes materially, a versioned sentinel prevents false positives from older installs.
Apply this diff to version the EditorPrefs key:
- private const string InstallationFlagKey = "UnityMCP.ServerInstalled"; + // Bump VERSION when the embedded server layout/requirements change + private const string InstallationFlagKey = "UnityMCP.ServerInstalled.v1";No other code changes needed; existing GetBool/SetBool calls continue to work with the new key.
Also applies to: 16-22, 31-33
28-39: Check branding consistencyLog messages use the "UNITY-MCP" tag. If the rebrand to "Coplay" is intended repo-wide, align these strings for consistency.
24-41: Optional UX: show a progress bar during installationLong installs can feel unresponsive. Consider a non-blocking EditorUtility.DisplayProgressBar with a finally block to clear it.
Happy to provide a small helper that wraps EnsureServerInstalled with a transient progress UI.
CursorHelp.md (1)
7-7: Minor editorial fixes for clarity and consistency
- Use straight quotes for readability in Markdown.
- Capitalize “Unity” consistently.
Apply this diff:
-- Cursor shows the UnityMCP server but never connects or reports it “can’t start.” +- Cursor shows the UnityMCP server but never connects or reports it "can't start."-- If the unity window keeps rewriting to a different `uv.exe`, pick the Links shim again; Unity MCP saves a pinned override and will stop auto-rewrites. +- If the Unity window keeps rewriting to a different `uv.exe`, pick the Links shim again; Unity MCP saves a pinned override and will stop auto-rewrites.Also applies to: 75-75
UnityMcpBridge/Editor/Data/McpClients.cs (3)
72-77: Honor XDG config on Linux: prefer ApplicationData over hardcoded ~/.configOn Linux, Environment.SpecialFolder.ApplicationData resolves to $XDG_CONFIG_HOME (falling back to ~/.config). This is more robust than concatenating ".config" to $HOME.
Apply this diff:
- linuxConfigPath = Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - ".config", - "Claude", - "claude_desktop_config.json" - ), + linuxConfigPath = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), + "Claude", + "claude_desktop_config.json" + ),- linuxConfigPath = Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - ".config", - "Code", - "User", - "mcp.json" - ), + linuxConfigPath = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), + "Code", + "User", + "mcp.json" + ),Also applies to: 92-97
103-113: Map more statuses if availableOnly "Not Configured" is mapped to McpStatus.NotConfigured. If other string states exist (e.g., "Configured", "Error", "Partial"), consider mapping them here to keep enum in sync.
I can add a small mapping helper and update this loop if you share the expected status strings.
10-101: Consider centralizing client path resolutionSeveral paths embed OS-specific conventions. A central helper (e.g., ExecPath.ResolveClientConfig(McpType)) would improve maintainability and testability.
tests/test_script_tools.py (1)
5-5: Remove unused importpytest is not used directly in this test module.
Apply this diff:
-import pytestUnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
8-73: Preserve original newline style and add case-insensitive anchor option
- Keep CRLF vs. LF to minimize noisy diffs on Windows.
- Allow ignore_case on anchor_insert, similar to regex_replace.
Apply this diff:
def _apply_edits_locally(original_text: str, edits: List[Dict[str, Any]]) -> str: text = original_text + # Preserve predominant newline style (default to "\n") + eol = "\r\n" if "\r\n" in text else ("\n" if "\n" in text else "\n") for edit in edits or []: op = ( (edit.get("op") or edit.get("operation") or edit.get("type") or edit.get("mode") or "") .strip() .lower() ) if not op: allowed = "anchor_insert, prepend, append, replace_range, regex_replace" raise RuntimeError( f"op is required; allowed: {allowed}. Use 'op' (aliases accepted: type/mode/operation)." ) if op == "prepend": prepend_text = edit.get("text", "") - text = (prepend_text if prepend_text.endswith("\n") else prepend_text + "\n") + text + if prepend_text and not prepend_text.endswith(eol): + prepend_text += eol + text = prepend_text + text elif op == "append": append_text = edit.get("text", "") - if not text.endswith("\n"): - text += "\n" + if not text.endswith(eol): + text += eol text += append_text - if not text.endswith("\n"): - text += "\n" + if append_text and not text.endswith(eol): + text += eol elif op == "anchor_insert": anchor = edit.get("anchor", "") position = (edit.get("position") or "before").lower() insert_text = edit.get("text", "") - flags = re.MULTILINE + flags = re.MULTILINE + if edit.get("ignore_case"): + flags |= re.IGNORECASE m = re.search(anchor, text, flags) if not m: if edit.get("allow_noop", True): continue raise RuntimeError(f"anchor not found: {anchor}") idx = m.start() if position == "before" else m.end() text = text[:idx] + insert_text + text[idx:] elif op == "replace_range": start_line = int(edit.get("startLine", 1)) end_line = int(edit.get("endLine", start_line)) replacement = edit.get("text", "") lines = text.splitlines(keepends=True) if start_line < 1 or end_line < start_line or end_line > len(lines): raise RuntimeError("replace_range out of bounds") a = start_line - 1 b = end_line rep = replacement - if rep and not rep.endswith("\n"): - rep += "\n" + if rep and not rep.endswith(eol): + rep += eol text = "".join(lines[:a]) + rep + "".join(lines[b:]) elif op == "regex_replace": pattern = edit.get("pattern", "") repl = edit.get("replacement", "") count = int(edit.get("count", 0)) # 0 = replace all flags = re.MULTILINE if edit.get("ignore_case"): flags |= re.IGNORECASE text = re.sub(pattern, repl, text, count=count, flags=flags) else: allowed = "anchor_insert, prepend, append, replace_range, regex_replace" raise RuntimeError(f"unknown edit op: {op}; allowed: {allowed}. Use 'op' (aliases accepted: type/mode/operation).") return text
121-124: Guard against invalid base64 in read responsesA malformed or empty encodedContents should return a structured failure rather than raising.
Apply this diff:
- if contents is None and data.get("contentsEncoded") and data.get("encodedContents"): - contents = base64.b64decode(data["encodedContents"]).decode("utf-8") + if contents is None and data.get("contentsEncoded") and data.get("encodedContents"): + try: + decoded = base64.b64decode(data["encodedContents"]) + contents = decoded.decode("utf-8") + except Exception as e: + return {"success": False, "message": f"Failed to decode base64-encoded contents: {e}"}UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (1)
62-74: Avoid CPU spin while waiting for PackageManager Client.List()The busy-wait can peg the editor thread. Sleep briefly while waiting.
Apply this diff:
- while (!list.IsCompleted) { } + while (!list.IsCompleted) { System.Threading.Thread.Sleep(50); }UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
1-8: Remove unused imports (ruff F401)
get_unity_connection,config, andtimeare unused here.Apply this diff:
-from mcp.server.fastmcp import FastMCP, Context -from typing import Dict, Any, List -from unity_connection import get_unity_connection, send_command_with_retry -from config import config -import time +from mcp.server.fastmcp import FastMCP, Context +from typing import Dict, Any, List +from unity_connection import send_command_with_retry import os import base64
13-23: More robust URI parsing for file:// on WindowsPlain slicing can yield paths like “/C:/…” for file URIs. Use urllib.parse and unquote to normalize cross-platform.
Apply this diff:
+ from urllib.parse import urlparse, unquote def _split_uri(uri: str) -> tuple[str, str]: - if uri.startswith("unity://path/"): - path = uri[len("unity://path/") :] - elif uri.startswith("file://"): - path = uri[len("file://") :] - else: - path = uri - path = path.replace("\\", "/") - name = os.path.splitext(os.path.basename(path))[0] - directory = os.path.dirname(path) - return name, directory + p = urlparse(uri) + if p.scheme == "unity" and p.netloc == "path": + path = unquote(p.path) + elif p.scheme == "file": + path = unquote(p.path) + # Windows: strip leading slash for drive letters (/C:/...) + if os.name == "nt" and len(path) > 3 and path[0] == "/" and path[2] == ":": + path = path[1:] + else: + path = uri + path = path.replace("\\", "/") + name = os.path.splitext(os.path.basename(path))[0] + directory = os.path.dirname(path) + return name, directory
134-142: Guard against missing encoded fields when decodingIf
contentsEncodedis true butencodedContentsis absent, the dict indexing will KeyError. Use .get() to be safe.Apply this diff:
- if response.get("data", {}).get("contentsEncoded"): - decoded_contents = base64.b64decode(response["data"]["encodedContents"]).decode('utf-8') - response["data"]["contents"] = decoded_contents - del response["data"]["encodedContents"] - del response["data"]["contentsEncoded"] + data = response.get("data", {}) + if data.get("contentsEncoded") and "encodedContents" in data: + decoded_contents = base64.b64decode(data["encodedContents"]).decode('utf-8') + data["contents"] = decoded_contents + data.pop("encodedContents", None) + data.pop("contentsEncoded", None)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (5)
91-96: Preserve exception context when re-raising (ruff B904)Chain exceptions with
from eto keep the root cause.Apply this diff:
- except socket.timeout: + except socket.timeout as e: logger.warning("Socket timeout during framed receive") - raise Exception("Timeout receiving Unity response") + raise Exception("Timeout receiving Unity response") from e except Exception as e: logger.error(f"Error during framed receive: {str(e)}") - raise + raise
144-149: Preserve exception context when re-raising (ruff B904)Same issue in the unframed receive path.
Apply this diff:
- except socket.timeout: + except socket.timeout as e: logger.warning("Socket timeout during receive") - raise Exception("Timeout receiving Unity response") - except Exception as e: + raise Exception("Timeout receiving Unity response") from e + except Exception as e: logger.error(f"Error during receive: {str(e)}") - raise + raise
192-195: Simplify nested condition (ruff SIM102)Combine the nested
ifchecks.Apply this diff:
- if not self.sock: - if not self.connect(): - raise Exception("Could not connect to Unity") + if (self.sock is None) and (not self.connect()): + raise Exception("Could not connect to Unity")
204-207: Replace try/except/pass with contextlib.suppress (ruff SIM105)Cleaner and intent-revealing around best-effort debug logging.
Apply this diff:
+ import contextlib # Send - try: - logger.debug(f"send {len(payload)} bytes; mode={'framed' if self.use_framing else 'legacy'}; head={(payload[:32]).decode('utf-8','ignore')}") - except Exception: - pass + with contextlib.suppress(Exception): + logger.debug(f"send {len(payload)} bytes; mode={'framed' if self.use_framing else 'legacy'}; head={(payload[:32]).decode('utf-8','ignore')}") @@ - try: - logger.debug(f"recv {len(response_data)} bytes; mode={'framed' if self.use_framing else 'legacy'}; head={(response_data[:32]).decode('utf-8','ignore')}") - except Exception: - pass + with contextlib.suppress(Exception): + logger.debug(f"recv {len(response_data)} bytes; mode={'framed' if self.use_framing else 'legacy'}; head={(response_data[:32]).decode('utf-8','ignore')}")Also applies to: 220-223
262-283: Remove unused variablebackoff(ruff F841) and simplify backoff logic
backoffis computed but never used. Drop it.Apply this diff:
- # Base exponential backoff - backoff = base_backoff * (2 ** attempt) # Decorrelated jitter multiplier jitter = random.uniform(0.1, 0.3) @@ - sleep_s = min(cap, jitter * (2 ** attempt)) + sleep_s = min(cap, jitter * (2 ** attempt))(Note: Only the unused assignment is removed.)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
97-118: Remove unused helpers to reduce surface area
IsDirectoryWritableandIsServerInstalledare currently unused.Apply this diff:
- private static bool IsDirectoryWritable(string path) - { - try - { - File.Create(Path.Combine(path, "test.txt")).Dispose(); - File.Delete(Path.Combine(path, "test.txt")); - return true; - } - catch - { - return false; - } - } - - /// <summary> - /// Checks if the server is installed at the specified location. - /// </summary> - private static bool IsServerInstalled(string location) - { - return Directory.Exists(location) - && File.Exists(Path.Combine(location, ServerFolder, "src", "server.py")); - } + // (Removed unused helpers IsDirectoryWritable and IsServerInstalled)
134-153: Use streaming enumeration when copying directoriesEnumerating rather than materializing arrays reduces memory footprint on large trees.
Apply this diff:
- foreach (string filePath in Directory.GetFiles(sourceDir)) + foreach (string filePath in Directory.EnumerateFiles(sourceDir)) { string fileName = Path.GetFileName(filePath); string destFile = Path.Combine(destinationDir, fileName); File.Copy(filePath, destFile, overwrite: true); } - foreach (string dirPath in Directory.GetDirectories(sourceDir)) + foreach (string dirPath in Directory.EnumerateDirectories(sourceDir)) { string dirName = Path.GetFileName(dirPath); foreach (var skip in _skipDirs) { if (dirName.Equals(skip, StringComparison.OrdinalIgnoreCase)) goto NextDir; } try { if ((File.GetAttributes(dirPath) & FileAttributes.ReparsePoint) != 0) continue; } catch { } string destSubDir = Path.Combine(destinationDir, dirName); CopyDirectoryRecursive(dirPath, destSubDir); NextDir: ; }UnityMcpBridge/UnityMcpServer~/src/server.py (2)
1-12: Remove unused imports (ruff F401)
Image,dataclass, andListare unused.Apply this diff:
-from mcp.server.fastmcp import FastMCP, Context, Image +from mcp.server.fastmcp import FastMCP, Context @@ -from dataclasses import dataclass @@ -from typing import AsyncIterator, Dict, Any, List +from typing import AsyncIterator, Dict, Any
121-137: Guard Assets traversal when cwd is not a Unity projectWhen run outside a Unity project,
Assetsmay not exist. Early-return avoids unnecessary os.walk.Apply this diff:
assets = [] try: - root = os.getcwd() - for base, _, files in os.walk(os.path.join(root, "Assets")): + root = os.getcwd() + assets_dir = os.path.join(root, "Assets") + if not os.path.isdir(assets_dir): + return assets + for base, _, files in os.walk(assets_dir): for f in files: if f.endswith(".cs"): rel = os.path.relpath(os.path.join(base, f), root).replace("\\", "/") assets.append({ "uri": f"unity://path/{rel}", "name": os.path.basename(rel) })UnityMcpBridge/Editor/UnityMcpBridge.cs (3)
7-7: Remove unused using directiveThe
System.Threadingnamespace is imported but not used in this file. Consider removing it to keep imports clean.-using System.Threading;
25-31: Consider extracting heartbeat configuration to constantsThe heartbeat interval and timing values are hardcoded inline. Consider extracting them to named constants for better maintainability.
+private const double InitRetryDelaySeconds = 0.2; +private const double HeartbeatIntervalSeconds = 0.5; private static readonly object startStopLock = new(); private static bool initScheduled = false; private static bool ensureUpdateHooked = false; private static bool isStarting = false; private static double nextStartAt = 0.0f; private static double nextHeartbeatAt = 0.0f; private static int heartbeatSeq = 0;Then update the usage:
-nextStartAt = EditorApplication.timeSinceStartup + 0.20f; +nextStartAt = EditorApplication.timeSinceStartup + InitRetryDelaySeconds;-nextHeartbeatAt = now + 0.5f; +nextHeartbeatAt = now + HeartbeatIntervalSeconds;
502-517: Remove redundant overload and consolidate timeout handlingYou have two
ReadExactAsyncmethods - one without timeout and one with timeout. The non-timeout version is only used once (line 434) and could be replaced with the timeout version using the constantFrameIOTimeoutMs.-private static async System.Threading.Tasks.Task<byte[]> ReadExactAsync(NetworkStream stream, int count) -{ - byte[] data = new byte[count]; - int offset = 0; - while (offset < count) - { - int r = await stream.ReadAsync(data, offset, count - offset); - if (r == 0) - { - throw new System.IO.IOException("Connection closed before reading expected bytes"); - } - offset += r; - } - return data; -} - // Timeout-aware exact read helper; avoids indefinite stalls private static async System.Threading.Tasks.Task<byte[]> ReadExactAsync(NetworkStream stream, int count, int timeoutMs)Then update the call site at line 434:
-byte[] header = await ReadExactAsync(stream, 8, FrameIOTimeoutMs); +byte[] header = await ReadExactAsync(stream, 8, FrameIOTimeoutMs);UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (2)
4-8: Remove unused using directivesSeveral using directives appear to be unused based on the code. Consider removing them to keep imports clean.
-using System.Security.Cryptography; -using System.Text; -using System.Net.Sockets; -using System.Net; +using System.Text; +using System.Net.Sockets; +using System.Net;
2203-2295: Consider extracting Python detection to a shared utilityThe
IsPythonDetectedmethod is quite long and handles platform-specific Python detection. This functionality might be useful elsewhere and could be extracted to a shared utility class similar toExecPathorServerInstaller.Consider creating a
PythonResolverutility class in the Helpers namespace:// In UnityMcpBridge/Editor/Helpers/PythonResolver.cs namespace UnityMcpBridge.Editor.Helpers { internal static class PythonResolver { internal static bool IsPythonInstalled() { /* implementation */ } internal static string FindPython() { /* implementation */ } // Other Python-related utilities } }This would improve code organization and reusability across the codebase.
UnityMcpBridge/Editor/Helpers/ExecPath.cs (3)
92-132: Improve NVM version parsing robustnessThe NVM version parsing logic could be more robust. The current implementation might fail on version strings with multiple dashes or unusual formats.
// Extract numeric portion: e.g., v18.19.0-nightly -> 18.19.0 string versionStr = name.Substring(1); -int dashIndex = versionStr.IndexOf('-'); -if (dashIndex > 0) -{ - versionStr = versionStr.Substring(0, dashIndex); -} +// Use regex to extract the version number more reliably +var match = System.Text.RegularExpressions.Regex.Match(versionStr, @"^(\d+\.\d+\.\d+)"); +if (match.Success) +{ + versionStr = match.Groups[1].Value; +} if (Version.TryParse(versionStr, out Version parsed))
166-232: Add XML documentation for the public TryRun methodThe
TryRunmethod is a key utility that would benefit from XML documentation explaining its parameters, return value, and behavior - especially the PowerShell script handling on Windows.+/// <summary> +/// Executes an external process with the specified arguments and captures output. +/// </summary> +/// <param name="file">The executable file path or PowerShell script (.ps1) to run</param> +/// <param name="args">Command line arguments</param> +/// <param name="workingDir">Working directory for the process (null for current directory)</param> +/// <param name="stdout">Captured standard output</param> +/// <param name="stderr">Captured standard error</param> +/// <param name="timeoutMs">Maximum time to wait for process completion in milliseconds</param> +/// <param name="extraPathPrepend">Additional paths to prepend to the PATH environment variable</param> +/// <returns>True if the process exited with code 0, false otherwise</returns> internal static bool TryRun( string file, string args, string workingDir, out string stdout, out string stderr, int timeoutMs = 15000, string extraPathPrepend = null)
200-200: Use Path.PathSeparator consistentlyLine 200 uses
System.IO.Path.PathSeparatorwhile line 246 usesPath.PathSeparator. Consider using the shorter form consistently since you haveusing System.IO;at the top.-: (extraPathPrepend + System.IO.Path.PathSeparator + currentPath); +: (extraPathPrepend + Path.PathSeparator + currentPath);
| def normalize_origin_to_https(url: str) -> str: | ||
| """Map common SSH origin forms to https for Unity's git URL scheme.""" | ||
| if url.startswith("git@github.com:"): | ||
| owner_repo = url.split(":", 1)[1] | ||
| if owner_repo.endswith(".git"): | ||
| owner_repo = owner_repo[:-4] | ||
| return f"https://github.com/{owner_repo}.git" | ||
| # already https or file: etc. | ||
| return url |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Normalize additional Git origin formats (ssh:// and https without .git) to robustly support option [2]
Currently only git@github.com:... is normalized. If origin is ssh://git@github.com/owner/repo.git or https://github.com/owner/repo (no .git), option [2] may produce a URL Unity can’t resolve.
Proposed normalization:
def normalize_origin_to_https(url: str) -> str:
"""Map common SSH origin forms to https for Unity's git URL scheme."""
- if url.startswith("git@github.com:"):
+ if url.startswith("git@github.com:"):
owner_repo = url.split(":", 1)[1]
if owner_repo.endswith(".git"):
owner_repo = owner_repo[:-4]
return f"https://github.com/{owner_repo}.git"
- # already https or file: etc.
+ # ssh://git@github.com/owner/repo(.git)
+ if url.startswith("ssh://git@github.com/"):
+ owner_repo = url.rsplit("/", 1)[-2:] # keep trailing path
+ tail = url.split("github.com/", 1)[1]
+ if tail.endswith(".git"):
+ return "https://github.com/" + tail
+ return "https://github.com/" + tail + ".git"
+ # https://github.com/owner/repo (ensure .git)
+ if url.startswith("https://github.com/") and not url.endswith(".git"):
+ return url + ".git"
+ # already https with .git, or file:, etc.
return urlTo verify in this repo after applying, run:
🏁 Script executed:
#!/usr/bin/env bash
# Show normalized outputs for common remote forms
python - <<'PY'
from mcp_source import normalize_origin_to_https
tests = [
"git@github.com:CoplayDev/unity-mcp.git",
"ssh://git@github.com/CoplayDev/unity-mcp.git",
"https://github.com/CoplayDev/unity-mcp",
"https://github.com/CoplayDev/unity-mcp.git",
"file:/tmp/unity-mcp"
]
for t in tests:
print(t, "=>", normalize_origin_to_https(t))
PYLength of output: 401
Normalize additional Git origin formats (ssh:// and https:// without .git)
The tests confirm that normalize_origin_to_https currently only handles the git@github.com: form. To support:
ssh://git@github.com/owner/repo.gitssh://git@github.com/owner/repohttps://github.com/owner/repo(no.git)
we should extend the function to normalize these to https://github.com/owner/repo.git.
mcp_source.py (lines 39–47):
def normalize_origin_to_https(url: str) -> str:
"""Map common SSH origin forms to https for Unity's git URL scheme."""
- if url.startswith("git@github.com:"):
- owner_repo = url.split(":", 1)[1]
- if owner_repo.endswith(".git"):
- owner_repo = owner_repo[:-4]
- return f"https://github.com/{owner_repo}.git"
+ # git@github.com:owner/repo(.git)
+ if url.startswith("git@github.com:"):
+ owner_repo = url.split(":", 1)[1]
+ if owner_repo.endswith(".git"):
+ owner_repo = owner_repo[:-4]
+ return f"https://github.com/{owner_repo}.git"
+
+ # ssh://git@github.com/owner/repo(.git)
+ if url.startswith("ssh://git@github.com/"):
+ owner_repo = url.split("github.com/", 1)[1]
+ if owner_repo.endswith(".git"):
+ owner_repo = owner_repo[:-4]
+ return f"https://github.com/{owner_repo}.git"
+
+ # https://github.com/owner/repo (ensure .git)
+ if url.startswith("https://github.com/"):
+ owner_repo = url.split("github.com/", 1)[1]
+ if owner_repo.endswith(".git"):
+ owner_repo = owner_repo[:-4]
+ return f"https://github.com/{owner_repo}.git"
# already https with .git, file:, etc.
return urlAfter applying, rerun your verification script:
python - <<'PY'
from mcp_source import normalize_origin_to_https
tests = [
"git@github.com:CoplayDev/unity-mcp.git",
"ssh://git@github.com/CoplayDev/unity-mcp.git",
"ssh://git@github.com/CoplayDev/unity-mcp",
"https://github.com/CoplayDev/unity-mcp",
"https://github.com/CoplayDev/unity-mcp.git",
"file:/tmp/unity-mcp"
]
for t in tests:
print(t, "=>", normalize_origin_to_https(t))
PY🤖 Prompt for AI Agents
mcp_source.py around lines 39-47: normalize_origin_to_https currently only
handles "git@github.com:" SSH shorthand; extend it to also detect and normalize
"ssh://git@github.com/owner/repo(.git)?" and "https://github.com/owner/repo" (no
.git) forms into "https://github.com/owner/repo.git". Implement logic to: 1)
parse and accept URLs that start with "ssh://git@github.com/" by extracting the
owner/repo path, strip a trailing ".git" if present, and return the https form
with ".git" appended; 2) if the URL starts with "https://github.com/" but does
not end with ".git", append ".git"; 3) leave other schemes (file:, other hosts,
or already-correct https with .git) unchanged. Ensure trimming and joining avoid
duplicate slashes and only add ".git" once.
| #### Proudly sponsored and maintained by [Coplay](https://www.coplay.dev/?ref=unity-mcp), the AI assistant for Unity. [Read the backstory here.](https://www.coplay.dev/blog/coplay-and-open-source-unity-mcp-join-forces) | ||
|
|
There was a problem hiding this comment.
Fix heading level increment (MD001) for proper outline structure
Use h2 instead of h4 for this subheading.
-#### Proudly sponsored and maintained by [Coplay](https://www.coplay.dev/?ref=unity-mcp), the AI assistant for Unity. [Read the backstory here.](https://www.coplay.dev/blog/coplay-and-open-source-unity-mcp-join-forces)
+## Proudly sponsored and maintained by [Coplay](https://www.coplay.dev/?ref=unity-mcp), the AI assistant for Unity. [Read the backstory here.](https://www.coplay.dev/blog/coplay-and-open-source-unity-mcp-join-forces)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### Proudly sponsored and maintained by [Coplay](https://www.coplay.dev/?ref=unity-mcp), the AI assistant for Unity. [Read the backstory here.](https://www.coplay.dev/blog/coplay-and-open-source-unity-mcp-join-forces) | |
| ## Proudly sponsored and maintained by [Coplay](https://www.coplay.dev/?ref=unity-mcp), the AI assistant for Unity. [Read the backstory here.](https://www.coplay.dev/blog/coplay-and-open-source-unity-mcp-join-forces) |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h4
(MD001, heading-increment)
🤖 Prompt for AI Agents
In README.md around lines 3 to 4, the heading uses an h4 ("####") which violates
MD001 outline structure; change the subheading to h2 by replacing the leading
four hashes with two ("##") so the document uses the correct heading level and
preserves content and links.
|  | ||
|  | ||
| [](https://opensource.org/licenses/MIT) | ||
| [](https://www.coplay.dev/?ref=unity-mcp) |
There was a problem hiding this comment.
Add alt text to badges and images (MD045) for accessibility
Badges and images should include meaningful alt text.
-
-
-[](https://opensource.org/licenses/MIT)
-[](https://www.coplay.dev/?ref=unity-mcp)
+
+
+[](https://opensource.org/licenses/MIT)
+[](https://www.coplay.dev/?ref=unity-mcp)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
|  | |
|  | |
| [](https://opensource.org/licenses/MIT) | |
| [](https://www.coplay.dev/?ref=unity-mcp) | |
|  | |
|  | |
| [](https://opensource.org/licenses/MIT) | |
| [](https://www.coplay.dev/?ref=unity-mcp) |
🧰 Tools
🪛 LanguageTool
[grammar] ~9-~9: There might be a mistake here.
Context: ...b/commit-activity/w/CoplayDev/unity-mcp) 
[grammar] ~10-~10: There might be a mistake here.
Context: ...ds.io/github/issues/CoplayDev/unity-mcp) [
🪛 markdownlint-cli2 (0.17.2)
11-11: Images should have alternate text (alt text)
(MD045, no-alt-text)
12-12: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 Prompt for AI Agents
In README.md around lines 9 to 12, the badge image tags lack meaningful alt text
(MD045); update each Markdown image/link to include descriptive alt text inside
the square brackets (e.g., "GitHub commit activity", "Open issues", "MIT
License", "Sponsor — Coplay") so the first [] contains that text for
accessibility while keeping the existing badge URLs and links intact.
| guid: de8f5721c34f7194392e9d8c7d0226c0 | ||
| MonoImporter: | ||
| externalObjects: {} | ||
| serializedVersion: 2 | ||
| defaultReferences: [] | ||
| executionOrder: 0 | ||
| icon: {instanceID: 0} | ||
| userData: | ||
| assetBundleName: | ||
| assetBundleVariant: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM on standardized MonoImporter block; verify repo-wide GUID and meta consistency
The metadata looks standard for a C# script (.cs.meta): GUID preserved, MonoImporter present, serializedVersion/executionOrder/defaults all conventional.
Run this script to sanity-check meta integrity across the repo (duplicate GUIDs, missing .meta for .cs, MonoImporter presence, and fileFormatVersion correctness):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Checking for duplicate GUIDs across all *.meta files..."
dups=$(fd -a -t f -e meta -0 | xargs -0 gawk '/^guid:/ {print FILENAME, $2}' | \
gawk '{guid=$2; file=$1; c[guid]++; files[guid]=files[guid]" "file} END{ for (g in c) if (c[g]>1) {print "DUPLICATE GUID: " g " ->" files[g]}}')
if [[ -n "${dups}" ]]; then
echo "${dups}"
echo "FAIL: Found duplicate GUIDs."
exit 1
else
echo "OK: No duplicate GUIDs."
fi
echo
echo "2) Checking that every .cs file has a companion .meta..."
missing_meta=0
while IFS= read -r cs; do
if [[ ! -f "${cs}.meta" ]]; then
echo "Missing .meta for: ${cs}"
missing_meta=1
fi
done < <(fd -a -t f -e cs)
if [[ $missing_meta -eq 1 ]]; then
echo "FAIL: Some .cs files are missing .meta files."
exit 1
else
echo "OK: All .cs files have .meta files."
fi
echo
echo "3) Ensuring all .cs.meta files contain a MonoImporter block..."
no_importer=0
while IFS= read -r meta; do
if ! rg -q '^\s*MonoImporter:\s*$' "${meta}"; then
echo "No MonoImporter in: ${meta}"
no_importer=1
fi
done < <(fd -a -t f -g "*.cs.meta")
if [[ $no_importer -eq 1 ]]; then
echo "FAIL: Some .cs.meta files are missing MonoImporter."
exit 1
else
echo "OK: MonoImporter present in all .cs.meta files."
fi
echo
echo "4) Verifying fileFormatVersion: 2 for all .cs.meta files..."
bad_ffv=0
while IFS= read -r meta; do
if ! rg -q '^fileFormatVersion:\s*2\s*$' "${meta}"; then
echo "Unexpected fileFormatVersion in: ${meta}"
bad_ffv=1
fi
done < <(fd -a -t f -g "*.cs.meta")
if [[ $bad_ffv -eq 1 ]]; then
echo "FAIL: Some .cs.meta files have unexpected fileFormatVersion."
exit 1
else
echo "OK: fileFormatVersion is 2 for all .cs.meta files."
fi
echo
echo "All meta checks passed."Length of output: 724
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "1) Checking for duplicate GUIDs across all *.meta files..."
dups=$(fd -a -t f -e meta -0 | xargs -0 gawk '/^guid:/ {print FILENAME, $2}' | \
gawk '{guid=$2; file=$1; c[guid]++; files[guid]=files[guid]" "file} END{ for (g in c) if (c[g]>1) {print "DUPLICATE GUID: " g " ->" files[g]}}')
if [[ -n "$dups" ]]; then
echo "$dups"
echo "FAIL: Found duplicate GUIDs."
exit 1
else
echo "OK: No duplicate GUIDs."
fi
echo; echo "2) Checking that every .cs file has a companion .meta..."
missing=$(fd -a -t f -e cs -0 | xargs -0 -I{} bash -c '[ ! -f "{}.meta" ] && printf "%s\n" "{}"')
if [[ -n "$missing" ]]; then
echo "Missing .meta for:"
printf "%s\n" "$missing"
echo "FAIL: Some .cs files are missing .meta files."
exit 1
else
echo "OK: All .cs files have .meta files."
fi
echo; echo "3) Ensuring all .cs.meta files contain a MonoImporter block..."
no_importer=$(fd -a -t f -g "*.cs.meta" -0 | xargs -0 grep -L '^[[:space:]]*MonoImporter:' || true)
if [[ -n "$no_importer" ]]; then
echo "No MonoImporter in:"
printf "%s\n" "$no_importer"
echo "FAIL: Some .cs.meta files are missing MonoImporter."
exit 1
else
echo "OK: MonoImporter present in all .cs.meta files."
fi
echo; echo "4) Verifying fileFormatVersion: 2 for all .cs.meta files..."
bad_ffv=$(fd -a -t f -g "*.cs.meta" -0 | xargs -0 grep -L '^[[:space:]]*fileFormatVersion:[[:space:]]*2[[:space:]]*$' || true)
if [[ -n "$bad_ffv" ]]; then
echo "Unexpected fileFormatVersion in:"
printf "%s\n" "$bad_ffv"
echo "FAIL: Some .cs.meta files have unexpected fileFormatVersion."
exit 1
else
echo "OK: fileFormatVersion is 2 for all .cs.meta files."
fi
echo; echo "All meta checks passed."Length of output: 753
Add missing .meta for UnityMcpEditorWindow.cs
The script check revealed that UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs does not have a corresponding .meta file. Unity requires a .meta alongside every asset to preserve GUIDs and import settings.
• Missing file:
– UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs.meta
Please add and commit the .meta for this script to avoid import/link errors in the Unity Editor.
🤖 Prompt for AI Agents
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs.meta (around lines 2-11):
the UnityMcpEditorWindow.cs asset is missing its .meta file; create and commit
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs.meta so Unity can preserve
the asset GUID and import settings. Add a .meta containing a valid guid (use the
existing asset GUID if you have it, otherwise generate a new one via Unity by
reimporting the script), and include standard MonoImporter keys
(externalObjects, serializedVersion, defaultReferences, executionOrder, icon,
userData, assetBundleName, assetBundleVariant) so the editor will import the
script consistently; save and commit the new .meta next to the .cs file.
| guid: 5862c6a6d0a914f4d83224f8d039cf7b | ||
| MonoImporter: | ||
| externalObjects: {} | ||
| serializedVersion: 2 | ||
| defaultReferences: [] | ||
| executionOrder: 0 | ||
| icon: {instanceID: 0} | ||
| userData: | ||
| assetBundleName: | ||
| assetBundleVariant: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Meta block is standard and sane; keep GUID stable across refactors
Structure matches Unity’s expected MonoImporter format for C# scripts. No functional impact; OK to merge.
If helpful, run this repo-wide check to ensure meta integrity (duplicate GUIDs, missing metas, and MonoImporter presence):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Scan for duplicate GUIDs across all .meta files..."
fd -a -g '*.meta' UnityMcpBridge | while read -r f; do
g=$(awk '/^guid:/{print $2}' "$f")
[ -n "$g" ] && echo "$g $f"
done | tee /tmp/meta_guids.txt >/dev/null
dups=$(cut -d' ' -f1 /tmp/meta_guids.txt | sort | uniq -d)
if [ -n "$dups" ]; then
echo "Duplicate GUIDs detected:"
while read -r d; do
grep "^$d " /tmp/meta_guids.txt
done <<< "$dups"
echo
else
echo "No duplicate GUIDs found."
fi
echo
echo "2) Ensure every .cs has a .cs.meta and vice versa..."
missing=0
while read -r cs; do
if [ ! -f "${cs}.meta" ]; then
echo "Missing meta for script: ${cs}.meta"
missing=1
fi
done < <(fd -a -g '*.cs' UnityMcpBridge)
while read -r meta; do
cs="${meta%.meta}"
if [ ! -f "$cs" ]; then
echo "Stray .cs.meta without corresponding script: $meta"
missing=1
fi
done < <(fd -a -g '*.cs.meta' UnityMcpBridge)
[ $missing -eq 0 ] && echo "All .cs/.cs.meta pairs look consistent."
echo
echo "3) Validate MonoImporter blocks (serializedVersion: 2) in all .cs.meta files..."
issues=0
while read -r meta; do
rg -q '^MonoImporter:' "$meta" || { echo "Missing MonoImporter in: $meta"; issues=1; }
rg -q '^ serializedVersion:\s*2\b' "$meta" || { echo "Unexpected or missing serializedVersion in: $meta"; issues=1; }
done < <(fd -a -g '*.cs.meta' UnityMcpBridge)
[ $issues -eq 0 ] && echo "MonoImporter blocks look consistent."Length of output: 604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Scan for duplicate GUIDs across all .meta files..."
fd -a -g '*.meta' UnityMcpBridge | while read -r f; do
guid=$(awk '/^guid:/{print $2}' "$f")
echo "$guid $f"
done > /tmp/meta_guids_all.txt
dups=$(cut -d' ' -f1 /tmp/meta_guids_all.txt | sort | uniq -d)
if [ -n "$dups" ]; then
echo "Duplicate GUIDs detected:"
for d in $dups; do
grep "^$d " /tmp/meta_guids_all.txt
done
else
echo "No duplicate GUIDs found."
fi
echo
echo "2) Ensure every .cs has a .cs.meta and vice versa..."
missing=0
while read -r cs; do
if [ ! -f "${cs}.meta" ]; then
echo "Missing meta for script: ${cs}.meta"
missing=1
fi
done < <(fd -a -g '*.cs' UnityMcpBridge)
while read -r meta; do
cs="${meta%.meta}"
if [ ! -f "$cs" ]; then
echo "Stray .cs.meta without corresponding script: $meta"
missing=1
fi
done < <(fd -a -g '*.cs.meta' UnityMcpBridge)
[ $missing -eq 0 ] && echo "All .cs/.cs.meta pairs look consistent."
echo
echo "3) Validate MonoImporter blocks (serializedVersion: 2) in all .cs.meta files..."
issues=0
for meta in $(fd -a -g '*.cs.meta' UnityMcpBridge); do
if ! grep -q '^MonoImporter:' "$meta"; then
echo "Missing MonoImporter in: $meta"
issues=1
fi
if ! grep -q '^ serializedVersion:[[:space:]]*2\b' "$meta"; then
echo "Unexpected or missing serializedVersion in: $meta"
issues=1
fi
done
[ $issues -eq 0 ] && echo "MonoImporter blocks look consistent."Length of output: 11619
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "2) Checking .cs/.cs.meta consistency..."
missing=0
for cs in $(fd -g '*.cs' UnityMcpBridge); do
if [[ ! -f "${cs}.meta" ]]; then
echo "Missing meta for script: ${cs}.meta"
missing=1
fi
done
for meta in $(fd -g '*.cs.meta' UnityMcpBridge); do
cs="${meta%.meta}"
if [[ ! -f "$cs" ]]; then
echo "Stray .cs.meta without corresponding script: $meta"
missing=1
fi
done
if [[ $missing -eq 0 ]]; then
echo "All .cs/.cs.meta pairs look consistent."
fi
echo
echo "3) Validating MonoImporter blocks..."
issues=0
for meta in $(fd -g '*.cs.meta' UnityMcpBridge); do
if ! grep -q '^MonoImporter:' "$meta"; then
echo "Missing MonoImporter in: $meta"
issues=1
fi
if ! grep -q '^ serializedVersion:[[:space:]]*2\b' "$meta"; then
echo "Unexpected or missing serializedVersion in: $meta"
issues=1
fi
done
if [[ $issues -eq 0 ]]; then
echo "MonoImporter blocks look consistent."
fiLength of output: 16803
Fix mismatched .cs/.cs.meta in Editor/Windows before merging
No duplicate GUIDs found and all MonoImporter blocks are correct, but there’s a name mismatch in Editor/Windows:
- Missing metadata for script:
•UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs→ no corresponding.cs.meta - Stray metadata without a script:
•UnityMcpBridge/Editor/Windows/UnityMCPEditorWindow.cs.meta
Please rename or add the .meta file so its filename exactly matches the .cs script.
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Helpers/ServerInstaller.cs.meta lines 2-11 and the
Editor/Windows folder, there is a name mismatch: the script
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs lacks a .meta and there is
a stray UnityMCPEditorWindow.cs.meta; rename the stray file to exactly match the
script filename (UnityMcpEditorWindow.cs.meta — preserving case) or create a new
.meta with the same GUID/MonoImporter block if needed, and verify the
MonoImporter block and GUID remain correct and unique after renaming.
| private static string ComputeProjectHash(string input) | ||
| { | ||
| try | ||
| { | ||
| using var sha1 = System.Security.Cryptography.SHA1.Create(); | ||
| byte[] bytes = System.Text.Encoding.UTF8.GetBytes(input ?? string.Empty); | ||
| byte[] hashBytes = sha1.ComputeHash(bytes); | ||
| var sb = new System.Text.StringBuilder(); | ||
| foreach (byte b in hashBytes) | ||
| { | ||
| sb.Append(b.ToString("x2")); | ||
| } | ||
| return sb.ToString()[..8]; | ||
| } | ||
| catch | ||
| { | ||
| return "default"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract hash computation to a shared utility
The ComputeProjectHash method is duplicated here and in PortManager.cs (lines 298-316 from the relevant code snippets). Consider extracting this to a shared utility class to avoid duplication.
Create a new utility class:
// In UnityMcpBridge/Editor/Helpers/HashUtility.cs
namespace UnityMcpBridge.Editor.Helpers
{
internal static class HashUtility
{
internal static string ComputeProjectHash(string input)
{
try
{
using var sha1 = System.Security.Cryptography.SHA1.Create();
byte[] bytes = System.Text.Encoding.UTF8.GetBytes(input ?? string.Empty);
byte[] hashBytes = sha1.ComputeHash(bytes);
var sb = new System.Text.StringBuilder();
foreach (byte b in hashBytes)
{
sb.Append(b.ToString("x2"));
}
return sb.ToString()[..8];
}
catch
{
return "default";
}
}
}
}Then update both files to use the shared utility:
-private static string ComputeProjectHash(string input)
-{
- try
- {
- using var sha1 = System.Security.Cryptography.SHA1.Create();
- byte[] bytes = System.Text.Encoding.UTF8.GetBytes(input ?? string.Empty);
- byte[] hashBytes = sha1.ComputeHash(bytes);
- var sb = new System.Text.StringBuilder();
- foreach (byte b in hashBytes)
- {
- sb.Append(b.ToString("x2"));
- }
- return sb.ToString()[..8];
- }
- catch
- {
- return "default";
- }
-}
// Update the call site:
-string filePath = Path.Combine(dir, $"unity-mcp-status-{ComputeProjectHash(Application.dataPath)}.json");
+string filePath = Path.Combine(dir, $"unity-mcp-status-{HashUtility.ComputeProjectHash(Application.dataPath)}.json");🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/UnityMcpBridge.cs around lines 853-871, the
ComputeProjectHash method is duplicated elsewhere (PortManager.cs lines
~298-316); extract this logic into a single internal static utility class
UnityMcpBridge.Editor.Helpers.HashUtility with a ComputeProjectHash(string)
method containing the existing try/catch SHA1 logic, then remove the duplicate
method from UnityMcpBridge.cs and PortManager.cs and replace calls to
ComputeProjectHash with HashUtility.ComputeProjectHash; ensure the new file is
added to the project namespace UnityMcpBridge.Editor.Helpers and that necessary
using statements are added where the utility is used.
| string[] strArgs = ((System.Collections.Generic.IEnumerable<object>)args) | ||
| .Select(x => x?.ToString() ?? string.Empty) | ||
| .ToArray(); | ||
| string dir = ExtractDirectoryArg(strArgs); | ||
| if (string.IsNullOrEmpty(dir)) return false; | ||
| return PathsEqual(dir, pythonDir); | ||
| } | ||
| catch { return false; } | ||
| } |
There was a problem hiding this comment.
Potential null reference when casting args
The cast (System.Collections.Generic.IEnumerable<object>)args could fail if args is not of the expected type, leading to an InvalidCastException. Consider using a safer approach.
-string[] strArgs = ((System.Collections.Generic.IEnumerable<object>)args)
- .Select(x => x?.ToString() ?? string.Empty)
- .ToArray();
+string[] strArgs = null;
+if (args != null)
+{
+ if (args is Newtonsoft.Json.Linq.JArray jArray)
+ {
+ strArgs = jArray.Select(x => x?.ToString() ?? string.Empty).ToArray();
+ }
+ else if (args is System.Collections.Generic.IEnumerable<object> enumerable)
+ {
+ strArgs = enumerable.Select(x => x?.ToString() ?? string.Empty).ToArray();
+ }
+}
+if (strArgs == null) return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| string[] strArgs = ((System.Collections.Generic.IEnumerable<object>)args) | |
| .Select(x => x?.ToString() ?? string.Empty) | |
| .ToArray(); | |
| string dir = ExtractDirectoryArg(strArgs); | |
| if (string.IsNullOrEmpty(dir)) return false; | |
| return PathsEqual(dir, pythonDir); | |
| } | |
| catch { return false; } | |
| } | |
| string[] strArgs = null; | |
| if (args != null) | |
| { | |
| if (args is Newtonsoft.Json.Linq.JArray jArray) | |
| { | |
| strArgs = jArray.Select(x => x?.ToString() ?? string.Empty).ToArray(); | |
| } | |
| else if (args is System.Collections.Generic.IEnumerable<object> enumerable) | |
| { | |
| strArgs = enumerable.Select(x => x?.ToString() ?? string.Empty).ToArray(); | |
| } | |
| } | |
| if (strArgs == null) return false; | |
| string dir = ExtractDirectoryArg(strArgs); | |
| if (string.IsNullOrEmpty(dir)) return false; | |
| return PathsEqual(dir, pythonDir); |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs around lines 634-642,
the direct cast (System.Collections.Generic.IEnumerable<object>)args can throw
if args isn't that exact type; change it to a safe check: first test if args is
IEnumerable (via "as IEnumerable" or "is IEnumerable") and if not return false,
then enumerate its elements and call ToString() on each element (handling nulls)
to build strArgs; avoid the explicit generic cast and the broad catch-all so
only expected failures return false.
| private static bool IsValidUv(string path) | ||
| { | ||
| return !string.IsNullOrEmpty(path) | ||
| && System.IO.Path.IsPathRooted(path) | ||
| && System.IO.File.Exists(path); | ||
| } | ||
|
|
||
| private static bool ValidateUvBinarySafe(string path) | ||
| { | ||
| try | ||
| { | ||
| if (string.IsNullOrEmpty(path) || !System.IO.File.Exists(path)) return false; | ||
| var psi = new System.Diagnostics.ProcessStartInfo | ||
| { | ||
| FileName = path, | ||
| Arguments = "--version", | ||
| UseShellExecute = false, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| CreateNoWindow = true | ||
| }; | ||
| using var p = System.Diagnostics.Process.Start(psi); | ||
| if (p == null) return false; | ||
| if (!p.WaitForExit(3000)) { try { p.Kill(); } catch { } return false; } | ||
| if (p.ExitCode != 0) return false; | ||
| string output = p.StandardOutput.ReadToEnd().Trim(); | ||
| return output.StartsWith("uv "); | ||
| } | ||
| catch { return false; } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate UV validation methods
The methods IsValidUv and ValidateUvBinarySafe appear to perform similar UV binary validation. Consider consolidating them into a single method to avoid duplication.
-private static bool IsValidUv(string path)
-{
- return !string.IsNullOrEmpty(path)
- && System.IO.Path.IsPathRooted(path)
- && System.IO.File.Exists(path);
-}
-
-private static bool ValidateUvBinarySafe(string path)
+private static bool ValidateUvBinary(string path, bool checkVersion = false)
{
try
{
if (string.IsNullOrEmpty(path) || !System.IO.File.Exists(path)) return false;
+ if (!checkVersion) return true;
+
var psi = new System.Diagnostics.ProcessStartInfo
{
FileName = path,
Arguments = "--version",
UseShellExecute = false,
RedirectStandardOutput = true,
RedirectStandardError = true,
CreateNoWindow = true
};
using var p = System.Diagnostics.Process.Start(psi);
if (p == null) return false;
if (!p.WaitForExit(3000)) { try { p.Kill(); } catch { } return false; }
if (p.ExitCode != 0) return false;
string output = p.StandardOutput.ReadToEnd().Trim();
return output.StartsWith("uv ");
}
catch { return false; }
}Then update usage:
-string uvPath = (ValidateUvBinarySafe(existingCommand) ? existingCommand : FindUvPath());
+string uvPath = (ValidateUvBinary(existingCommand, true) ? existingCommand : FindUvPath());-if (IsValidUv(uvPath)) UnityEditor.EditorPrefs.SetString("UnityMCP.UvPath", uvPath);
+if (ValidateUvBinary(uvPath)) UnityEditor.EditorPrefs.SetString("UnityMCP.UvPath", uvPath);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static bool IsValidUv(string path) | |
| { | |
| return !string.IsNullOrEmpty(path) | |
| && System.IO.Path.IsPathRooted(path) | |
| && System.IO.File.Exists(path); | |
| } | |
| private static bool ValidateUvBinarySafe(string path) | |
| { | |
| try | |
| { | |
| if (string.IsNullOrEmpty(path) || !System.IO.File.Exists(path)) return false; | |
| var psi = new System.Diagnostics.ProcessStartInfo | |
| { | |
| FileName = path, | |
| Arguments = "--version", | |
| UseShellExecute = false, | |
| RedirectStandardOutput = true, | |
| RedirectStandardError = true, | |
| CreateNoWindow = true | |
| }; | |
| using var p = System.Diagnostics.Process.Start(psi); | |
| if (p == null) return false; | |
| if (!p.WaitForExit(3000)) { try { p.Kill(); } catch { } return false; } | |
| if (p.ExitCode != 0) return false; | |
| string output = p.StandardOutput.ReadToEnd().Trim(); | |
| return output.StartsWith("uv "); | |
| } | |
| catch { return false; } | |
| } | |
| private static bool ValidateUvBinary(string path, bool checkVersion = false) | |
| { | |
| try | |
| { | |
| if (string.IsNullOrEmpty(path) || !System.IO.File.Exists(path)) return false; | |
| if (!checkVersion) return true; | |
| var psi = new System.Diagnostics.ProcessStartInfo | |
| { | |
| FileName = path, | |
| Arguments = "--version", | |
| UseShellExecute = false, | |
| RedirectStandardOutput = true, | |
| RedirectStandardError = true, | |
| CreateNoWindow = true | |
| }; | |
| using var p = System.Diagnostics.Process.Start(psi); | |
| if (p == null) return false; | |
| if (!p.WaitForExit(3000)) | |
| { | |
| try { p.Kill(); } catch { } | |
| return false; | |
| } | |
| if (p.ExitCode != 0) return false; | |
| string output = p.StandardOutput.ReadToEnd().Trim(); | |
| return output.StartsWith("uv "); | |
| } | |
| catch | |
| { | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs around lines 948 to
977, there are two overlapping UV validation methods (IsValidUv and
ValidateUvBinarySafe); consolidate them into a single robust method (e.g.,
ValidateUv) that checks non-empty path, rooted path, file existence, and safely
executes the binary with --version (use ProcessStartInfo with
RedirectStandardOutput/Error, timeout and safe kill) returning true only if the
output matches the expected prefix; then replace all references to IsValidUv and
ValidateUvBinarySafe with the new method and remove the duplicate method to
avoid redundancy.
| string[] possibleNames = { "UnityMCP", "unityMCP", "unity-mcp", "UnityMcpServer" }; | ||
| bool success = false; | ||
|
|
||
| foreach (string serverName in possibleNames) | ||
| { | ||
| if (ExecPath.TryRun(claudePath, $"mcp remove {serverName}", projectDir, out var stdout, out var stderr, 10000, pathPrepend)) | ||
| { | ||
| // Force refresh the configuration status | ||
| var claudeClient = mcpClients.clients.FirstOrDefault(c => c.mcpType == McpTypes.ClaudeCode); | ||
| if (claudeClient != null) | ||
| { | ||
| CheckClaudeCodeConfiguration(claudeClient); | ||
| } | ||
| Repaint(); | ||
| UnityEngine.Debug.Log("UnityMCP server successfully registered from Claude Code."); | ||
|
|
||
|
|
||
| success = true; | ||
| UnityEngine.Debug.Log($"Successfully removed MCP server: {serverName}"); | ||
| break; | ||
| } | ||
| else if (!string.IsNullOrEmpty(errors)) | ||
| else if (!string.IsNullOrEmpty(stderr) && | ||
| !stderr.Contains("No MCP server found", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| UnityEngine.Debug.LogWarning($"Claude MCP errors: {errors}"); | ||
| // If it's not a "not found" error, log it and stop trying | ||
| UnityEngine.Debug.LogWarning($"Error removing {serverName}: {stderr}"); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant iteration over server names
The code first checks for existing servers using claude mcp get, then attempts to remove them. The second loop (lines 1689-1704) iterates through the same names unnecessarily. Consider using the existingNames list directly.
-// Try different possible server names
-string[] possibleNames = { "UnityMCP", "unityMCP", "unity-mcp", "UnityMcpServer" };
bool success = false;
-foreach (string serverName in possibleNames)
+foreach (string serverName in existingNames)
{
if (ExecPath.TryRun(claudePath, $"mcp remove {serverName}", projectDir, out var stdout, out var stderr, 10000, pathPrepend))
{
success = true;
UnityEngine.Debug.Log($"Successfully removed MCP server: {serverName}");
break;
}
else if (!string.IsNullOrEmpty(stderr) &&
!stderr.Contains("No MCP server found", StringComparison.OrdinalIgnoreCase))
{
// If it's not a "not found" error, log it and stop trying
UnityEngine.Debug.LogWarning($"Error removing {serverName}: {stderr}");
break;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| string[] possibleNames = { "UnityMCP", "unityMCP", "unity-mcp", "UnityMcpServer" }; | |
| bool success = false; | |
| foreach (string serverName in possibleNames) | |
| { | |
| if (ExecPath.TryRun(claudePath, $"mcp remove {serverName}", projectDir, out var stdout, out var stderr, 10000, pathPrepend)) | |
| { | |
| // Force refresh the configuration status | |
| var claudeClient = mcpClients.clients.FirstOrDefault(c => c.mcpType == McpTypes.ClaudeCode); | |
| if (claudeClient != null) | |
| { | |
| CheckClaudeCodeConfiguration(claudeClient); | |
| } | |
| Repaint(); | |
| UnityEngine.Debug.Log("UnityMCP server successfully registered from Claude Code."); | |
| success = true; | |
| UnityEngine.Debug.Log($"Successfully removed MCP server: {serverName}"); | |
| break; | |
| } | |
| else if (!string.IsNullOrEmpty(errors)) | |
| else if (!string.IsNullOrEmpty(stderr) && | |
| !stderr.Contains("No MCP server found", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| UnityEngine.Debug.LogWarning($"Claude MCP errors: {errors}"); | |
| // If it's not a "not found" error, log it and stop trying | |
| UnityEngine.Debug.LogWarning($"Error removing {serverName}: {stderr}"); | |
| break; | |
| } | |
| } | |
| bool success = false; | |
| foreach (string serverName in existingNames) | |
| { | |
| if (ExecPath.TryRun(claudePath, $"mcp remove {serverName}", projectDir, out var stdout, out var stderr, 10000, pathPrepend)) | |
| { | |
| success = true; | |
| UnityEngine.Debug.Log($"Successfully removed MCP server: {serverName}"); | |
| break; | |
| } | |
| else if (!string.IsNullOrEmpty(stderr) && | |
| !stderr.Contains("No MCP server found", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| // If it's not a "not found" error, log it and stop trying | |
| UnityEngine.Debug.LogWarning($"Error removing {serverName}: {stderr}"); | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs around lines 1686 to
1704, the removal loop currently iterates over a hard-coded possibleNames array
even though you previously collected existingNames; replace possibleNames with
the existingNames collection and iterate that directly, preserving the same
ExecPath.TryRun call, success flag setting, logging on success, and the stderr
handling that logs non-"No MCP server found" errors and breaks; ensure you keep
the break on success and on unexpected stderr and remove the redundant
possibleNames variable.
| # Use centralized retry helper | ||
| resp = send_command_with_retry("execute_menu_item", params_dict) | ||
| return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid blocking in an async tool: use async_send_command_with_retry or make the tool sync
execute_menu_item is async but invokes a blocking helper, which can block the event loop. Prefer the async helper, or convert the tool to sync for consistency with other tools.
Option A (preferred if available):
-from unity_connection import get_unity_connection, send_command_with_retry # Import retry helper
+from unity_connection import async_send_command_with_retry # Import async retry helper
- resp = send_command_with_retry("execute_menu_item", params_dict)
+ resp = await async_send_command_with_retry("execute_menu_item", params_dict)
return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}Verify the async helper exists:
If not present, fallback:
# inside the function
import asyncio
resp = await asyncio.to_thread(send_command_with_retry, "execute_menu_item", params_dict)🏁 Script executed:
#!/bin/bash
rg -nP -C2 'def\s+async_send_command_with_retry\s*\(' UnityMcpBridge/UnityMcpServer~/Length of output: 648
Switch to the async retry helper to prevent blocking the event loop
The async_send_command_with_retry helper is already defined in unity_connection.py, so we should use it instead of the blocking send_command_with_retry.
• Replace the import and call in src/tools/execute_menu_item.py:
-from unity_connection import get_unity_connection, send_command_with_retry
+from unity_connection import async_send_command_with_retry
@@
- resp = send_command_with_retry("execute_menu_item", params_dict)
+ resp = await async_send_command_with_retry("execute_menu_item", params_dict)
return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Use centralized retry helper | |
| resp = send_command_with_retry("execute_menu_item", params_dict) | |
| return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} | |
| from unity_connection import async_send_command_with_retry | |
| # Use centralized retry helper | |
| resp = await async_send_command_with_retry("execute_menu_item", params_dict) | |
| return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} |
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py around lines 47
to 49, replace the blocking send_command_with_retry usage with the async helper:
import and call async_send_command_with_retry from unity_connection.py, make the
surrounding function async (add async def) and await
async_send_command_with_retry("execute_menu_item", params_dict), and preserve
the return value shape by returning the awaited result if it's a dict or
{"success": False, "message": str(result)}; also update any callers to await
this function or propagate async changes.
3 participants
Just want code rabbit review not actually ready
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores