Feat/nl edits ai planner prep #226
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
…ard-compatibility
…ard-compatibility-g252rq
…n-c#-and-python
…ow-in-byte-count
…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
…xt ops; preview+confirm for regex; safe_script_edit wrapper; immediate compile & verification; header-safe anchors
…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.
…add SHA precondition and immediate refresh; keep verification slice; minor test and uv.lock updates
…cription with canonical fields & examples; alias/wrapper normalization; machine-parsable validation hints; auto applyMode=sequential for mixed insert+replace; echo normalizedEdits
…edit'; add routing='text' for pure text; echo normalizedEdits; C#: include 'code' in error payloads
|
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. WalkthroughAdds macOS Claude Desktop config path support; changes editor-server transport to an 8‑byte big‑endian framed protocol with handshake, limits, and timeouts; extends editor script tooling with secure atomic/text/structured edits, validation levels, debounced refresh, and URI responses; hardens Python MCP server lifecycle, resource APIs, and connection retry/framing; and adds tests/CI for framing, resources, and NL-driven edit workflows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PythonClient as UnityConnection (Python)
participant Editor as UnityMcpBridge (Editor)
Note over PythonClient,Editor: Framed handshake and request/response
PythonClient->>Editor: TCP connect
Editor-->>PythonClient: "WELCOME UNITY-MCP 1 FRAMING=1"
PythonClient->>PythonClient: set use_framing = true
PythonClient->>Editor: [8-byte BE len][JSON command]
Editor->>Editor: ReadExact(header+payload), validate size
Editor->>Editor: Dispatch to handler (ManageScript/ManageEditor/...)
Editor-->>PythonClient: [8-byte BE len][JSON response]
sequenceDiagram
autonumber
participant Tool as manage_script_edits (Python)
participant Conn as send_command_with_retry
participant Editor as UnityMcpBridge
participant ManageScript as ManageScript (Editor)
participant FS as File System
Note over Tool,FS: apply_text_edits flow with precondition and refresh
Tool->>Conn: apply_text_edits(uri, edits, precondition_sha256)
Conn->>Editor: framed command
Editor->>ManageScript: HandleCommand: apply_text_edits
ManageScript->>FS: Read & validate, atomic write (.tmp -> replace), compute sha256
ManageScript-->>Editor: { success, uri, sha256, scheduledRefresh }
Editor-->>Conn: framed response
Conn-->>Tool: result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR prepares the Unity MCP Server for AI planner integration by implementing sophisticated script editing capabilities, transport framing support, and enhanced tool infrastructure. The changes focus on surgical code modifications rather than full-file replacements, improved networking reliability, and comprehensive test coverage for key functionality.
- Enhanced script editing with surgical text operations and structured AST-backed edits
- Implemented strict transport framing with handshake negotiation for reliable communication
- Added comprehensive test infrastructure with mock Unity server simulations
Reviewed Changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_transport_framing.py | Tests for framed transport protocol with handshake validation |
| tests/test_script_tools.py | Unit tests for script management tools with dependency mocking |
| tests/test_script_editing.py | Placeholder tests for comprehensive script editing workflows |
| tests/test_resources_api.py | Tests for resource listing and path traversal protection |
| tests/test_logging_stdout.py | AST-based validation ensuring no stdout pollution in server code |
| test_unity_socket_framing.py | Standalone framing protocol test utility |
| UnityMcpBridge/UnityMcpServer~/src/unity_connection.py | Enhanced connection handling with strict framing support |
| UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py | Resource wrapper tools for file listing and reading |
| UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py | Surgical script editing with structured operations |
| UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py | Enhanced script management with text edit support |
| UnityMcpBridge/UnityMcpServer~/src/tools/init.py | Updated tool registration with script edits priority |
| UnityMcpBridge/UnityMcpServer~/src/server.py | Enhanced logging and MCP resources support |
| UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json | Type checking configuration for development |
| UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs | Enhanced config management with atomic writes and BOM handling |
| UnityMcpBridge/Editor/UnityMcpBridge.cs | Strict framing implementation in Unity C# bridge |
| UnityMcpBridge/Editor/Tools/ManageScript.cs | Comprehensive script editing with validation and atomic operations |
| UnityMcpBridge/Editor/Tools/ManageEditor.cs | Added project root discovery functionality |
| UnityMcpBridge/Editor/Models/McpClient.cs | Added macOS-specific config path support |
| UnityMcpBridge/Editor/Helpers/Response.cs | Enhanced error responses with machine-parsable codes |
| UnityMcpBridge/Editor/Data/McpClients.cs | Added macOS config path for Claude Desktop |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -0,0 +1,94 @@ | |||
| #!/usr/bin/env python3 | |||
| import socket, struct, json, sys | |||
There was a problem hiding this comment.
Multiple imports on a single line violate PEP 8 style guidelines. Consider separating these into individual import statements.
| import socket, struct, json, sys | |
| import socket | |
| import struct | |
| import json | |
| import sys |
Copilot uses AI. Check for mistakes.
|
|
||
| # Always include the canonical spec resource so NL clients can discover it | ||
| if "unity://spec/script-edits" not in matches: | ||
| matches.append("unity://spec/script-edits") |
There was a problem hiding this comment.
The hardcoded URI string is duplicated in multiple places. Consider defining it as a constant to improve maintainability and reduce the risk of typos.
| matches.append("unity://spec/script-edits") | |
| matches.append(f"{UNITY_PATH_PREFIX}{rel}") | |
| if len(matches) >= max(1, limit): | |
| break | |
| # Always include the canonical spec resource so NL clients can discover it | |
| if UNITY_SPEC_SCRIPT_EDITS not in matches: | |
| matches.append(UNITY_SPEC_SCRIPT_EDITS) |
Copilot uses AI. Check for mistakes.
| logger.propagate = False | ||
| except Exception: | ||
| # If file logging setup fails, continue with stderr logging only | ||
| pass |
There was a problem hiding this comment.
Catching all exceptions with a bare 'except Exception:' can mask important errors. Consider catching more specific exceptions or at least logging the exception for debugging purposes.
| pass | |
| except Exception as e: | |
| # If file logging setup fails, continue with stderr logging only | |
| logger.warning(f"File logging setup failed: {e}") |
Copilot uses AI. Check for mistakes.
| > commandQueue = new(); | ||
| private static int currentUnityPort = 6400; // Dynamic port, starts with default | ||
| private static bool isAutoConnectMode = false; | ||
| private const ulong MaxFrameBytes = 64UL * 1024 * 1024; // 64 MiB hard cap for framed payloads |
There was a problem hiding this comment.
The magic number 64 MiB should be configurable or at least documented with rationale for this specific limit. Consider making it a configurable parameter.
| private const ulong MaxFrameBytes = 64UL * 1024 * 1024; // 64 MiB hard cap for framed payloads | |
| // Maximum allowed frame size for payloads. Default is 64 MiB, which is a balance between supporting large assets and preventing excessive memory usage or abuse. | |
| // This value can be configured via EditorPrefs ("UnityMCP.MaxFrameBytes"). | |
| private static ulong MaxFrameBytes | |
| { | |
| get | |
| { | |
| // Read from EditorPrefs, fallback to default (64 MiB) | |
| try | |
| { | |
| // EditorPrefs only supports int, so store as MB | |
| int mb = EditorPrefs.GetInt("UnityMCP.MaxFrameBytesMB", 64); | |
| return (ulong)mb * 1024 * 1024; | |
| } | |
| catch | |
| { | |
| return 64UL * 1024 * 1024; | |
| } | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
| /// <summary> | ||
| /// Apply simple text edits specified by line/column ranges. Applies transactionally and validates result. | ||
| /// </summary> | ||
| private const int MaxEditPayloadBytes = 15 * 1024; |
There was a problem hiding this comment.
[nitpick] The 15KB limit for edit payloads seems arbitrary and could be restrictive for legitimate use cases. Consider making this configurable or providing clear documentation about why this limit exists.
| private const int MaxEditPayloadBytes = 15 * 1024; | |
| /// <summary> | |
| /// Maximum allowed size (in bytes) for edit payloads. Default is 15KB. | |
| /// This limit helps prevent excessive memory usage and potential performance issues. | |
| /// You can override this value via Unity EditorPrefs key "UnityMcpBridge.MaxEditPayloadBytes". | |
| /// </summary> | |
| private static int MaxEditPayloadBytes | |
| { | |
| get | |
| { | |
| // Try to get from EditorPrefs, fallback to default | |
| int value = 15 * 1024; | |
| if (EditorPrefs.HasKey("UnityMcpBridge.MaxEditPayloadBytes")) | |
| { | |
| value = EditorPrefs.GetInt("UnityMcpBridge.MaxEditPayloadBytes", value); | |
| } | |
| return value; | |
| } | |
| } | |
| /// Maximum allowed size (in bytes) for edit payloads. Default is 15KB. | |
| /// This limit helps prevent excessive memory usage and potential performance issues. | |
| /// You can override this value via Unity EditorPrefs key "UnityMcpBridge.MaxEditPayloadBytes". | |
| /// </summary> | |
| private static int MaxEditPayloadBytes | |
| { | |
| get | |
| { | |
| // Try to get from EditorPrefs, fallback to default | |
| int value = 15 * 1024; | |
| if (EditorPrefs.HasKey("UnityMcpBridge.MaxEditPayloadBytes")) | |
| { | |
| value = EditorPrefs.GetInt("UnityMcpBridge.MaxEditPayloadBytes", value); | |
| } | |
| return value; | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
|
|
||
| // 2) Canonical args order | ||
| var newArgs = new[] { "run", "--directory", serverSrc, "server.py" }; | ||
| // 2) Canonical args order (add quiet flag to prevent stdout noise breaking MCP stdio) |
There was a problem hiding this comment.
[nitpick] The addition of the '-q' flag is mentioned in the comment but the flag's purpose (preventing stdout noise) could be better documented for future maintainers.
| // 2) Canonical args order (add quiet flag to prevent stdout noise breaking MCP stdio) | |
| // 2) Canonical args order | |
| // The '-q' (quiet) flag is required to suppress extraneous output from the UV package manager. | |
| // Without this flag, UV may emit status or progress messages to stdout, which can interfere with | |
| // the MCP protocol's standard input/output communication and cause protocol errors or failures. | |
| // Future maintainers: Do not remove this flag unless you are certain that MCP can tolerate | |
| // additional output on stdout from UV. |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
1637-1663: Path match uses substring Contains; switch to exact comparison of the --directory argUsing substring Contains can yield false positives (e.g., overlapping paths). You already have ExtractDirectoryArg and PathsEqual — use them for an exact, normalized comparison.
Apply this diff:
- bool matches = pythonDir != null && Array.Exists(args, arg => arg.Contains(pythonDir, StringComparison.Ordinal)); + string dirArg = ExtractDirectoryArg(args); + bool matches = pythonDir != null + && !string.IsNullOrEmpty(dirArg) + && PathsEqual(dirArg, pythonDir);
🧹 Nitpick comments (41)
UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json (2)
3-3: Consider not fully disabling missing-import diagnosticsSetting reportMissingImports to "none" can hide real dependency or path issues. Two alternatives:
- Set it to "warning" to surface issues without failing CI.
- Keep "none" but configure resolution so you can enable it later (e.g., via venvPath/venv or extraPaths).
If you want, I can help wire this to your venv layout.
4-4: Avoid redundant pythonVersion declarationsYou set pythonVersion at the top and again inside the execution environment. Redundant but harmless. To keep things DRY, drop the per-environment pythonVersion so it inherits from the top-level.
Apply this diff:
{ "typeCheckingMode": "basic", "reportMissingImports": "none", "pythonVersion": "3.11", "executionEnvironments": [ { "root": ".", - "pythonVersion": "3.11" } ] }Also applies to: 8-9
UnityMcpBridge/Editor/Data/McpClients.cs (1)
72-78: Consider parity: add macConfigPath for other clients (VSCode, Windsurf, etc.)Optional, but adding macConfigPath consistently across clients would remove OS-branching quirks in downstream selection and improve UX on macOS.
Example snippets you could add in the respective initializers:
- VSCode GitHub Copilot:
macConfigPath = Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.Personal), "Library", "Application Support", "Code", "User", "mcp.json" ),
- Windsurf:
macConfigPath = Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.Personal), "Library", "Application Support", "Codeium", "windsurf", "mcp_config.json" ),
- Cursor and Claude Code likely can just mirror their Linux paths on macOS (under the home dir, not under Library), e.g.:
macConfigPath = Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".cursor", "mcp.json" ),UnityMcpBridge/Editor/Helpers/Response.cs (3)
38-41: XML doc param name mismatchThe XML comment still refers to errorMessage but the method parameter is errorCodeOrMessage. Align the docs.
Apply this diff:
- /// <param name="errorMessage">A message describing the error.</param> + /// <param name="errorCodeOrMessage">An error code or message describing the error.</param> /// <param name="data">Optional additional data (e.g., error details) to include.</param> /// <returns>An object representing the error response.</returns> - public static object Error(string errorCodeOrMessage, object data = null) + public static object Error(string errorCodeOrMessage, object data = null)
49-53: Clarify semantics of code vs. errorRight now code and error are identical. If you plan to carry machine-readable codes (e.g., E123) alongside human-friendly messages, consider introducing an overload to avoid overloading a single parameter.
For example, add an overload (non-breaking) and delegate:
public static object Error(string code, string message, object data = null) { var payload = new Dictionary<string, object> { ["success"] = false, ["code"] = code, ["error"] = message, }; if (data != null) payload["data"] = data; return payload; }Existing callers can keep using Error(string errorCodeOrMessage, object data = null) during the transition.
46-59: Potential breaking change: error payload no longer includes 'message'Downstream code that expects a message field for errors may break. Consider keeping message as an alias to error for one or two releases to preserve compatibility.
Apply this diff to mirror the message field:
if (data != null) { // Note: The key is "error" for error messages, not "message" return new { success = false, - // Preserve original behavior while adding a machine-parsable code field. - // If callers pass a code string, it will be echoed in both code and error. code = errorCodeOrMessage, error = errorCodeOrMessage, + message = errorCodeOrMessage, // back-compat alias data = data, }; } else { - return new { success = false, code = errorCodeOrMessage, error = errorCodeOrMessage }; + return new { success = false, code = errorCodeOrMessage, error = errorCodeOrMessage, message = errorCodeOrMessage }; }Please double-check consumers across the Unity editor and the Python server for any reliance on error.message. If present, this alias avoids a breaking change while you migrate them to error/code.
tests/test_script_editing.py (1)
4-36: Good scaffolding; consider promoting one “happy path” to a real smoke test soonAll tests are xfail placeholders. To catch regressions early, promote test_script_edit_happy_path into a minimal “green” scenario (even if it’s only exercising a dry-run path) and leave the rest as xfail. When closer to ready, flip remaining xfails to strict=True.
I can help wire a minimal fixture that stubs Unity I/O and verifies atomic write + path resolution without requiring a Unity runtime. Want me to sketch it?
tests/test_logging_stdout.py (1)
47-58: Expand detection and simplify nested checks (Ruff SIM102)Catches more stdout variants (sys.stdout.write and sys.stdout.buffer.write) and flattens nested ifs.
Apply this diff:
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 + # sys.(stdout|__stdout__).write(...) and sys.stdout.buffer.write(...) + if ( + isinstance(node.func, ast.Attribute) + and node.func.attr == "write" + and ( + ( + isinstance(node.func.value, ast.Attribute) + and node.func.value.attr in ("stdout", "__stdout__") + and isinstance(node.func.value.value, ast.Name) + and node.func.value.value.id == "sys" + ) + or ( + isinstance(node.func.value, ast.Attribute) + and node.func.value.attr == "buffer" + and isinstance(node.func.value.value, ast.Attribute) + and node.func.value.value.attr == "stdout" + and isinstance(node.func.value.value.value, ast.Name) + and node.func.value.value.value.id == "sys" + ) + ) + ): + self.hit = True self.generic_visit(node)test_unity_socket_framing.py (4)
30-46: Reduce O(n^2) copies in recv_legacy_json by streaming into a bytearrayRepeatedly joining chunks on every recv causes quadratic copying as payloads grow. Stream into a single bytearray and only materialize bytes when needed. Keeps semantics (ping detection and JSON check) while avoiding repeated joins.
-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() + while True: + chunk = sock.recv(65536) + if not chunk: + if not buf: + raise RuntimeError("no data, socket closed") + return bytes(buf) + buf.extend(chunk) + # Fast-path for tiny "ping" payloads to avoid heavy parsing + if len(buf) <= 8 and bytes(buf).strip() == b"ping": + return bytes(buf) + # Heuristic: only attempt JSON parse when a plausible terminator arrives + if buf and buf[-1] in (ord('}'), ord(']'), ord('\n')): + data = bytes(buf) + if is_valid_json(data): + return data
59-61: Optional safety guard: warn when request body exceeds a large frame thresholdLarge SIZE_MB can explode memory (string + JSON + bytes). Consider warning when the body exceeds a generous cap (aligned with your 128 MB response cap).
- body_bytes = json.dumps(body, ensure_ascii=False).encode("utf-8") + body_bytes = json.dumps(body, ensure_ascii=False).encode("utf-8") + MAX_REQ = 128 * 1024 * 1024 + if len(body_bytes) > MAX_REQ: + print(f"Warning: request size {len(body_bytes)} exceeds {MAX_REQ} bytes; server may reject or OOM.", file=sys.stderr)
85-86: Optional: add newline terminator for legacy JSON readersSome legacy servers read line-delimited JSON. Appending a newline improves compatibility without affecting framed mode.
- s.sendall(body_bytes) + s.sendall(body_bytes + b"\n")
6-10: Defer CLI parsing to main() to avoid side effects on importParsing sys.argv at import time can surprise runners (pytest) and tools. Moving this logic under the main-guard keeps the module import-safe.
-try: - SIZE_MB = int(sys.argv[1]) -except (IndexError, ValueError): - SIZE_MB = 5 # e.g., 5 or 10 +SIZE_MB = 5 # default; may be overridden in main()And inside main(), set from argv:
def main(): + global SIZE_MB + try: + SIZE_MB = int(sys.argv[1]) + except (IndexError, ValueError): + passtests/test_resources_api.py (1)
4-11: These xfail stubs are fine as placeholders; consider turning them into concrete assertions nextOnce resource_tools is finalized, assert that:
- Only Assets/**/*.cs URIs are returned for a broad scope.
- Inputs with traversal (../) or absolute/outside paths (including drive letters or symlinks) are rejected with success=False and an error/code.
I can wire a test harness similar to tests/test_script_tools.py that loads resource_tools.py in isolation and stubs project-root resolution if helpful.
UnityMcpBridge/Editor/Tools/ManageEditor.cs (1)
93-95: LGTM: get_project_root action and implementation look solid
- Derivation from Application.dataPath is correct and normalized to '/'.
- Errors are wrapped via Response.Error consistently.
- Default unknown-action message updated to include get_project_root.
Optional: consider Path.GetFullPath(projectRoot) before returning to normalize redundant segments; if symlink resolution is a concern on macOS, you could also surface both logical and resolved paths.
Also applies to: 171-188
tests/test_script_tools.py (3)
5-5: Remove unused pytest import (Ruff F401)pytest is not referenced directly; fixtures are injected by name. Safe to remove.
-import pytest
12-27: Isolate sys.modules/sys.path mutations to tests to avoid cross-test leakageStubbing mcp.server.fastmcp and prepending SRC at import time affects the whole session. Prefer monkeypatching sys.modules and syspath within each test or a fixture so pytest handles cleanup automatically.
I can refactor this into a session-scoped fixture that yields loaded modules and cleans up after, if you want.
28-33: Harden load_module against missing loader/specIf spec or spec.loader is None, exec_module will raise a less-informative error. Add a guard to produce a clearer failure in test output.
def load_module(path, name): - spec = importlib.util.spec_from_file_location(name, path) - module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) + spec = importlib.util.spec_from_file_location(name, path) + if spec is None or spec.loader is None: + raise RuntimeError(f"Cannot load module {name} from {path}") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) return moduleUnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
17-30: Make tool registration resilient to partial failures.A single registrar throwing during boot will prevent registering subsequent tools. Wrap each registration to isolate failures and log them, keeping the rest of the toolset available.
Apply this diff within register_all_tools:
def register_all_tools(mcp): """Register all refactored tools with the MCP server.""" - logger.info("Registering Unity MCP Server refactored tools...") - # Prefer the surgical edits tool so LLMs discover it first - register_manage_script_edits_tools(mcp) - register_manage_script_tools(mcp) - register_manage_scene_tools(mcp) - register_manage_editor_tools(mcp) - register_manage_gameobject_tools(mcp) - register_manage_asset_tools(mcp) - register_manage_shader_tools(mcp) - register_read_console_tools(mcp) - register_execute_menu_item_tools(mcp) - # Expose resource wrappers as normal tools so IDEs without resources primitive can use them - register_resource_tools(mcp) - logger.info("Unity MCP Server tool registration complete.") + logger.info("Registering Unity MCP Server refactored tools...") + def _safe_register(name, fn): + try: + fn(mcp) + logger.info(f"Tool registration OK: {name}") + except Exception as e: + logger.exception(f"Tool registration failed: {name}: {e}") + + # Prefer the surgical edits tool so LLMs discover it first + _safe_register("manage_script_edits", register_manage_script_edits_tools) + _safe_register("manage_script", register_manage_script_tools) + _safe_register("manage_scene", register_manage_scene_tools) + _safe_register("manage_editor", register_manage_editor_tools) + _safe_register("manage_gameobject", register_manage_gameobject_tools) + _safe_register("manage_asset", register_manage_asset_tools) + _safe_register("manage_shader", register_manage_shader_tools) + _safe_register("read_console", register_read_console_tools) + _safe_register("execute_menu_item", register_execute_menu_item_tools) + # Expose resource wrappers as normal tools so IDEs without resources primitive can use them + _safe_register("resource_tools", register_resource_tools) + logger.info("Unity MCP Server tool registration complete.")tests/test_transport_framing.py (3)
47-55: Tighten _read_exact to fail fast on short reads.Returning a short buffer on premature close can mask framing bugs in tests. Raise on EOF to surface failures deterministically.
- def _read_exact(n: int) -> bytes: - buf = b"" - while len(buf) < n: - chunk = conn.recv(n - len(buf)) - if not chunk: - break - buf += chunk - return buf + def _read_exact(n: int) -> bytes: + buf = b"" + while len(buf) < n: + chunk = conn.recv(n - len(buf)) + if not chunk: + raise ConnectionError("EOF while reading exact bytes") + buf += chunk + return buf
90-99: Reduce CI flakiness by relaxing the pre-handshake poll window.On slower CI VMs, a 0.5s poll can race with connect/send, producing intermittent passes. A slightly longer window improves stability.
- deadline = time.time() + 0.5 + deadline = time.time() + 1.0
146-169: Offer to implement the remaining framing edge-case tests.The skipped tests (zero-length payload, oversized payload, partial frame timeout, concurrency, reconnect mid-command) are valuable for protocol robustness. I can flesh these out with deterministic dummy servers that exercise the failure paths.
Do you want me to add concrete implementations for these tests in this PR?
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (2)
198-229: NL slicing helpers are pragmatic; minor precedence notes.Head bytes take precedence over tail/window, which is reasonable for size-capping. The “show N lines around MethodName” regex is conservative and should be fine for Unity C# patterns.
Consider reusing the content already read for method-line discovery to avoid a second file read in the NL-path. This is minor and only affects large files.
Also applies to: 230-249
130-191: Extract and reuse the script-edits spec to avoid driftBoth
src/server.py(around line 148) andsrc/tools/resource_tools.py(around line 130) embed an identical JSON spec forunity://spec/script-edits. Even though they’re in sync today, duplication risks divergence as the spec evolves. Please:
- Factor the spec JSON into a single helper or constant (e.g.
get_script_edits_spec()) in a shared module.- Import and invoke that helper in both
server.pyandresource_tools.pyinstead of embedding the literal.- Remove the duplicated
spec_json = (…)block fromresource_tools.py.UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
8-74: Support anchor_replace and anchor_delete in local preview.The tool advertises anchor_replace/anchor_delete, but _apply_edits_locally doesn’t handle them. This can degrade preview flows or local diffs for text-path workflows.
Extend _apply_edits_locally to cover these ops:
def _apply_edits_locally(original_text: str, edits: List[Dict[str, Any]]) -> str: text = original_text for edit in edits or []: op = ( (edit.get("op") or edit.get("operation") or edit.get("type") or edit.get("mode") or "") .strip() .lower() ) @@ elif op == "anchor_insert": anchor = edit.get("anchor", "") position = (edit.get("position") or "before").lower() insert_text = edit.get("text", "") flags = re.MULTILINE m = re.search(anchor, text, flags) if not m: if edit.get("allow_noop", True): continue raise RuntimeError(f"anchor not found: {anchor}") idx = m.start() if position == "before" else m.end() text = text[:idx] + insert_text + text[idx:] + elif op == "anchor_replace": + anchor = edit.get("anchor", "") + repl_text = edit.get("text", edit.get("replacement", "")) + flags = re.MULTILINE + m = re.search(anchor, text, flags) + if not m: + if edit.get("allow_noop", True): + continue + raise RuntimeError(f"anchor not found: {anchor}") + text = text[:m.start()] + repl_text + text[m.end():] + elif op == "anchor_delete": + anchor = edit.get("anchor", "") + flags = re.MULTILINE + m = re.search(anchor, text, flags) + if not m: + if edit.get("allow_noop", True): + continue + raise RuntimeError(f"anchor not found: {anchor}") + text = text[:m.start()] + text[m.end():] elif op == "replace_range": start_line = int(edit.get("startLine", 1)) end_line = int(edit.get("endLine", start_line)) replacement = edit.get("text", "") lines = text.splitlines(keepends=True) max_end = len(lines) + 1 if start_line < 1 or end_line < start_line or end_line > max_end: raise RuntimeError("replace_range out of bounds")
424-433: Robustness: accept encoded contents variants consistently.You check both
contentsEncodedandencodedContentsthen decodeencodedContents. If upstream ever returns onlyencodedContentswithout the boolean, this branch won’t trigger.Relax the guard to decode when
encodedContentsis present regardless of the boolean.- if contents is None and data.get("contentsEncoded") and data.get("encodedContents"): + if contents is None and data.get("encodedContents"): contents = base64.b64decode(data["encodedContents"]).decode("utf-8")UnityMcpBridge/Editor/UnityMcpBridge.cs (2)
427-443: Simplify the framed-only path; remove dead code.
usedFramingandif (true)are leftovers from legacy mode. Simplify to framed-only for clarity and avoid confusing logs.- // 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 only + string commandText = await ReadFrameAsUtf8Async(stream, FrameIOTimeoutMs); @@ - 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}");Also applies to: 455-466
476-526: ReadExactAsync/ReadFrameAsUtf8Async are solid; consider partial-frame drop policy.Current behavior throws on invalid length or timeout. That’s fine. Optionally, you could proactively close the client on invalid lengths to harden against misbehaving peers.
If desirable, catch the
IOExceptionat the call site andbreakafter logging, ensuring the client socket is disposed immediately.UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (4)
1136-1147: Atomic, BOM-free writes + validation: solid; consider a backup path for ReplaceAtomically writing then re-parsing the file is a great reliability boost. One minor hardening: File.Replace without a backup can make recovery harder under rare fs hiccups. Using a backup file is safer and still atomic.
Apply this diff to keep a backup during Replace:
- try - { - WriteJsonAtomicallyNoBom(configPath, mergedJson); - } + try + { + WriteJsonAtomicallyNoBom(configPath, mergedJson); + }And update the helper below (see next comment) to retain a .bak during Replace.
1168-1183: WriteJsonAtomicallyNoBom: add backup on replace and clean up safelyKeeping a .bak during Replace makes the write path more resilient to power/fs faults. Clean up the backup best-effort after success.
Apply this diff:
- private static void WriteJsonAtomicallyNoBom(string path, string json) + 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); + if (System.IO.File.Exists(path)) + { + var bak = path + ".bak"; + System.IO.File.Replace(tmp, path, bak); + try { if (System.IO.File.Exists(bak)) System.IO.File.Delete(bak); } catch { /* ignore */ } + } else System.IO.File.Move(tmp, path); }
1414-1429: macOS Claude Desktop mirroring to Linux-style path: good for back-compatMirroring to the Linux-like path on macOS addresses existing installs. Consider logging at debug level when the mirror write occurs for future troubleshooting.
1086-1089: Propagate quiet flag to manual config UIs (VSCode/standard) to avoid stdio noiseYou added -q for the canonical args, but the manual JSON builders still use args without -q. For consistency and to prevent stdout interference when launched by IDEs, consider adding "-q" in:
- VSCode manual config (lines 891-905)
- Standard MCP manual config (lines 1241-1253)
If you want, I can open a follow-up PR or supply the exact patch.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
2-7: Remove unused importsRuff flags these and they’re safe to drop.
Apply this diff:
-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/unity_connection.py (2)
101-111: Frame-length cap should be configurable; avoid hardcoded 64 MiBExpose the cap via config to keep tests and deployments aligned.
Apply this diff:
- payload_len = struct.unpack('>Q', header)[0] + 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}") + max_len = getattr(config, "max_frame_bytes", 64 * 1024 * 1024) + if payload_len > max_len: + raise Exception(f"Invalid framed length: {payload_len} > {max_len}")
179-181: Retry controls: prefer getattr with defaults to avoid surprising minimumsUsing max(config.max_retries, 5) forces a minimum that may surprise tuned setups; consider defaults instead.
Apply this diff:
- attempts = max(config.max_retries, 5) - base_backoff = max(0.5, config.retry_delay) + attempts = getattr(config, "max_retries", 5) + base_backoff = max(0.5, getattr(config, "retry_delay", 0.5))UnityMcpBridge/UnityMcpServer~/src/server.py (4)
5-5: Remove unused dataclass importIt’s flagged by Ruff and safe to drop.
Apply this diff:
-from dataclasses import dataclass
7-7: Remove unused List importAlso flagged by Ruff.
Apply this diff:
-from typing import AsyncIterator, Dict, Any, List +from typing import AsyncIterator, Dict, Any
127-146: Avoid getattr with constant attribute in capability guardsUse short-circuit attribute access to satisfy Ruff B009 and keep the guard correct.
Apply this diff:
-if hasattr(mcp, "resource") and hasattr(getattr(mcp, "resource"), "list"): +if hasattr(mcp, "resource") and hasattr(mcp.resource, "list"): @@ -if hasattr(mcp, "resource") and hasattr(getattr(mcp, "resource"), "read"): +if hasattr(mcp, "resource") and hasattr(mcp.resource, "read"):
99-107: Resource API duplication with tools.resource_tools: consider de-dupingserver.py re-implements _resolve_safe_path_from_uri and the spec resource response. Drift risk is high; consider importing from tools.resource_tools to keep a single source of truth.
I can refactor server.py to import and reuse the helpers, keeping only the capability wiring here.
UnityMcpBridge/Editor/Tools/ManageScript.cs (3)
58-101: Assets/ path hardening: good; symlink guard may miss Unix symlinksUsing FileAttributes.ReparsePoint catches Windows symlinks. On macOS/Linux, DirectoryInfo.Attributes may not set ReparsePoint for symlinks. If feasible in your target Unity/.NET profile, consider checking DirectoryInfo.LinkTarget or traversing DirectoryInfo.Parent to detect symlinks.
Would you like me to propose a cross-platform symlink check compatible with your minimum Unity/.NET version?
1167-1169: Allowed modes message omits supported anchor_ variants*The error lists anchor_insert but not anchor_delete or anchor_replace which are supported above. Add them to guide callers.
Apply this diff:
- return Response.Error($"Unknown edit mode: '{mode}'. Allowed: replace_class, delete_class, replace_method, delete_method, insert_method, anchor_insert."); + return Response.Error($"Unknown edit mode: '{mode}'. Allowed: replace_class, delete_class, replace_method, delete_method, insert_method, anchor_insert, anchor_delete, anchor_replace.");
1232-1235: Include uri in structured edit success payload (parity with apply_text_edits)apply_text_edits returns a uri; structured edits should too for consistency.
Apply this diff:
- var ok = Response.Success( - $"Applied {appliedCount} structured edit(s) to '{relativePath}'.", - new { path = relativePath, editsApplied = appliedCount, scheduledRefresh = !immediate } - ); + var ok = Response.Success( + $"Applied {appliedCount} structured edit(s) to '{relativePath}'.", + new { path = relativePath, uri = $"unity://path/{relativePath}", editsApplied = appliedCount, scheduledRefresh = !immediate } + );
📜 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 ignored due to path filters (1)
UnityMcpBridge/UnityMcpServer~/src/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
UnityMcpBridge/Editor/Data/McpClients.cs(1 hunks)UnityMcpBridge/Editor/Helpers/Response.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/UnityMcpBridge.cs(5 hunks)UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs(8 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/tools/resource_tools.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py(7 hunks)test_unity_socket_framing.py(1 hunks)tests/test_logging_stdout.py(1 hunks)tests/test_resources_api.py(1 hunks)tests/test_script_editing.py(1 hunks)tests/test_script_tools.py(1 hunks)tests/test_transport_framing.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
tests/test_script_tools.py (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
register_manage_script_tools(10-167)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (2)
register_manage_asset_tools(12-79)manage_asset(16-79)
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)
_(103-104)_resolve_safe_path_from_uri(109-124)list_resources(129-142)read_resource(146-215)tests/test_script_tools.py (1)
tool(42-46)
UnityMcpBridge/Editor/Tools/ManageEditor.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-61)Error(41-60)Success(18-33)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(351-370)tests/test_script_tools.py (1)
tool(42-46)
tests/test_transport_framing.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (4)
_read_exact(89-96)connect(37-77)receive_full_response(98-169)disconnect(79-87)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
register_manage_script_edits_tools(177-680)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
register_resource_tools(71-291)
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (3)
UnityEngine(1972-2069)List(1131-1290)IEnumerable(1293-1306)UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-61)Error(41-60)Success(18-33)
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)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(318-334)send_command_with_retry(351-370)tests/test_script_tools.py (1)
tool(42-46)
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-113)read_resource(116-250)
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
UnityMcpBridge/Editor/Models/McpClient.cs (1)
SetStatus(33-45)
🪛 Ruff (0.12.2)
tests/test_script_tools.py
5-5: pytest imported but unused
Remove unused import: pytest
(F401)
tests/test_logging_stdout.py
54-55: Use a single if statement instead of nested if statements
(SIM102)
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/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)
127-127: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
144-144: 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 (25)
UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json (1)
1-11: Solid baseline Pyright config for the Python server scopeBasic mode with Python 3.11 and a scoped execution environment at "." makes sense for the server code under this folder. Good starting point.
UnityMcpBridge/Editor/Models/McpClient.cs (1)
7-7: macConfigPath addition: LGTMAdding a dedicated macOS config path is clean and forwards-compatible with the existing OS-specific fields.
UnityMcpBridge/Editor/Data/McpClients.cs (1)
72-78: Correct macOS path for Claude DesktopThe resolved path expands to ~/Library/Application Support/Claude/claude_desktop_config.json on macOS, which matches Claude Desktop’s expected location. Looks good.
tests/test_logging_stdout.py (1)
7-19: Dynamic SRC resolution and module-level skip: LGTMNice, avoids hardcoded layouts and fails gracefully when server sources aren’t present.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
76-80: LGTM: unified return shape from async retry pathReturning the raw result when it’s a dict and falling back to a consistent failure dict otherwise keeps caller expectations simple. Matches the pattern used elsewhere in the tools.
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
13-13: Good: central logger and prioritized registration for surgical edits.
- Using a module-scoped logger and emitting start/finish messages is appropriate.
- Prioritizing
register_manage_script_edits_tools(mcp)ahead of the legacy script tool should improve discovery.- Exposing
register_resource_tools(mcp)broadens capability for IDEs lacking the resources primitive.Also applies to: 17-20, 28-30
tests/test_transport_framing.py (1)
107-126: Solid coverage of handshake strictness and framed ping/pong.
- Validates handshake failure without FRAMING=1 and ensures socket cleanup.
- Confirms framed negotiation and length-prefixed ping/pong path. This aligns with UnityConnection.use_framing and the server framing work.
Also applies to: 128-144
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (2)
53-69: Good path containment with project-root scoping.The URI handling and containment check via Path.resolve() + relative_to(project) correctly guard against path traversal and absolute path escapes (including file://). This mirrors server.py’s logic.
88-111: Listing logic and spec inclusion look good.
- Caps result count and guards non-project base paths.
- Always includes
unity://spec/script-editsfor discoverability.UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
386-399: Anchors appear in both STRUCT and TEXT; confirm intended routing.
anchor_insertis included in both STRUCT and TEXT sets. In mixed cases you treat it as text (converted to apply_text_edits); when it’s the only op, it routes as structured. This split looks intentional but is subtle.Confirm this duality is desired, or consolidate to a single path to reduce surprises for clients.
Also applies to: 532-537
UnityMcpBridge/Editor/UnityMcpBridge.cs (2)
38-40: Reasonable framing limits and timeouts.64 MiB cap and 30s per-read timeout are sane defaults for editor-side traffic.
401-421: Handshake emission aligns with strict FRAMING=1 negotiation.Sending a clear handshake line and enabling NoDelay early is good practice.
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (4)
1086-1089: Quietuvflag (-q) to suppress stdout: good change; verify compatibility across uv versionsAdding "-q" before "run" reduces noise on stdio transports and is aligned with the intent. Some older uv builds may not support global -q; if users are on older uv, this could fail. Consider gating via a pref (e.g., UnityMCP.UvQuiet, default true) or falling back on error.
Would you like me to add a runtime check that reverts to non-quiet args if the quiet attempt fails once?
1093-1111: BOM detection forcing rewrite: solid guardThe first-3-byte BOM probe is a pragmatic way to kill UTF‑8 BOMs that tend to trip downstream parsers. No functional issues spotted.
1385-1392: macOS config path preference with linux fallback: sensiblePreferring macConfigPath and falling back to linuxConfigPath on macOS is pragmatic for Claude Desktop compatibility.
1558-1564: macOS fallback read for Claude Desktop: good; status mapping consistentThe fallback to linuxConfigPath when the mac path is missing is a good UX. Status transitions (NotConfigured/MissingConfig) are consistent with the rest of the file.
Also applies to: 1576-1595
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
25-44: apply_text_edits tool wiring: LGTMParameters and passthrough to send_command_with_retry look correct, including precondition handling and dict fallback.
45-69: create_script: encoding discipline and naming are correctBase64 encoding for contents on create aligns with the Unity handler’s expectations (contentsEncoded/encodedContents). No issues.
93-168: Legacy manage_script router: good deprecation path; response normalization is robustBlocking full-file update and decoding encodedContents when present aligns behavior with the new tools. No functional blockers spotted.
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
46-66: Strict FRAMING=1 handshake: good; ensure config default matches deployed bridgeRequiring FRAMING=1 by default is sound with the new server, but will break older bridges. The require_framing and handshake_timeout controls mitigate that. Make sure packaged config defaults won’t strand existing users.
Would you like a compatibility fallback that auto-retries in legacy mode once if the strict handshake fails?
222-255: I/O serialized with a lock: goodGuarding send/recv with _io_lock avoids interleaved frames across callers. Short-timeout on retries is a nice touch.
UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
190-197: Deprecation warnings for read/update: good UXKeeping the legacy paths working while nudging callers toward resources/read and apply_text_edits is a nice transition approach.
462-496: ApplyTextEdits: precondition and header guards are well-designedRequiring a precondition hash, bounding payload, and refusing edits before the first using directive reduce the chance of corrupting files. Nice work.
680-691: Return payload includes uri and new sha: greatConsistent responses with uri and checksum will simplify client-side coordination.
2363-2430: Debounced refresh is thoughtfully implementedCoalescing imports/compiles over a sliding window reduces editor churn. Using delayCall avoids blocking the reply path. Good use of interlocked and coarse locking here.
| if (sp.start <= mStart + 2 && sp.end >= mStart + 1) | ||
| { | ||
| var structEdits = new JArray(); | ||
| var op = new JObject | ||
| { | ||
| ["mode"] = "replace_method", | ||
| ["className"] = name, | ||
| ["methodName"] = methodName, | ||
| ["replacement"] = original.Remove(sp.start, sp.end - sp.start).Insert(sp.start, sp.text ?? string.Empty).Substring(mStart, (sp.text ?? string.Empty).Length + (sp.start - mStart) + (mLen - (sp.end - mStart))) | ||
| }; | ||
| structEdits.Add(op); | ||
| // Reuse structured path | ||
| return EditScript(fullPath, relativePath, name, structEdits, new JObject{ ["refresh"] = "immediate", ["validate"] = "standard" }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Auto-upgrade to replace_method: guard against out-of-range when edit leaks outside method span
The current check allows sp.end beyond the method end, which can cause incorrect substring lengths or exceptions. Only auto-upgrade when the edit range is fully inside the detected method.
Apply this diff:
- if (sp.start <= mStart + 2 && sp.end >= mStart + 1)
+ if (sp.start >= mStart && sp.end <= mStart + mLen && sp.start <= mStart + 2)
{
var structEdits = new JArray();
var op = new JObject
{
["mode"] = "replace_method",
["className"] = name,
["methodName"] = methodName,
["replacement"] = original.Remove(sp.start, sp.end - sp.start).Insert(sp.start, sp.text ?? string.Empty).Substring(mStart, (sp.text ?? string.Empty).Length + (sp.start - mStart) + (mLen - (sp.end - mStart)))
};🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Tools/ManageScript.cs around lines 561 to 575, the
auto-upgrade condition for replace_method allows edits that extend outside the
detected method span which can produce out-of-range substring lengths or
exceptions; change the guard so the edit range is fully contained in the method
(e.g. require sp.start >= mStart && sp.end <= mStart + mLen) and then compute
the replacement substring using safe, clamped offsets relative to mStart and
mLen (or validate lengths before calling Substring) so no negative or oversize
lengths are passed.
| def _split_uri(uri: str) -> tuple[str, str]: | ||
| if uri.startswith("unity://path/"): | ||
| path = uri[len("unity://path/") :] | ||
| elif uri.startswith("file://"): | ||
| path = uri[len("file://") :] | ||
| else: | ||
| path = uri | ||
| path = path.replace("\\", "/") | ||
| name = os.path.splitext(os.path.basename(path))[0] | ||
| directory = os.path.dirname(path) | ||
| return name, directory | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
file:// URI handling may produce absolute paths that Unity rejects; normalize to project-relative when possible
ManageScript on the Unity side intentionally refuses non-Assets paths. For file:// URIs, directory can be absolute (outside Assets), causing “Invalid path. Target directory must be within 'Assets/'”. Normalize file URIs into project-relative paths when they’re under UNITY_PROJECT_ROOT.
Apply this diff:
def _split_uri(uri: str) -> tuple[str, str]:
if uri.startswith("unity://path/"):
path = uri[len("unity://path/") :]
elif uri.startswith("file://"):
- path = uri[len("file://") :]
+ path = uri[len("file://") :]
+ # If absolute and under the Unity project root, map to a project-relative path
+ try:
+ project_root = os.environ.get("UNITY_PROJECT_ROOT")
+ if project_root:
+ abs_path = os.path.abspath(path)
+ pr_abs = os.path.abspath(project_root)
+ if abs_path.startswith(pr_abs + os.sep):
+ path = os.path.relpath(abs_path, pr_abs)
+ except Exception:
+ pass
else:
path = uri
path = path.replace("\\", "/")
name = os.path.splitext(os.path.basename(path))[0]
directory = os.path.dirname(path)
return name, directoryFollow-up: If you prefer, we can import the shared resolver from tools.resource_tools to avoid drift.
📝 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.
| def _split_uri(uri: str) -> tuple[str, str]: | |
| if uri.startswith("unity://path/"): | |
| path = uri[len("unity://path/") :] | |
| elif uri.startswith("file://"): | |
| path = uri[len("file://") :] | |
| else: | |
| path = uri | |
| path = path.replace("\\", "/") | |
| name = os.path.splitext(os.path.basename(path))[0] | |
| directory = os.path.dirname(path) | |
| return name, directory | |
| def _split_uri(uri: str) -> tuple[str, str]: | |
| if uri.startswith("unity://path/"): | |
| path = uri[len("unity://path/") :] | |
| elif uri.startswith("file://"): | |
| path = uri[len("file://") :] | |
| # If absolute and under the Unity project root, map to a project-relative path | |
| try: | |
| project_root = os.environ.get("UNITY_PROJECT_ROOT") | |
| if project_root: | |
| abs_path = os.path.abspath(path) | |
| pr_abs = os.path.abspath(project_root) | |
| if abs_path.startswith(pr_abs + os.sep): | |
| path = os.path.relpath(abs_path, pr_abs) | |
| except Exception: | |
| pass | |
| else: | |
| path = uri | |
| path = path.replace("\\", "/") | |
| name = os.path.splitext(os.path.basename(path))[0] | |
| directory = os.path.dirname(path) | |
| return name, directory |
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py around lines 13 to
24, the file:// branch can produce absolute directories that Unity rejects;
detect when a file:// URI resolves under the UNITY_PROJECT_ROOT (use
os.path.abspath/realpath) and convert the path to a project-relative path
(relative to UNITY_PROJECT_ROOT, preserving forward slashes and ensuring it
begins with 'Assets/' where applicable) before extracting name and directory; if
the resolved absolute path is not under UNITY_PROJECT_ROOT, keep the absolute
path behavior; consider reusing the shared resolver from tools.resource_tools
instead of duplicating logic.
…ontext access might be invalid' on forks
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
.claude/prompts/nl-unity-suite.md (5)
103-103: Remove trailing punctuation in heading to satisfy markdownlint (MD026).The heading ends with a period, tripping MD026.
-# Emit the JUnit file to reports/claude-nl-tests.xml and a summary markdown to reports/claude-nl-tests.md. +# Emit the JUnit file to reports/claude-nl-tests.xml and a summary markdown to reports/claude-nl-tests.md
3-8: Tighten phrasing for the “allowed tools” list for clarity.Minor copyedit improves flow and clears LanguageTool nags without changing meaning.
-You are running in CI at the repository root. Use only the tools that are allowed by the workflow: -- View, GlobTool, GrepTool for reading. -- Bash for local shell (git is allowed). -- BatchTool for grouping. -- MCP tools from server "unity" (exposed as mcp__unity__*). +You are running in CI at the repository root. Use only the tools allowed by the workflow: +- View, GlobTool, and GrepTool for reading +- Bash for local shell (git is allowed) +- BatchTool for grouping +- MCP tools from server "unity" (exposed as mcp__unity__*)
15-18: Clarify JUnit formatting requirement to reduce ambiguity for test harnesses.Explicit element naming avoids misinterpretation by consumers.
-- Each test = one `<testcase>` with `classname="UnityMCP.NL"` or `UnityMCP.T`. +- Each test must be a `<testcase>` element with `classname="UnityMCP.NL"` or `classname="UnityMCP.T"`.
21-24: Add explicit “always clean up” guidance for partial-write failures.Slightly stronger wording to ensure atomicity and a clean tree across steps.
-- If a write fails midway, ensure the file is restored before proceeding. +- If any write fails (including partial batches), immediately restore the file(s) to the pre‑edit state before proceeding (no intermediate states should persist).
98-101: Make evidence and checksum capture requirements unambiguous.Clarify that reports directory must exist and that both pre- and post-hashes should be logged for each write.
-- Always capture pre- and post‑windows (±20–40 lines) as evidence in the JUnit `<failure>` or as `<system-out>`. -- For any file write, include `precondition_sha256` and verify the post‑hash in your log. +- Always capture pre- and post‑windows (±20–40 lines) as evidence; include them in the JUnit as `<system-out>` (or `<failure>` on errors). +- For each file write, include `precondition_sha256` and record both the pre‑ and post‑file hashes in your log..github/workflows/claude-nl-suite.yml (5)
31-35: Support both potential requirements.txt locations for the Unity MCP server.Repo paths vary (UnityMcpServer vs UnityMcpBridge/UnityMcpServer~). Make the step robust by checking both.
- name: Prepare Unity MCP server deps (adjust path or remove if N/A) run: | - if [ -f UnityMcpServer/requirements.txt ]; then - uv pip install -r UnityMcpServer/requirements.txt - fi + if [ -f UnityMcpServer/requirements.txt ]; then + uv pip install -r UnityMcpServer/requirements.txt + elif [ -f UnityMcpBridge/UnityMcpServer~/requirements.txt ]; then + uv pip install -r UnityMcpBridge/UnityMcpServer~/requirements.txt + fi
65-71: Avoid failing the workflow when no JUnit is produced; mark artifact upload as optional.If the suite aborts early, the XML might be absent. Prevent hard failure.
- name: Upload JUnit (Claude NL/T) if: always() uses: actions/upload-artifact@v4 with: name: claude-nl-tests path: reports/claude-nl-tests.xml + if-no-files-found: ignore
72-79: Guard test report annotation to only run when a JUnit exists.Avoid error noise on missing files.
- - name: Annotate PR with test results (Claude NL/T) - if: always() + - name: Annotate PR with test results (Claude NL/T) + if: ${{ always() && hashFiles('reports/claude-nl-tests.xml') != '' }} uses: dorny/test-reporter@v1 with: name: Claude NL/T path: reports/claude-nl-tests.xml reporter: java-junit
7-11: Consider least-privilege permissions.If issue creation isn’t needed, drop
issues: write. Keep only what’s required for artifacts and PR annotations.permissions: contents: write # allow Claude to write test artifacts pull-requests: write # allow annotations / comments - issues: write + # issues: write # enable only if you actually create/modify issues
105-110: Optional: perform a stronger clean if intermediate files may be created.If the suite writes extra untracked files, consider cleaning ignored artifacts as well.
- name: Clean working tree (discard temp edits) if: always() run: | git restore -SW :/ - git clean -fd + git clean -fdx
📜 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)
.claude/prompts/nl-unity-suite.md(1 hunks).github/workflows/claude-nl-suite.yml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.claude/prompts/nl-unity-suite.md
[grammar] ~3-~3: There might be a mistake here.
Context: ... tools that are allowed by the workflow: - View, GlobTool, GrepTool for reading. - ...
(QB_NEW_EN)
[grammar] ~4-~4: There might be a mistake here.
Context: ... - View, GlobTool, GrepTool for reading. - Bash for local shell (git is allowed). -...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...it XML at reports/claude-nl-tests.xml. - Each test = one <testcase> with `class...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...fter GetCurrentTarget logging 1,2,3. - Verify by reading 20 lines around the an...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...tent. ## NL-2. Anchor comment insertion - Add a comment Build marker OK immediat...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...TestSelectObjectToPlace` attribute line. - Pass if the comment appears directly a...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...e the last method (preview, then apply). - Pass if windowed read shows the three ...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...ethod (preview, then apply). - Pass if windowed read shows the three lines at ...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ... change only inside braces; then revert. - Pass on exact-range change + revert. ...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ...ched (inline or previous-line variants). - Pass if attributes unchanged. ## T-D....
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... ## T-D. End-of-class insertion (anchor) - Find final class brace; `position: befor...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...dits, then delete with regex_replace`. - Pass if lifecycle completes and file r...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ... duplication. ## T-H. Validation levels - After edits, run validate with `level:...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...basic"` for temporarily unbalanced text ops; final state must be valid. - Pass i...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...ced text ops; final state must be valid. - Pass if validation OK and final file c...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...t be valid. - Pass if validation OK and final file compiles in CI step. ## T-I...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...f validation OK and final file compiles in CI step. ## T-I. Failure surfaces (exp...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ... idempotently. ### Implementation notes - Always capture pre- and post‑windows (±2...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ... JUnit <failure> or as <system-out>. - For any file write, include `preconditio...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ...6` and verify the post‑hash in your log. - At the end, restore the repository to it...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.claude/prompts/nl-unity-suite.md
103-103: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
🪛 actionlint (1.7.7)
.github/workflows/claude-nl-suite.yml
83-83: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
95-95: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🔇 Additional comments (2)
.github/workflows/claude-nl-suite.yml (2)
60-63: Confirm model slug availability
- File:
.github/workflows/claude-nl-suite.yml(line 60) sets
model: "claude-3-7-sonnet-20250219"
Please verify that this model is enabled for your account/quota and accessible viasecrets.ANTHROPIC_API_KEYto avoid workflow failures if the slug changes or is unavailable.
39-39: Slug Verified: no changes requiredThe GitHub Action reference
uses: anthropics/claude-code-base-action@betais correct—anthropics/claude-code-base-actionexists (HTTP 200) andanthropic/claude-code-base-actiondoes not (HTTP 404).
No updates needed here.
.claude/prompts/nl-unity-suite.md
Outdated
| - Primary file: `Assets/Scripts/Interaction/SmartReach.cs` | ||
| - For each operation, prefer structured edit tools (`replace_method`, `insert_method`, `delete_method`, `anchor_insert`, `apply_text_edits`, `regex_replace`) via the MCP server. | ||
| - Include `precondition_sha256` for any text path write. | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the target file path exists in the repo (prevents suite flakiness).
Sanity-check SmartReach.cs so the suite can early-exit gracefully if missing.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for SmartReach.cs under Assets/"
rg -n --glob 'Assets/**' -S '^' | sed -n '1p' >/dev/null # warm cache
fd -a --glob 'Assets/**/SmartReach.cs' || true
test -e reports || mkdir -p reportsLength of output: 307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for SmartReach.cs anywhere in the repo"
fd SmartReach.cs || trueLength of output: 145
SmartReach.cs Not Found – Critical Fix Required
The target file Assets/Scripts/Interaction/SmartReach.cs does not exist in the repository. Any suite logic or scripts referencing this path will fail.
• Update or restore the file at Assets/Scripts/Interaction/SmartReach.cs if it was renamed or moved.
• If the intent is to guard against its absence, modify your test suite or MCP prompts to early-exit gracefully when the file isn’t present.
• Review all references to SmartReach.cs (e.g., in .claude/prompts/nl-unity-suite.md, test definitions, or CI scripts) and correct the paths or add existence checks.
🤖 Prompt for AI Agents
In .claude/prompts/nl-unity-suite.md around lines 10 to 13, the prompt
references Assets/Scripts/Interaction/SmartReach.cs which does not exist; update
the repo to restore or move the file, or change references to the correct path,
and add guards: either restore/rename the missing SmartReach.cs to that exact
path, or update the prompt, tests, and any CI/MCP scripts to point to the new
filename/location and include existence checks (early-exit) where applicable;
also scan the repo for all references to SmartReach.cs and fix each to the new
path or add conditional handling so suite logic fails gracefully when the file
is absent.
| mcp_config: | | ||
| { | ||
| "mcpServers": { | ||
| "unity": { | ||
| "command": "python", | ||
| "args": ["UnityMcpServer/src/server.py"] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify MCP server path; consider resolving dynamically if location differs.
The configured path is UnityMcpServer/src/server.py, while other parts of the repo may use UnityMcpBridge/UnityMcpServer~/src/server.py. Confirm the correct entrypoint.
Run:
If needed, update args accordingly. For maximal robustness, you can wrap the MCP config in a tiny launcher script that resolves the path at runtime.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for 'server.py' candidates used by MCP:"
fd -a --glob '*Unity*Mcp*/*/server.py' || true
sed -n '1,120p' .claude/prompts/nl-unity-suite.md | rg -n 'UnityMcpServer|UnityMcpBridge' -n -C2 || trueLength of output: 279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for all server.py files in the repo:"
fd --glob 'server.py' -t f || true
echo ""
echo "Listing directories matching UnityMcpServer*:"
fd --glob 'UnityMcpServer*' -t d || true
echo ""
echo "Listing directories matching UnityMcpBridge*:"
fd --glob 'UnityMcpBridge*' -t d || true
echo ""
echo "Searching for any references to 'server.py' in workflow files:"
rg -n --glob '*.yml' 'server.py' -C2 || trueLength of output: 881
Fix MCP server entrypoint path in workflow
The workflow is pointing to a non-existent path. The only server.py in the repo lives under UnityMcpBridge/UnityMcpServer~/src/server.py, not UnityMcpServer/src/server.py. Update the args in .github/workflows/claude-nl-suite.yml accordingly.
Locations to update:
.github/workflows/claude-nl-suite.yml(around lines 49–57)
Suggested diff:
mcp_config: |
{
"mcpServers": {
"unity": {
"command": "python",
- "args": ["UnityMcpServer/src/server.py"]
+ "args": ["UnityMcpBridge/UnityMcpServer~/src/server.py"]
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/claude-nl-suite.yml around lines 49 to 57, the MCP server
entrypoint path in the mcp_config args points to a non-existent
UnityMcpServer/src/server.py; update the args value to the correct repository
location UnityMcpBridge/UnityMcpServer~/src/server.py so the workflow runs the
actual server.py file (replace the existing args entry with the corrected path).
| if: ${{ always() && hashFiles('ProjectSettings/ProjectVersion.txt') != '' && secrets.UNITY_LICENSE != '' }} | ||
| uses: game-ci/unity-test-runner@v4 | ||
| env: | ||
| UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} # OR UNITY_* for Pro | ||
| with: | ||
| projectPath: . | ||
| githubToken: ${{ secrets.GITHUB_TOKEN }} | ||
| # Even with no tests, this compiles; add EditMode/PlayMode tests later. | ||
| testMode: EditMode |
There was a problem hiding this comment.
Fix “secrets” usage in if: expressions (actionlint error; GitHub disallows secrets. in conditions).*
Current “if” references secrets.UNITY_LICENSE, which is not available in expressions and will cause validation/runtime errors.
Apply this diff to route the secret through env and check env.UNITY_LICENSE instead:
jobs:
nl-suite:
if: github.event_name == 'workflow_dispatch'
runs-on: ubuntu-latest
+ env:
+ UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }}
@@
- - name: Unity compile (Project)
- if: ${{ always() && hashFiles('ProjectSettings/ProjectVersion.txt') != '' && secrets.UNITY_LICENSE != '' }}
+ - name: Unity compile (Project)
+ if: ${{ always() && hashFiles('ProjectSettings/ProjectVersion.txt') != '' && env.UNITY_LICENSE != '' }}
uses: game-ci/unity-test-runner@v4
- env:
- UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} # OR UNITY_* for Pro
+ env:
+ UNITY_LICENSE: ${{ env.UNITY_LICENSE }} # OR UNITY_* for Pro📝 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.
| if: ${{ always() && hashFiles('ProjectSettings/ProjectVersion.txt') != '' && secrets.UNITY_LICENSE != '' }} | |
| uses: game-ci/unity-test-runner@v4 | |
| env: | |
| UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} # OR UNITY_* for Pro | |
| with: | |
| projectPath: . | |
| githubToken: ${{ secrets.GITHUB_TOKEN }} | |
| # Even with no tests, this compiles; add EditMode/PlayMode tests later. | |
| testMode: EditMode | |
| jobs: | |
| nl-suite: | |
| if: github.event_name == 'workflow_dispatch' | |
| runs-on: ubuntu-latest | |
| env: | |
| UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} | |
| steps: | |
| - name: Unity compile (Project) | |
| if: ${{ always() && hashFiles('ProjectSettings/ProjectVersion.txt') != '' && env.UNITY_LICENSE != '' }} | |
| uses: game-ci/unity-test-runner@v4 | |
| env: | |
| UNITY_LICENSE: ${{ env.UNITY_LICENSE }} # OR UNITY_* for Pro | |
| with: | |
| projectPath: . | |
| githubToken: ${{ secrets.GITHUB_TOKEN }} | |
| # Even with no tests, this compiles; add EditMode/PlayMode tests later. | |
| testMode: EditMode |
🧰 Tools
🪛 actionlint (1.7.7)
83-83: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 Prompt for AI Agents
.github/workflows/claude-nl-suite.yml around lines 83-91: the step if-condition
currently uses secrets.UNITY_LICENSE which is disallowed in expressions; replace
that reference with env.UNITY_LICENSE and ensure the secret is routed into an
environment variable that's visible to expressions (move UNITY_LICENSE: ${{
secrets.UNITY_LICENSE }} into the job- or workflow-level env or otherwise expose
it before the if runs) so the if uses env.UNITY_LICENSE instead of
secrets.UNITY_LICENSE.
| if: ${{ always() && hashFiles('Packages/manifest.json') != '' && hashFiles('ProjectSettings/ProjectVersion.txt') == '' && secrets.UNITY_LICENSE != '' }} | ||
| uses: game-ci/unity-test-runner@v4 | ||
| env: | ||
| UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} | ||
| with: | ||
| packageMode: true | ||
| unityVersion: 2022.3.45f1 # <-- set explicitly for packages | ||
| projectPath: . # or a small sample project path | ||
| githubToken: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Fix “secrets” usage in if: expressions for Package mode as well.
Mirror the env-based gating for the package compilation block.
- - name: Unity compile (Package)
- if: ${{ always() && hashFiles('Packages/manifest.json') != '' && hashFiles('ProjectSettings/ProjectVersion.txt') == '' && secrets.UNITY_LICENSE != '' }}
+ - name: Unity compile (Package)
+ if: ${{ always() && hashFiles('Packages/manifest.json') != '' && hashFiles('ProjectSettings/ProjectVersion.txt') == '' && env.UNITY_LICENSE != '' }}
uses: game-ci/unity-test-runner@v4
- env:
- UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }}
+ env:
+ UNITY_LICENSE: ${{ env.UNITY_LICENSE }}📝 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.
| if: ${{ always() && hashFiles('Packages/manifest.json') != '' && hashFiles('ProjectSettings/ProjectVersion.txt') == '' && secrets.UNITY_LICENSE != '' }} | |
| uses: game-ci/unity-test-runner@v4 | |
| env: | |
| UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} | |
| with: | |
| packageMode: true | |
| unityVersion: 2022.3.45f1 # <-- set explicitly for packages | |
| projectPath: . # or a small sample project path | |
| githubToken: ${{ secrets.GITHUB_TOKEN }} | |
| - name: Unity compile (Package) | |
| if: ${{ always() && hashFiles('Packages/manifest.json') != '' && hashFiles('ProjectSettings/ProjectVersion.txt') == '' && env.UNITY_LICENSE != '' }} | |
| uses: game-ci/unity-test-runner@v4 | |
| env: | |
| UNITY_LICENSE: ${{ env.UNITY_LICENSE }} | |
| with: | |
| packageMode: true | |
| unityVersion: 2022.3.45f1 # <-- set explicitly for packages | |
| projectPath: . # or a small sample project path | |
| githubToken: ${{ secrets.GITHUB_TOKEN }} |
🧰 Tools
🪛 actionlint (1.7.7)
95-95: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 Prompt for AI Agents
In .github/workflows/claude-nl-suite.yml around lines 95-103, the package-mode
step's if expression uses secrets.UNITY_LICENSE directly while the other package
compilation block gates on the environment variable; change the condition to
check env.UNITY_LICENSE instead (e.g. env.UNITY_LICENSE != '') and ensure
UNITY_LICENSE is exported into the step/job env from secrets (UNITY_LICENSE: ${{
secrets.UNITY_LICENSE }}) before this if is evaluated so the gating mirrors the
env-based check used elsewhere; keep the existing hashFiles checks and other
conditions unchanged.
…enrich spec examples; add routing metadata; add standalone long test script
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
15-18: Deduplicate magic URIs with named constants (prevents typos and eases reuse).The strings "unity://path/" and "unity://spec/script-edits" are repeated in several places. Centralizing them as constants improves maintainability and reduces risk of typos. This mirrors prior feedback on the same duplication.
Apply:
@@ -from mcp.server.fastmcp import FastMCP, Context -from unity_connection import send_command_with_retry +from mcp.server.fastmcp import FastMCP, Context +from unity_connection import send_command_with_retry + +# Canonical Unity URI constants +UNITY_PATH_PREFIX = "unity://path/" +UNITY_SPEC_SCRIPT_EDITS = "unity://spec/script-edits" @@ - rel = p.relative_to(project).as_posix() - matches.append(f"unity://path/{rel}") + rel = p.relative_to(project).as_posix() + matches.append(f"{UNITY_PATH_PREFIX}{rel}") if len(matches) >= max(1, limit): break @@ - if "unity://spec/script-edits" not in matches: - matches.append("unity://spec/script-edits") + if UNITY_SPEC_SCRIPT_EDITS not in matches: + matches.append(UNITY_SPEC_SCRIPT_EDITS) @@ - if uri == "unity://spec/script-edits": + if uri == UNITY_SPEC_SCRIPT_EDITS:Also applies to: 103-110, 132-133
UnityMcpBridge/UnityMcpServer~/src/server.py (1)
40-42: Don’t silence file-logging setup failures; log a warning instead.Silently swallowing exceptions here makes debugging harder (prior feedback). Log the exception and proceed with stderr-only logging.
-except Exception: - # If file logging setup fails, continue with stderr logging only - pass +except Exception as e: + # If file logging setup fails, continue with stderr logging only + logger.warning(f"File logging setup failed: {e}")
🧹 Nitpick comments (14)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (4)
97-105: Avoid scanning the entire tree when a glob pattern is available.You can let Path.rglob filter by the provided pattern to reduce filesystem traversal and drop the manual fnmatch.
- for p in base.rglob("*"): - if not p.is_file(): - continue - if pattern and not fnmatch.fnmatch(p.name, pattern): - continue + glob_pat = pattern or "*" + for p in base.rglob(glob_pat): + if not p.is_file(): + continue
268-268: Remove redundant local import of re.re is already imported at module scope (Line 9). The local import is unnecessary.
- import re
31-40: Consider logging Unity query failures when resolving project root.Swallowing exceptions here can obscure configuration issues. A debug-level log keeps noise low while preserving insights.
- try: + try: resp = send_command_with_retry("manage_editor", {"action": "get_project_root"}) if isinstance(resp, dict) and resp.get("success"): pr = Path(resp.get("data", {}).get("projectRoot", "")).expanduser().resolve() if pr and (pr / "Assets").exists(): return pr - except Exception: - pass + except Exception as e: + # Non-fatal: fall back to scanning/walking + # Using debug to avoid noisy logs for transient startup conditions + # (Unity not yet ready, handshake still in progress, etc.) + # logger.debug(f"manage_editor.get_project_root failed: {e}") + passIf you'd like, I can wire in a module-level logger to enable this debug message.
198-229: NL “around method” windowing: reasonable heuristic; minor robustness nits only.The regex and line-window heuristic are sensible. No functional issues spotted. Consider pre-validating that window > 0 (ignore zeros) and guarding against extremely large “window” values if untrusted input is possible.
UnityMcpBridge/UnityMcpServer~/src/server.py (3)
127-142: Replace getattr-with-constant with a simple attribute guard to satisfy Ruff B009 and clarify intent.Use a single hasattr guard, bind the attribute, then test the desired member. Also use the bound object for the decorator to avoid re-resolving the attribute.
-if hasattr(mcp, "resource") and hasattr(getattr(mcp, "resource"), "list"): - @mcp.resource.list() - def list_resources(ctx: Context) -> list[dict]: +if hasattr(mcp, "resource"): + resource_api = mcp.resource + if hasattr(resource_api, "list"): + @resource_api.list() + def list_resources(ctx: Context) -> list[dict]: assets = [] try: for p in ASSETS_ROOT.rglob("*.cs"): rel = p.relative_to(PROJECT_ROOT).as_posix() assets.append({"uri": f"unity://path/{rel}", "name": p.name}) except Exception: pass # Add spec resource so clients (e.g., Claude Desktop) can learn the exact contract assets.append({ "uri": "unity://spec/script-edits", "name": "Unity Script Edits – Required JSON" }) return assets
144-216: Apply the same getattr simplification for resource.read; also include sha256 for the spec to align with file reads.This removes the B009 pattern and makes the spec response consistent with file reads by returning metadata.sha256 as well.
-if hasattr(mcp, "resource") and hasattr(getattr(mcp, "resource"), "read"): - @mcp.resource.read() - def read_resource(ctx: Context, uri: str) -> dict: +if hasattr(mcp, "resource"): + resource_api = mcp.resource + if hasattr(resource_api, "read"): + @resource_api.read() + def read_resource(ctx: Context, uri: str) -> dict: # Serve script-edits spec if uri == "unity://spec/script-edits": spec_json = ( '{\n' @@ - '}\n' - ) - return {"mimeType": "application/json", "text": spec_json} + '}\n' + ) + sha = hashlib.sha256(spec_json.encode("utf-8")).hexdigest() + return {"mimeType": "application/json", "text": spec_json, "metadata": {"sha256": sha}} p = _resolve_safe_path_from_uri(uri) if not p or not p.exists(): return {"mimeType": "text/plain", "text": f"Resource not found: {uri}"} try: text = p.read_text(encoding="utf-8") sha = hashlib.sha256(text.encode("utf-8")).hexdigest() return {"mimeType": "text/plain", "text": text, "metadata": {"sha256": sha}} except Exception as e: return {"mimeType": "text/plain", "text": f"Error reading resource: {e}"}
132-142: Avoid bare except in asset scan as a general practice (optional).Catching all exceptions and ignoring them can hide path permission or encoding bugs during asset enumeration. Consider logging at debug level.
- except Exception: - pass + except Exception as e: + logger.debug(f"Asset scan failed: {e}").claude/prompts/nl-unity-suite.md (3)
3-8: Tighten wording of the “allowed tools” bulletsMinor grammar/clarity improvements for the opening bullets.
Apply this diff:
-You are running in CI at the repository root. Use only the tools that are allowed by the workflow: -- View, GlobTool, GrepTool for reading. -- Bash for local shell (git is allowed). -- BatchTool for grouping. -- MCP tools from server "unity" (exposed as mcp__unity__*). +While running in CI at the repository root, use only the tools allowed by the workflow: +- View, GlobTool, and GrepTool for reading. +- Bash for local shell (git is allowed). +- BatchTool for grouping. +- MCP tools from the "unity" server (exposed as mcp__unity__*).
103-103: Fix markdownlint MD026: remove trailing punctuation in headingThe H1 line ends with a period; markdownlint flags this (MD026).
Apply this diff:
-# Emit the JUnit file to reports/claude-nl-tests.xml and a summary markdown to reports/claude-nl-tests.md. +# Emit the JUnit file to reports/claude-nl-tests.xml and a summary markdown to reports/claude-nl-tests.md
25-29: Ensure the NL-Unity suite fails gracefully if the test script is missingTo prevent CI breakage when
ClaudeTests/longUnityScript-claudeTest.csis moved or deleted, add an existence check at the start of your suite script. For example, in your CI step before running the NL suite:#!/bin/bash set -euo pipefail target="ClaudeTests/longUnityScript-claudeTest.cs" if [[ ! -f "$target" ]]; then echo "::warning::Expected test target '$target' not found. Update .claude/prompts/nl-unity-suite.md or restore the file." mkdir -p reports echo "- [ ] NL suite skipped: missing $target" >> reports/claude-nl-tests.md exit 1 fi• File to update:
.claude/prompts/nl-unity-suite.md(lines 25–29)
• Purpose: Skip or fail early with a clear warning if the Unity test script isn’t present.
• Optional: Emit a minimal JUnit report to mark the suite as skipped rather than errored.ClaudeTests/longUnityScript-claudeTest.cs (1)
62-69: Optional: make Update private to match typical Unity styleUnity event methods do not need to be public; private reduces surface without changing behavior.
Apply this diff:
- public void Update() + private void Update()UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (3)
386-394: Anchor-insert is in both STRUCT and TEXT sets — document routing nuanceanchor_insert appears in both STRUCT and TEXT. Current behavior:
- If only anchor_insert ops are present, route “structured” (action=edit).
- In mixed batches, anchor_insert is treated as a text op and converted to apply_text_edits.
That’s reasonable, but can surprise clients expecting consistent routing. Consider documenting this or forcing a single route for anchor_insert for consistency.
Proposed doc addition (no code change needed): “anchor_insert may route via structured edit when alone, and via apply_text_edits when combined with text ops, to preserve atomicity of text batches.”
656-674: Update path uses encodedContents and validation defaults — consider precondition for full updates“update” writes the whole file with contentsEncoded, but lacks a precondition hash. If Unity’s “update” action doesn’t enforce staleness checks itself, concurrent edits could overwrite. Consider adding an optional precondition SHA to the “update” path (if supported by the server), matching the safety of apply_text_edits.
Would you like a follow-up patch that adds an optional precondition_sha256 to the update action when the server supports it?
277-291: LSP range aliasing: cover newText when present and ensure consistent field namingYou already map range to 1-based lines/cols and try to lift newText into text. To reduce downstream branching, consider standardizing on “text” for local previews and “newText” only for apply_text_edits payloads.
Apply this minimal tweak to ensure “text” is populated whenever range->replace_range conversion happens, even if “newText” is absent:
e["endCol"] = int(end.get("character", 0)) + 1 - if "newText" in edit and "text" not in e: - e["text"] = edit.get("newText", "") + # Normalize replacement field for downstream consumers + if "newText" in edit and "text" not in e: + e["text"] = edit.get("newText", "") + e.setdefault("text", e.get("replacement", ""))
📜 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)
.claude/prompts/nl-unity-suite.md(1 hunks)ClaudeTests/longUnityScript-claudeTest.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
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-113)read_resource(116-250)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(351-370)tests/test_script_tools.py (1)
tool(42-46)
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)
_(103-104)_resolve_safe_path_from_uri(109-124)list_resources(129-142)read_resource(146-215)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)
127-127: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
144-144: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🪛 LanguageTool
.claude/prompts/nl-unity-suite.md
[grammar] ~3-~3: There might be a mistake here.
Context: ... tools that are allowed by the workflow: - View, GlobTool, GrepTool for reading. - ...
(QB_NEW_EN)
[grammar] ~4-~4: There might be a mistake here.
Context: ... - View, GlobTool, GrepTool for reading. - Bash for local shell (git is allowed). -...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...it XML at reports/claude-nl-tests.xml. - Each test = one <testcase> with `class...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...fter GetCurrentTarget logging 1,2,3. - Verify by reading 20 lines around the an...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...brace (preview, then apply). - Pass if windowed read shows the three lines at ...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ... change only inside braces; then revert. - Pass on exact-range change + revert. ...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... ## T-D. End-of-class insertion (anchor) - Find final class brace; `position: befor...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ... append a temporary helper; then remove. - Pass if insert/remove verified. ## T-...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...dits, then delete with regex_replace`. - Pass if lifecycle completes and file r...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ... duplication. ## T-H. Validation levels - After edits, run validate with `level:...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...basic"` for temporarily unbalanced text ops; final state must be valid. - Pass i...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...ced text ops; final state must be valid. - Pass if validation OK and final file c...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...t be valid. - Pass if validation OK and final file compiles in CI step. ## T-I...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...f validation OK and final file compiles in CI step. ## T-I. Failure surfaces (exp...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...elper via regex_replace → clean no-op. - Pass if both behave idempotently. ###...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ... idempotently. ### Implementation notes - Always capture pre- and post‑windows (±2...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ... JUnit <failure> or as <system-out>. - For any file write, include `preconditio...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ...6` and verify the post‑hash in your log. - At the end, restore the repository to it...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.claude/prompts/nl-unity-suite.md
103-103: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
🔇 Additional comments (7)
UnityMcpBridge/UnityMcpServer~/src/server.py (1)
30-39: Good logging setup that avoids stdout; minor improvement already covered above.Stderr-only default with rotating file logs is appropriate for stdio transports. With the warning addition on failure, this section looks solid.
.claude/prompts/nl-unity-suite.md (1)
9-13: Primary test target updated — resolves prior SmartReach.cs path issueGood switch to ClaudeTests/longUnityScript-claudeTest.cs as the canonical target. This closes the earlier concern about Assets/Scripts/Interaction/SmartReach.cs not existing.
ClaudeTests/longUnityScript-claudeTest.cs (2)
30-43: Surface/API layout looks good for NL/T edit workflowsPublic accessors and simple selection logic are well-scoped and friendly to anchor‑based edits and method replacement tests.
86-109: Blend helpers are safe and side-effect minimalNull checks and Animator parameter updates are straightforward; good for predictable test diffs.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (3)
414-430: Robust read-path normalization and content decodingGraceful fallback to encoded contents is good; responses are normalized even when non-dict. This improves tool resilience during Unity reloads.
548-624: Text-op conversion to apply_text_edits: good safeguards and precondition enforcement
- Converts anchors/ranges/regex to line/col spans with a precondition SHA, ensuring server-side guards run.
- Updates local snapshot between anchors to keep subsequent spans stable.
- Clear, structured error payloads for anchor-not-found and missing fields.
No changes requested.
627-638: Preview gating for regex_replace is helpful; limit diff size is sensibleLocal preview before a potentially broad regex is a good safety net. Truncation of oversized diffs prevents tool responses from ballooning.
|
Mistakenly PR'd to main repo, meant to go to my personal -- pay no attention |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation