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
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.
1 similar comment
![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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
UnityMcpBridge/Editor/Tools/ManageScript.cs (6)
54-101: Harden symlink traversal checks to include ancestor directories under Assets.Currently only the final target dir is checked for a reparse point. A symlinked ancestor within Assets (e.g., Assets/Scripts linked) would pass and could route writes through a symlinked path segment. Consider rejecting if any ancestor under Assets is a reparse point as well.
Apply this diff within TryResolveUnderAssets to scan ancestors:
- // Best-effort symlink guard: if directory is a reparse point/symlink, reject + // Best-effort symlink guard: if directory or any ancestor under Assets 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; - } - } + var di = new DirectoryInfo(full); + for (var d = di; d != null; d = d.Parent) + { + var dn = d.FullName.Replace('\\', '/'); + if (!dn.StartsWith(assets, StringComparison.OrdinalIgnoreCase)) break; + if (!d.Exists) continue; + var attrs = d.Attributes; + if ((attrs & FileAttributes.ReparsePoint) != 0) + { + fullPathDir = null; + relPathSafe = null; + return false; + } + } } catch { /* best effort; proceed */ }
203-231: Validate action: level handling and diagnostics shaping are clear.Explicit mapping for basic/standard/comprehensive/strict and returning normalized diagnostics is clean. Consider including a quick “isValid” boolean in the response data for clients that only need pass/fail.
- var result = new { diagnostics = diags }; + var result = new { isValid = ok, diagnostics = diags };
454-491: apply_text_edits: transactional, bounded, and validated; minor enhancements to consider.This is robust: index translation with CRLF support, stale-file precondition, payload bounds, overlap detection, optional Roslyn validation, atomic write, and refresh scheduling. Two small improvements you might consider:
- Include the new file SHA in stale_file responses to help clients reconcile faster (you already do; good), and also echo the file URI consistently for parity with success.
- When edits are many but each tiny, totalBytes may be low but the final file diff large; consider an optional guard on the number of edits to prevent pathological batches.
- return Response.Error("stale_file", new { status = "stale_file", expected_sha256 = preconditionSha256, current_sha256 = currentSha }); + return Response.Error("stale_file", new { + status = "stale_file", + expected_sha256 = preconditionSha256, + current_sha256 = currentSha, + uri = $"unity://path/{relativePath}" + });Also applies to: 523-535, 577-614
767-806: Structured edits: solid design; add one guard for replacement overlap in sequential mode.The AST-backed where available + balanced fallback makes this broadly useful. In sequential mode, since edits mutate “working” as you go, overlapping edit intents may still produce surprising results. If this becomes an issue with agents, consider an optional “disallowOverlaps” in options that pre-computes spans and rejects if overlaps are detected even when applySequentially=true.
If desired, I can draft the option parsing and early-span computation.
Also applies to: 997-1010, 1012-1066
1118-1123: NormalizeNewlines: good for structured edits; consider using it for apply_text_edits too.Structured edits normalize to ‘\n’, but apply_text_edits does not. If upstream line/col was computed on normalized text, edits on CRLF files may be harder to reason about cross-platform. Optional: normalize before applying spans and then re-normalize (or preserve original newline style).
I can wire this behind an option (e.g., options.normalizeNewlines=true) or make it consistent across both paths.
1184-1221: Balanced-scan fallbacks: resilient but regex anchors are still a bit optimistic.The manual class/method scanners are careful with strings/comments and brace depth. The method header regex is already anchored and allows attributes. If this sees a lot of real-world variability (e.g., generics, constraints, attributes on the same line as visibility), consider compiling the regex and tightening with word boundaries around identifiers to reduce accidental matches.
Example tweak for name and return type tokens:
- string rtPattern = string.IsNullOrEmpty(returnType) ? @"[^\s]+" : Regex.Escape(returnType).Replace("\\ ", "\\s+"); - string namePattern = Regex.Escape(methodName); + string rtPattern = string.IsNullOrEmpty(returnType) ? @"[^\s]+" : $@"\b{Regex.Escape(returnType).Replace("\\ ", "\\s+")}\b"; + string namePattern = $@"\b{Regex.Escape(methodName)}\b";Also applies to: 1396-1423, 1437-1473
📜 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(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
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 (6)
UnityMcpBridge/Editor/Tools/ManageScript.cs (6)
189-195: Good deprecation strategy with backward-compatible forwarding.The deprecated actions read/update/edit now warn and forward to the modern flows, preserving compatibility while nudging clients. This unblocks existing tooling without breaking. Nice.
Also applies to: 232-236
151-161: Path canonicalization and file path construction look solid.Using TryResolveUnderAssets, then building full and relative “Assets/…” paths with normalized separators avoids traversal and inconsistent casing. Good guardrails here.
299-326: Create: atomic write and deferred refresh are correct.Writing to a temp file, then move/copy fallback, and scheduling refresh after responding minimizes editor stalls and avoids half-written files. LGTM.
402-447: Update: safe replace with backup cleanup and debounced refresh are well-implemented.This path handles cross-volume fallbacks and avoids leaving .bak artifacts in Assets. The pre-reply Response before scheduling import avoids timing issues with domain reload. Good defensive coding.
621-654: CRLF-aware line/col indexing fix looks correct.Treating CRLF as a single newline and returning EOF positions safely prevents Windows off-by-one. This will reduce edit misalignment. Nice.
2177-2252: Debounced refresh: thread-safe and main-thread compliant.Using Interlocked + lock for path accumulation, windowing on last-request, and delayCall for main-thread imports avoids redundant compiles and ensures correctness. LGTM.
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.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores