Protocol framing #5
Conversation
…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
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughImplements strict framed TCP transport and handshake; adds transactional, preconditioned and AST-backed C# script edits with validation and atomic writes; exposes resource list/read/search and lifecycle/retry logic in the Python MCP server; adds Python edit tools and tests; small editor config and model additions. Changes
Sequence Diagram(s)sequenceDiagram
participant IDE as MCP Client (LLM / IDE)
participant PySrv as Python MCP Server
participant Conn as UnityConnection
participant UBridge as Unity Editor Bridge
IDE->>PySrv: Tool call (apply_text_edits / create / validate / list/read)
PySrv->>Conn: send_command_with_retry(manage_script, params)
Note over Conn: FRAMING=1 handshake (establish once)
Conn->>UBridge: [Framed] 8B len + JSON command
UBridge->>UBridge: Handle action (apply/validate/create/read/delete/edit)
UBridge-->>Conn: [Framed] 8B len + JSON response
Conn-->>PySrv: Response (with retry/backoff/reload hints)
PySrv-->>IDE: Tool result
sequenceDiagram
participant Tool as script_apply_edits (Python tool)
participant PySrv as MCP Server
participant UBridge as Unity Bridge
alt Structured AST edits
Tool->>PySrv: manage_script action "edit" (structured ops)
PySrv->>UBridge: Forward structured edit
UBridge-->>PySrv: Result (atomic/validated)
PySrv-->>Tool: Return result
else Text edits local path
Tool->>PySrv: manage_script action "read"
PySrv->>UBridge: Read request -> returns contents + sha256
Tool->>Tool: Apply local edits (prepend/append/replace/regex)
Tool->>PySrv: manage_script action "update" (contentsEncoded)
PySrv->>UBridge: Update request -> atomic write (tmp → final)
UBridge-->>PySrv: Write result
PySrv-->>Tool: Return result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ 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: 3
🧹 Nitpick comments (32)
UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json (1)
2-3: Consider pinning Python version and scoping execution environmentsPinning the Python version and scoping the root helps avoid environment drift across dev machines and CI.
Apply this diff to enrich the config:
{ "typeCheckingMode": "basic", - "reportMissingImports": "none" + "reportMissingImports": "none", + "pythonVersion": "3.11", + "executionEnvironments": [ + { + "root": ".", + "pythonVersion": "3.11" + } + ] }test_unity_socket_framing.py (2)
74-81: Add a sanity limit on framed response length to prevent OOMIf the server misbehaves (or an attacker sits in the middle), blindly allocating resp_len bytes can exhaust memory.
Apply this diff to cap the maximum accepted frame size (example: 128 MB) and guard against negative/zero values:
if framing: header = struct.pack(">Q", len(body_bytes)) s.sendall(header + body_bytes) resp_len = struct.unpack(">Q", recv_exact(s, 8))[0] print(f"Response framed length: {resp_len}") - resp = recv_exact(s, resp_len) + MAX_RESP = 128 * 1024 * 1024 + if resp_len <= 0 or resp_len > MAX_RESP: + raise RuntimeError(f"invalid framed length: {resp_len} (max {MAX_RESP})") + resp = recv_exact(s, resp_len)
30-46: recv_legacy_json may accept concatenated JSON; tighten detection optionallyCurrent detection treats any buffer that decodes as JSON as complete, which can accept concatenated messages. For a diagnostic script it's acceptable, but if you want stricter behavior, parse incrementally or detect a single top-level JSON object/array.
I can provide a stricter incremental JSON parser if you want to harden this.
UnityMcpBridge/Editor/Tools/ManageScript.cs (5)
391-425: Update path is robust, but now only used via compatibility aliasIf we keep update deprecated, either:
- keep it as a runtime alias (as suggested above), or
- remove this method to reduce dead code.
Your call; just avoid drift.
706-994: Large structured EditScript implementation is now unreachable from HandleCommandGiven the current deprecation branch, nothing calls this method. Either wire it behind the "edit" compatibility alias (see earlier diff) or remove it to avoid complicating maintenance and code size.
1379-1385: Use word boundaries for 'class ' token search to avoid false positivesA plain IndexOf("class Foo") can hit comments/identifiers like "class Food". Prefer a regex with word boundaries.
Apply this diff:
- private static int IndexOfClassToken(string s, string className) - { - // simple token search; could be tightened with Regex for word boundaries - var pattern = "class " + className; - return s.IndexOf(pattern, StringComparison.Ordinal); - } + private static int IndexOfClassToken(string s, string className) + { + var m = Regex.Match(s, $@"\bclass\s+{Regex.Escape(className)}\b"); + return m.Success ? m.Index : -1; + }
1699-1706: Standard-level Roslyn handling is surprising; consider failing on Roslyn errors consistentlyReturning
level != ValidationLevel.Standardon Roslyn failure makes Standard effectively ignore Roslyn errors. If Roslyn is enabled, consider honoring its errors at Standard too, or gate Roslyn entirely behind higher levels.I can propose a clearer policy (e.g., only run Roslyn at Standard+ and fail on syntax errors across all levels that run it).
2077-2113: Place RefreshDebounce/ManageScriptRefreshHelpers inside the same namespaceThese helper classes live in the global namespace. Nest them under UnityMcpBridge.Editor.Tools to avoid namespace leaks.
Apply this diff to wrap them:
-// Debounced refresh/compile scheduler to coalesce bursts of edits -static class RefreshDebounce +namespace UnityMcpBridge.Editor.Tools +{ +// Debounced refresh/compile scheduler to coalesce bursts of edits +static class RefreshDebounce { @@ -} - -static class ManageScriptRefreshHelpers +} + +static class ManageScriptRefreshHelpers { @@ -} +} +}UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
74-78: Remove unused connection variable; rely on retry helperYou fetch the connection but never use it.
Apply this diff:
- # 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)UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
11-24: Revisit tool interop given ManageScript deprecationsSince manage_script now deprecates read/update/edit, ensure manage_script_edits routes to apply_text_edits/resources APIs. Otherwise, calls registered here will fail at runtime.
I can scan manage_script_edits.py and related tools for legacy action use and propose a follow-up PR to align. Do you want me to do that?
tests/test_script_tools.py (2)
5-5: Remove unused import (Ruff F401)
pytestis not used in this module. Removing it will satisfy Ruff and reduce noise.-import pytest
28-32: Cast Path to str when building import spec for broader Python compatibility
importlib.util.spec_from_file_locationexpects a string location. Passing a Path generally works, but casting tostris more portable across environments.-def load_module(path, name): - spec = importlib.util.spec_from_file_location(name, path) +def load_module(path, name): + spec = importlib.util.spec_from_file_location(name, str(path)) module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) return moduleUnityMcpBridge/Editor/UnityMcpBridge.cs (2)
400-421: Gate verbose connection/handshake logs behind the debug preferenceThese Debug.Log entries will print for every client even when verbose logging is off, which can spam the Console. You already have IsDebugEnabled(); use it here for consistency with the rest of the bridge.
- try - { - var ep = client.Client?.RemoteEndPoint?.ToString() ?? "unknown"; - Debug.Log($"<b><color=#2EA3FF>UNITY-MCP</color></b>: Client connected {ep}"); - } - catch { } + try + { + if (IsDebugEnabled()) + { + var ep = client.Client?.RemoteEndPoint?.ToString() ?? "unknown"; + Debug.Log($"<b><color=#2EA3FF>UNITY-MCP</color></b>: Client connected {ep}"); + } + } + catch { } // Strict framing: always require FRAMING=1 and frame all I/O try { client.NoDelay = true; } catch { } try { string handshake = "WELCOME UNITY-MCP 1 FRAMING=1\n"; byte[] handshakeBytes = System.Text.Encoding.ASCII.GetBytes(handshake); await stream.WriteAsync(handshakeBytes, 0, handshakeBytes.Length); } catch { /* ignore */ } - Debug.Log("<b><color=#2EA3FF>UNITY-MCP</color></b>: Sent handshake FRAMING=1 (strict)"); + if (IsDebugEnabled()) + { + Debug.Log("<b><color=#2EA3FF>UNITY-MCP</color></b>: Sent handshake FRAMING=1 (strict)"); + }
445-451: Gate per-message “recv” logs behind debug flagPer-message previews are very helpful, but should be opt-in to avoid noisy consoles by default.
- try - { - var preview = commandText.Length > 120 ? commandText.Substring(0, 120) + "…" : commandText; - Debug.Log($"<b><color=#2EA3FF>UNITY-MCP</color></b>: recv {(usedFraming ? "framed" : "legacy")}: {preview}"); - } - catch { } + if (IsDebugEnabled()) + { + try + { + var preview = commandText.Length > 120 ? commandText.Substring(0, 120) + "…" : commandText; + Debug.Log($"<b><color=#2EA3FF>UNITY-MCP</color></b>: recv {(usedFraming ? "framed" : "legacy")}: {preview}"); + } + catch { } + }UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
3-6: Remove unused imports (Ruff F401)These imports are currently unused and trigger linter warnings.
-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
121-124: Clarify the ‘update’ deprecation note to avoid confusion with internal flowsmanage_script_edits still uses the “update” action for local-patch flows. The hard “Deprecated” message can confuse users if they observe internal usage.
Suggest softening the guidance to discourage direct tool use while acknowledging internal usage.
- if action == 'update': - return {"success": False, "message": "Deprecated: use apply_text_edits or resources/read + small edits."} + if action == 'update': + return {"success": False, "message": "Direct 'update' is discouraged; prefer apply_text_edits or resources/read + small edits. (Internal flows may still use 'update'.)"}UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
132-145: Optionally support precondition SHA to reduce race conditions on full-file updatesWhen falling back to the “update” path, consider accepting an optional precondition SHA (e.g., via options["precondition_sha256"]) and forwarding it to Unity. This helps avoid overwriting concurrent changes.
- params: Dict[str, Any] = { + params: Dict[str, Any] = { "action": "update", "name": name, "path": path, "namespace": namespace, "scriptType": script_type, "encodedContents": base64.b64encode(new_contents.encode("utf-8")).decode("ascii"), "contentsEncoded": True, } - if options is not None: - params["options"] = options + if options is not None: + params["options"] = options + # Pass through edit precondition if provided by caller + if isinstance(options, dict) and options.get("precondition_sha256"): + params["precondition_sha256"] = options["precondition_sha256"]UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (9)
43-55: Make handshake tolerant to partial greets (optional)A single recv(256) with a 1s timeout can miss a split/partial greeting on slower starts. Consider reading until either 'FRAMING=1' is found, a newline/terminator, or the timeout expires.
Example approach (pseudocode):
- Loop recv into a small buffer while time.monotonic() < deadline
- Break early when 'FRAMING=1' in accumulated bytes
91-96: Preserve original exception context when re-raisingNew exceptions raised inside except blocks lose the original cause. Chain them using raise ... from e for better diagnostics (Ruff B904).
Apply this diff:
- except socket.timeout: - logger.warning("Socket timeout during framed receive") - raise Exception("Timeout receiving Unity response") - except Exception as e: + except socket.timeout as e: + logger.warning("Socket timeout during framed receive") + raise Exception("Timeout receiving Unity response") from e + except Exception as e: logger.error(f"Error during framed receive: {str(e)}") raiseAnd similarly for the legacy path:
- except socket.timeout: - logger.warning("Socket timeout during receive") - raise Exception("Timeout receiving Unity response") + except socket.timeout as e: + logger.warning("Socket timeout during receive") + raise Exception("Timeout receiving Unity response") from eAlso applies to: 145-147
110-113: Avoid brittle JSON pre-processing and handle decode boundariesTwo issues here:
- Decoding the ever-growing prefix can fail with UnicodeDecodeError if the last multi-byte sequence is incomplete.
- The content unescape hack mutates JSON (“content” field) and risks corrupting valid payloads.
Recommend: catch UnicodeDecodeError and continue reading; remove the quote-unescape block and rely on json.loads to handle escapes.
Apply this diff:
- data = b''.join(chunks) - decoded_data = data.decode('utf-8') + data = b''.join(chunks) + try: + decoded_data = data.decode('utf-8') + except UnicodeDecodeError: + # Incomplete multibyte sequence; keep reading + continue @@ - # Handle escaped quotes in the content - if '"content":' in decoded_data: - # Find the content field and its value - content_start = decoded_data.find('"content":') + 9 - content_end = decoded_data.rfind('"', content_start) - if content_end > content_start: - # Replace escaped quotes in content with regular quotes - content = decoded_data[content_start:content_end] - content = content.replace('\\"', '"') - decoded_data = decoded_data[:content_start] + content + decoded_data[content_end:] + # Do not mutate JSON; allow the parser to handle escaped content correctlyAlso applies to: 121-130
98-136: Consider lowering log level for per-response INFO messagesBoth framed and legacy paths log at INFO for each successful receive. This can get noisy under load. Suggest demoting to DEBUG.
Example:
- logger.info(f"Received framed response ({len(payload)} bytes)") + logger.debug(f"Received framed response ({len(payload)} bytes)") ... - logger.info(f"Received complete response ({len(data)} bytes)") + logger.debug(f"Received complete response ({len(data)} bytes)")
192-195: Flatten nested connectivity check (style)Simplify the nested if into a single condition (Ruff SIM102).
Apply this diff:
- if not self.sock: - if not self.connect(): - raise Exception("Could not connect to Unity") + if not self.sock and not self.connect(): + raise Exception("Could not connect to Unity")
204-207: Use contextlib.suppress for best-effort debug loggingReplace try/except/pass with contextlib.suppress(Exception) (Ruff SIM105).
Apply this diff:
- 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')}")Add this import near the top of the file:
import contextlibAlso applies to: 220-223
225-227: Restore the previously saved socket timeout instead of hardcodingYou save the prior timeout in last_short_timeout but restore to config.connection_timeout, ignoring the saved value.
Apply this diff:
- if last_short_timeout is not None: - self.sock.settimeout(config.connection_timeout) - last_short_timeout = None + if last_short_timeout is not None: + self.sock.settimeout(last_short_timeout) + last_short_timeout = None
260-284: Use the computed base_backoff; currently unusedbackoff = base_backoff * (2 ** attempt) is computed but not applied. Sleep currently uses only jitter * 2**attempt, ignoring base_backoff. Use backoff to respect configuration.
Apply this diff:
- # Base exponential backoff - backoff = base_backoff * (2 ** attempt) + # Base exponential backoff (scaled by configured base_backoff) + backoff = base_backoff * (2 ** attempt) @@ - sleep_s = min(cap, jitter * (2 ** attempt)) + sleep_s = min(cap, jitter * backoff)
204-207: Minimize payload/response content leakage in logs (optional)Even at DEBUG, logging payload heads can inadvertently expose user content. Consider logging sizes and mode only, or a shorter head (e.g., 8 bytes) and redact strings.
Example:
-logger.debug(f"send {len(payload)} bytes; mode={...}; head={(payload[:32]).decode('utf-8','ignore')}") +logger.debug(f"send {len(payload)} bytes; mode={...}")Also applies to: 220-223
UnityMcpBridge/UnityMcpServer~/src/server.py (6)
5-5: Remove unused imports (dataclass, List)These imports are unused and can be removed (Ruff F401).
Apply this diff:
-from dataclasses import dataclass ... -from typing import AsyncIterator, Dict, Any, List +from typing import AsyncIterator, Dict, AnyAlso applies to: 7-7
17-17: Remove unused variablehandlershandlers = [stderr_handler] is never used.
Apply this diff:
-handlers = [stderr_handler]
32-36: Specify UTF-8 encoding for the rotating log fileAvoid platform-default encoding surprises by setting encoding="utf-8".
Apply this diff:
- file_handler = RotatingFileHandler(str(log_dir / "server.log"), maxBytes=5*1024*1024, backupCount=3) + file_handler = RotatingFileHandler( + str(log_dir / "server.log"), + maxBytes=5*1024*1024, + backupCount=3, + encoding="utf-8", + )
96-100: Capabilities marker class is fine, but consider a named classUsing class _ works, but a named marker (e.g., Capabilities) improves readability for future contributors.
No code change strictly required.
104-108: Remove or implement_unity_assets_root(dead code)This helper returns None and isn’t used. Either remove it or implement it to centralize asset root handling.
Proposed removal:
-def _unity_assets_root() -> str: - # Heuristic: from the Unity project root (one level up from Library/ProjectSettings), 'Assets' - # Here, assume server runs from repo; let clients pass absolute paths under project too. - return None
109-120: _safe_path is pragmatic; consider pathlib for more robust URI handling (optional)Current slicing works for known prefixes. If URIs diversify, pathlib/urllib.parse would improve correctness across platforms.
No change required now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
UnityMcpBridge/Editor/Tools/ManageScript.cs(9 hunks)UnityMcpBridge/Editor/UnityMcpBridge.cs(5 hunks)UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py(7 hunks)test_unity_socket_framing.py(1 hunks)tests/test_script_tools.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
tests/test_script_tools.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (2)
register_manage_asset_tools(12-79)manage_asset(16-79)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
register_manage_script_edits_tools(76-145)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(324-343)tests/test_script_tools.py (1)
tool(42-46)
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
UnityMcpBridge/Editor/Models/McpClient.cs (1)
SetStatus(32-44)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(291-307)send_command_with_retry(324-343)tests/test_script_tools.py (1)
tool(42-46)
UnityMcpBridge/UnityMcpServer~/src/server.py (2)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
register_all_tools(11-24)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(291-307)UnityConnection(22-286)
🪛 Ruff (0.12.2)
tests/test_script_tools.py
5-5: pytest imported but unused
Remove unused import: pytest
(F401)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
3-3: unity_connection.get_unity_connection imported but unused
Remove unused import: unity_connection.get_unity_connection
(F401)
4-4: config.config imported but unused
Remove unused import: config.config
(F401)
5-5: time imported but unused
Remove unused import: time
(F401)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py
93-93: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
192-193: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
204-207: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
220-223: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
UnityMcpBridge/UnityMcpServer~/src/server.py
5-5: dataclasses.dataclass imported but unused
Remove unused import: dataclasses.dataclass
(F401)
7-7: typing.List imported but unused
Remove unused import: typing.List
(F401)
🔇 Additional comments (23)
UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json (1)
1-4: Pyright config looks reasonable for lightweight checksBasic mode with missing-imports disabled is fine for editor-time linting without noise.
UnityMcpBridge/Editor/Tools/ManageScript.cs (5)
54-101: Good hardening: canonicalize and fence paths under Assets, reject reparse pointsPrevents traversal and basic symlink escape. Nice addition.
151-161: Path resolution and relative URI construction look correctUsing the hardened dir plus explicit relative URI prevents leaking absolute paths.
292-319: Atomic create with tmp + move/copy fallback is solid; delayed refresh is a nice touchWriting first, replying, then scheduling refresh avoids blocking the caller on domain reloads.
432-569: apply_text_edits: transactional application + guards look goodNon-overlapping spans, payload cap, delimiter balance, Roslyn parse (when enabled), atomic write, and SHA are all well thought out.
600-667: Delimiter-balancing pass is pragmatic and comment/quote-awareThis is a useful fast-fail before Roslyn. Nice.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (2)
79-79: Return normalization is goodWrapping non-dict results keeps the tool contract consistent.
8-9: Absolute imports from unity_connection are consistent across the codebase—no change neededAll tool modules (server.py, manage_* scripts) already use
from unity_connection import …and the file at
UnityMcpBridge/UnityMcpServer~/src/unity_connection.pyconfirms this layout. As long assrc/is onPYTHONPATHor the package is installed so thatunity_connectionis discoverable (which the existing imports rely on), the change is safe.UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (2)
1-1: Register edits tool first: good for discoverabilityPrioritizing the surgical edits tool aligns with the new text-edit-first workflow.
13-16: Good: remove stdout prints; MCP transports consume stdoutThe comments are a helpful reminder; avoid accidental protocol breakage.
UnityMcpBridge/Editor/UnityMcpBridge.cs (2)
38-40: Reasonable framing defaults64 MiB cap and a 30s per-read timeout are sane for editor tooling and prevent abuse or stalls.
502-569: Helpers look solidExact-read helpers and big-endian (un)packing are correct and robust, including timeout handling.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
13-24: URI splitter is correct and path-normalizingCovers unity://path/, file://, and plain paths, normalizes separators, and returns (name, directory). Good.
25-44: apply_text_edits aligns with new Unity action and supports preconditionsParameters and None-filtering look right; returns are normalized. LGTM.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
8-73: Local edit engine is well-structured with clear operations and bounds checksOperations, newline handling, and error messages read cleanly. This is a solid foundation.
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (4)
43-55: Strict FRAMING handshake looks solidRequiring FRAMING=1 on connect and resetting the timeout afterward is clear and defensive. Good guardrail to prevent silent legacy-mode mismatches.
291-307: Connection lifecycle accessor looks goodThe new get_unity_connection avoids per-call pings and clearly raises on startup failure. Good balance of resilience and transparency.
324-343: Retry helper is clear and non-blocking for clientsPolite retry loop with configurable caps and structured pass-through looks good.
346-359: Async wrapper is safe for sync coreLocal asyncio import and structured error dict on failure is a good pattern.
UnityMcpBridge/UnityMcpServer~/src/server.py (4)
13-40: Robust logging setup that stays off stdoutStderr + rotating file handler with propagation disabled prevents MCP stdio interference. Good defensive logging posture for stdio transports.
45-65: Lifespan management is cleanStartup connect, yielding bridge in context, and orderly shutdown are well-structured and resilient to startup failures.
121-138: Resource listing is conservative and safeScans Assets for .cs and emits unity://path URIs; reasonable default scope. Normalizes separators for cross-platform consistency. LGTM.
139-151: Graceful resource read with checksum is goodReturns text/plain with sha256 metadata and handles errors without raising. Solid UX for clients.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
189-195: Deprecated actions routed for compatibility (as requested previously)read/update now warn and forward to real handlers, preserving backward compatibility for existing callers.
576-609: CRLF line/col indexing fix implemented as requestedThe line/col translation now treats CRLF as a single newline; this resolves the off-by-one in Windows files.
🧹 Nitpick comments (6)
test_unity_socket_framing.py (2)
30-46: Legacy JSON receive: avoid O(n^2) joins and consider explicit timeout handlingThe current approach rebuilds the entire buffer each iteration, which becomes quadratic for large responses. Also, socket.timeout will bubble up without a clear message.
Consider:
- Maintain a single bytearray buffer and only convert to bytes when validating or returning.
- Catch socket.timeout to raise a clearer error.
Apply this diff to reduce copying and improve error messaging:
-def recv_legacy_json(sock, timeout=60): - sock.settimeout(timeout) - chunks = [] - while True: - chunk = sock.recv(65536) - if not chunk: - data = b"".join(chunks) - if not data: - raise RuntimeError("no data, socket closed") - return data - chunks.append(chunk) - data = b"".join(chunks) - if data.strip() == b"ping": - return data - if is_valid_json(data): - return data +def recv_legacy_json(sock, timeout=60): + sock.settimeout(timeout) + buf = bytearray() + try: + while True: + chunk = sock.recv(65536) + if not chunk: + if not buf: + raise RuntimeError("no data, socket closed") + return bytes(buf) + buf += chunk + # Fast path checks without repeated re-joins + if bytes(buf).strip() == b"ping": + return bytes(buf) + if is_valid_json(bytes(buf)): + return bytes(buf) + except socket.timeout as e: + raise RuntimeError(f"legacy receive timed out after {timeout}s with {len(buf)} bytes buffered") from e
74-87: Framed path: add guardrail for request size symmetry with response capYou cap responses at 128 MiB but don’t cap requests. If the server enforces a lower max-frame (common), large payloads will be rejected. Consider mirroring a client-side MAX_REQ to fail-fast before sending.
Example:
- header = struct.pack(">Q", len(body_bytes)) + MAX_REQ = 128 * 1024 * 1024 + if len(body_bytes) > MAX_REQ: + raise RuntimeError(f"request too large: {len(body_bytes)} (max {MAX_REQ})") + header = struct.pack(">Q", len(body_bytes))UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
398-430: Update: transactional replace with backup and deferred refreshGood use of File.Replace with cross-platform fallbacks, and returning prior to scheduling the refresh. Consider optionally deleting old .bak to avoid buildup, but leaving it is acceptable for recovery.
689-706: Delete: avoid immediate AssetDatabase.Refresh; schedule like other flowsDelete currently triggers a synchronous refresh before returning, unlike create/update/edit which schedule refreshes. To keep responses snappy and consistent, prefer the debounced scheduler.
Apply this diff:
- if (deleted) - { - AssetDatabase.Refresh(); - return Response.Success( - $"Script '{Path.GetFileName(relativePath)}' moved to trash successfully.", - new { deleted = true } - ); - } + if (deleted) + { + // Prefer debounced, targeted refresh to avoid blocking the reply + ManageScriptRefreshHelpers.ScheduleScriptRefresh(relativePath); + return Response.Success( + $"Script '{Path.GetFileName(relativePath)}' moved to trash successfully.", + new { deleted = true, scheduledRefresh = true, uri = $"unity://path/{relativePath}" } + ); + }
2098-2125: RefreshDebounce: throttle is immediate, not a true debounce; consider batching multiple pathsCurrent logic triggers work on the next editor tick if past the time window. It doesn’t actually wait for “quiet” and only imports one relPath. For bursts across multiple files, others may be skipped.
Consider:
- Collect pending paths in a HashSet and schedule a single delayed import after window elapses.
- Use EditorApplication.update to wait until window passes since the last Schedule call, then import all pending.
This will better coalesce bursts and ensure all touched assets are refreshed.
1520-1555: Roslyn strictness in ValidateScriptSyntax: verify intended behavior for 'standard'Returning success on Roslyn errors when level == Standard is a notable policy choice. If that’s intentional, consider documenting it in the response or including a flag indicating Roslyn errors were present but tolerated. If not intentional, invert the return so standard fails on Roslyn errors too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
UnityMcpBridge/Editor/Tools/ManageScript.cs(9 hunks)UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json(1 hunks)test_unity_socket_framing.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json
- UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs
🔇 Additional comments (10)
test_unity_socket_framing.py (3)
12-21: recv_exact: solid and safe exact-read loopThe exact-read implementation is correct and handles short reads properly. Good use of memoryview and recv_into for efficiency.
61-73: Greeting probe and framing detection: pragmatic and compatibleThe non-fatal greeting read with a short timeout and simple FRAMING=1 feature flag check looks good and keeps compatibility with legacy peers.
88-93: Nice diagnostic outputCompact summary (length and head preview) is helpful for quick validation.
UnityMcpBridge/Editor/Tools/ManageScript.cs (7)
54-101: Assets path hardening: good normalization and symlink guard
- Normalizes and constrains to Assets/ correctly, with case-insensitive check and canonical Assets-relative derivation.
- Best-effort symlink/reparse-point guard is a sensible safety check.
Looks good.
151-161: Full/relative path construction: consistent and safeThe combination of normalized fullPathDir with a sanitized Assets-relative path is correct. Good use of Replace to normalize separators.
299-324: Create: atomic write and deferred refresh are spot on
- tmp → move/replace fallback is correct and resilient across volumes.
- Returning immediately and scheduling refresh avoids blocking the reply path.
Nice.
437-574: apply_text_edits: transactional, validated, and guarded
- Precondition SHA check prevents clobbering stale files.
- Non-overlap check and back-to-front application are correct.
- Balanced-delimiter and (optional) Roslyn validation are strong safety nets.
- Response includes new SHA for the caller to chain preconditions. Good.
No blocking issues here.
1017-1026: Overlap detection helper: correct and simpleSorted by start then linear scan is sufficient here.
1099-1145: Balanced class span fallback: pragmatic and safe enoughThe balanced-brace scanner accounts for strings/chars and comments. This is a solid fallback when Roslyn is unavailable.
1147-1347: Method span computation: robust header matching and body parsingThe combination of regex header find, attribute inclusion, generic constraints handling, and expression-bodied/block-bodied detection is comprehensive. Good work.
|
@Greptile review |
…ard-compatibility-g252rq
|
@Greptile |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
UnityMcpBridge/Editor/Tools/ManageScript.cs (3)
189-195: Deprecated read/update now forward with warnings — compatibility preservedThis addresses prior breakage risks and matches previous feedback. Good call keeping a deprecation log while serving the old flows.
232-237: Deprecated structured edit forwarding — compatibility preservededit now logs a warning and forwards to EditScript. This keeps existing Python tools working.
578-611: CRLF handling in line/col → index: LGTMTreating CRLF as a single newline fixes off-by-one issues on Windows. Matches the earlier suggestion.
🧹 Nitpick comments (8)
UnityMcpBridge/Editor/Tools/ManageScript.cs (8)
54-101: Harden Assets/ path resolution against symlink escapes (walk all segments, cross-platform)Current check rejects only if the target directory itself is a reparse point; nested symlinks along the path can still escape Assets/ (and FileAttributes.ReparsePoint is Windows-centric). Walk each segment up to Assets/ and reject any symlink/reparse point encountered. Best-effort is fine.
Apply this diff to strengthen the guard:
- // Best-effort symlink guard: if directory is a reparse point/symlink, reject - try - { - var di = new DirectoryInfo(full); - if (di.Exists) - { - var attrs = di.Attributes; - if ((attrs & FileAttributes.ReparsePoint) != 0) - { - fullPathDir = null; - relPathSafe = null; - return false; - } - } - } - catch { /* best effort; proceed */ } + // Best-effort symlink guard: reject if any segment between Assets/ and the target is a reparse point/symlink + try + { + var di = new DirectoryInfo(full); + var assetsDir = new DirectoryInfo(assets); + while (di != null && !string.Equals(di.FullName.Replace('\\','/'), assetsDir.FullName.Replace('\\','/'), StringComparison.OrdinalIgnoreCase)) + { + if (di.Exists) + { + var attrs = di.Attributes; + if ((attrs & FileAttributes.ReparsePoint) != 0) + { + fullPathDir = null; + relPathSafe = null; + return false; + } + } + di = di.Parent; + } + } + catch { /* best effort; proceed */ }
203-214: Align default validate level with GUI and docs ('standard', not 'basic')The GUI defaults to "standard" (GetValidationLevelFromGUI). Defaulting this endpoint to "basic" is inconsistent and may under-validate.
Apply this diff:
- string level = @params["level"]?.ToString()?.ToLowerInvariant() ?? "basic"; + string level = @params["level"]?.ToString()?.ToLowerInvariant() ?? "standard";
400-431: Clean up backup files after replace/copy to avoid .bak accumulationFile.Replace creates/uses a backup that persists unless deleted. Same for fallback copy where the backup may or may not exist. Clean it up best-effort.
Apply this diff:
string backupPath = fullPath + ".bak"; try { File.Replace(tempPath, fullPath, backupPath); } catch (PlatformNotSupportedException) { File.Copy(tempPath, fullPath, true); try { File.Delete(tempPath); } catch { } } catch (IOException) { // Cross-volume moves can throw IOException; fallback to copy File.Copy(tempPath, fullPath, true); try { File.Delete(tempPath); } catch { } } + // Best-effort cleanup of backup created by File.Replace + try { if (File.Exists(backupPath)) File.Delete(backupPath); } catch { }
439-571: Transactional text edits: good safeguards; consider two small refinements
- The non-overlap check + reverse apply order is correct; balanced-delimiter check is a pragmatic guard. Nice.
- Suggest also cleaning up .bak after write (like in update) to avoid backup accumulation.
Apply this diff in the write block:
string backup = fullPath + ".bak"; try { File.Replace(tmp, fullPath, backup); } catch (PlatformNotSupportedException) { File.Copy(tmp, fullPath, true); try { File.Delete(tmp); } catch { } } catch (IOException) { File.Copy(tmp, fullPath, true); try { File.Delete(tmp); } catch { } } + // Best-effort cleanup of backup created by File.Replace + try { if (File.Exists(backup)) File.Delete(backup); } catch { }
521-544: Optional: avoid unconditional Roslyn formatting on editsFormatting every edit via Formatter.Format may be heavy and can cause unexpected whitespace diffs. Consider a toggle (e.g., options.format=true) or apply only when USE_ROSLYN && level >= Standard in edit mode.
1522-1529: Standard-level validation currently ignores Roslyn syntax errorsReturning true when Roslyn reports errors at Standard level can allow invalid scripts to be created/updated. Suggested: fail on Roslyn errors for Standard and above.
Apply this diff:
- if (!ValidateScriptSyntaxRoslyn(contents, level, errorList)) - { - errors = errorList.ToArray(); - return level != ValidationLevel.Standard; //TODO: Allow standard to run roslyn right now, might formalize it in the future - } + if (!ValidateScriptSyntaxRoslyn(contents, level, errorList)) + { + errors = errorList.ToArray(); + return false; // Fail on Roslyn syntax errors for Standard+ to prevent broken scripts + }
2101-2126: Debounce only imports one asset per window; coalesce multiple pathsIf several scripts change within the debounce window, only the scheduled relPath gets an explicit import. Unity may still auto-detect others, but we can deterministically import all changed paths without spamming.
Apply this diff to accumulate and import all pending paths per tick:
-static class RefreshDebounce -{ - private static int _pending; - private static DateTime _last; - - public static void Schedule(string relPath, TimeSpan window) - { - Interlocked.Exchange(ref _pending, 1); - var now = DateTime.UtcNow; - if ((now - _last) < window) return; - _last = now; - - EditorApplication.delayCall += () => - { - if (Interlocked.Exchange(ref _pending, 0) == 1) - { - // Prefer targeted import and script compile over full refresh - AssetDatabase.ImportAsset(relPath, ImportAssetOptions.ForceUpdate); -#if UNITY_EDITOR - UnityEditor.Compilation.CompilationPipeline.RequestScriptCompilation(); -#endif - // Fallback if needed: - // AssetDatabase.Refresh(); - } - }; - } -} +static class RefreshDebounce +{ + private static readonly object _gate = new object(); + private static readonly HashSet<string> _pending = new HashSet<string>(StringComparer.OrdinalIgnoreCase); + private static DateTime _last; + private static bool _scheduled; + + public static void Schedule(string relPath, TimeSpan window) + { + lock (_gate) + { + if (!string.IsNullOrEmpty(relPath)) + _pending.Add(relPath); + + var now = DateTime.UtcNow; + if (_scheduled && (now - _last) < window) return; + _last = now; + _scheduled = true; + + EditorApplication.delayCall += () => + { + List<string> toImport; + lock (_gate) + { + toImport = _pending.ToList(); + _pending.Clear(); + _scheduled = false; + } + + foreach (var p in toImport) + AssetDatabase.ImportAsset(p, ImportAssetOptions.ForceUpdate); +#if UNITY_EDITOR + UnityEditor.Compilation.CompilationPipeline.RequestScriptCompilation(); +#endif + // Optionally: AssetDatabase.Refresh(); // as a last resort + }; + } + } +}
691-708: Delete via MoveAssetToTrash + success payload: LGTMGood UX and safer than direct file delete. Minor nit: consider scheduling a refresh (debounced) for consistency with create/update/edit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Tools/ManageScript.cs(9 hunks)
🔇 Additional comments (2)
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
151-161: Path construction under Assets/ looks correctSafe directory resolution + normalized relative path. Name validation ensures no separators. Good.
299-326: Atomic create + deferred refresh is solidUsing tmp→final with same-volume File.Move and fallback copy, and scheduling refresh post-reply, avoids UI stalls and domain reload hazards. Returning a unity://path URI is a nice touch.
|
@greptile |
|
@Greptile |
1 similar comment
|
@Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR introduces a comprehensive overhaul of the Unity MCP (Model Context Protocol) Bridge, implementing a robust framed communication protocol and enhanced script editing capabilities. The changes center around replacing the previous JSON-delimited messaging with a length-prefixed binary protocol that ensures reliable message boundaries and prevents partial message corruption.
The core protocol enhancement implements strict 8-byte big-endian length headers for all messages, with a mandatory FRAMING=1 handshake that enforces the new protocol. This includes comprehensive error handling, retry logic with exponential backoff, connection lifecycle management, and safety limits (64MB max payload, 30s timeouts). The framing protocol addresses critical reliability issues where partial message reads could cause JSON parsing failures, particularly important for large Unity asset transfers.
On the tooling front, the PR introduces sophisticated script management capabilities including transactional text editing with SHA256 precondition checks to prevent edit conflicts, structured AST-based C# code editing using Roslyn, atomic file operations with backup mechanisms, and debounced asset refresh to optimize Unity's compilation pipeline. Security has been significantly enhanced with path traversal prevention, symlink detection, cross-volume write fallbacks, and comprehensive URI validation.
Additionally, the system now exposes a public resource API allowing AI clients to discover and read Unity project files through custom unity:// URIs, enabling context-aware assistance. The IDE configuration management has been made opt-in through an EditorPrefs setting, giving users control over automatic configuration updates while still surfacing mismatches for debugging.
The Python server component has been restructured with proper MCP stdio protocol compliance (all logging redirected to stderr/files to prevent stdout interference), comprehensive logging configuration with rotating file handlers, and improved error handling. New test infrastructure validates both the framing protocol and script editing functionality using sophisticated module stubbing techniques to avoid heavy dependencies.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
UnityMcpBridge/Editor/UnityMcpBridge.cs |
4/5 | Implements strict framed I/O protocol with length-prefixed headers, removes legacy unframed mode |
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py |
4/5 | Adds robust connection handling with framing protocol, retry/backoff logic, and structured error responses |
UnityMcpBridge/Editor/Tools/ManageScript.cs |
4/5 | Major overhaul adding transactional editing, AST-based structured edits, enhanced security, and atomic operations |
UnityMcpBridge/UnityMcpServer~/src/server.py |
4/5 | Implements resource API, comprehensive logging configuration, and MCP stdio protocol compliance |
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py |
4/5 | Adds new surgical script editing tool registration and fixes stdout interference with MCP protocol |
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs |
5/5 | Introduces opt-in IDE configuration management with EditorPrefs gating |
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py |
3/5 | New targeted script editing tool with multi-path routing and URI-based operations |
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py |
2/5 | Adds specialized tools and URI parsing but contains syntax error in response handling |
test_unity_socket_framing.py |
4/5 | New test script validating framing protocol implementation and stress-testing large payloads |
tests/test_script_tools.py |
4/5 | Comprehensive test infrastructure for script and asset management tools with dependency stubbing |
UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json |
5/5 | Standard Pyright configuration for Python type checking with Unity/MCP environment settings |
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py |
5/5 | Trivial formatting fix adding missing newline at end of file |
Confidence score: 4/5
- This PR implements significant protocol improvements with proper testing, but the breaking changes and complex interdependencies require careful deployment coordination
- Score reflects the comprehensive nature of changes with good error handling and testing, but lowered due to potential compatibility issues with existing Unity Editor instances
- Pay close attention to
manage_script.pywhich has a syntax error in response handling that would cause runtime failures
Sequence Diagram
sequenceDiagram
participant User
participant Claude/MCP_Client
participant Python_Server
participant Unity_Bridge
participant Unity_Editor
participant Asset_Database
User->>Claude/MCP_Client: "Create a new script called PlayerController"
Claude/MCP_Client->>Python_Server: manage_script(action="create", name="PlayerController", ...)
Python_Server->>Unity_Bridge: Framed JSON command over TCP
Note over Unity_Bridge: Protocol framing with length prefix
Unity_Bridge->>Unity_Bridge: Parse framed payload (8-byte header + JSON)
Unity_Bridge->>Unity_Bridge: Route to ManageScript.HandleCommand()
Unity_Bridge->>Unity_Bridge: Validate script name and path
Unity_Bridge->>Unity_Bridge: Generate default MonoBehaviour template
Unity_Bridge->>Unity_Bridge: Validate syntax with Roslyn (if available)
Unity_Bridge->>Asset_Database: Create .cs file atomically (.tmp → .cs)
Asset_Database-->>Unity_Bridge: File created
Unity_Bridge->>Unity_Bridge: Schedule debounced asset refresh
Unity_Bridge->>Python_Server: Framed JSON response {"status":"success", "result":{...}}
Note over Unity_Bridge: Async refresh happens after response
Unity_Bridge->>Asset_Database: ImportAsset() and RequestScriptCompilation()
Python_Server->>Claude/MCP_Client: {"success": True, "uri": "unity://path/Assets/Scripts/PlayerController.cs"}
Claude/MCP_Client->>User: "Script created successfully at Assets/Scripts/PlayerController.cs"
12 files reviewed, 11 comments
| { | ||
| throw new System.IO.IOException($"Invalid framed length: {payloadLen}"); | ||
| } | ||
| int payloadLenInt = checked((int)payloadLen); |
There was a problem hiding this comment.
logic: checked cast from ulong to int could throw on very large frames near the 64 MiB limit
| return response if isinstance(response, dict) else { | ||
| "success": False, | ||
| "message": str(response), | ||
| } |
There was a problem hiding this comment.
syntax: Missing return keyword - this creates a syntax error that will cause the function to return None instead of the intended dictionary
|
|
||
| # Base64 encode the contents if they exist to avoid JSON escaping issues | ||
| if contents is not None: | ||
| if action in ['create', 'update']: |
There was a problem hiding this comment.
logic: The update action is deprecated but still processes contents encoding - this creates inconsistent behavior since update operations are now blocked
| if action in ['create', 'update']: | |
| if action in ['create']: |
| # If the edits request structured class/method ops, route directly to Unity's 'edit' action | ||
| for e in edits or []: | ||
| op = (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() | ||
| if op in ("replace_class", "delete_class", "replace_method", "delete_method", "insert_method"): | ||
| params: Dict[str, Any] = { | ||
| "action": "edit", | ||
| "name": name, | ||
| "path": path, | ||
| "namespace": namespace, | ||
| "scriptType": script_type, | ||
| "edits": edits, | ||
| } | ||
| if options is not None: | ||
| params["options"] = options | ||
| resp = send_command_with_retry("manage_script", params) | ||
| return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} |
There was a problem hiding this comment.
style: The routing logic creates potential inconsistency - structured operations bypass local validation and encoding while text operations don't. Consider unifying the approach or documenting the behavioral differences.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
Outdated
Show resolved
Hide resolved
| def _safe_path(uri: str) -> str | None: | ||
| # URIs: unity://path/Assets/... or file:///absolute | ||
| if uri.startswith("unity://path/"): | ||
| p = uri[len("unity://path/"):] | ||
| return p | ||
| if uri.startswith("file://"): | ||
| return uri[len("file://"):] | ||
| # Minimal tolerance for plain Assets/... paths | ||
| if uri.startswith("Assets/"): | ||
| return uri | ||
| return None |
There was a problem hiding this comment.
style: Path validation in _safe_path() is minimal and could be vulnerable to path traversal attacks. Consider using os.path.normpath() and checking for '..' components.
| root = os.getcwd() | ||
| for base, _, files in os.walk(os.path.join(root, "Assets")): | ||
| for f in files: | ||
| if f.endswith(".cs"): | ||
| rel = os.path.relpath(os.path.join(base, f), root).replace("\\", "/") |
There was a problem hiding this comment.
style: Using os.getcwd() assumes server runs from project root. This could fail if server is launched from different directory. Consider making root path configurable.
| else: | ||
| raise ConnectionError(f'Unity MCP requires FRAMING=1, got: {text!r}') |
There was a problem hiding this comment.
logic: Breaking change: existing Unity Editor instances without framing support will fail to connect. Consider a graceful fallback or migration period.
| try: | ||
| header = self._read_exact(sock, 8) | ||
| payload_len = struct.unpack('>Q', header)[0] | ||
| if payload_len == 0 or payload_len > (64 * 1024 * 1024): |
There was a problem hiding this comment.
logic: The zero-length check prevents empty payloads, but legitimate empty responses should be allowed. Consider allowing zero-length frames.
| # Ensure connected (perform handshake each time so framing stays correct) | ||
| if not self.sock: | ||
| # During retries use short connect timeout | ||
| self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| self.sock.settimeout(1.0) | ||
| self.sock.connect((self.host, self.port)) | ||
| # restore steady-state timeout for receive | ||
| self.sock.settimeout(config.connection_timeout) | ||
| logger.info(f"Connected to Unity at {self.host}:{self.port}") | ||
| if not self.connect(): | ||
| raise Exception("Could not connect to Unity") |
There was a problem hiding this comment.
style: Performing handshake on every reconnect ensures framing stays correct but adds latency. Consider caching handshake result per connection.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
93-104: Make optional params optional in the legacy router to avoid call-time frictionmanage_script currently requires contents, script_type, and namespace even for actions that don’t need them (e.g., read/delete). Defaulting them avoids tool invocation errors and matches the newer dedicated tools’ ergonomics.
- def manage_script( + def manage_script( ctx: Context, action: str, name: str, path: str, - contents: str, - script_type: str, - namespace: str, + contents: str = "", + script_type: str | None = None, + namespace: str | None = None, ) -> Dict[str, Any]:
♻️ Duplicate comments (2)
UnityMcpBridge/Editor/UnityMcpBridge.cs (1)
519-524: Overflow guard before casting to int looks correct nowYou guard payloadLen against int.MaxValue before casting. This addresses prior overflow concerns around the 64 MiB cap.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
49-63: Bounds check now correctly allows end_line == len(lines)+1 for appendingThis fixes earlier off-by-one concerns and keeps newline handling consistent.
🧹 Nitpick comments (13)
UnityMcpBridge/Editor/UnityMcpBridge.cs (3)
427-443: Simplify unconditional framed path; remove dead code and vestigial “legacy” loggingThis block always enforces framed I/O. The if (true), usedFraming flag, and the “legacy” branch in the log are now dead weight. Also, buffer declared above (Line 422) is unused.
Suggested cleanup:
- byte[] buffer = new byte[8192]; while (isRunning) { try { - // Strict framed mode - string commandText = null; - bool usedFraming = true; - - if (true) - { - // Enforced framed mode for this connection - commandText = await ReadFrameAsUtf8Async(stream, FrameIOTimeoutMs); - } + // Strict framed mode + string commandText = await ReadFrameAsUtf8Async(stream, FrameIOTimeoutMs); try { - var preview = commandText.Length > 120 ? commandText.Substring(0, 120) + "…" : commandText; - Debug.Log($"<b><color=#2EA3FF>UNITY-MCP</color></b>: recv {(usedFraming ? "framed" : "legacy")}: {preview}"); + var preview = commandText.Length > 120 ? commandText.Substring(0, 120) + "…" : commandText; + Debug.Log($"<b><color=#2EA3FF>UNITY-MCP</color></b>: recv framed: {preview}"); } catch { }
476-498: Avoid orphaned read tasks on timeout; prefer cancellation over WhenAnyReadExactAsync times out with Task.WhenAny but never cancels the pending ReadAsync. On timeout, the read task can continue running until the stream is disposed, creating unnecessary work and potential noisy exceptions. Prefer a CancellationToken with CancelAfter (supported in modern Unity/.NET profiles) to cancel the read cleanly.
Proposed pattern:
- private static async System.Threading.Tasks.Task<byte[]> ReadExactAsync(NetworkStream stream, int count, int timeoutMs) + private static async System.Threading.Tasks.Task<byte[]> ReadExactAsync(NetworkStream stream, int count, int timeoutMs) { byte[] data = new byte[count]; int offset = 0; while (offset < count) { - var readTask = stream.ReadAsync(data, offset, count - offset); - var completed = await System.Threading.Tasks.Task.WhenAny(readTask, System.Threading.Tasks.Task.Delay(timeoutMs)); - if (completed != readTask) - { - throw new System.IO.IOException("Read timed out"); - } - int r = readTask.Result; + using var cts = new System.Threading.CancellationTokenSource(timeoutMs); + int r = await stream.ReadAsync(data, offset, count - offset, cts.Token); if (r == 0) { throw new System.IO.IOException("Connection closed before reading expected bytes"); } offset += r; } return data; }If your Unity target doesn’t support the CT overload, alternatively set Socket.ReceiveTimeout and use synchronous Read with built-in timeouts to avoid runaway async reads.
511-527: Permit zero-length frames if you foresee heartbeats/acks without payloadsCurrently, payloadLen == 0 throws. If you ever want to support empty acks/keep-alives, allow 0 and return an empty string. Note: Python side currently accepts 0-length frames; allowing them here would align both ends.
- if (payloadLen == 0UL || payloadLen > MaxFrameBytes) + if (payloadLen > MaxFrameBytes) { throw new System.IO.IOException($"Invalid framed length: {payloadLen}"); }UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (6)
43-61: Strict FRAMING=1 handshake is a breaking change; confirm rollout planThe connection now requires FRAMING=1 and fails otherwise. Given Editor also enforces FRAMING=1, this is consistent within this PR but will break older Editor plugins/clients. If backward compatibility is needed, gate the strict mode behind a config flag and fall back to legacy mode temporarily.
If strict-only is intended, consider surfacing a more actionable error to users (e.g., “Please update Unity MCP Bridge in the Editor to a version that supports framing.”).
93-108: Framed receive logic is solid; consider aligning zero-length behavior with EditorYou accept payload_len == 0, which is fine in general, but the Editor forbids zero-length frames. This asymmetry won’t break normal flows but might cause confusion if future clients rely on empty frames. Aligning both sides (either allow or disallow) would reduce surprises.
200-206: Double-check retry count semantics (off-by-one) and naming clarityattempts = max(config.max_retries, 5) combined with range(attempts + 1) yields at least 6 tries. If attempts is intended to be the total number of tries, iterate range(attempts). If it’s intended to be “retries after the first attempt,” rename for clarity.
- attempts = max(config.max_retries, 5) + attempts = max(config.max_retries, 5) ... - for attempt in range(attempts + 1): + for attempt in range(attempts):
214-225: Use contextlib.suppress for optional debug loggingThe defensive try/except around the debug decode can be simplified.
+from contextlib import suppress ... - 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 suppress(Exception): + logger.debug( + f"send {len(payload)} bytes; mode={'framed' if self.use_framing else 'legacy'}; " + f"head={(payload[:32]).decode('utf-8','ignore')}" + )
231-234: Use contextlib.suppress when best-effort closing socketsMinor readability win on the best-effort close path:
- try: - if self.sock: - self.sock.close() - finally: - self.sock = None + with contextlib.suppress(Exception): + if self.sock: + self.sock.close() + self.sock = None
269-296: Remove unused backoff variable and clarify sleep logicYou compute backoff but don’t use it. The final sleep uses jitter * 2**attempt capped by state. Either use backoff in sleep or remove it to avoid confusion.
- # 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))Optionally incorporate base_backoff:
- sleep_s = min(cap, jitter * (2 ** attempt)) + sleep_s = min(cap, base_backoff * jitter * (2 ** attempt))UnityMcpBridge/UnityMcpServer~/src/server.py (2)
3-5: Remove unused imports (dataclass, List) to keep module leanRuff flagged these as unused. Safe to drop.
-from dataclasses import dataclass ... -from typing import AsyncIterator, Dict, Any, List +from typing import AsyncIterator, Dict, Any
106-122: Path traversal guard is good; one small hardening tweakresolve + relative_to guard is correct. Consider normalizing leading slashes in raw for file:// to reduce edge cases, though relative_to already blocks escapes. Otherwise, this is solid.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
2-8: Trim unused imports (get_unity_connection, config, time) for cleanlinessThese names aren’t used in this module.
-from typing import Dict, Any, List -from unity_connection import get_unity_connection, send_command_with_retry -from config import config -import time +from typing import Dict, Any, List +from unity_connection import send_command_with_retryUnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
37-49: Clarify that anchor is a regex; consider an option for literal matchinganchor_insert uses re.search with the provided anchor, treating it as a regex. If you anticipate users passing literal strings (e.g., braces, parentheses), consider supporting an option like anchor_is_literal to wrap with re.escape.
I can add an anchor_is_literal: bool option and wire it through both local and Unity-routed paths if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
UnityMcpBridge/Editor/UnityMcpBridge.cs(5 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- UnityMcpBridge/UnityMcpServer~/src/tools/init.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
UnityMcpBridge/UnityMcpServer~/src/server.py (2)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
register_all_tools(14-27)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(302-318)UnityConnection(22-297)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(335-354)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(302-318)send_command_with_retry(335-354)tests/test_script_tools.py (1)
tool(42-46)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/server.py
5-5: dataclasses.dataclass imported but unused
Remove unused import: dataclasses.dataclass
(F401)
7-7: typing.List imported but unused
Remove unused import: typing.List
(F401)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
3-3: unity_connection.get_unity_connection imported but unused
Remove unused import: unity_connection.get_unity_connection
(F401)
4-4: config.config imported but unused
Remove unused import: config.config
(F401)
5-5: time imported but unused
Remove unused import: time
(F401)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py
104-104: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
203-204: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
215-218: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
231-234: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
UnityMcpBridge/Editor/UnityMcpBridge.cs (1)
38-40: Good hard caps and sane defaults for framing/timeoutsSetting a 64 MiB cap and a 30s per-read timeout is a solid baseline for robustness and abuse resistance.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
121-140: Deprecation handling and base64 encoding logic look consistentBlocking 'update' in favor of apply_text_edits/resources is aligned with the new model; create-only base64 encoding avoids JSON escaping issues while preserving plain contents for other actions.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
110-147: Well-factored two-path edit flow (structured vs. local text); good error surfacingThe direct “edit” path for semantic operations and the read→local-edit→update path for text ops are cleanly separated, with base64 encoding on update and clear failure messages. Looks good.
|
@greptile new review please |
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit 031f5d7), not the entire PR.
The most recent changes address feedback from previous reviews by implementing backward compatibility and UI state consistency improvements. The primary changes include:
-
Backward Compatibility Restoration: The
ManageScript.csfile now provides deprecated but functional aliases for the "read", "update", and "edit" actions instead of returning hard errors. This ensures existing tooling inmanage_script_edits.pycontinues to work while warning users about deprecation. -
UI State Consistency: The Unity MCP Editor Window now properly sets the
IncorrectPathstatus when IDE configuration mismatches are detected but auto-manage is disabled. This prevents stale "Configured" status from being displayed in the UI. -
Enhanced Validation Support: The validation system now explicitly handles all documented validation levels (basic, standard, comprehensive, strict) rather than falling back to standard for comprehensive mode.
-
Cross-Platform Newline Handling: The line/column to index conversion logic now properly handles CRLF (Windows) line endings by treating
\r\nsequences as single newlines, preventing column offset errors.
These changes maintain the robust protocol framing implementation while addressing compatibility concerns and improving user experience through better status reporting and cross-platform support.
Important Files Changed
File Changes
| Filename | Score | Overview |
|---|---|---|
| UnityMcpBridge/Editor/Tools/ManageScript.cs | 4/5 | Added backward-compatible deprecated aliases for read/update/edit actions, enhanced validation level support, and fixed CRLF newline handling |
| UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs | 5/5 | Fixed UI state consistency by properly setting IncorrectPath status when auto-manage is disabled but config mismatch detected |
Confidence score: 4/5
- These changes are safe to merge with minimal risk as they primarily restore functionality and fix UI consistency
- Score reflects successful resolution of previous review feedback with straightforward implementation
- No files require special attention as both changes are well-contained bug fixes and compatibility improvements
Sequence Diagram
sequenceDiagram
participant User
12 files reviewed, 2 comments
| "count": 1000, | ||
| "format": "detailed", | ||
| "includeStacktrace": True, | ||
| "filterText": FILL * (SIZE_MB * 1024 * 1024) |
There was a problem hiding this comment.
style: Large payload generation could cause memory issues - the FILL * (SIZE_MB * 1024 * 1024) creates strings that could be several MB in memory before JSON encoding
| if (eidx < sidx) (sidx, eidx) = (eidx, sidx); | ||
|
|
||
| spans.Add((sidx, eidx, newText)); | ||
| totalBytes += System.Text.Encoding.UTF8.GetByteCount(newText); |
There was a problem hiding this comment.
logic: Potential integer overflow when computing UTF-8 byte count for very large text replacements - consider using checked arithmetic or long
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
1525-1532: Roslyn error-handling logic is inverted — errors are ignored for all levels except 'standard'If ValidateScriptSyntaxRoslyn returns false, the method currently returns true for Basic/Comprehensive/Strict and false only for Standard. This is backwards and will let errors slip through at the stricter levels.
Apply this diff to gate Roslyn by level and fail on errors for Standard+:
-#if USE_ROSLYN - // Advanced Roslyn-based validation - if (!ValidateScriptSyntaxRoslyn(contents, level, errorList)) - { - errors = errorList.ToArray(); - return level != ValidationLevel.Standard; //TODO: Allow standard to run roslyn right now, might formalize it in the future - } -#endif +#if USE_ROSLYN + // Advanced Roslyn-based validation: only run for Standard+; fail on Roslyn errors + if (level >= ValidationLevel.Standard) + { + if (!ValidateScriptSyntaxRoslyn(contents, level, errorList)) + { + errors = errorList.ToArray(); + return false; + } + } +#endif
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
203-231: Default validate level should be 'standard' for consistency with GUI and docsThe switch correctly supports basic/standard/comprehensive/strict, but the default is set to "basic". The GUI default and broader API expectations appear to be "standard". Recommend aligning the default to avoid surprises.
Apply this diff:
- string level = @params["level"]?.ToString()?.ToLowerInvariant() ?? "basic"; + string level = @params["level"]?.ToString()?.ToLowerInvariant() ?? "standard";
🧹 Nitpick comments (5)
UnityMcpBridge/Editor/Tools/ManageScript.cs (5)
400-432: Response payload shape: include a uri for parity with create/apply_text_editsCreate and apply_text_edits return a unity:// uri; Update returns only path. Consider including uri too for client-side consistency.
Apply this diff:
- var ok = Response.Success( - $"Script '{name}.cs' updated successfully at '{relativePath}'.", - new { path = relativePath, scheduledRefresh = true } - ); + var uri = $"unity://path/{relativePath}"; + var ok = Response.Success( + $"Script '{name}.cs' updated successfully at '{relativePath}'.", + new { uri, path = relativePath, scheduledRefresh = true } + );
342-353: Optional: also return a uri from read for consistencyRead returns path and content; adding uri aligns with other endpoints and simplifies clients that prefer URIs.
Apply this diff:
- var responseData = new - { - path = relativePath, - contents = contents, - // For large files, also include base64-encoded version - encodedContents = isLarge ? EncodeBase64(contents) : null, - contentsEncoded = isLarge, - }; + var uri = $"unity://path/{relativePath}"; + var responseData = new + { + uri, + path = relativePath, + contents = contents, + encodedContents = isLarge ? EncodeBase64(contents) : null, + contentsEncoded = isLarge, + };
516-523: Balanced-delimiter guard is useful but can be noisy; consider per-change hintsCurrent error returns only one location. If you start seeing false positives on legitimate edits (e.g., intentional incomplete snippets during multi-step flows), consider an option to downgrade to a warning or return the set of recently edited line ranges to inspect.
2103-2129: Debounce imports: coalesce multiple paths in the windowOnly the first scheduled path is imported; subsequent paths within the window set _pending but aren’t imported individually. While compilation usually catches all changes, it’s safer to coalesce and import all touched assets in the window.
Apply this diff:
static class RefreshDebounce { - private static int _pending; - private static DateTime _last; + private static int _pending; + private static DateTime _last; + private static readonly object _lock = new object(); + private static readonly HashSet<string> _paths = new HashSet<string>(StringComparer.OrdinalIgnoreCase); public static void Schedule(string relPath, TimeSpan window) { - Interlocked.Exchange(ref _pending, 1); + Interlocked.Exchange(ref _pending, 1); + lock (_lock) { _paths.Add(relPath); } var now = DateTime.UtcNow; if ((now - _last) < window) return; _last = now; EditorApplication.delayCall += () => { if (Interlocked.Exchange(ref _pending, 0) == 1) { - // Prefer targeted import and script compile over full refresh - AssetDatabase.ImportAsset(relPath, ImportAssetOptions.ForceUpdate); + string[] toImport; + lock (_lock) { toImport = _paths.ToArray(); _paths.Clear(); } + foreach (var p in toImport) + AssetDatabase.ImportAsset(p, ImportAssetOptions.ForceUpdate); #if UNITY_EDITOR UnityEditor.Compilation.CompilationPipeline.RequestScriptCompilation(); #endif // Fallback if needed: // AssetDatabase.Refresh(); } }; } }
54-101: Assets/ path hardening is solid; symlink check is best-effortCanonicalization + prefix check prevents traversal out of Assets. Note FileAttributes.ReparsePoint is a best-effort symlink indicator across platforms/Unity versions. If you hit edge cases, consider adding an explicit check for DirectoryInfo.LinkTarget when available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Tools/ManageScript.cs(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
189-196: Backward-compatibility for deprecated actions is preserved — LGTMThe "read", "update", and "edit" actions now log deprecation warnings and forward to the actual implementations. This avoids breaking existing callers while nudging migration.
Also applies to: 232-236
301-326: Atomic create and deferred refresh are well-implemented — niceUsing tmp + move/copy and scheduling refresh after responding avoids blocking the caller and reduces domain-reload hazards. Good call.
439-466: Edit payload validation is tight; integer overflow risk mitigated
- Non-overlap checks and back-to-front application are correct.
- Using a long accumulator and checked arithmetic for UTF-8 byte counts addresses overflow concerns on large batches.
Also applies to: 468-490, 497-515
581-614: CRLF-aware line/col → index conversion — LGTMTreating CRLF as one newline fixes off-by-one on Windows files. This aligns the column semantics with typical editors.
Add initial transport handshake tests with plan placeholders
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/test_resources_api.py (2)
4-6: Turn skips into actionable coverage (convert to xfail and sketch fixtures).These placeholders won’t exercise the new resource endpoints. Consider making them xfail so they’re visible in CI while you implement them, and scaffold fixtures around a temp Unity-like project (Assets/) to validate traversal filtering.
Apply this diff to convert to xfail:
-@pytest.mark.skip(reason="TODO: resource.list returns only Assets/**/*.cs and rejects traversal") +@pytest.mark.xfail(strict=False, reason="resource.list should return only Assets/**/*.cs and reject traversal") def test_resource_list_filters_and_rejects_traversal(): passExample fixture outline (outside current range) for later implementation:
# conftest.py (example) import sys from pathlib import Path import pytest @pytest.fixture def unity_src_root(tmp_path, monkeypatch): # Create minimal Unity-like project layout assets = tmp_path / "Assets" assets.mkdir() (assets / "Foo.cs").write_text("class Foo {}", encoding="utf-8") # Danger paths (tmp_path / "outside.cs").write_text("class Outside {}", encoding="utf-8") # Discover and add server src to sys.path then import helpers # Reuse the SRC detection pattern from framing tests if needed. return tmp_path
9-11: Add explicit outside-path cases and Windows drive-letter coverage plan.When you implement this, include:
- file:// URIs outside the project root
- Absolute paths
- Windows drive-letter and UNC forms, and symlink/junction escapes
Gate platform-specific cases with sys.platform checks to avoid brittle failures.
I can draft parametrized tests for file://, unity://, symlink, and junction paths that assert rejection and proper error messaging. Want me to open a follow-up PR with them?
tests/test_logging_stdout.py (1)
26-37: Use AST-based detection to avoid false positives from comments/strings.Regex picks up occurrences inside strings/comments. AST walk is more robust for calls to print(...) and sys.stdout.write(...).
Add this import at top of file (outside current range):
import astThen replace the test body with:
-def test_no_print_statements_in_codebase(): - """Ensure no stray print statements remain in server source.""" - offenders = [] - for py_file in SRC.rglob("*.py"): - text = py_file.read_text(encoding="utf-8") - if re.search(r"^\s*print\(", text, re.MULTILINE) or re.search( - r"sys\.stdout\.write\(", text - ): - offenders.append(py_file.relative_to(SRC)) - assert not offenders, ( - "stdout writes found in: " + ", ".join(str(o) for o in offenders) - ) +def test_no_print_statements_in_codebase(): + """Ensure no stray print/sys.stdout writes remain in server source.""" + offenders = [] + for py_file in SRC.rglob("*.py"): + try: + text = py_file.read_text(encoding="utf-8", errors="strict") + except UnicodeDecodeError: + # Be tolerant of encoding edge cases in source tree + text = py_file.read_text(encoding="utf-8", errors="ignore") + try: + tree = ast.parse(text, filename=str(py_file)) + except SyntaxError: + offenders.append(py_file.relative_to(SRC)) + continue + + class StdoutVisitor(ast.NodeVisitor): + def __init__(self): + self.hit = False + def visit_Call(self, node: ast.Call): + # print(...) + if isinstance(node.func, ast.Name) and node.func.id == "print": + self.hit = True + # sys.stdout.write(...) + if isinstance(node.func, ast.Attribute) and node.func.attr == "write": + val = node.func.value + if isinstance(val, ast.Attribute) and val.attr == "stdout": + if isinstance(val.value, ast.Name) and val.value.id == "sys": + self.hit = True + self.generic_visit(node) + + v = StdoutVisitor() + v.visit(tree) + if v.hit: + offenders.append(py_file.relative_to(SRC)) + + assert not offenders, "stdout writes found in: " + ", ".join(str(o) for o in offenders)tests/test_script_editing.py (1)
4-6: Prioritize a minimal “happy path” and atomic write test; convert the rest to xfail.Everything is skipped, so none of the new editing logic is exercised. Implement at least:
- Create .cs file under Assets/, call validate_script/apply_text_edits, assert compile/build result (or stub it), and atomic write guarantees (temp + replace).
Convert the remaining scenarios to xfail to keep them visible and prevent silent rot.
Suggested diffs to switch decorators:
-@pytest.mark.skip(reason="TODO: create new script, validate, apply edits, build and compile scene") +@pytest.mark.xfail(strict=False, reason="pending: create new script, validate, apply edits, build and compile scene") def test_script_edit_happy_path(): pass @@ -@pytest.mark.skip(reason="TODO: multiple micro-edits debounce to single compilation") +@pytest.mark.xfail(strict=False, reason="pending: multiple micro-edits debounce to single compilation") def test_micro_edits_debounce(): pass @@ -@pytest.mark.skip(reason="TODO: line ending variations handled correctly") +@pytest.mark.xfail(strict=False, reason="pending: line ending variations handled correctly") def test_line_endings_and_columns(): pass @@ -@pytest.mark.skip(reason="TODO: regex_replace no-op with allow_noop honored") +@pytest.mark.xfail(strict=False, reason="pending: regex_replace no-op with allow_noop honored") def test_regex_replace_noop_allowed(): pass @@ -@pytest.mark.skip(reason="TODO: large edit size boundaries and overflow protection") +@pytest.mark.xfail(strict=False, reason="pending: large edit size boundaries and overflow protection") def test_large_edit_size_and_overflow(): pass @@ -@pytest.mark.skip(reason="TODO: symlink and junction protections on edits") +@pytest.mark.xfail(strict=False, reason="pending: symlink and junction protections on edits") def test_symlink_and_junction_protection(): pass @@ -@pytest.mark.skip(reason="TODO: atomic write guarantees") +@pytest.mark.xfail(strict=False, reason="pending: atomic write guarantees") def test_atomic_write_guarantees(): passIf helpful, I can wire a tmp_path-based Assets/ project, import the server tools, and provide concrete tests for atomic writes (same-dir temp + os.replace), symlink/junction rejection, and debounce behavior using time.monotonic + monkeypatch.
Also applies to: 9-11, 14-16, 19-21, 24-26, 29-31, 34-36
tests/test_transport_framing.py (3)
71-96: Reduce race in pre-handshake detection with a short select loop.Single 0.2s select can miss a just-sent pre-handshake write under load. Looping for a bounded time decreases flakes.
def start_handshake_enforcing_server(): @@ - # if client sends any data before greeting, disconnect - # give clients a bit more time to send pre-handshake data before we greet - r, _, _ = select.select([conn], [], [], 0.2) - if r: - conn.close() - sock.close() - return + # If client sends any data before greeting, disconnect (poll briefly) + deadline = time.time() + 0.5 + while time.time() < deadline: + r, _, _ = select.select([conn], [], [], 0.05) + if r: + conn.close() + sock.close() + return conn.sendall(b"MCP/0.1 FRAMING=1\n")
120-135: Minor flake risk; increase wait before recv or assert closed via select.After sending "BAD", you sleep 0.1s. On slow CI, the server might not have closed yet. Bump to 0.3–0.5s or wait with select for readability.
- time.sleep(0.1) + time.sleep(0.4)
137-160: Plan for the skipped cases; note zero-length framed payload isn’t currently rejected.When you unskip:
- Zero-length payload: UnityConnection.receive_full_response accepts 0-sized frames today and returns b""; your test should fail until the client rejects length == 0.
- Oversized payload: client already rejects >64MiB; ensure the test expects an exception and that the socket is closed by caller.
- Partial header/payload: a server sending fewer bytes should trigger a timeout in _read_exact and surface an error.
Proposed client fix (other file) to enforce zero-length rejection:
# File: UnityMcpBridge/UnityMcpServer~/src/unity_connection.py def receive_full_response(self, sock, buffer_size=config.buffer_size) -> bytes: if self.use_framing: try: header = self._read_exact(sock, 8) payload_len = struct.unpack('>Q', header)[0] + if payload_len == 0: + raise Exception("Invalid framed length: 0") if payload_len > (64 * 1024 * 1024): raise Exception(f"Invalid framed length: {payload_len}") payload = self._read_exact(sock, payload_len) logger.info(f"Received framed response ({len(payload)} bytes)") return payloadIf you want, I can add a minimal server helper here that explicitly sends a 0-length frame to validate the new behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
tests/test_logging_stdout.py(1 hunks)tests/test_resources_api.py(1 hunks)tests/test_script_editing.py(1 hunks)tests/test_transport_framing.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_transport_framing.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (3)
connect(34-70)receive_full_response(91-160)disconnect(72-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
tests/test_transport_framing.py (2)
99-105: Handshake negative path looks correct and aligns with UnityConnection.This asserts the strict FRAMING=1 requirement and validates cleanup on failure.
106-118: Ping/pong flow correctly exercises framed I/O.Good coverage of handshake, framing bit, and a minimal framed request/response using UnityConnection.receive_full_response.
…ests Convert skip decorators to xfail and harden framing tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/test_logging_stdout.py (1)
13-19: Good fix: module-level skip instead of import-time failure.Skipping the module when SRC isn't found avoids hard failures during collection and matches pytest best practices.
tests/test_transport_framing.py (1)
46-63: Nice hardening: exact-length recv prevents flakiness.Looping to read the full header/payload addresses partial recv issues noted earlier.
🧹 Nitpick comments (11)
tests/test_logging_stdout.py (2)
52-56: Flatten nested checks for sys.stdout.write detection (ruff SIM102).Reduce nesting and satisfy SIM102 by collapsing the conditions.
Apply this diff:
- # sys.stdout.write(...) - if isinstance(node.func, ast.Attribute) and node.func.attr == "write": - val = node.func.value - if isinstance(val, ast.Attribute) and val.attr == "stdout": - if isinstance(val.value, ast.Name) and val.value.id == "sys": - self.hit = True + # sys.stdout.write(...) + if ( + isinstance(node.func, ast.Attribute) + and node.func.attr == "write" + and isinstance(node.func.value, ast.Attribute) + and node.func.value.attr == "stdout" + and isinstance(node.func.value.value, ast.Name) + and node.func.value.value.id == "sys" + ): + self.hit = True
47-51: Optionally detect builtins.print as well.If someone calls builtins.print(...), it will bypass the current Name("print") check.
Apply this diff to broaden detection:
# print(...) - if isinstance(node.func, ast.Name) and node.func.id == "print": + if isinstance(node.func, ast.Name) and node.func.id == "print": self.hit = True + # builtins.print(...) + elif ( + isinstance(node.func, ast.Attribute) + and node.func.attr == "print" + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "builtins" + ): + self.hit = TrueUnityMcpBridge/UnityMcpServer~/src/unity_connection.py (7)
43-61: Strict FRAMING-only handshake: consider a gated fallback and configurable handshake timeout.This is a breaking change for legacy editors. If that's intentional, great—tests reflect it. Otherwise, add a config flag to allow (temporary) legacy mode and make the handshake timeout configurable.
Apply this diff to add both a config flag and configurable timeout:
- try: - self.sock.settimeout(1.0) + try: + require_framing = getattr(config, "require_framing", True) + self.sock.settimeout(getattr(config, "handshake_timeout", 1.0)) greeting = self.sock.recv(256) text = greeting.decode('ascii', errors='ignore') if greeting else '' if 'FRAMING=1' in text: self.use_framing = True logger.debug('Unity MCP handshake received: FRAMING=1 (strict)') else: - try: - msg = b'Unity MCP requires FRAMING=1' - header = struct.pack('>Q', len(msg)) - self.sock.sendall(header + msg) - except Exception: - pass - raise ConnectionError(f'Unity MCP requires FRAMING=1, got: {text!r}') + if require_framing: + # Best-effort advisory; peer may ignore if not framed-capable + with contextlib.suppress(Exception): + msg = b'Unity MCP requires FRAMING=1' + header = struct.pack('>Q', len(msg)) + self.sock.sendall(header + msg) + raise ConnectionError(f'Unity MCP requires FRAMING=1, got: {text!r}') + else: + self.use_framing = False + logger.warning('Unity MCP handshake missing FRAMING=1; proceeding in legacy mode by configuration')You’ll also need
import contextlibat the top.
104-107: Preserve original timeout context when re-raising.Use a specific exception and chain it to the original to aid debugging.
Apply this diff:
- except socket.timeout: - logger.warning("Socket timeout during framed receive") - raise Exception("Timeout receiving Unity response") + except socket.timeout as e: + logger.warning("Socket timeout during framed receive") + raise TimeoutError("Timeout receiving Unity response") from e
205-208: Nit: collapse nested connect check and correct the comment.The handshake runs inside connect(), not “each time” here.
Apply this diff:
- # Ensure connected (perform handshake each time so framing stays correct) - if not self.sock: - if not self.connect(): - raise Exception("Could not connect to Unity") + # Ensure connected (handshake occurs within connect()) + if not self.sock and not self.connect(): + raise Exception("Could not connect to Unity")
217-221: Use contextlib.suppress for defensive debug logging (ruff SIM105).Prefer suppress over bare try/except pass for clarity.
Apply these diffs:
- 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'}; " + f"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'}; " + f"head={(response_data[:32]).decode('utf-8','ignore')}" + )Add this import near the top:
import contextlibAlso applies to: 233-237
229-241: Restore the original socket timeout instead of forcing config value.You capture the previous timeout into last_short_timeout but don’t use it; restore it for correctness.
Apply this diff:
if attempt > 0 and last_short_timeout is None: - last_short_timeout = self.sock.gettimeout() + last_short_timeout = self.sock.gettimeout() self.sock.settimeout(1.0) response_data = self.receive_full_response(self.sock) @@ # restore steady-state timeout if changed if last_short_timeout is not None: - self.sock.settimeout(config.connection_timeout) + self.sock.settimeout(last_short_timeout) last_short_timeout = None
123-156: Legacy JSON reassembly is fragile; decoding per-chunk can raise and the content hack is unsafe.If legacy mode remains supported, consider:
- Keeping data as bytes until you know you have a full message.
- Using an incremental UTF-8 decoder to avoid UnicodeDecodeError on split multibyte sequences.
- Avoiding ad-hoc '"content"' rewrites; rely on proper framing or a robust delimiter strategy.
I can propose a minimal incremental decoder path if you plan to keep legacy mode.
164-256: Concurrency: single shared socket needs send/recv serialization.Global connection with concurrent calls risks interleaved frames and corrupted replies. If tools can call in parallel, guard send/receive with a lock.
Would you like me to add a simple IO lock around the send/receive critical section? Example approach:
In post_init:
- self._io_lock = threading.Lock()
Around send + receive_full_response in send_command():
- with self._io_lock: ... IO ...
Also import threading.
tests/test_transport_framing.py (2)
130-137: Prevent potential blocking on recv after pre-handshake send.Set a short timeout on the client socket before the recv; protects against platform differences and errant server behavior.
Apply this diff:
sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) sock.connect(("127.0.0.1", port)) + sock.settimeout(1.0) sock.sendall(b"BAD") time.sleep(0.4) try: data = sock.recv(1024)
114-124: Optional: exercise send_command('ping') path for end-to-end coverage.Current test manually frames a ping; that’s fine. If you want to cover UnityConnection.send_command as well, adjust the dummy server to accept the non-JSON ping payload (b"ping") and respond accordingly, then add an assertion via send_command('ping').
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py(7 hunks)tests/test_logging_stdout.py(1 hunks)tests/test_resources_api.py(1 hunks)tests/test_script_editing.py(1 hunks)tests/test_transport_framing.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_resources_api.py
- tests/test_script_editing.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_transport_framing.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (4)
_read_exact(82-89)connect(34-70)receive_full_response(91-162)disconnect(72-80)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
tests/test_transport_framing.py (1)
_read_exact(47-54)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py
106-106: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
205-206: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
217-220: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
233-236: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
tests/test_logging_stdout.py
54-55: Use a single if statement instead of nested if statements
(SIM102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
…-logging Add handshake fallback and logging checks
|
@greptileai -- whaddya think of latest brosef? |
…ck; add uv -q for quieter stdio; MCP server: compatibility guards for capabilities/resource decorators and indentation fix; ManageScript: shadow var fix; robust mac config path.
…text edits; anchor aliasing and text-op conversion; immediate compile on NL/structured; add resource_tools (tail_lines, find_in_file); update test cases
There was a problem hiding this comment.
Looking at this Protocol framing PR, I'm impressed with the comprehensive approach to making the Unity MCP bridge more robust and production-ready. Here's what stands out:
Strong Points
Framing Protocol Implementation - The strict handshake enforcement with "FRAMING=1" requirement is smart. It ensures both sides agree on the communication protocol upfront, preventing message boundary issues that plagued the legacy mode.
Thread Safety & Concurrency - Adding _io_lock to serialize socket operations and the improved debounced refresh system shows good understanding of Unity's editor threading model. The debouncer now properly handles multiple concurrent requests.
Robustness Improvements - The symlink protection, atomic write fallbacks across different platforms (handling PlatformNotSupportedException and cross-volume IOException), and proper cleanup of backup files demonstrates attention to edge cases.
Testing Infrastructure - The comprehensive test suite covering framing, script editing, resources, and logging shows this isn't just feature-driven development but quality-focused.
Technical Highlights
The validation level change from "basic" to "standard" as default suggests you're enabling more thorough syntax checking by default, which is good for catching issues early.
The URI support (unity://path/) is a nice touch for resource identification and will make the API more RESTful.
The debounce improvements with proper locking and path tracking will significantly reduce Unity's asset import churn during bulk operations.
Minor Concerns
The size limit checks and overflow protection are good, but I'd be curious about the 64MB limit rationale - seems generous for typical Unity scripts but appropriate for preventing abuse.
The fallback behavior when framing requirements aren't met could be clearer in the error messages to help users troubleshoot configuration issues.
Overall, this feels like a mature iteration that takes the rough edges off the initial implementation. The combination of protocol enforcement, better error handling, and comprehensive testing suggests this is ready for broader adoption. Nice work!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_logging_stdout.py (1)
13-19: Good fix: module-level skip instead of import-time failureUsing pytest.skip with allow_module_level=True cleanly handles missing source layouts and keeps CI green. This addresses the earlier feedback.
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
104-107: Zero-length frames: consider allowing them (duplicate of prior feedback)Strictly rejecting length 0 prevents representing empty payloads. If the protocol can legitimately send empty content, allow 0 and return b''.
- if payload_len == 0: - raise Exception("Invalid framed length: 0") + # Allow zero-length frames to represent empty payloads + if payload_len == 0: + logger.debug("Received empty framed response (0 bytes)") + return b""
🧹 Nitpick comments (5)
tests/test_logging_stdout.py (1)
48-58: Combine print detection branches (ruff SIM114)
You can simplify the two branches and satisfy the linter.- def visit_Call(self, node: ast.Call): - # print(...) - if isinstance(node.func, ast.Name) and node.func.id == "print": - self.hit = True - # builtins.print(...) - elif ( - isinstance(node.func, ast.Attribute) - and node.func.attr == "print" - and isinstance(node.func.value, ast.Name) - and node.func.value.id == "builtins" - ): - self.hit = True + def visit_Call(self, node: ast.Call): + # print(...) or builtins.print(...) + if ( + (isinstance(node.func, ast.Name) and node.func.id == "print") + or ( + isinstance(node.func, ast.Attribute) + and node.func.attr == "print" + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "builtins" + ) + ): + self.hit = TrueUnityMcpBridge/UnityMcpServer~/src/unity_connection.py (4)
46-67: Handshake may miss split greetings; accumulate until timeout or markerA single recv(256) risks missing FRAMING=1 if the greeting is split across packets. Accumulate up to a small cap or until FRAMING=1 is seen within the handshake timeout.
- # Strict handshake: require FRAMING=1 - try: - require_framing = getattr(config, "require_framing", True) - self.sock.settimeout(getattr(config, "handshake_timeout", 1.0)) - greeting = self.sock.recv(256) - text = greeting.decode('ascii', errors='ignore') if greeting else '' - if 'FRAMING=1' in text: - self.use_framing = True - logger.debug('Unity MCP handshake received: FRAMING=1 (strict)') - else: - if require_framing: - # Best-effort advisory; peer may ignore if not framed-capable - with contextlib.suppress(Exception): - msg = b'Unity MCP requires FRAMING=1' - header = struct.pack('>Q', len(msg)) - self.sock.sendall(header + msg) - raise ConnectionError(f'Unity MCP requires FRAMING=1, got: {text!r}') - else: - self.use_framing = False - logger.warning('Unity MCP handshake missing FRAMING=1; proceeding in legacy mode by configuration') + # Strict handshake: require FRAMING=1 + try: + require_framing = getattr(config, "require_framing", True) + timeout = getattr(config, "handshake_timeout", 1.0) + self.sock.settimeout(timeout) + buf = bytearray() + deadline = time.time() + timeout + while time.time() < deadline and len(buf) < 512: + try: + chunk = self.sock.recv(256) + if not chunk: + break + buf += chunk + if b'FRAMING=1' in buf: + break + except socket.timeout: + break + text = buf.decode('ascii', errors='ignore') + if 'FRAMING=1' in text: + self.use_framing = True + logger.debug('Unity MCP handshake received: FRAMING=1 (strict)') + else: + if require_framing: + # Best-effort advisory; peer may ignore if not framed-capable + with contextlib.suppress(Exception): + msg = b'Unity MCP requires FRAMING=1' + header = struct.pack('>Q', len(msg)) + self.sock.sendall(header + msg) + raise ConnectionError(f'Unity MCP requires FRAMING=1, got: {text!r}') + else: + self.use_framing = False + logger.warning('Unity MCP handshake missing FRAMING=1; proceeding in legacy mode by configuration') finally: self.sock.settimeout(config.connection_timeout)
42-45: Optional: disable Nagle to reduce command/response latencySetting TCP_NODELAY can help for small request/response cycles.
self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) self.sock.connect((self.host, self.port)) + with contextlib.suppress(Exception): + self.sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) logger.debug(f"Connected to Unity at {self.host}:{self.port}")
109-110: Reduce log verbosity for hot pathsINFO on every receive can overwhelm logs under load. Debug is more appropriate.
- logger.info(f"Received framed response ({len(payload)} bytes)") + logger.debug(f"Received framed response ({len(payload)} bytes)")- logger.info(f"Received complete response ({len(data)} bytes)") + logger.debug(f"Received complete response ({len(data)} bytes)")Also applies to: 155-156
285-312: Backoff math ignores base_backoff; fix jittered exponential backoffThe computed backoff variable is unused, and sleep uses only jitter * 2**attempt. Use base_backoff in sleep computation.
- # Base exponential backoff - backoff = base_backoff * (2 ** attempt) - # Decorrelated jitter multiplier - jitter = random.uniform(0.1, 0.3) + # Decorrelated jitter around an exponential backoff + jitter = random.uniform(0.8, 1.2) @@ - sleep_s = min(cap, jitter * (2 ** attempt)) + sleep_s = min(cap, base_backoff * (2 ** attempt) * jitter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py(7 hunks)tests/test_logging_stdout.py(1 hunks)tests/test_transport_framing.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_transport_framing.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (2)
PortDiscovery(24-155)discover_unity_port(93-134)tests/test_transport_framing.py (1)
_read_exact(47-54)
🪛 Ruff (0.12.2)
tests/test_logging_stdout.py
49-58: Combine if branches using logical or operator
Combine if branches
(SIM114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
tests/test_logging_stdout.py (1)
27-77: AST-based scan is robust and pragmaticScanning the AST (with tolerant decoding and SyntaxError collection) is a solid approach to enforce “no stdout” policy without regex false positives. Assertions produce actionable paths.
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
222-255: Good synchronization: serialize send/recv on shared socketUsing a per-connection lock around both send and receive avoids interleaving across concurrent callers. The head previews are also helpful for debugging.
319-334: Connection retrieval without ping is a good changeAvoiding per-call pings prevents thundering herds. send_command handles reconnects and errors anyway.
| # Strict handshake: require FRAMING=1 | ||
| try: | ||
| require_framing = getattr(config, "require_framing", True) | ||
| self.sock.settimeout(getattr(config, "handshake_timeout", 1.0)) | ||
| greeting = self.sock.recv(256) | ||
| text = greeting.decode('ascii', errors='ignore') if greeting else '' | ||
| if 'FRAMING=1' in text: | ||
| self.use_framing = True | ||
| logger.debug('Unity MCP handshake received: FRAMING=1 (strict)') | ||
| else: | ||
| if require_framing: | ||
| # Best-effort advisory; peer may ignore if not framed-capable | ||
| with contextlib.suppress(Exception): | ||
| msg = b'Unity MCP requires FRAMING=1' | ||
| header = struct.pack('>Q', len(msg)) | ||
| self.sock.sendall(header + msg) | ||
| raise ConnectionError(f'Unity MCP requires FRAMING=1, got: {text!r}') | ||
| else: | ||
| self.use_framing = False | ||
| logger.warning('Unity MCP handshake missing FRAMING=1; proceeding in legacy mode by configuration') | ||
| finally: | ||
| self.sock.settimeout(config.connection_timeout) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify PortDiscovery probe vs. strict framing handshake
With strict framing/handshake, unframed “ping” probes may fail. Ensure PortDiscovery._try_probe_unity_mcp is compatible (e.g., read greeting to detect FRAMING=1 rather than sending raw ping), or gate strict mode via config during migration.
🏁 Script executed:
#!/bin/bash
# Inspect PortDiscovery probe to ensure it doesn't rely on raw 'ping'
rg -n -C3 'def _try_probe_unity_mcp' UnityMcpBridge/UnityMcpServer~/src/port_discovery.py
echo
rg -n -C2 'sendall\(.+ping' UnityMcpBridge/UnityMcpServer~/src/port_discovery.py
echo
# Also check for greeting/FRAMING handling in the probe
rg -n -C2 'FRAMING=1|WELCOME' UnityMcpBridge/UnityMcpServer~/src/port_discovery.pyLength of output: 789
Ensure PortDiscovery._try_probe_unity_mcp supports strict FRAMING handshake
PortDiscovery’s _try_probe_unity_mcp (port_discovery.py: lines 57–67) still sends an unframed b"ping" and expects a JSON “pong”, which will be rejected when require_framing=True in unity_connection.py (handshake block at lines 46–67). To avoid probe failures, please refactor one of the following approaches:
- Perform the same initial handshake as
UnityConnection: read the greeting, detectFRAMING=1, then wrap probe payloads in the 8-byte length header viastruct.pack. - Introduce a config flag to temporarily disable strict framing for discovery probes during migration.
Affected locations:
- UnityMcpBridge/UnityMcpServer~/src/port_discovery.py:
_try_probe_unity_mcp(lines 57–67) - UnityMcpBridge/UnityMcpServer~/src/unity_connection.py: strict handshake block (lines 46–67)
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/port_discovery.py around lines 57 to 67,
the probe currently sends an unframed b"ping" and expects an unframed JSON
response which will fail against
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py's strict FRAMING=1
handshake (lines 46–67) that requires framed payloads; update
_try_probe_unity_mcp to perform the same initial handshake: read the greeting
from the socket, detect 'FRAMING=1' in the greeting, and if present wrap probe
payloads with the 8-byte big-endian length header (struct.pack('>Q',
len(payload))) and parse framed responses accordingly; alternatively, add and
check a new discovery config flag so the probe can temporarily disable strict
framing and continue using unframed pings during migration—pick one approach and
apply symmetric changes to probing send/receive logic so probes succeed when
require_framing=True.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
1647-1683: Mismatch surfaced when auto-manage is disabled — addressedIncorrectPath is now set when auto-manage is off and a mismatch is detected, preventing stale “Configured” UI states. This aligns with prior feedback.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
213-228: Structured vs text path behavior divergence remains; document or unifyStructured ops are forwarded to Unity while text ops take local/encoding paths. This behavioral split can surprise tool consumers mixing ops.
Consider:
- Explicitly documenting precedence when mixed ops are provided.
- Or splitting into two tools (semantic_edits vs text_edits) to make behavior explicit.
🧹 Nitpick comments (13)
UnityMcpBridge/Editor/Data/McpClients.cs (1)
72-78: Mac path for Claude Desktop: consider SpecialFolder consistency (nit)Using Environment.SpecialFolder.Personal resolves to $HOME, which is fine. For consistency with other entries (Linux paths here use UserProfile), consider swapping to UserProfile. Functionally equivalent in practice on macOS.
Apply within this hunk:
- macConfigPath = Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.Personal), + macConfigPath = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "Library", "Application Support", "Claude", "claude_desktop_config.json" ),UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (2)
1168-1184: Make atomic writer robust on non-Windows (File.Replace can be unsupported)On macOS/Linux (and some Mono profiles), File.Replace may throw PlatformNotSupportedException. You already wrap the outer call in a try/catch, but handling it in-method preserves atomicity better on Windows and provides a best-effort overwrite elsewhere.
Apply within this hunk:
- private static void WriteJsonAtomicallyNoBom(string path, string json) - { - string tmp = path + ".tmp"; - var encNoBom = new System.Text.UTF8Encoding(encoderShouldEmitUTF8Identifier: false); - using (var fs = new System.IO.FileStream(tmp, System.IO.FileMode.Create, System.IO.FileAccess.Write, System.IO.FileShare.None)) - using (var sw = new System.IO.StreamWriter(fs, encNoBom)) - { - sw.Write(json); - sw.Flush(); - fs.Flush(true); - } - if (System.IO.File.Exists(path)) - System.IO.File.Replace(tmp, path, null); - else - System.IO.File.Move(tmp, path); - } + private static void WriteJsonAtomicallyNoBom(string path, string json) + { + string tmp = path + ".tmp"; + var encNoBom = new System.Text.UTF8Encoding(encoderShouldEmitUTF8Identifier: false); + using (var fs = new System.IO.FileStream(tmp, System.IO.FileMode.Create, System.IO.FileAccess.Write, System.IO.FileShare.None)) + using (var sw = new System.IO.StreamWriter(fs, encNoBom)) + { + sw.Write(json); + sw.Flush(); + fs.Flush(true); + } + + bool destExists = System.IO.File.Exists(path); + if (destExists) + { + try + { + // Prefer true atomic replace when supported (Windows) + System.IO.File.Replace(tmp, path, null); + return; + } + catch (System.PlatformNotSupportedException) + { + // Fall through to best-effort overwrite below + } + catch (System.IO.IOException) + { + // Fall through to best-effort overwrite below + } + // Best-effort overwrite: delete then rename (non-atomic on Unix, but avoids partial writes) + try { System.IO.File.Delete(path); } catch { } + } + System.IO.File.Move(tmp, path); + }
1385-1391: macOS config path + Claude Desktop mirror: good; add visibility and align error-paths
- Selecting macConfigPath on macOS, with Linux-path fallback, is correct.
- Mirroring Claude Desktop to the Linux-style path helps with older setups.
Minor improvements:
- Emit a debug log when the mirror write succeeds to aid troubleshooting.
- Ensure error-path/catch blocks that construct a configPath on macOS prefer macConfigPath (some catch paths in this class still use linuxConfigPath for OSX).
Add a debug log within this hunk:
if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX) && mcpClient?.mcpType == McpTypes.ClaudeDesktop) { string altPath = mcpClient.linuxConfigPath; if (!string.IsNullOrEmpty(altPath) && !string.Equals(configPath, altPath, StringComparison.Ordinal)) { try { Directory.CreateDirectory(Path.GetDirectoryName(altPath)); - WriteToConfig(pythonDir, altPath, mcpClient); + WriteToConfig(pythonDir, altPath, mcpClient); + if (debugLogsEnabled) + { + UnityEngine.Debug.Log($"UnityMCP: Mirrored Claude Desktop config to fallback path: {altPath}"); + } } catch { } } }Outside this hunk (reference only): In the ConfigureMcpClient catch block where you set configPath for error reporting on OSX/Linux, prefer macConfigPath on OSX:
// In catch path: if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { configPath = string.IsNullOrEmpty(mcpClient.macConfigPath) ? mcpClient.linuxConfigPath : mcpClient.macConfigPath; } else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { configPath = mcpClient.linuxConfigPath; }Also applies to: 1414-1429
UnityMcpBridge/Editor/Tools/ManageEditor.cs (2)
143-144: Consider centralizing supported actions to avoid string driftHard-coding the supported actions list in the error string invites divergence as actions evolve. Consider generating the list programmatically from the switch cases or a shared constant.
171-189: Project root resolution is sound; minor robustness suggestionThe approach based on Application.dataPath is correct. Two optional improvements:
- Prefer Path.GetDirectoryName(Application.dataPath) over Directory.GetParent to avoid a null-conditional and intermediate DirectoryInfo.
- Optionally verify that assetsPath ends with "/Assets" and log a warning if not, to help diagnose unusual editor environments.
Example minimally invasive tweak:
- string assetsPath = Application.dataPath.Replace('\\', '/'); - string projectRoot = Directory.GetParent(assetsPath)?.FullName.Replace('\\', '/'); + var assetsPath = Application.dataPath; + var projectRoot = Path.GetDirectoryName(assetsPath)?.Replace('\\', '/');UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
348-357: Preview only works on the local-apply pathIf you keep the server-side apply_text_edits path (above) for convertible ops, consider offering a preview mode there too (e.g., compute and return a diff locally without sending, or add a Unity-side dry-run option if supported).
82-87: Remove unused_extract_code_afterhelperThe
_extract_code_afterfunction is defined but never invoked anywhere in the repository. Deleting it will clean up dead code.Suggested removal in
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py:--- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py @@ -def _extract_code_after(keyword: str, request: str) -> str: - idx = request.lower().find(keyword) - if idx >= 0: - return request[idx + len(keyword):].strip() - return ""UnityMcpBridge/UnityMcpServer~/src/server.py (5)
5-5: Remove unused import: dataclassdataclasses.dataclass is unused.
Apply:
-from dataclasses import dataclass
7-7: Remove unused import: Listtyping.List is unused.
Apply:
-from typing import AsyncIterator, Dict, Any, List +from typing import AsyncIterator, Dict, Any
126-126: Avoid getattr with constant attribute (B009)Use direct attribute access for clarity and to satisfy linters.
Apply:
-if hasattr(mcp, "resource") and hasattr(getattr(mcp, "resource"), "list"): +if hasattr(mcp, "resource") and hasattr(mcp.resource, "list"):
138-138: Avoid getattr with constant attribute (B009)Same as above for the read handler.
Apply:
-if hasattr(mcp, "resource") and hasattr(getattr(mcp, "resource"), "read"): +if hasattr(mcp, "resource") and hasattr(mcp.resource, "read"):
105-107: Root resolution is simplistic here; consider aligning with tools’ robust strategyPROJECT_ROOT defaults to cwd or UNITY_PROJECT_ROOT. In non-repo working directories this can drift. Consider adopting the multi-strategy resolution used in tools/resource_tools.py (env, ask Unity manage_editor.get_project_root, up-walk), to make server resources align with the editor’s actual project.
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
71-109: list_resources: good guardrails; consider name metadataThe implementation is safe and efficient. Optional: return names alongside URIs (like the server’s MCP resources endpoint) to save a round trip for display-oriented clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
UnityMcpBridge/Editor/Data/McpClients.cs(1 hunks)UnityMcpBridge/Editor/Models/McpClient.cs(1 hunks)UnityMcpBridge/Editor/Tools/ManageEditor.cs(4 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs(11 hunks)UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs(8 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- UnityMcpBridge/Editor/Tools/ManageScript.cs
🧰 Additional context used
🧬 Code Graph Analysis (6)
UnityMcpBridge/Editor/Tools/ManageEditor.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-58)Error(41-57)Success(18-33)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
register_manage_script_edits_tools(168-377)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
register_resource_tools(71-225)
UnityMcpBridge/UnityMcpServer~/src/server.py (3)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
register_all_tools(15-30)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(318-334)UnityConnection(24-313)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (3)
_resolve_safe_path_from_uri(53-68)list_resources(75-109)read_resource(112-184)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(351-370)UnityMcpBridge/UnityMcpServer~/src/server.py (4)
_(102-103)_resolve_safe_path_from_uri(108-123)list_resources(128-136)read_resource(140-149)tests/test_script_tools.py (1)
tool(42-46)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (3)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (1)
List(1131-1290)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(351-370)tests/test_script_tools.py (1)
tool(42-46)
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
UnityMcpBridge/Editor/Models/McpClient.cs (1)
SetStatus(33-45)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/server.py
5-5: dataclasses.dataclass imported but unused
Remove unused import: dataclasses.dataclass
(F401)
7-7: typing.List imported but unused
Remove unused import: typing.List
(F401)
126-126: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
138-138: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (13)
UnityMcpBridge/Editor/Models/McpClient.cs (1)
7-7: Added macConfigPath field — looks goodClear, minimal API addition that unblocks macOS-specific configuration. No issues spotted.
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
1558-1564: CheckMcpConfiguration mac path selection + fallback — LGTMCorrectly uses macConfigPath on macOS and falls back to the Linux-style path for Claude Desktop when the primary file doesn’t exist, surfacing NotConfigured otherwise. This matches the ConfigureMcpClient behavior.
Also applies to: 1576-1595
UnityMcpBridge/Editor/Tools/ManageEditor.cs (2)
4-4: Import is appropriate for path operationsAdding System.IO is necessary for Directory.GetParent usage in GetProjectRoot().
93-95: New action wiring looks correctget_project_root is correctly routed and returns the helper result.
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (3)
13-20: Switch to logger is appropriateUsing a module-scoped logger and info-level messages instead of prints is a solid improvement and plays well with the server’s logging setup.
18-20: Registering surgical edits first is a good discovery hintPlacing register_manage_script_edits_tools before the broader manage_script tools increases the chance that clients discover the safer, transactional edits.
28-30: Exposing resource wrappers is a good compatibility bridgeRegistering resource_tools via normal tools helps clients lacking resource primitives interoperate.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
49-63: Range-replace bounds handling is correctThe max_end and min(...) logic properly allows appending at EOF and avoids off-by-one errors.
UnityMcpBridge/UnityMcpServer~/src/server.py (1)
15-43: Good: logging confined to stderr/file to protect stdio transportThe stricter logging configuration prevents contamination of MCP stdio and adds a rotating file handler with a safe fallback. Solid improvement.
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (4)
19-51: Robust project root resolutionThe multi-strategy approach (override/env/Unity query/up-walk) is resilient and aligns server and editor perspectives well.
53-69: Safe path mapping looks correctMapping URIs to paths with resolve + relative_to prevents traversal outside the project root.
111-185: read_resource: solid slicing and NL helpersThe precedence and error handling are sensible. Nice touch with “around MethodName” heuristics.
186-226: find_in_file: pragmatic and cappedRegex search with line-capped results is a useful complement to read/list.
| // 2) Canonical args order (add quiet flag to prevent stdout noise breaking MCP stdio) | ||
| var newArgs = new[] { "-q", "run", "--directory", serverSrc, "server.py" }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quiet uv args + BOM stripping + atomic write: solid improvements; align all callsites with -q to avoid stdio noise
Nice work:
- Prepending -q to uv args reduces stdout noise that can break MCP stdio.
- BOM detection + rewrite avoids JSON parse quirks in some IDEs.
- Atomic write with fsync reduces transient corruption risk.
- Post-write re-parse is a good integrity guard.
Action item: Ensure all code paths that construct launch args also include -q, otherwise manual setups or Claude CLI registration can still produce stdout and break the protocol.
Update these callsites (outside this hunk) to include -q:
- VSCode manual JSON (ShowManualInstructionsWindow, VSCode branch):
args = new[] { "-q", "run", "--directory", pythonDir, "server.py" } - Standard MCP manual JSON (ShowManualInstructionsWindow default):
args = new[] { "-q", "run", "--directory", pythonDir, "server.py" } - Cursor manual JSON (ShowCursorManualConfigurationInstructions):
args = new[] { "-q", "run", "--directory", pythonDir, "server.py" } - Claude Code registration (RegisterWithClaudeCode):
string args = $"mcp add UnityMCP -- "{uvPath}" -q run --directory "{srcDir}" server.py";
This keeps behavior consistent with the auto-write path and avoids intermittent breakage during initial installs.
Also applies to: 1093-1111, 1136-1147, 1148-1157
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs around lines 1086-1088
(and also affecting 1093-1111, 1136-1147, 1148-1157), several manual launch-arg
callsites do not include the "-q" flag which can produce stdout noise and break
MCP stdio; update each callsite to prepend "-q" to the uv/python launch args so
they match the auto-write path: change the VSCode manual JSON
(ShowManualInstructionsWindow, VSCode branch) args to start with "-q", change
the standard MCP manual JSON default in ShowManualInstructionsWindow to start
with "-q", change the Cursor manual JSON in
ShowCursorManualConfigurationInstructions to start with "-q", and update the
Claude Code registration string in RegisterWithClaudeCode to include "-q" before
"run" (i.e., ensure the constructed args for uv/python begin with "-q" in all
these locations).
| text_ops = { (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() for e in (edits or []) } | ||
| structured_kinds = {"replace_class","delete_class","replace_method","delete_method","insert_method"} | ||
| if not text_ops.issubset(structured_kinds): | ||
| # Convert to apply_text_edits payload | ||
| try: | ||
| current_text = contents | ||
| def line_col_from_index(idx: int) -> Tuple[int, int]: | ||
| # 1-based line/col | ||
| line = current_text.count("\n", 0, idx) + 1 | ||
| last_nl = current_text.rfind("\n", 0, idx) | ||
| col = (idx - (last_nl + 1)) + 1 if last_nl >= 0 else idx + 1 | ||
| return line, col | ||
|
|
||
| at_edits: List[Dict[str, Any]] = [] | ||
| import re as _re | ||
| for e in edits or []: | ||
| op = (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() | ||
| # aliasing for text field | ||
| text_field = e.get("text") or e.get("insert") or e.get("content") or "" | ||
| if op == "anchor_insert": | ||
| anchor = e.get("anchor") or "" | ||
| position = (e.get("position") or "before").lower() | ||
| m = _re.search(anchor, current_text, _re.MULTILINE) | ||
| if not m: | ||
| return {"success": False, "message": f"anchor not found: {anchor}"} | ||
| idx = m.start() if position == "before" else m.end() | ||
| sl, sc = line_col_from_index(idx) | ||
| at_edits.append({ | ||
| "startLine": sl, | ||
| "startCol": sc, | ||
| "endLine": sl, | ||
| "endCol": sc, | ||
| "newText": text_field or "" | ||
| }) | ||
| # Update local snapshot to keep subsequent anchors stable | ||
| current_text = current_text[:idx] + (text_field or "") + current_text[idx:] | ||
| elif op == "replace_range": | ||
| # Directly forward if already in line/col form | ||
| if "startLine" in e: | ||
| at_edits.append({ | ||
| "startLine": int(e.get("startLine", 1)), | ||
| "startCol": int(e.get("startCol", 1)), | ||
| "endLine": int(e.get("endLine", 1)), | ||
| "endCol": int(e.get("endCol", 1)), | ||
| "newText": text_field | ||
| }) | ||
| else: | ||
| # If only indices provided, skip (we don't support index-based here) | ||
| return {"success": False, "message": "replace_range requires startLine/startCol/endLine/endCol"} | ||
| elif op == "regex_replace": | ||
| pattern = e.get("pattern") or "" | ||
| repl = text_field | ||
| m = _re.search(pattern, current_text, _re.MULTILINE) | ||
| if not m: | ||
| continue | ||
| sl, sc = line_col_from_index(m.start()) | ||
| el, ec = line_col_from_index(m.end()) | ||
| at_edits.append({ | ||
| "startLine": sl, | ||
| "startCol": sc, | ||
| "endLine": el, | ||
| "endCol": ec, | ||
| "newText": repl | ||
| }) | ||
| current_text = current_text[:m.start()] + repl + current_text[m.end():] | ||
| else: | ||
| return {"success": False, "message": f"Unsupported text edit op for server-side apply_text_edits: {op}"} | ||
|
|
||
| # Send to Unity with precondition SHA to enforce guards | ||
| import hashlib | ||
| sha = hashlib.sha256(contents.encode("utf-8")).hexdigest() | ||
| params: Dict[str, Any] = { | ||
| "action": "apply_text_edits", | ||
| "name": name, | ||
| "path": path, | ||
| "namespace": namespace, | ||
| "scriptType": script_type, | ||
| "edits": at_edits, | ||
| "precondition_sha256": sha, | ||
| "options": { | ||
| "refresh": (options or {}).get("refresh", "immediate"), | ||
| "validate": (options or {}).get("validate", "standard") | ||
| } | ||
| } | ||
| resp = send_command_with_retry("manage_script", params) | ||
| return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} | ||
| except Exception as e: | ||
| return {"success": False, "message": f"Edit conversion failed: {e}"} | ||
|
|
There was a problem hiding this comment.
Text-edit routing bug: prepend/append cannot execute due to unconditional conversion path
Currently, any presence of non-structured ops triggers the apply_text_edits conversion path. Ops not supported by conversion (e.g., prepend, append) return “Unsupported…” and never fall back to local apply, making them unusable. This contradicts the tool’s stated support for prepend/append.
Refactor the branching so you only perform server-side conversion when all ops are in the supported-for-conversion set; otherwise fall back to local apply. Minimal diff:
- # Supported conversions: anchor_insert, replace_range, regex_replace (first match only).
- text_ops = { (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() for e in (edits or []) }
- structured_kinds = {"replace_class","delete_class","replace_method","delete_method","insert_method"}
- if not text_ops.issubset(structured_kinds):
+ # Supported conversions: anchor_insert, replace_range, regex_replace (first match only).
+ text_ops = { (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() for e in (edits or []) }
+ structured_kinds = {"replace_class","delete_class","replace_method","delete_method","insert_method"}
+ supported_for_apply_text = {"anchor_insert","replace_range","regex_replace"}
+ # If all ops are convertible text edits, use Unity-side apply_text_edits; otherwise fall back to local apply below.
+ if text_ops and text_ops.issubset(supported_for_apply_text):
# Convert to apply_text_edits payload
try:
current_text = contents
def line_col_from_index(idx: int) -> Tuple[int, int]:
# 1-based line/col
line = current_text.count("\n", 0, idx) + 1
last_nl = current_text.rfind("\n", 0, idx)
col = (idx - (last_nl + 1)) + 1 if last_nl >= 0 else idx + 1
return line, col
at_edits: List[Dict[str, Any]] = []
import re as _re
for e in edits or []:
op = (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower()
# aliasing for text field
text_field = e.get("text") or e.get("insert") or e.get("content") or ""
if op == "anchor_insert":
anchor = e.get("anchor") or ""
position = (e.get("position") or "before").lower()
m = _re.search(anchor, current_text, _re.MULTILINE)
if not m:
return {"success": False, "message": f"anchor not found: {anchor}"}
idx = m.start() if position == "before" else m.end()
sl, sc = line_col_from_index(idx)
at_edits.append({
"startLine": sl,
"startCol": sc,
"endLine": sl,
"endCol": sc,
"newText": text_field or ""
})
# Update local snapshot to keep subsequent anchors stable
current_text = current_text[:idx] + (text_field or "") + current_text[idx:]
elif op == "replace_range":
# Directly forward if already in line/col form
if "startLine" in e:
at_edits.append({
"startLine": int(e.get("startLine", 1)),
"startCol": int(e.get("startCol", 1)),
"endLine": int(e.get("endLine", 1)),
"endCol": int(e.get("endCol", 1)),
"newText": text_field
})
else:
# If only indices provided, skip (we don't support index-based here)
return {"success": False, "message": "replace_range requires startLine/startCol/endLine/endCol"}
elif op == "regex_replace":
pattern = e.get("pattern") or ""
repl = text_field
m = _re.search(pattern, current_text, _re.MULTILINE)
if not m:
continue
sl, sc = line_col_from_index(m.start())
el, ec = line_col_from_index(m.end())
at_edits.append({
"startLine": sl,
"startCol": sc,
"endLine": el,
"endCol": ec,
"newText": repl
})
current_text = current_text[:m.start()] + repl + current_text[m.end():]
- else:
- return {"success": False, "message": f"Unsupported text edit op for server-side apply_text_edits: {op}"}
+ # Other ops (prepend/append) are handled via local apply path; we won't enter here due to the set check above.
# Send to Unity with precondition SHA to enforce guards
import hashlib
sha = hashlib.sha256(contents.encode("utf-8")).hexdigest()
params: Dict[str, Any] = {
"action": "apply_text_edits",
"name": name,
"path": path,
"namespace": namespace,
"scriptType": script_type,
"edits": at_edits,
"precondition_sha256": sha,
"options": {
"refresh": (options or {}).get("refresh", "immediate"),
"validate": (options or {}).get("validate", "standard")
}
}
resp = send_command_with_retry("manage_script", params)
return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}
except Exception as e:
return {"success": False, "message": f"Edit conversion failed: {e}"}This preserves server-side validation for convertible ops while restoring prepend/append via the local apply path.
📝 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.
| text_ops = { (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() for e in (edits or []) } | |
| structured_kinds = {"replace_class","delete_class","replace_method","delete_method","insert_method"} | |
| if not text_ops.issubset(structured_kinds): | |
| # Convert to apply_text_edits payload | |
| try: | |
| current_text = contents | |
| def line_col_from_index(idx: int) -> Tuple[int, int]: | |
| # 1-based line/col | |
| line = current_text.count("\n", 0, idx) + 1 | |
| last_nl = current_text.rfind("\n", 0, idx) | |
| col = (idx - (last_nl + 1)) + 1 if last_nl >= 0 else idx + 1 | |
| return line, col | |
| at_edits: List[Dict[str, Any]] = [] | |
| import re as _re | |
| for e in edits or []: | |
| op = (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() | |
| # aliasing for text field | |
| text_field = e.get("text") or e.get("insert") or e.get("content") or "" | |
| if op == "anchor_insert": | |
| anchor = e.get("anchor") or "" | |
| position = (e.get("position") or "before").lower() | |
| m = _re.search(anchor, current_text, _re.MULTILINE) | |
| if not m: | |
| return {"success": False, "message": f"anchor not found: {anchor}"} | |
| idx = m.start() if position == "before" else m.end() | |
| sl, sc = line_col_from_index(idx) | |
| at_edits.append({ | |
| "startLine": sl, | |
| "startCol": sc, | |
| "endLine": sl, | |
| "endCol": sc, | |
| "newText": text_field or "" | |
| }) | |
| # Update local snapshot to keep subsequent anchors stable | |
| current_text = current_text[:idx] + (text_field or "") + current_text[idx:] | |
| elif op == "replace_range": | |
| # Directly forward if already in line/col form | |
| if "startLine" in e: | |
| at_edits.append({ | |
| "startLine": int(e.get("startLine", 1)), | |
| "startCol": int(e.get("startCol", 1)), | |
| "endLine": int(e.get("endLine", 1)), | |
| "endCol": int(e.get("endCol", 1)), | |
| "newText": text_field | |
| }) | |
| else: | |
| # If only indices provided, skip (we don't support index-based here) | |
| return {"success": False, "message": "replace_range requires startLine/startCol/endLine/endCol"} | |
| elif op == "regex_replace": | |
| pattern = e.get("pattern") or "" | |
| repl = text_field | |
| m = _re.search(pattern, current_text, _re.MULTILINE) | |
| if not m: | |
| continue | |
| sl, sc = line_col_from_index(m.start()) | |
| el, ec = line_col_from_index(m.end()) | |
| at_edits.append({ | |
| "startLine": sl, | |
| "startCol": sc, | |
| "endLine": el, | |
| "endCol": ec, | |
| "newText": repl | |
| }) | |
| current_text = current_text[:m.start()] + repl + current_text[m.end():] | |
| else: | |
| return {"success": False, "message": f"Unsupported text edit op for server-side apply_text_edits: {op}"} | |
| # Send to Unity with precondition SHA to enforce guards | |
| import hashlib | |
| sha = hashlib.sha256(contents.encode("utf-8")).hexdigest() | |
| params: Dict[str, Any] = { | |
| "action": "apply_text_edits", | |
| "name": name, | |
| "path": path, | |
| "namespace": namespace, | |
| "scriptType": script_type, | |
| "edits": at_edits, | |
| "precondition_sha256": sha, | |
| "options": { | |
| "refresh": (options or {}).get("refresh", "immediate"), | |
| "validate": (options or {}).get("validate", "standard") | |
| } | |
| } | |
| resp = send_command_with_retry("manage_script", params) | |
| return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} | |
| except Exception as e: | |
| return {"success": False, "message": f"Edit conversion failed: {e}"} | |
| # Supported conversions: anchor_insert, replace_range, regex_replace (first match only). | |
| text_ops = { | |
| (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "") | |
| .strip() | |
| .lower() | |
| for e in (edits or []) | |
| } | |
| structured_kinds = { | |
| "replace_class", | |
| "delete_class", | |
| "replace_method", | |
| "delete_method", | |
| "insert_method", | |
| } | |
| supported_for_apply_text = {"anchor_insert", "replace_range", "regex_replace"} | |
| # If all ops are convertible text edits, use Unity-side apply_text_edits; | |
| # otherwise fall back to the local apply path below. | |
| if text_ops and text_ops.issubset(supported_for_apply_text): | |
| # Convert to apply_text_edits payload | |
| try: | |
| current_text = contents | |
| def line_col_from_index(idx: int) -> Tuple[int, int]: | |
| # 1-based line/col | |
| line = current_text.count("\n", 0, idx) + 1 | |
| last_nl = current_text.rfind("\n", 0, idx) | |
| col = (idx - (last_nl + 1)) + 1 if last_nl >= 0 else idx + 1 | |
| return line, col | |
| at_edits: List[Dict[str, Any]] = [] | |
| import re as _re | |
| for e in edits or []: | |
| op = ( | |
| e.get("op") | |
| or e.get("operation") | |
| or e.get("type") | |
| or e.get("mode") | |
| or "" | |
| ).strip().lower() | |
| text_field = e.get("text") or e.get("insert") or e.get("content") or "" | |
| if op == "anchor_insert": | |
| anchor = e.get("anchor") or "" | |
| position = (e.get("position") or "before").lower() | |
| m = _re.search(anchor, current_text, _re.MULTILINE) | |
| if not m: | |
| return { | |
| "success": False, | |
| "message": f"anchor not found: {anchor}", | |
| } | |
| idx = m.start() if position == "before" else m.end() | |
| sl, sc = line_col_from_index(idx) | |
| at_edits.append( | |
| { | |
| "startLine": sl, | |
| "startCol": sc, | |
| "endLine": sl, | |
| "endCol": sc, | |
| "newText": text_field or "", | |
| } | |
| ) | |
| current_text = ( | |
| current_text[:idx] + (text_field or "") + current_text[idx:] | |
| ) | |
| elif op == "replace_range": | |
| if "startLine" in e: | |
| at_edits.append( | |
| { | |
| "startLine": int(e.get("startLine", 1)), | |
| "startCol": int(e.get("startCol", 1)), | |
| "endLine": int(e.get("endLine", 1)), | |
| "endCol": int(e.get("endCol", 1)), | |
| "newText": text_field, | |
| } | |
| ) | |
| else: | |
| return { | |
| "success": False, | |
| "message": "replace_range requires startLine/startCol/endLine/endCol", | |
| } | |
| elif op == "regex_replace": | |
| pattern = e.get("pattern") or "" | |
| repl = text_field | |
| m = _re.search(pattern, current_text, _re.MULTILINE) | |
| if not m: | |
| continue | |
| sl, sc = line_col_from_index(m.start()) | |
| el, ec = line_col_from_index(m.end()) | |
| at_edits.append( | |
| { | |
| "startLine": sl, | |
| "startCol": sc, | |
| "endLine": el, | |
| "endCol": ec, | |
| "newText": repl, | |
| } | |
| ) | |
| current_text = current_text[: m.start()] + repl + current_text[m.end() :] | |
| # Other ops (prepend/append) are handled via local apply path; | |
| # we won’t enter here due to the set check above. | |
| import hashlib | |
| sha = hashlib.sha256(contents.encode("utf-8")).hexdigest() | |
| params: Dict[str, Any] = { | |
| "action": "apply_text_edits", | |
| "name": name, | |
| "path": path, | |
| "namespace": namespace, | |
| "scriptType": script_type, | |
| "edits": at_edits, | |
| "precondition_sha256": sha, | |
| "options": { | |
| "refresh": (options or {}).get("refresh", "immediate"), | |
| "validate": (options or {}).get("validate", "standard"), | |
| }, | |
| } | |
| resp = send_command_with_retry("manage_script", params) | |
| return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} | |
| except Exception as e: | |
| return {"success": False, "message": f"Edit conversion failed: {e}"} |
…-response Fix ApplyTextEdits replacement index calculation and port probe handshake
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (3)
65-73: Avoid redefining_read_exacton every probe; hoist to module-level to reduce duplication and align withunity_connection.py.This nested helper duplicates logic already present in
unity_connection.py(see that file’s_read_exact). Hoisting to a module-level helper avoids per-call redefinition and makes it reusable across this module; it also makes it easier to keep behavior consistent across the codebase.Apply this diff within the selected lines to remove the nested definition:
- def _read_exact(sock: socket.socket, count: int) -> bytes: - buf = bytearray() - while len(buf) < count: - chunk = sock.recv(count - len(buf)) - if not chunk: - raise ConnectionError("Connection closed before reading expected bytes") - buf.extend(chunk) - return bytes(buf)Then add this module-level helper (place it after imports, before class PortDiscovery) to preserve functionality:
# Module-level helper to avoid duplication and per-call redefinition def _read_exact(sock: socket.socket, count: int) -> bytes: buf = bytearray() while len(buf) < count: chunk = sock.recv(count - len(buf)) if not chunk: raise ConnectionError("Connection closed before reading expected bytes") buf.extend(chunk) return bytes(buf)If you prefer a single source of truth, we can also extract a small
framing.pyutility used by both this module andunity_connection.py. Happy to provide that patch.
82-87: Bound framed response size to prevent pathological allocations; also use a named constant for header size.Defensively cap
resp_len(e.g., 1 MiB) to avoid attempting to read unbounded data if a misbehaving listener responds with a bogus length. Using a constant for the frame header size improves readability and consistency with the rest of the codebase.Apply this diff to the selected lines:
- resp_header = _read_exact(s, 8) - resp_len = struct.unpack('>Q', resp_header)[0] - data = _read_exact(s, resp_len) + resp_header = _read_exact(s, FRAME_HEADER_SIZE) + resp_len = struct.unpack('>Q', resp_header)[0] + # Defensive cap against unreasonable frame sizes + if resp_len > MAX_FRAME_SIZE: + return False + data = _read_exact(s, resp_len)And add these constants near the top of the file (after imports):
FRAME_HEADER_SIZE = 8 # Keep small; we're only looking for a tiny pong. 1 MiB is generous for probes. MAX_FRAME_SIZE = 1 << 20
78-89: Slightly more robust unframed read to avoid false negatives whenpongspans recv boundaries.In the unframed path, a single
recv(512)may miss"message":"pong"if it lands across buffer boundaries. A tiny bounded loop retains the snappy behavior while improving reliability.Apply this diff:
- s.sendall(payload) - data = s.recv(512) + s.sendall(payload) + # Read a small bounded amount looking for pong + chunks = [] + total = 0 + data = b"" + while total < 1024: + try: + part = s.recv(512) + except socket.timeout: + break + if not part: + break + chunks.append(part) + total += len(part) + if b'"message":"pong"' in part: + break + if chunks: + data = b"".join(chunks)Optional: we could also attempt to read the greeting a bit more robustly (until newline or a small cap like 256–512 bytes) to avoid missing
FRAMING=1if it arrives fragmented. Let me know if you want that tweak as well.UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
54-101: Path hardening under Assets/ is solid; consider broadening symlink detection for non-WindowsThe normalization and containment checks look good. The symlink guard via FileAttributes.ReparsePoint primarily helps on Windows; on macOS/Linux it may not catch symlinks. Optional: add a broader symlink check when available (e.g., DirectoryInfo.LinkTarget in newer runtimes), gated by defines, to strengthen the guard cross-platform.
524-537: Header guard regex should include ‘global using’ and ‘using static’Today this will miss legitimate directive forms and may over-restrict edits near the top. Consider expanding the match.
Apply this diff:
- var mUsing = System.Text.RegularExpressions.Regex.Match(original, @"(?m)^(?:\uFEFF)?using\s+\w+", System.Text.RegularExpressions.RegexOptions.None); + var mUsing = System.Text.RegularExpressions.Regex.Match( + original, + @"(?m)^(?:\uFEFF)?(?:global\s+)?using(?:\s+static)?\b", + System.Text.RegularExpressions.RegexOptions.None + );
539-582: Auto-upgrade to structured replace_method: consider falling back to “containing class” when file/class names divergeHeuristic relies on name (file/class) alignment. In Unity, they usually match, but partials/nested or different class names will cause TryComputeClassSpan(original, name, ...) to fail. As a fallback, derive the enclosing class span around sp.start (e.g., scan backward to nearest “class ... {” with balanced braces) instead of requiring the file name.
I can draft a small helper (FindEnclosingClassSpan) if you want to harden this path.
727-793: Balanced delimiter checker: treat lone ‘\r’ as newline to keep line counts accurate on CR-only inputsLine tracking only increments on ‘\n’. Rare, but on CR-only files you’ll under-count lines. Low priority, but simple to fix.
Apply this diff:
- if (c == '\n') { line++; if (inSingle) inSingle = false; } + if (c == '\r') + { + // Treat CRLF as a single newline; skip the LF if present + if (next == '\n') { i++; } + line++; if (inSingle) inSingle = false; + } + else if (c == '\n') + { + line++; if (inSingle) inSingle = false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
UnityMcpBridge/Editor/Tools/ManageScript.cs(11 hunks)UnityMcpBridge/UnityMcpServer~/src/port_discovery.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
_read_exact(89-96)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-58)Error(41-57)Success(18-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (11)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (3)
21-21: Import for framing length encoding looks correct.
structis the right choice for 8-byte big-endian length encoding/decoding.
60-63: Docstring clearly documents framed vs. unframed probe behavior.Good clarity on the 8-byte big-endian framing and fallback.
78-83: Handshake greeting is unframed ASCII
Verified that the Unity bridge/server sends its initial greeting as a plain ASCII line (e.g. in UnityMcpBridge/Editor/UnityMcpBridge.cs:string handshake = "WELCOME UNITY-MCP 1 FRAMING=1\n"; byte[] handshakeBytes = Encoding.ASCII.GetBytes(handshake); await stream.WriteAsync(handshakeBytes, …);No 8-byte length header is ever prepended to the greeting. PortDiscovery’s probe logic—reading an unframed ASCII greeting and checking for “FRAMING=1”—is therefore correct and needs no change.
UnityMcpBridge/Editor/Tools/ManageScript.cs (8)
189-197: Deprecated manage_script actions correctly forward with warningsGood compatibility layer: read/update now warn and call the underlying implementations, preserving existing callers.
203-231: Validate action: level mapping now complete and coherentExplicit handling for basic/standard/comprehensive/strict is correct. The diagnostics packaging is also reasonable for non-Roslyn environments.
301-326: Create: atomic-ish write and deferred refresh look goodWriting to .tmp and moving (with copy fallback) is safe. Scheduling the refresh after emitting the response avoids blocking the caller.
407-447: Update: atomic replace with cleanup is correct; backup deletion prevents stray assetsUsing File.Replace with .bak cleanup and robust fallbacks is the right approach. Returning before scheduling refresh also avoids domain-reload timing issues.
486-492: Precondition SHA guard is a good defensive additionRequiring precondition_sha256 and returning current hash on mismatch prevents write races and stale edits.
611-635: Roslyn diagnostics + optional formatting: sensible defaults and guardrailsRestricting to first few errors and best-effort formatting inside a try/catch is pragmatic. Good use of AdhocWorkspace to avoid project coupling.
682-715: Line/col → index: CRLF handling fix is correctTreating CRLF as a single newline eliminates Windows off-by-one issues. Loop bounds and end-of-text checks are sound.
2257-2333: RefreshDebounce: coalescing logic is safe and editor-threadedGood use of Interlocked + lock to track pending work and a single ticking callback via delayCall. Importing specific assets then requesting compilation is lighter than a full Refresh.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (2)
30-38: Avoid duplicating _read_exact; reuse the existing helper to reduce drift.There’s an almost identical
_read_exactinUnityMcpBridge/UnityMcpServer~/src/unity_connection.py(Lines 88–95 in the provided snippet). Duplicating this logic across modules makes maintenance error-prone.Consider moving
_read_exactto a small shared module (e.g.,io_utils.py) and import it here and inunity_connection.py. If you want to keep it local for now, at least align exception types/messages for consistency.Also applies to: 88-96
97-115: Unframed read loop is bounded and resilient; one small improvement.The bounded loop with timeout and a final assembled check handles split matches across chunk boundaries. To improve early exits, consider checking the accumulator for
"pong"after each append to avoid waiting for the next iteration if the split occurs across chunks.Apply this diff:
- chunks.append(part) + chunks.append(part) total += len(part) - if b'"message":"pong"' in part: + joined = b"".join(chunks) if len(chunks) <= 2 else None + if (b'"message":"pong"' in part) or (joined is not None and b'"message":"pong"' in joined): breakUnityMcpBridge/Editor/Tools/ManageScript.cs (5)
116-116: Normalize action with ToLowerInvariant to avoid culture issues.Using culture-aware
ToLower()can misparse action names under certain locales (e.g., Turkish-I). UseToLowerInvariant()consistently.Apply this diff:
- string action = @params["action"]?.ToString().ToLower(); + string action = @params["action"]?.ToString().ToLowerInvariant();
551-596: Auto-upgrade heuristic: gate with an option to avoid surprising rewrites.The replace-method auto-upgrade is helpful but can be surprising if a user intends a small text edit outside the method header yet near it, especially in large files. Consider adding an opt-in flag (e.g.,
options.autoUpgradeTextEdit=true) or requiring the edit to fully cover the method header token range to trigger the upgrade.Proposed change (conceptual, apply where options are parsed):
- Default
autoUpgradeTextEditto false.- Trigger the replace_method path only if explicitly enabled or if the edit fully overlaps the method header span (not just “near” it).
1540-1545: Method token matching can false-match substrings; prefer boundaries.
IndexOfTokenWithin(source, methodName, ...)can match inside a longer identifier (e.g.,DoWorkmatchingDoWorkExtra). Consider a regex or boundary checks.Apply this diff to add a simple boundary-aware search:
- private static int IndexOfTokenWithin(string s, string token, int start, int end) - { - int idx = s.IndexOf(token, start, StringComparison.Ordinal); - return (idx >= 0 && idx < end) ? idx : -1; - } + private static int IndexOfTokenWithin(string s, string token, int start, int end) + { + int idx = start - 1; + while (true) + { + idx = s.IndexOf(token, idx + 1, StringComparison.Ordinal); + if (idx < 0 || idx >= end) return -1; + bool leftOk = idx == 0 || !char.IsLetterOrDigit(s[idx - 1]) && s[idx - 1] != '_'; + int right = idx + token.Length; + bool rightOk = right >= s.Length || right >= end || (!char.IsLetterOrDigit(s[right]) && s[right] != '_'); + if (leftOk && rightOk) return idx; + } + }Also applies to: 1638-1660
2023-2026: Cache expiry should use UTC to avoid DST/local time issues.Using
DateTime.Nowcan behave oddly across DST changes. PreferDateTime.UtcNowfor cache time and comparisons.Apply this diff:
- if (_cachedReferences != null && DateTime.Now - _cacheTime < CacheExpiry) + if (_cachedReferences != null && DateTime.UtcNow - _cacheTime < CacheExpiry) { return _cachedReferences; } @@ - _cacheTime = DateTime.Now; + _cacheTime = DateTime.UtcNow;Also applies to: 2077-2078
1180-1189: Overlap detection helper is correct; small perf nit.This is O(n log n) due to the sort; acceptable for small batches. If you expect large edit sets, consider keeping edits sorted as you gather them, or use interval merging. Not urgent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
UnityMcpBridge/Editor/Tools/ManageScript.cs(11 hunks)UnityMcpBridge/UnityMcpServer~/src/port_discovery.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-58)Error(41-57)Success(18-33)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
_read_exact(89-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (8)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (1)
25-28: Good framed flow and defensive caps.Using an 8-byte big-endian length, enforcing
resp_len <= MAX_FRAME_SIZE, and reading exactlyresp_lenis correct and avoids over-reads. The final"pong"check on the assembled data is also sound.Also applies to: 90-96, 115-117
UnityMcpBridge/Editor/Tools/ManageScript.cs (7)
188-206: Backward-compatible routing for deprecated actions looks good.The deprecation warnings with forwarding to
ReadScript,UpdateScript, andEditScriptpreserve existing callers while nudging migration. The updated valid-actions list is clear.Also applies to: 240-245, 246-248
213-221: Validation: complete level mapping and structured diagnostics are solid.Explicit handling for basic/standard/strict/comprehensive matches the documented API and GUI options. Converting Roslyn/regex output into uniform diagnostics with line/severity is helpful for clients.
Also applies to: 226-239
310-324: Atomic create path is correct; reply-before-refresh is a nice touch.Writing to a temp file, moving/copying atomically, and scheduling refresh after sending the response reduces chances of domain-reload races.
Also applies to: 325-334
415-443: Great: cleanup of .bak backups after replace and cross-volume fallbacks.This avoids leaking stray
*.bakfiles into Assets and covers platform/cross-volume cases. Good sequencing to prepare the response before scheduling refresh.Also applies to: 444-455
603-616: Non-overlap check + reverse-apply is correct.Sorting by descending start and rejecting
end > previous.startavoids inter-edit interference. Applying from back to front preserves earlier indices.
696-729: CRLF-aware line/col mapping fix is correct.Treating CRLF as a single newline prevents off-by-one column errors on Windows files, and handles lone CR safely.
1149-1167: Main-thread enforcement via delayCall is correct.Import/compile calls wrapped in
EditorApplication.delayCallensure they execute on the main thread. Debounced refresh infra also looks thread-safe.Also applies to: 2321-2358
| greeting = s.recv(256) | ||
| text = greeting.decode('ascii', errors='ignore') if greeting else '' | ||
| payload = b"ping" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Framing detection can fail on partial greeting; read until delimiter or timeout.
A single recv(256) may return a partial greeting (e.g., “FRAM” or split across packets). If the server is strictly framed and expects the client to switch modes based on the greeting, failing to read the full line risks misclassifying the server as unframed and sending an unframed ping that will never be answered, causing false negatives during discovery.
Tighten the read to accumulate up to a sane cap or until newline/timeout:
Apply this diff:
- greeting = s.recv(256)
- text = greeting.decode('ascii', errors='ignore') if greeting else ''
+ # Read a small greeting line robustly: up to 256 bytes or until newline/timeout
+ greeting = bytearray()
+ while len(greeting) < 256:
+ try:
+ part = s.recv(256 - len(greeting))
+ except socket.timeout:
+ break
+ if not part:
+ break
+ greeting.extend(part)
+ if b'\n' in part:
+ break
+ text = bytes(greeting).decode('ascii', errors='ignore') if greeting else ''📝 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.
| greeting = s.recv(256) | |
| text = greeting.decode('ascii', errors='ignore') if greeting else '' | |
| payload = b"ping" | |
| # Read a small greeting line robustly: up to 256 bytes or until newline/timeout | |
| greeting = bytearray() | |
| while len(greeting) < 256: | |
| try: | |
| part = s.recv(256 - len(greeting)) | |
| except socket.timeout: | |
| break | |
| if not part: | |
| break | |
| greeting.extend(part) | |
| if b'\n' in part: | |
| break | |
| text = bytes(greeting).decode('ascii', errors='ignore') if greeting else '' | |
| payload = b"ping" |
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/port_discovery.py around lines 84-86,
replace the single recv(256) with a loop that accumulates bytes until a framing
delimiter (newline or CRLF) is seen or a short timeout/cap is reached (e.g., 1s
or 1024 bytes), then decode the full accumulated greeting; this prevents
treating a partial greeting as absent — implement a short per-socket timeout,
repeatedly recv into a buffer until delimiter or cap/timeout, then decode using
'ascii' with errors='ignore' and proceed to framing detection.
* Fix HTTP instance routing and per-project session IDs * Drop confusing log message * Ensure lock file references later version of uvicorn with key fixes * Fix test imports * Update refs in docs --------- Co-authored-by: David Sarno <david@lighthaus.us>
* Remove temp folder from repo * Ignore boot.config * Remove buttons to download or rebuild the server * Remove embedded MCP server in plugin We'll reference the remote server in GitHub and configure clients to use `uvx` * As much as possible, rip out logic that installs a server * feat: migrate to uvx-based server configuration - Replaced local server execution with uvx package-based configuration for improved reliability - Added GetUvxCommand helper to generate correct package version command string - Updated config generation to use `uvx mcp-for-unity` instead of local Python server - Modified Codex and client configuration validation to support uvx-based setup - Removed unused server source directory handling and related preferences - Updated tests to verify uvx command generation * Cleanup the temp folders created by tests We don't commit temp folders, tests are expected to clean up after themselves * The test kept failing but the results looked correct, floating point comparisons are not precise * feat: migrate from local server to uvx-based configuration - Replaced local server path detection with uvx-based package installation from git repository - Updated all configuration generators to use structured uvx command parts (command, --from URL, package) - Renamed UV path references to UVX for clarity and consistency - Added GetUvxCommandParts() helper to centralize uvx command generation - Added GetMcpServerGitUrl() to handle git repository URL construction - Updated client configuration validation * refactor: use dynamic package version instead of hardcoded value * Update CI so it only updates the Server folder * feat: implement uvx package source path resolution - Added GetUvxPackageSourcePath method to locate unity-mcp package in uv cache by traversing git checkouts - Replaced hardcoded "Dummy" path in PythonToolSyncProcessor with dynamic path resolution - Added validation for Server directory structure and pyproject.toml to ensure correct package location * refactor: replace Python tool syncing with custom tool registration system - Removed PythonToolsAsset and file-based sync processor in favor of attribute-based tool discovery - Implemented CustomToolRegistrationProcessor with automatic registration on startup and script reload - Added registration enable/disable preference and force re-registration capability * feat: add HTTP transport support and cache management - Implemented HTTP transport option with configurable URL/port alongside existing stdio mode - Added cache management service with menu item to clear uvx package cache - Updated config builder to generate transport-specific arguments and VSCode type field based on selected mode * refactor: simplify HTTP configuration to use URL-based approach - Replaced separate host/port arguments with single --http-url parameter for cleaner configuration - Updated server to parse URL and allow individual host/port overrides when needed - Consolidated HTTP client implementation with connection testing and tool execution support * refactor: standardize transport configuration with explicit --transport flag - Replaced --enable-http-server flag with --transport choice parameter (stdio/http) for clearer intent - Removed redundant HTTP port field from UI since HTTP mode uses the same URL/port as MCP client - Simplified server startup logic by consolidating transport mode determination * refactor: move MCP menu items under Window menu * feat: restructure config generation for HTTP transport mode - Changed HTTP mode to use URL-based configuration instead of command-line arguments - Added proper cleanup of incompatible fields when switching between stdio and HTTP transports - Moved uvx command parsing inside stdio-specific block to avoid unnecessary processing in HTTP mode * feat: add local HTTP server management with Git URL override - Implemented server management service with menu item to start local HTTP server in new terminal window - Added Git URL override setting in advanced configuration to allow custom server source for uvx --from - Integrated server management into service locator with validation for local-only server startup * fix: remove automatic HTTP protocol prefix from URL field - Removed auto-prefixing logic that added "http://" to URLs without protocol - Added placeholder text to guide users on expected URL format - Created dedicated url-field style class for better URL input styling * feat: implement proper MCP session lifecycle with HTTP transport - Added initialize, ping, and disconnect methods to HttpMcpClient for proper MCP protocol session management - Implemented session ID tracking and header management for stateful HTTP connections - Added cross-platform terminal launcher support for Windows and Linux (previously macOS-only) * feat: implement JSON-RPC protocol for MCP tool execution - Added proper JSON-RPC 2.0 request/response handling with request ID tracking - Included MCP protocol headers (version, session ID) for standard compliance - Added error handling for JSON-RPC error responses * feat: improve text wrapping in editor window UI - Added white-space: normal and flex-shrink properties to section headers and override labels to prevent text overflow - Created new help-text style class for consistent formatting of help text elements * refactor: refresh git URL override from EditorPrefs on validation * fix: improve responsive layout for editor window settings - Added flex-wrap to setting rows to prevent overflow on narrow windows - Set flex-shrink: 0 on labels to maintain consistent width - Replaced max-width and margin-left with flex-basis for better flex behavior * refactor: improve thread safety in tool registration - Capture Unity API calls on main thread before async operations to prevent threading issues - Update RegisterAllTools to use Task.Run pattern instead of GetAwaiter().GetResult() to avoid potential deadlocks - Add optional projectId parameter to RegisterAllToolsAsync for pre-captured values * refactor: replace MCP tool calls with direct HTTP endpoints for tool registration - Removed synchronous registration method and unused MCP bridge logic from CustomToolRegistrationService - Changed tool registration to use direct HTTP POST to /register-tools endpoint instead of MCP protocol - Added FastAPI HTTP routes alongside existing MCP tools for more flexible tool management access * refactor: centralize HTTP endpoint URL management - Created HttpEndpointUtility to normalize and manage base URLs consistently - Replaced scattered EditorPrefs calls with utility methods that handle URL normalization - Ensured base URL storage excludes trailing paths like "/mcp" for cleaner configuration * refactor: simplify custom tools management with in-memory registry - Removed CustomToolsManager and fastmcp_tool_registry modules in favor of inline implementation - Replaced class-based tool management with direct HTTP route handlers using FastMCP's custom_route decorator - Consolidated tool registration logic into simple dictionary-based storage with helper functions * feat: add dynamic custom tool registration system - Implemented CustomToolService to manage project-scoped tool registration with validation and conflict detection - Added HTTP endpoints for registering, listing, and unregistering custom tools with proper error handling - Converted health and registry endpoints from HTTP routes to MCP tools for better integration * feat: add AutoRegister flag to control tool registration - Added AutoRegister property to McpForUnityToolAttribute (defaults to true) - Modified registration service to filter and only register tools with AutoRegister enabled - Disabled auto-registration for all built-in tools that already exist server-side * feat: add function signature generation for dynamic tools - Implemented _build_signature method to create proper inspect.Signature objects for dynamically created tools - Signature includes Context parameter and all tool parameters with correct required/optional defaults - Attached generated signature to dynamic_tool functions to improve introspection and type checking * refactor: remove unused custom tool registry endpoints * test: add transport configuration validation for MCP client tests - Added HTTP transport preference setup in test fixtures to ensure consistent behavior - Implemented AssertTransportConfiguration helper to validate both HTTP and stdio transport modes - Added tests to verify stdio transport fallback when HTTP preference is disabled * refactor: simplify uvx path resolution to use PATH by default - Removed complex platform-specific path detection logic and verification - Changed to rely on system PATH environment variable instead of searching common installation locations - Streamlined override handling to only use EditorPrefs when explicitly set by user * feat: use serverUrl property for Windsurf HTTP transport - Changed Windsurf configs to use "serverUrl" instead of "url" for HTTP transport to match Windsurf's expected format - Added cleanup logic to remove stale transport properties when switching between HTTP and stdio modes - Updated Windsurf to exclude "env" block (only required for Kiro), while preserving it for clients that need it * feat: ensure client configurations stay current on each setup - Removed skip logic for already-configured clients to force re-validation of core fields - Added forced re-registration for ClaudeCode clients to keep transport settings up-to-date * feat: add automatic migration for legacy embedded server configuration - Created LegacyServerSrcMigration to detect and migrate old EditorPrefs keys on startup - Automatically reconfigures all detected clients to use new uvx/stdio path - Removes legacy keys only after successful migration to prevent data loss * feat: add automatic stdio config migration on package updates - Implemented StdIoVersionMigration to detect package version changes and refresh stdio MCP client configurations - Added support for detecting stdio usage across different client types (Codex, VSCode, and generic JSON configs) - Integrated version tracking via EditorPrefs to prevent redundant migrations * Centralize where editor prefs are defined It's really hard to get a view of all the editor prfes in use. This should help humans and AI know what's going on at a glance * Update custom tools docs * refactor: consolidate server management UI into main editor window - Removed server and maintenance menu items from top-level menu - Moved "Start Local HTTP Server" and "Clear UVX Cache" buttons into editor window settings - Added dynamic button state management based on transport protocol and server availability * Don't show error logs when custom tools are already registerd with the server * Only autoconnect to port 6400 if the user is using stdio for connections * Don't double register tools on startup * feat: switch to HTTP transport as default connection method - Changed default transport from stdio to HTTP with server running on localhost:8080 - Added UI controls to start/stop local HTTP server directly from Unity window - Updated all documentation and configuration examples to reflect HTTP-first approach with stdio as fallback option * Automatically bump the versions in the READMEs. The `main` branch gets updated before we do a release. Using versions helps users get a stable, tested installation * docs: add HTTP transport configuration examples - Added HTTP transport setup instructions alongside existing stdio examples - Included port mapping and URL configuration for Docker deployments - Reorganized client configuration sections to clearly distinguish between HTTP and stdio transports * feat: add WebSocket-based plugin hub for Unity connections - Implemented persistent WebSocket connections with session management, heartbeat monitoring, and command routing - Created PluginRegistry for tracking active Unity instances with hash-based lookup and automatic reconnect handling - Added HTTP endpoints for session listing and health checks, plus middleware integration for instance-based routing * refactor: consolidate Unity instance discovery with shared registry - Introduced StdioPortRegistry for centralized caching of Unity instance discovery results - Refactored UnityConnection to use stdio_port_registry instead of direct PortDiscovery calls - Improved error handling with specific exception types and enhanced logging clarity * Use websockets so that local and remote MCP servers can communicate with Unity The MCP server supports HTTP and stdio protocols, and the MCP clients use them to communicate. However, communication from the MCP server to Unity is done on the local port 6400, that's somewhat hardcoded. So we add websockets so oure remotely hosted MCP server has a valid connection to the Unity plugin, and can communicate with - Created ProjectIdentityUtility for centralized project hash, name, and session ID management - Moved command processing logic from MCPForUnityBridge to new TransportCommandDispatcher service - Added WebSocket session ID and URL override constants to EditorPrefKeys - Simplified command queue processing with async/await pattern and timeout handling - Removed duplicate command execution code in favor of shared dispatcher implementation * refactor: simplify port management and improve port field validation - Removed automatic port discovery and fallback logic from GetPortWithFallback() - Changed GetPortWithFallback() to return stored port or default without availability checks - Added SetPreferredPort() method for explicit port persistence with validation - Replaced Debug.Log calls with McpLog.Info/Warn for consistent logging - Added port field validation on blur and Enter key press with error handling - Removed automatic port waiting * Launch the actual local webserver via the button * Autoformat * Minor fixes so the server can start * Make clear uvx button work * Don't show a dialog after clearing cache/starting server successfully It's annoying, we can just log when successful, and popup if something failed * We no longer need a Python importer * This folder has nothing in it * Cleanup whitespace Most AI generated code contains extra space, unless they're hooked up to a linter. So I'm just cleaning up what's there * We no longer need this folder * refactor: move MCPForUnityBridge to StdioBridgeHost and reorganize transport layer - Renamed MCPForUnityBridge class to StdioBridgeHost and moved to Services.Transport.Transports namespace - Updated all references to StdioBridgeHost throughout codebase (BridgeControlService, TelemetryHelper, GitHub workflow) - Changed telemetry bridge_version to use AssetPathUtility.GetPackageVersion() instead of hardcoded version - Removed extensive inline comments and documentation throughout StdioBridgeHost * Skip tools registration if the user is not connected to an HTTP server * Fix VS Code configured status in UI Serializing the config as dynamic and then reading null properties (in this case, args) caused the error. So we just walk through the properities and use JObject, handling null value explicitily * Stop blocking the main thread when connecting via HTTP Now that the bridge service is asynchronous, messages back and forth the server work well (including the websocket connection) * Separate socket keep-alive interval from application keep-alive interval Split the keep-alive configuration into two distinct intervals: _keepAliveInterval for application-level keep-alive and _socketKeepAliveInterval for WebSocket-level keep-alive. This allows independent control of socket timeout behavior based on server configuration while maintaining the application's keep-alive settings. * Add a debug log line * Fix McpLog.Debug method, so it actually reads the checkbox value from the user * Add HTTP bridge auto-resume after domain reload Implement HttpBridgeReloadHandler to automatically resume HTTP/HttpPush transports after Unity domain reloads, matching the behavior of the legacy stdio bridge. Add ResumeHttpAfterReload EditorPref key to persist state across reloads and expose ActiveMode property in IBridgeControlService to check current transport mode. * Add health verification after HTTP bridge auto-resume Trigger health check in all open MCPForUnityEditorWindow instances after successful HTTP bridge resume following domain reload. Track open windows using static HashSet and schedule async health verification via EditorApplication.delayCall to ensure UI updates reflect the restored connection state. * Add name and path fields to code coverage settings Initialize m_Name and m_Path fields in code coverage Settings.json to match Unity's expected settings file structure. * Only register custom tools AFTER we established a healthy HTTP connection * Convert custom tool handlers to async functions Update dynamic_tool wrapper to use async/await pattern and replace synchronous send_with_unity_instance/send_command_with_retry calls with their async counterparts (async_send_with_unity_instance/async_send_command_with_retry). * Correctly parse responses from Unity in the server so tools and resources can process them We also move the logic to better places than the __init__.py file for tools, since they're shared across many files, including resources * Make some clarifications for custom tools in docs * Use `async_send_with_unity_instance` instead of `send_with_unity_instance` The HTTP protocol doesn't working with blocking commands, so now we have our tools set up to work with HTTP and stdio fullly. It's coming together :-) * Fix calls to async_send_with_unity_instance in manage_script * Rename async_send_with_unity_instance to send_with_unity_instance * Fix clear uv cache command Helps a lot with local development * Refactor HTTP server command generation into reusable method and display in UI Extract HTTP server command building logic from StartLocalHttpServer into new TryGetLocalHttpServerCommand method. Add collapsible foldout in editor window to display the generated command with copy button, allowing users to manually start the server if preferred. Update UI state management to refresh command display when transport or URL settings change. * Ctrl/Cmd + Shift + M now toggles the window Might as well be able to close the window as well * Fallback to a git URL that points to the main branch for the MCP git URL used by uvx * Add test setup/teardown to preserve and reset Git URL override EditorPref Implement OneTimeSetUp/OneTimeTearDown to save and restore the GitUrlOverride EditorPref state, and add SetUp to delete the key before each test. This ensures tests run with deterministic Git URLs while preserving developer overrides between test runs. * Update docs, scripts and GH workflows to use the new MCP server code location * Update plugin README * Convert integration tests to async/await pattern Update all integration tests to use pytest.mark.asyncio decorator and async/await syntax. Change test functions to async, update fake_send/fake_read mocks to async functions with **kwargs parameter, and patch async_send_command_with_retry instead of send_command_with_retry. Add await to all tool function calls that now return coroutines. * Update image with new UI * Remove unused HttpTransportClient client Before I had the realization that I needed webscokets, this was my first attempt * Remove copyright notice * Add a guide to all the changes made for this version A lot of code was written by AI, so I think it's important that humans can step through how all these new systems work, and know where to find things. All of these docs were written by hand, as a way to vet that I understand what the code I wrote and generated are doing, but also to make ti easy to read for you. * Organize imports and remove redundant import statements Clean up import organization by moving imports to the top of the file, removing duplicate imports scattered throughout the code, and sorting imports alphabetically within their groups (standard library, third-party, local). Remove unnecessary import aliases and consolidate duplicate urlparse and time imports. * Minor edits * Fix stdio serializer to use the new type parameter like HTTP * Fix: Automatic bridge reconnection after domain reload without requiring Unity focus - Add immediate restart attempt in OnAfterAssemblyReload() when Unity is not compiling - Enhanced compile detection to check both EditorApplication.isCompiling and CompilationPipeline.isCompiling - Add brief port release wait in StdioBridgeHost before switching ports to reduce port thrash - Fallback to delayCall/update loop only when Unity is actively compiling This fixes the issue where domain reloads (e.g., script edits) would cause connection loss until Unity window was refocused, as EditorApplication.update only fires when Unity has focus. * Make the server work in Docker We use HTTP mode by default in docker, this is what will be hosted remotely if one chooses to. We needed to update the uvicorn package to a version with websockets, at least so the right version is explicitly retrieved * Cache project identity on initialization to avoid repeated computation Add static constructor with [InitializeOnLoad] attribute to cache project hash and name at startup. Introduce volatile _identityCached flag and cached values (_cachedProjectName, _cachedProjectHash) to store computed identity. Schedule cache refresh on initialization and when project changes via EditorApplication.projectChanged event. Extract ComputeProjectHash and ComputeProjectName as private methods that perform the actual computation. Update public * Fix typos * Add unity_instance_middleware to py-modules list in pyproject.toml * Remove Foldout UI elements and simplify HTTP server command section Replace Foldout with VisualElement for http-server-command-section to display HTTP server command directly without collapsible wrapper. Remove unused manualConfigFoldout field and associated CSS styles. Remove unused _identityCached volatile flag from ProjectIdentityUtility as caching logic no longer requires it. * Reduce height of HTTP command box * Refresh HTTP server command display when Git URL override changes * Make the box a bit smaller * Split up main window into various components Trying to avoid to monolithic files, this is easier to work, for humans and LLMs * Update the setup wizard to be a simple setup popup built with UI toolkit We also fix the Python/uv detectors. Instead of searching for binaries, we just test that they're available in the PATH * Ensure that MCP configs are updated when users switch between HTTP and stdio These only work for JSON configs, we'll have to handle Codex and Claude Code separately * Detect Codex configuration when using HTTP or stdio configs * Use Claude Code's list command to detect whether this MCP is configured It's better than checking the JSON and it can verify both HTTP and stdio setups * Fix and add tests for building configs * Handle Unity reload gaps by retrying plugin session resolution * Add polling support for long-running tools with state persistence Introduce polling middleware to handle long-running operations that may span domain reloads. Add McpJobStateStore utility to persist tool state in Library folder across reloads. Extend McpForUnityToolAttribute with RequiresPolling and PollAction properties. Update Response helper with Pending method for standardized polling responses. Implement Python-side polling logic in custom_tool_service.py with configurable intervals and 10-minute timeout. * Polish domain reload resilience tests and docs * Refactor Response helper to use strongly-typed classes instead of anonymous objects Replace static Response.Success/Error/Pending methods with SuccessResponse, ErrorResponse, and PendingResponse classes. Add IMcpResponse interface for type safety. Include JsonProperty attributes for serialization and JsonIgnore properties for backward compatibility with reflection-based tests. Update all tool and resource classes to use new response types. * Rename Setup Wizard to Setup Window and improve UV detection on macOS/Linux Rename SetupWizard class to SetupWindowService and update all references throughout the codebase. Implement platform-specific UV detection for macOS and Linux with augmented PATH support, including TryValidateUv methods and BuildAugmentedPath helpers. Split single "Open Installation Links" button into separate Python and UV install buttons. Update UI styling to improve installation section layout with proper containers and button * Update guide on what's changed in v8 Lots of feedback, lots of changes * Update custom tool docs to use new response objects * Update image used in README Slightly more up to date but not final * Restructure backend Just make it more organized, like typical Python projects * Remove server_version.txt * Feature/http instance routing (#5) * Fix HTTP instance routing and per-project session IDs * Drop confusing log message * Ensure lock file references later version of uvicorn with key fixes * Fix test imports * Update refs in docs --------- Co-authored-by: David Sarno <david@lighthaus.us> * Generate the session ID from the server We also make the identifying hashes longer * Force LLMs to choose a Unity instance when multiple are connected OK, this is outright the best OSS Unity MCP available * Fix tests caused by changes in session management * Whitespace update * Exclude stale builds so users always get the latest version * Set Pythonpath env var so Python looks at the src folder for modules Not required for the fix, but it's a good guarantee regardless of the working directory * Replace Optional type hints with modern union syntax (Type | None) Update all Optional[Type] annotations to use the PEP 604 union syntax Type | None throughout the transport layer and mcp_source.py script * Replace Dict type hints with modern dict syntax throughout codebase Update all Dict[K, V] annotations to use the built-in dict[K, V] syntax across services, transport layer, and models for consistency with PEP 585 * Remove unused type imports across codebase Clean up unused imports of Dict, List, and Path types that are no longer needed after migration to modern type hint syntax * Remove the old telemetry test It's working, we have a better integration test in any case * Clean up stupid imports No AI slop here lol * Replace dict-based session data with Pydantic models for type safety Introduce Pydantic models for all WebSocket messages and session data structures. Replace dict.get() calls with direct attribute access throughout the codebase. Add validation and error handling for incoming messages in PluginHub. * Correctly call `ctx.info` with `await` No AI slop here! * Replace printf-style logging with f-string formatting across transport and telemetry modules Convert all logger calls using %-style string formatting to use f-strings for consistency with modern Python practices. Update telemetry configuration logging, port discovery debug messages, and Unity connection logging throughout the codebase. * Register custom tools via websockets Since we'll end up using websockets for HTTP and stdio, this will ensure custom tools are available to both. We want to compartmentalize the custom tools to the session. Custom tools in 1 unity project don't apply to another one. To work with our multi-instance logic, we hide the custom tools behind a custom tool function tool. This is the execute_custom_tool function. The downside is that the LLM has to query before using it. The upside is that the execute_custom_tool function goes through the standard routing in plugin_hub, so custom tools are always isolated by project. * Add logging decorator to track tool and resource execution with arguments and return values Create a new logging_decorator module that wraps both sync and async functions to log their inputs, outputs, and exceptions. Apply this decorator to all tools and resources before the telemetry decorator to provide detailed execution traces for debugging. * Fix JSONResponse serialization by converting Pydantic model to dict in plugin sessions endpoint * Whitespace * Move import of get_unity_instance_from_context to module level in unity_transport Relocate the import from inside the with_unity_instance decorator function to the top of the file with other imports for better code organization and to avoid repeated imports on each decorator call. * Remove the tool that reads resources They don't perform well at all, and confuses the models most times. However, if they're required, we'll revert * We have buttons for starting and stopping local servers Instead of a button to clear uv cache, we have start and stop buttons. The start button pulls the latest version of the server as well. The stop button finds the local process of the server and kills. Need to test on Windows but it works well * Consolidate cache management into ServerManagementService and remove standalone CacheManagementService Move the ClearUvxCache method from CacheManagementService into ServerManagementService since cache clearing is primarily used during server operations. Remove the separate ICacheManagementService interface and CacheManagementService class along with their service locator registration. Update StartLocalServer to call the local ClearUvxCache method instead of going through the service locator. * Update MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update .github/workflows/claude-nl-suite.yml Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Cancel existing background loops before starting a new connection Nice bug found from CodeRabbit * Try to kill all processes using the port of the local webserver * Some better error handling when stopping a server * Cache fallback session ID to maintain consistency when EditorPrefs are unavailable Store the fallback session ID in a static field instead of generating a new GUID on each call when EditorPrefs are unavailable during batch tests. Clear the cached fallback ID when resetting the session to ensure a fresh ID is generated on the next session. * Clean up empty parent temp folder after domain reload tests complete Check if Assets/Temp folder is empty after deleting test-specific temp directories and remove it if no other files or directories remain. Also remove trailing blank lines from the file. * Minor fixes * Change "UV" to "uv" in strings. Capitlization looks weird * Rename functions that capitalized "UV" * Ensure WebSocket transport is properly stopped before disposing shared resources Add disposal guard and call StopAsync() in Dispose() to prevent race conditions when disposing the WebSocket transport while background loops are still running. Log warnings if cleanup fails but continue with resource disposal. * Replace volatile bool with Interlocked operations for reconnection flag to prevent race conditions * Replace byte array allocation with ArrayPool to reduce GC pressure in WebSocket message receiving Rent buffer from ArrayPool<byte>.Shared instead of allocating new byte arrays for each receive operation. Pre-size MemoryStream to 8192 bytes and ensure rented buffer is returned in finally block to prevent memory leaks. * Consolidate some of the update/refresh logic * UI tweak disable start/stop buttons while they code is being fired * Add error dialog when Unity socket port persistence fails * Rename WebSocketSessionId to SessionId in EditorPrefKeys By the next version stdio will use Websockets as well, so why be redundant * No need to send session ID in pong payload * Add a debug message when we don't have an override for the uvx path * Remove unused function * Remove the unused verifyPath argument * Simplify server management logic * Remove unused `GetUvxCommand()` function We construct it in parts now * Remove `IsUvxDetected()` The flow changed so it checks editor prefs and then defaults to the command line default. So it's always true. * Add input validation and improve shell escaping in CreateTerminalProcessStartInfo - Validate command is not empty before processing - Strip carriage returns and newlines from command - macOS: Use osascript directly instead of bash to avoid shell injection, escape backslashes and quotes for AppleScript - Windows: Add window title and escape quotes in command - Linux: Properly escape single quotes for bash -c and double quotes for process arguments * Update technical changes guide * Add custom_tools resource and execute_custom_tool to README documentation * Update v8 docs * Update docs UI image * Handle when properties are sent as a JSON string in manage_asset * Fix backend tests --------- Co-authored-by: David Sarno <david@lighthaus.us> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores