Claude‑friendly edit tools + framed transport + live Unity NL test framework #243
Conversation
CI: add Claude Desktop parity workflow
…x optional + project_root fallback; workflows: set UNITY_PROJECT_ROOT for CI
…rompts: mini NL/T clarifications
…ver: accept relative UNITY_PROJECT_ROOT and bare spec URI
…to avoid absolute paths
…w batch-mode bridge via UNITY_MCP_ALLOW_BATCH
… readiness timeout
…ss from container
…e readiness; fix YAML
… validation; explicit status dir + license env
…; ready to run happy path
…ense; create dirs before start
…th shared mounts; mount .config and .cache too
… probe python; stabilize licensing flow
… validated; fix indentation
…n MCP server with python; tighten secret scoping
…nes in Python conversion paths; C# path ensures trailing newline and skips duplicate insertion within class.
…reflight+strict; no-op evidence in C#; update NL suite prompt; add unit tests
…t; add robust tests; prompt uses delete_method for temp helper
…-length response with C#; update test
…for structured ops; forward options on apply_text_edits; add validate=relaxed support and scoped checks; update NL/T prompt; add tests for options forwarding, relaxed mode, and atomic batches
…=atomic for T-F; emphasize always writing testcase and restoring on errors
…indows; early regex compile with hints; debug_preview for apply_text_edits
|
Warning Rate limit exceeded@dsarno has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughAdds CI workflows, NL agent prompts, server/bridge framing and framing-aware clients, extensive script-editing and resource tools (server + bridge), macOS path handling, a long Unity test script, many unit tests, and a JUnit post-processor to mark environment failures as skipped. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as "GitHub Actions"
participant Claude as "Claude NL/T Agent"
participant MCPPy as "Python MCP Server"
participant Unity as "Unity MCP Bridge (C#)"
participant FS as "Repository / Assets"
GH->>Claude: trigger NL/T suite (config, allowed tools)
Claude->>MCPPy: script_apply_edits / apply_text_edits (uri, edits, pre-sha)
MCPPy->>Unity: manage_script (apply_text_edits) [framed JSON]
Note over Unity: Validate pre-sha, apply edits atomically or sequentially,\noptionally run Roslyn validation, schedule debounced refresh
Unity-->>MCPPy: result { success, diagnostics, sha256, uri }
MCPPy-->>Claude: normalized response (normalizedEdits, warnings, sha)
Claude->>MCPPy: validate_script / get_sha / read_resource (as needed)
MCPPy->>FS: resource_tools read/list/find (safe project resolution)
MCPPy-->>Claude: aggregated outcomes
Claude-->>GH: write JUnit fragments and markdown summary
GH->>GH: run .github/scripts/mark_skipped.py to convert env failures -> skipped
sequenceDiagram
autonumber
participant Client as "MCP Client"
participant Unity as "Unity MCP Bridge (C#)"
Note over Unity: on connect => "WELCOME UNITY-MCP 1 FRAMING=1"
Client->>Unity: read greeting
alt server advertises framing
Client->>Unity: [8-byte len][JSON payload]
Unity-->>Client: [8-byte len][JSON response]
Note right of Unity: 0-length frames used as heartbeats (ignored up to cap)
else legacy allowed or fallback
Client->>Unity: raw JSON bytes
Unity-->>Client: raw JSON bytes
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
607-722: Don’t hold the lock while parsing/executing commands.Heavy JSON/handler work happens under lock, blocking enqueues and increasing latency. Snapshot under lock, process outside, then clean up quickly.
- private static void ProcessCommands() + private static void ProcessCommands() { - List<string> processedIds = new(); - lock (lockObj) - { - // Periodic heartbeat while editor is idle/processing - double now = EditorApplication.timeSinceStartup; - if (now >= nextHeartbeatAt) - { - WriteHeartbeat(false); - nextHeartbeatAt = now + 0.5f; - } - - foreach ( - KeyValuePair< - string, - (string commandJson, TaskCompletionSource<string> tcs) - > kvp in commandQueue.ToList() - ) - { - string id = kvp.Key; - string commandText = kvp.Value.commandJson; - TaskCompletionSource<string> tcs = kvp.Value.tcs; + // Heartbeat + double now = EditorApplication.timeSinceStartup; + if (now >= nextHeartbeatAt) { WriteHeartbeat(false); nextHeartbeatAt = now + 0.5f; } + + // Snapshot under lock + List<(string id, string text, TaskCompletionSource<string> tcs)> work; + lock (lockObj) + { + work = commandQueue.Select(kvp => (kvp.Key, kvp.Value.commandJson, kvp.Value.tcs)).ToList(); + } + + foreach (var item in work) + { + string id = item.id; + string commandText = item.text; + var tcs = item.tcs; try { // Special case handling if (string.IsNullOrEmpty(commandText)) { var emptyResponse = new { status = "error", error = "Empty command received", }; tcs.SetResult(JsonConvert.SerializeObject(emptyResponse)); - processedIds.Add(id); continue; } // Trim the command text to remove any whitespace commandText = commandText.Trim(); // Non-JSON direct commands handling (like ping) if (commandText == "ping") { var pingResponse = new { status = "success", result = new { message = "pong" }, }; tcs.SetResult(JsonConvert.SerializeObject(pingResponse)); - processedIds.Add(id); continue; } // Check if the command is valid JSON before attempting to deserialize if (!IsValidJson(commandText)) { var invalidJsonResponse = new { status = "error", error = "Invalid JSON format", receivedText = commandText.Length > 50 ? commandText[..50] + "..." : commandText, }; tcs.SetResult(JsonConvert.SerializeObject(invalidJsonResponse)); - processedIds.Add(id); continue; } // Normal JSON command processing Command command = JsonConvert.DeserializeObject<Command>(commandText); if (command == null) { var nullCommandResponse = new { status = "error", error = "Command deserialized to null", details = "The command was valid JSON but could not be deserialized to a Command object", }; tcs.SetResult(JsonConvert.SerializeObject(nullCommandResponse)); } else { string responseJson = ExecuteCommand(command); tcs.SetResult(responseJson); } } catch (Exception ex) { Debug.LogError($"Error processing command: {ex.Message}\n{ex.StackTrace}"); var response = new { status = "error", error = ex.Message, commandType = "Unknown (error during processing)", receivedText = commandText?.Length > 50 ? commandText[..50] + "..." : commandText, }; string responseJson = JsonConvert.SerializeObject(response); tcs.SetResult(responseJson); } - - processedIds.Add(id); - } - - foreach (string id in processedIds) - { - commandQueue.Remove(id); - } + // Remove quickly under lock + lock (lockObj) { commandQueue.Remove(id); } } }UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
1857-1935: Collapse bespoke UV discovery to shared helper; remove dead codeThese local UV helpers duplicate logic in ServerInstaller and will drift. Replace with a shim and delete unused methods.
- private string FindUvPath() - { - ... (platform-specific scanning, which/where, PATH probing) ... - return uvPath; - } + private string FindUvPath() + { + try { return ServerInstaller.FindUvPath(); } catch { return null; } + } - - private bool IsValidUvInstallation(string uvPath) { ... } - private string FindWindowsUvPath() { ... }I can follow up with a PR-wide search to remove remaining references if you’d like.
Also applies to: 1973-2135
🧹 Nitpick comments (76)
mcp_source.py (4)
94-108: Unify Git URL scheme and remove hardcoded subpath.Remote-current-branch uses bare https while upstream uses git+https, and upstream hardcodes "UnityMcpBridge" instead of BRIDGE_SUBPATH. Make both consistent to avoid UPM parsing edge cases and duplication.
Apply within this hunk:
- upstream = "git+https://github.com/CoplayDev/unity-mcp.git?path=/UnityMcpBridge" + upstream = f"git+https://github.com/CoplayDev/unity-mcp.git?path=/{BRIDGE_SUBPATH}" @@ - origin_remote = origin + origin_remote = origin @@ - ("[2] Remote current branch", f"{origin_remote}?path=/{BRIDGE_SUBPATH}#{branch}"), + ("[2] Remote current branch", f"{ensure_git_plus_https(origin_remote)}?path=/{BRIDGE_SUBPATH}#{branch}"),Add this small helper (outside the hunk) to normalize:
def ensure_git_plus_https(url: str) -> str: if url.startswith("git+"): return url if url.startswith("https://"): return "git+" + url return url
39-47: Handle additional SSH origin forms.normalize_origin_to_https() doesn’t cover ssh://[email protected]/owner/repo.git. Add this case to reduce surprises on repos with alternate remote syntax.
def normalize_origin_to_https(url: str) -> str: """Map common SSH origin forms to https for Unity's git URL scheme.""" - if url.startswith("[email protected]:"): + if url.startswith("[email protected]:"): owner_repo = url.split(":", 1)[1] if owner_repo.endswith(".git"): owner_repo = owner_repo[:-4] return f"https://github.com/{owner_repo}.git" + if url.startswith("ssh://[email protected]/"): + owner_repo = url.split("github.com/", 1)[1] + if not owner_repo.endswith(".git"): + owner_repo += ".git" + return f"https://github.com/{owner_repo}" # already https or file: etc. return url
119-128: Defer git detection unless needed.Currently we exit if not in a git repo, even if the user wants option [1] or [3]. Defer branch/origin queries until after choice selection so non‑git environments can still switch to upstream/local.
Minimal approach: move detect_repo_root/branch/origin after computing
choice, and only call them ifchoice == "2".
150-164: Validate local workspace path before writing manifest.If [3] is selected and the folder is missing or lacks package.json, Unity import will fail later. Fail fast with a clear message.
idx = int(choice) - 1 _, chosen = options[idx] + # Validate local workspace selection + if idx == 2: # [3] Local workspace + local_path_str = chosen[5:] if chosen.startswith("file:") else chosen + local_path = pathlib.Path(local_path_str) + if not local_path.exists() or not (local_path / "package.json").exists(): + print(f"Error: Local workspace '{local_path}' is invalid (missing directory or package.json).", file=sys.stderr) + sys.exit(1) + data = read_json(manifest_path)UnityMcpBridge/Editor/Helpers/PackageDetector.cs (3)
35-38: Log full exception details for easier root-cause analysis.Using the full exception (or LogException) preserves stack traces and inner exceptions.
Apply this diff:
- Debug.LogWarning("MCP for Unity: Auto-detect on load failed: " + ex.Message); + Debug.LogWarning($"MCP for Unity: Auto-detect on load failed: {ex}"); + // Alternatively: Debug.LogException(ex);
72-89: Broaden legacy path detection to macOS and Windows.Today only Linux-style roots are checked, which may miss legacy installs on other platforms and suppress the intended force-run behavior.
Apply this diff:
- string home = System.Environment.GetFolderPath(System.Environment.SpecialFolder.UserProfile) ?? string.Empty; - string[] roots = - { - System.IO.Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer", "src"), - System.IO.Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer", "src") - }; - foreach (var r in roots) + string home = System.Environment.GetFolderPath(System.Environment.SpecialFolder.UserProfile) ?? string.Empty; + var roots = new System.Collections.Generic.List<string> + { + // Linux + System.IO.Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer", "src"), + System.IO.Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer", "src"), + }; + // macOS + if (UnityEngine.Application.platform == UnityEngine.RuntimePlatform.OSXEditor) + { + roots.Add(System.IO.Path.Combine(home, "Library", "Application Support", "UnityMCP", "UnityMcpServer", "src")); + } + // Windows (Roaming and Local) + var appData = System.Environment.GetFolderPath(System.Environment.SpecialFolder.ApplicationData); + if (!string.IsNullOrEmpty(appData)) + roots.Add(System.IO.Path.Combine(appData, "UnityMCP", "UnityMcpServer", "src")); + var localAppData = System.Environment.GetFolderPath(System.Environment.SpecialFolder.LocalApplicationData); + if (!string.IsNullOrEmpty(localAppData)) + roots.Add(System.IO.Path.Combine(localAppData, "UnityMCP", "UnityMcpServer", "src")); + + foreach (var r in roots) { try { if (System.IO.File.Exists(System.IO.Path.Combine(r, "server.py"))) return true; } catch { } }
26-40: Offload ServerInstaller.EnsureServerInstalled I/O from main threadEnsureServerInstalled does large synchronous file and directory operations (copying, version checks, cleanup) on the main thread via EditorApplication.delayCall, which can stall the Editor. Move the heavy I/O into a background thread and marshal back only for Unity APIs (EditorPrefs, logging).
TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs.meta (1)
1-2: Meta file is minimal; consider adding MonoImporter block to avoid Unity auto-regeneration churn.Unity often writes MonoImporter settings for .cs metas. Keeping only guid/fileFormatVersion can cause Unity to rewrite the file locally, creating noisy diffs.
Apply:
fileFormatVersion: 2 guid: dfbabf507ab1245178d1a8e745d8d283 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant:Please confirm your repo’s convention for .cs .meta content; if you intentionally keep metas minimal, we can leave as-is.
UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json (1)
1-11: Dial up missing-import visibility for CI signal.
reportMissingImports: "none"can hide env/config issues. Consider “warning” to catch broken venvs while staying non-blocking.Apply:
- "reportMissingImports": "none", + "reportMissingImports": "warning",Optionally set execution environment roots/includes later if type-checking scope grows.
UnityMcpBridge/Editor/Tools/ManageEditor.cs (2)
171-189: Prefer Path helpers and invariant normalization for robustness.Slightly simplify and harden path derivation; also normalize with GetFullPath.
Apply:
- string assetsPath = Application.dataPath.Replace('\\', '/'); - string projectRoot = Directory.GetParent(assetsPath)?.FullName.Replace('\\', '/'); + var assetsPath = Application.dataPath; + var parent = Path.GetDirectoryName(assetsPath); + string projectRoot = parent != null ? Path.GetFullPath(parent).Replace('\\', '/') : null;
30-31: Use culture-invariant casing for command dispatch.Avoid locale issues (e.g., Turkish i).
Apply:
- string action = @params["action"]?.ToString().ToLower(); + string action = @params["action"]?.ToString().ToLowerInvariant();UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
17-31: Harden tool registration against partial failures.A single import/register failure will abort all subsequent tools. Wrap each registration to log and continue.
Apply within this range:
-def register_all_tools(mcp): +def register_all_tools(mcp): """Register all refactored tools with the MCP server.""" - # Prefer the surgical edits tool so LLMs discover it first - logger.info("Registering MCP for Unity Server refactored tools...") - 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("MCP for Unity Server tool registration complete.") + # Prefer the surgical edits tool so LLMs discover it first + logger.info("Registering MCP for Unity Server refactored tools...") + _safe_register(mcp, register_manage_script_edits_tools, "manage_script_edits") + _safe_register(mcp, register_manage_script_tools, "manage_script") + _safe_register(mcp, register_manage_scene_tools, "manage_scene") + _safe_register(mcp, register_manage_editor_tools, "manage_editor") + _safe_register(mcp, register_manage_gameobject_tools, "manage_gameobject") + _safe_register(mcp, register_manage_asset_tools, "manage_asset") + _safe_register(mcp, register_manage_shader_tools, "manage_shader") + _safe_register(mcp, register_read_console_tools, "read_console") + _safe_register(mcp, register_execute_menu_item_tools, "execute_menu_item") + # Expose resource wrappers as normal tools so IDEs without resources primitive can use them + _safe_register(mcp, register_resource_tools, "resource_tools") + logger.info("MCP for Unity Server tool registration complete.")Add this helper just above register_all_tools:
def _safe_register(mcp, fn, name: str) -> None: try: fn(mcp) logger.debug("Registered tool group: %s", name) except Exception as e: logger.exception("Failed registering %s: %s", name, e)UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (1)
57-58: Misleading variable name.isCursor is true for any non‑VSCode client. Consider renaming to isNonVSCode for clarity.
- bool isCursor = !isVSCode && (client == null || client.mcpType != McpTypes.VSCode); - if (isCursor && !string.IsNullOrEmpty(directory)) + bool isNonVSCode = !isVSCode; + if (isNonVSCode && !string.IsNullOrEmpty(directory))tests/test_logging_stdout.py (2)
27-69: Ruff S101 in tests: silence or scope it.If Ruff runs on tests, Bandit’s S101 flags assert. Either disable S101 for tests in pyproject or annotate these two asserts.
- assert not syntax_errors, "syntax errors in: " + ", ".join(str(e) for e in syntax_errors) - assert not offenders, "stdout writes found in: " + ", ".join(str(o) for o in offenders) + assert not syntax_errors, "syntax errors in: " + ", ".join(str(e) for e in syntax_errors) # noqa: S101 + assert not offenders, "stdout writes found in: " + ", ".join(str(o) for o in offenders) # noqa: S101Alternatively, configure Ruff to ignore S101 under tests/.
33-35: Skip common junk dirs.Minor: also skip pycache/build artifacts to reduce noise.
- parts = set(py_file.parts) - if ".venv" in parts or "site-packages" in parts: + parts = set(py_file.parts) + if ".venv" in parts or "site-packages" in parts or "__pycache__" in parts: continue.github/scripts/mark_skipped.py (1)
80-83: Prefer attribute message over node text for skip reason.Minor: show the human-friendly message first.
- msg = (n.get("message") or "") + "\n" + (n.text or "") + msg = (n.get("message") or "") + "\n" + (n.text or "") if should_skip(msg): - first_match_text = (n.text or "").strip() or first_match_text + first_match_text = ((n.get("message") or "").strip()) or ((n.text or "").strip()) or first_match_text to_skip = TrueUnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
346-351: Heartbeat reason on Stop() should be “stopped”, not “ready”.Currently writes “ready” during shutdown.
- // Mark heartbeat one last time before stopping - WriteHeartbeat(false); + // Mark heartbeat one last time before stopping + WriteHeartbeat(false, "stopped");
434-435: Remove unused buffer allocation.Not used.
- byte[] buffer = new byte[8192];
556-576: Zero-length frames: align behavior with PR objective.PR says “reject zero‑length frames,” but code treats them as empty commands. Choose one consistently. If rejection is desired:
- if (payloadLen == 0UL) - { - // Allow zero-length frames (e.g., heartbeats/empty responses) - return string.Empty; - } + if (payloadLen == 0UL) + throw new System.IO.IOException("Zero-length frames are not allowed");TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs (2)
39-41: Avoid implicit UnityEngine.Object truthiness for analyzers.Use explicit null check for clarity.
- if (currentTarget && Vector3.Distance(reachOrigin.position, currentTarget.position) <= maxReachDistance) + if (currentTarget != null && Vector3.Distance(reachOrigin.position, currentTarget.position) <= maxReachDistance) return currentTarget;
63-68: Update should be non-public.Unity doesn’t require public; prefer private to reduce API surface.
- public void Update() + private void Update()UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
482-497: Max edit payload: confirm 15 KiB is sufficient.Large structured edits may exceed this limit. Consider raising to 64 KiB to align with framing headroom.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
10-10: Remove unused import.Minor cleanup.
-from config import configUnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs (3)
121-125: macOS fallback: add a null/empty guard to avoid showing a blank pathIf
macConfigPathandconfigPathare both empty, the UI shows nothing. Add a cascade fallback (e.g., Linux path) or default toconfigPathonly when non-empty.- displayPath = string.IsNullOrEmpty(mcpClient.macConfigPath) - ? configPath - : mcpClient.macConfigPath; + displayPath = string.IsNullOrEmpty(mcpClient.macConfigPath) + ? (!string.IsNullOrEmpty(configPath) ? configPath : mcpClient.linuxConfigPath) + : mcpClient.macConfigPath;
141-148: Path text won’t wrap in a single-line TextField; use a TextArea (null-safe) for long paths
EditorGUILayout.TextFieldignoreswordWrapand truncates long paths. Replace with a TextArea and ensure null-safety.- GUIStyle pathStyle = new(EditorStyles.textField) { wordWrap = true }; - EditorGUILayout.TextField( - displayPath, - pathStyle, - GUILayout.Height(EditorGUIUtility.singleLineHeight) - ); + GUIStyle pathStyle = new(EditorStyles.textArea) { wordWrap = true }; + EditorGUILayout.TextArea( + displayPath ?? string.Empty, + pathStyle, + GUILayout.MinHeight(32) + );
181-190: More robust “Open File”: prefer RevealInFinder and handle missing paths
Process.StartwithUseShellExecute = truecan fail silently or do nothing if the file doesn’t exist. Reveal in Finder/Explorer gives clearer UX and works for both files and folders.- // Open the file using the system's default application - System.Diagnostics.Process.Start( - new System.Diagnostics.ProcessStartInfo - { - FileName = displayPath, - UseShellExecute = true, - } - ); + try + { + if (System.IO.File.Exists(displayPath) || System.IO.Directory.Exists(displayPath)) + { + UnityEditor.EditorUtility.RevealInFinder(displayPath); + } + else + { + UnityEditor.EditorUtility.DisplayDialog("Open Path", $"Path not found:\n{displayPath}", "OK"); + } + } + catch (System.Exception e) + { + Debug.LogError($"Failed to open path: {displayPath}\n{e}"); + UnityEditor.EditorUtility.DisplayDialog("Open Path", "Failed to open path. See Console for details.", "OK"); + }README.md (1)
46-48: Fix markdown list indentation (MD007) for new tool entriesRemove leading spaces so bullets align with the rest of the list, or normalize the whole block. This clears markdownlint MD007.
- * `apply_text_edits`: Precise text edits with precondition hashes and atomic multi-edit batches. - * `script_apply_edits`: Structured C# method/class edits (insert/replace/delete) with safer boundaries. - * `validate_script`: Fast validation (basic/standard) to catch syntax/structure issues before/after writes. +* `apply_text_edits`: Precise text edits with precondition hashes and atomic multi-edit batches. +* `script_apply_edits`: Structured C# method/class edits (insert/replace/delete) with safer boundaries. +* `validate_script`: Fast validation (basic/standard) to catch syntax/structure issues before/after writes.If you prefer consistent formatting across the entire “Available Tools” list, apply the same indentation fix to lines 38–45 as well.
UnityMcpBridge/UnityMcpServer~/src/config.py (1)
20-22: Add validation and clarify framing vs buffer_size
• In ServerConfig, implement a__post_init__to enforceframed_receive_timeout > 0andmax_heartbeat_frames > 0.
• Update the class doc or inline comment to note thatbuffer_sizeonly applies to the non-framed path; framed receives use_read_exactand enforceFRAMED_MAX(64 MiB) regardless ofbuffer_size.tests/test_get_sha.py (3)
42-47: Silence Ruff ARG002 in dummy decorator by underscoring unused params- def tool(self, *args, **kwargs): + def tool(self, *_args, **_kwargs):
69-73: Silence Ruff S101 for test asserts- assert captured["cmd"] == "manage_script" - assert captured["params"]["action"] == "get_sha" - assert captured["params"]["name"] == "A" - assert captured["params"]["path"].endswith("Assets/Scripts") - assert resp["success"] is True + assert captured["cmd"] == "manage_script" # noqa: S101 + assert captured["params"]["action"] == "get_sha" # noqa: S101 + assert captured["params"]["name"] == "A" # noqa: S101 + assert captured["params"]["path"].endswith("Assets/Scripts") # noqa: S101 + assert resp["success"] is True # noqa: S101Optionally, add a file-level directive at the top instead:
# ruff: noqa: S101.
55-66: Add a case for file:// and absolute paths to harden URI parsing coverageA second test covering
file://URIs (including UNC and drive-letter forms) strengthens_split_uriguarantees.I can add a parametric test for
unity://path/…,file://…(POSIX/Windows), and plainAssets/...inputs with expected(name, directory)outcomes.README-DEV.md (4)
69-78: Minor copy edits for readabilityInsert a blank line before “Test target script” and tighten phrasing.
-We provide a CI job to run a Natural Language Editing mini-suite against the Unity test project. It spins up a headless Unity container and connects via the MCP bridge. +We provide a CI job to run a Natural Language Editing mini-suite against the Unity test project. +It spins up a headless Unity container and connects via the MCP bridge.
84-87: Tighten bullet phrasing (grammar)-- Follow the conventions: single `<testsuite>` root, one `<testcase>` per sub-test, end system-out with `VERDICT: PASS|FAIL`. -– Keep edits minimal and reversible; include evidence windows and compact diffs. +- Follow the conventions: a single `<testsuite>` root, one `<testcase>` per sub-test, and end system-out with `VERDICT: PASS|FAIL`. +- Keep edits minimal and reversible; include evidence windows and compact diffs.
89-91: Use standard ordered list markers for consistency-1) Push your branch, then manually run the workflow from the Actions tab. -2) The job writes reports into `reports/` and uploads artifacts. -3) The “JUnit Test Report” check summarizes results; open the Job Summary for full markdown. +1. Push your branch, then manually run the workflow from the Actions tab. +2. The job writes reports into `reports/` and uploads artifacts. +3. The “JUnit Test Report” check summarizes results; open the Job Summary for full markdown.
99-103: Clarify log wording and reduce passive voice-- In CI, the job tails Unity logs (redacted for serial/license/password/token) and prints socket/status JSON diagnostics if startup fails. +- In CI, the job tails Unity logs (serial/license/password/token redacted) and prints socket/status JSON diagnostics if startup fails.UnityMcpBridge/Editor/Data/McpClients.cs (2)
164-174: Avoid duplicating textual status and enum.Relying on configStatus == "Not Configured" to backfill status risks drift. Consider deriving configStatus from the enum everywhere or removing the string field from the source of truth.
- if (client.configStatus == "Not Configured") - { - client.status = McpStatus.NotConfigured; - } + // Source of truth = enum; string derived for display + client.status = client.status == default ? McpStatus.NotConfigured : client.status; + client.configStatus = client.status.ToString().Replace('_', ' ');
137-161: Comment numbering is inconsistent (minor).“// 3) Kiro” should be “// 6) Kiro”.
- // 3) Kiro + // 6) Kirotests/test_edit_normalization_and_noop.py (2)
67-79: Add an assertion to verify the index‑range path actually applied.Currently the test doesn’t assert anything after the index pair route. Capture and assert the final manage_script call shape.
apply(None, uri="unity://path/Assets/Scripts/F.cs", edits=edits, precondition_sha256="x") - # last call is apply_text_edits - + # verify last call is apply_text_edits with normalized fields + last = calls[-1] + assert last.get("action") == "apply_text_edits" + e2 = last["edits"][0] + assert {"startLine","startCol","endLine","endCol","newText"} <= set(e2.keys()) + assert (e2["startLine"], e2["startCol"], e2["endLine"], e2["endCol"]) == (1, 1, 1, 1)
93-116: Drop unused structured‑tools registration to keep the test focused.tools_struct is not used. Remove to reduce noise.
- tools_struct = DummyMCP(); manage_script_edits.register_manage_script_edits_tools(tools_struct) + # structured tools not needed in this testtests/test_resources_api.py (3)
1-1: Remove duplicate pytest import.Minor cleanup; avoids F811 redefinition.
-import pytest +import pytest @@ -import pytestAlso applies to: 6-6
42-58: Remove unused monkeypatch arg.It’s not used in this test.
-def test_resource_list_filters_and_rejects_traversal(resource_tools, tmp_path, monkeypatch): +def test_resource_list_filters_and_rejects_traversal(resource_tools, tmp_path):
65-70: Also assert the canonical spec URI is included.The tool guarantees unity://spec/script-edits; assert it to lock the contract.
uris = resp["data"]["uris"] assert any(u.endswith("Assets/Scripts/A.cs") for u in uris) assert not any(u.endswith("B.txt") for u in uris) assert not any(u.endswith("Outside.cs") for u in uris) + assert "unity://spec/script-edits" in uris.claude/prompts/nl-unity-suite-full.md (2)
5-7: Tweak AllowedTools line for readability.Add spaces after commas so the agent log is easier to scan.
-AllowedTools: Write,Bash(printf:*),Bash(echo:*),Bash(scripts/nlt-revert.sh:*),mcp__unity__manage_editor,mcp__unity__list_resources,mcp__unity__read_resource,mcp__unity__apply_text_edits,mcp__unity__script_apply_edits,mcp__unity__validate_script,mcp__unity__find_in_file,mcp__unity__read_console,mcp__unity__get_sha +AllowedTools: Write, Bash(printf:*), Bash(echo:*), Bash(scripts/nlt-revert.sh:*), mcp__unity__manage_editor, mcp__unity__list_resources, mcp__unity__read_resource, mcp__unity__apply_text_edits, mcp__unity__script_apply_edits, mcp__unity__validate_script, mcp__unity__find_in_file, mcp__unity__read_console, mcp__unity__get_sha
97-109: Optional: run markdownlint autofix to normalize list indentation/headings.There are minor MD lint findings (MD004/5/7/26/36). Non‑blocking, but a quick pass will reduce noise in future reviews.
Also applies to: 185-191
tests/test_transport_framing.py (2)
78-119: Reduce flakiness in pre‑handshake detection.Slightly longer window and SO_REUSEADDR lower spurious failures on busy CI.
- sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) @@ - deadline = time.time() + 0.15 # short, reduces race with legitimate clients + deadline = time.time() + 0.25 # still short; reduces flakes on slow CI
160-199: Also assert framed mode in heartbeat test.Minor addition: ensure use_framing is set after connect.
conn = UnityConnection(host="127.0.0.1", port=port) try: - assert conn.connect() is True + assert conn.connect() is True + assert conn.use_framing is True # Receive should skip heartbeat and return the pong payload (or empty if only heartbeats seen) resp = conn.receive_full_response(conn.sock)test_unity_socket_framing.py (4)
47-59: Align framing limits with server cap (64 MiB) and clamp request size.Server frames are capped at 64 MiB. Clamp the generated payload to a safe max and reuse the same cap for response validation to avoid masking boundary regressions and accidental OOMs.
@@ -FILL = "R" +FILL = "R" +MAX_FRAME = 64 * 1024 * 1024 @@ def main(): - body = { + # Cap filler to stay within framing limit (reserve small overhead for JSON) + safe_max = max(1, MAX_FRAME - 4096) + filler_len = min(SIZE_MB * 1024 * 1024, safe_max) + body = { "type": "read_console", "params": { @@ - "includeStacktrace": True, - "filterText": FILL * (SIZE_MB * 1024 * 1024) + "includeStacktrace": True, + "filterText": FILL * filler_len } } @@ - MAX_RESP = 128 * 1024 * 1024 + MAX_RESP = MAX_FRAME if resp_len <= 0 or resp_len > MAX_RESP: raise RuntimeError(f"invalid framed length: {resp_len} (max {MAX_RESP})")Also applies to: 80-83, 10-11
23-28: Narrow exception scope in JSON check.Catching Exception hides decoding errors. Limit to the expected failures.
def is_valid_json(b): try: json.loads(b.decode("utf-8")) return True - except Exception: + except (UnicodeDecodeError, json.JSONDecodeError): return False
30-31: Don’t override caller timeout in legacy mode.recv_legacy_json resets the socket timeout to 60s, fighting the 120s set by main() and causing avoidable flakiness. Respect the caller’s timeout unless explicitly provided.
-def recv_legacy_json(sock, timeout=60): - sock.settimeout(timeout) +def recv_legacy_json(sock, timeout=None): + if timeout is not None: + sock.settimeout(timeout) @@ - resp = recv_legacy_json(s) + resp = recv_legacy_json(s, timeout=None)Also applies to: 86-86
64-69: Narrow the greeting read exception.Only silence socket timeouts; other errors should surface.
- try: - greeting = s.recv(256) - except Exception: + try: + greeting = s.recv(256) + except socket.timeout: greeting = b"".github/workflows/claude-nl-suite-mini.yml (2)
26-27: YAML lint: normalize spacing after colons and a stray space in allowed_tools.Reduces noise from linters; no behavior change.
- MD_OUT: reports/junit-nl-suite.md + MD_OUT: reports/junit-nl-suite.md @@ - UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} - UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} - UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} + UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} @@ - UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} - UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} @@ - UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} + UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} @@ - UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} @@ - UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} + UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} @@ - allowed_tools: "Write,mcp__unity__manage_editor,mcp__unity__list_resources,mcp__unity__read_resource,mcp__unity__apply_text_edits,mcp__unity__script_apply_edits,mcp__unity__validate_script,mcp__unity__find_in_file, mcp__unity__read_console" + allowed_tools: "Write,mcp__unity__manage_editor,mcp__unity__list_resources,mcp__unity__read_resource,mcp__unity__apply_text_edits,mcp__unity__script_apply_edits,mcp__unity__validate_script,mcp__unity__find_in_file,mcp__unity__read_console"Also applies to: 219-219, 33-36, 79-82, 107-109, 224-224
206-206: Trim trailing spaces flagged by YAML linters.Removes trailing spaces on blank lines to satisfy stricter linters.
- + @@ - + @@ - +Also applies to: 331-331, 343-343
tests/test_script_tools.py (1)
103-107: Quiet minor lints: underscore unused args in fakes.Keeps tests clean without changing behavior.
-def fake_send(cmd, params): +def fake_send(_cmd, params): captured["params"] = params return {"success": True} @@ -def fake_send(cmd, params): +def fake_send(_cmd, params): captured["params"] = params return {"success": True} @@ -async def fake_async(cmd, params, loop=None): +async def fake_async(cmd, params, _loop=None): captured["cmd"] = cmd captured["params"] = params return {"success": True}Also applies to: 119-123, 138-141
tests/test_manage_script_uri.py (2)
28-30: Fix one-line class style for readability and Ruff E701.-class _Dummy: pass +class _Dummy: + pass
114-116: Underscore unused arg in test helper.-def fake_send(cmd, params): +def fake_send(_cmd, params): captured['params'] = params return {"success": True, "message": "ok"}tests/test_edit_strict_and_warnings.py (2)
35-39: Expand one-liners to quiet Ruff and aid readability.-class DummyMCP: - def __init__(self): self.tools = {} - def tool(self, *args, **kwargs): - def deco(fn): self.tools[fn.__name__] = fn; return fn - return deco +class DummyMCP: + def __init__(self): + self.tools = {} + def tool(self, *args, **kwargs): + def deco(fn): + self.tools[fn.__name__] = fn + return fn + return deco
52-55: Underscore unused args in fakes.-def fake_send(cmd, params): +def fake_send(_cmd, _params): # Simulate Unity path returning minimal success return {"success": True} @@ -def fake_send(cmd, params): +def fake_send(_cmd, _params): return {"success": True}Also applies to: 74-76
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)
725-731: macOS path equality may be wrong on case-sensitive volumesTreating macOS paths as case-insensitive can mis-detect matches on APFS/HFS+ case-sensitive volumes. Consider Windows-only IgnoreCase and otherwise ordinal, or probe FS sensitivity per path.
Example:
- if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) - { - return string.Equals(na, nb, StringComparison.OrdinalIgnoreCase); - } - return string.Equals(na, nb, StringComparison.Ordinal); + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + return string.Equals(na, nb, StringComparison.OrdinalIgnoreCase); + // Default to ordinal on Unix; optionally detect FS case-sensitivity at runtime if needed + return string.Equals(na, nb, StringComparison.Ordinal);
2192-2196: Use PathsEqual for project-path match (OS semantics)Avoid OrdinalIgnoreCase here; reuse PathsEqual for consistent semantics across OSes.
- if (string.Equals(normalizedProjectPath, normalizedProjectDir, StringComparison.OrdinalIgnoreCase) && project.Value?.mcpServers != null) + if (PathsEqual(normalizedProjectPath, normalizedProjectDir) && project.Value?.mcpServers != null).github/workflows/claude-nl-suite.yml (3)
1-543: Fix YAML lint issues (trailing spaces, indentation, EOF newline)Numerous trailing spaces, a few indentation warnings, and missing newline at EOF. Clean formatting to reduce CI noise.
Minimal actions:
- Strip trailing spaces
- Fix over-indented keys (see hints)
- Add a newline at end of file
271-294: Pin external action to a commit SHA for reproducibilityanthropics/claude-code-base-action@beta is mutable. Pin to a commit to avoid surprise breaks.
- uses: anthropics/claude-code-base-action@beta + uses: anthropics/claude-code-base-action@<commit-sha>
140-179: Secret redaction is good; add GitHub masking for env echoesYou already sed‑redact logs. Also add explicit masking for any echoed envs to be safe.
- docker logs -f unity-mcp 2>&1 | sed -E 's/((serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' & LOGPID=$! + docker logs -f unity-mcp 2>&1 | sed -E 's/((serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' & LOGPID=$! + echo "::add-mask::${UNITY_EMAIL:-}" + echo "::add-mask::${UNITY_PASSWORD:-}" + echo "::add-mask::${UNITY_SERIAL:-}"tests/test_regex_delete_guard.py (1)
16-19: Tidy single-line defs for readability (ruff E701/E702)Split single-line class and decorator bodies; keeps style quiet without changing behavior.
-class _D: pass +class _D: + pass ... -class DummyMCP: - def __init__(self): self.tools = {} - def tool(self, *args, **kwargs): - def deco(fn): self.tools[fn.__name__] = fn; return fn - return deco +class DummyMCP: + def __init__(self): + self.tools = {} + def tool(self, *args, **kwargs): + def deco(fn): + self.tools[fn.__name__] = fn + return fn + return decoAlso applies to: 37-41
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (3)
92-93: Defensive: use getattr for connection timeout restoreAvoid assuming config.connection_timeout exists.
- self.sock.settimeout(config.connection_timeout) + self.sock.settimeout(float(getattr(config, "connection_timeout", 1.0)))
95-102: Prefer logger.exception for caught errorsKeeps tracebacks in logs for hard-to-repro socket issues.
- logger.error(f"Failed to connect to Unity: {str(e)}") + logger.exception("Failed to connect to Unity") ... - logger.error(f"Error during framed receive: {str(e)}") + logger.exception("Error during framed receive")Also applies to: 151-152
247-287: Short receive-timeout toggle: simplify unused statelast_short_timeout is never set; simplify by removing it or actually use it to avoid repeated get/settimeout calls.
- restore_timeout = None - if attempt > 0 and last_short_timeout is None: - restore_timeout = self.sock.gettimeout() - self.sock.settimeout(1.0) + restore_timeout = None + if attempt > 0: + restore_timeout = self.sock.gettimeout() + self.sock.settimeout(1.0) ... - if restore_timeout is not None: - self.sock.settimeout(restore_timeout) - last_short_timeout = None + if restore_timeout is not None: + self.sock.settimeout(restore_timeout)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (3)
41-43: Replace blind except/pass with debug logsMultiple silent failures make triage difficult. Capture context at debug level; keep behavior unchanged.
+import logging +logger = logging.getLogger(__name__) @@ - except Exception: - pass + except Exception: + logger.debug("manage_editor.get_project_root probe failed", exc_info=True) @@ - except Exception: - pass + except Exception: + logger.debug("Shallow project search failed", exc_info=True) @@ - except Exception: - pass + except Exception: + logger.debug("Windows file:// normalization failed", exc_info=True) @@ - except Exception: - continue + except Exception: + logger.debug("Symlink/containment check failed for %s", p, exc_info=True) + continue @@ - except Exception as e: - return {"success": False, "error": str(e)} + except Exception as e: + logger.debug("list_resources error", exc_info=True) + return {"success": False, "error": str(e)} @@ - except Exception as e: - return {"success": False, "error": str(e)} + except Exception as e: + logger.debug("find_in_file error", exc_info=True) + return {"success": False, "error": str(e)}Also applies to: 71-72, 92-93, 151-152, 168-169, 313-314, 354-355
294-305: Validate negative/zero windowing argumentsNegative head_bytes or tail_lines currently fall through to full reads. Be explicit to avoid surprises and large payloads.
- if head_bytes and head_bytes > 0: + if head_bytes is not None and head_bytes <= 0: + return {"success": False, "error": "head_bytes must be > 0"} + if tail_lines is not None and tail_lines < 0: + return {"success": False, "error": "tail_lines must be >= 0"} + if head_bytes: raw = p.read_bytes()[: head_bytes] text = raw.decode("utf-8", errors="replace")
111-116: Doc tweak: be explicit that .cs is default, not enforcedSince the implementation now respects pattern, adjust the description to avoid confusion.
- "Notes: Only .cs files are returned by default; always appends unity://spec/script-edits.\n" + "Notes: Defaults to returning .cs files (pattern=*.cs); always appends unity://spec/script-edits.\n"UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (4)
146-201: Unify URI/name/path normalization to avoid drift_local locator parsing differs from manage_script._split_uri (file://, Assets detection). Prefer a shared helper to avoid edge-case regressions (Windows, UNC, duplicate tails).
I can extract a small utils module (e.g., tools/path_utils.py) with a single normalize_uri_to_name_path(uri_or_name, path) used by both modules. Want me to send a patch?
620-626: Misleading variable nametext_opsThis set includes all ops, not only text ops. Rename to ops or similar for clarity.
- text_ops = { (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() for e in (edits or []) } + ops_all = { (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() for e in (edits or []) } @@ - if not text_ops.issubset(structured_kinds): + if not ops_all.issubset(structured_kinds):
742-759: Diff list slicing: favor list spread for truncation nitsTiny readability improvement per Ruff hint.
- if len(diff) > 800: - diff = diff[:800] + ["... (diff truncated) ..."] + if len(diff) > 800: + diff = [*diff[:800], "... (diff truncated) ..."] @@ - if len(diff) > 2000: - diff = diff[:2000] + ["... (diff truncated) ..."] + if len(diff) > 2000: + diff = [*diff[:2000], "... (diff truncated) ..."]
583-599: Narrow broad exception when compiling regex anchorsRegex compilation errors are expected; catch re.error specifically to avoid masking unrelated issues.
- except Exception as ex: + except re.error as ex:UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
12-60: _split_uri: good coverage; consider Windows drive/UNC edge-string checksLogic looks solid and consistent with resource handling. Minor: normalize drive-letter check using a regex for readability and future-proofing. Otherwise LGTM.
- if os.name == "nt" and len(raw_path) >= 3 and raw_path[0] == "/" and raw_path[2] == ":": + if os.name == "nt" and re.match(r"^/[A-Za-z]:/", raw_path): raw_path = raw_path[1:]
268-279: debug_preview block assigns unused localsRemove unused variables to satisfy lints and reduce noise.
- import difflib - # Apply locally to preview final result - lines = [] - # Build an indexable original from a read if we normalized from read; otherwise skip - prev = "" + # Return normalized spans without writing
411-466: Legacy 'update' migration path: solid, but consider payload cap configurabilityGood migration to apply_text_edits with precondition. Suggest making 256 KiB cap a constant or read from server capability when available.
- if payload_bytes > 256 * 1024: + MAX_PAYLOAD = 256 * 1024 # TODO: read from capabilities when exposed + if payload_bytes > MAX_PAYLOAD: return {"success": False, "code": "payload_too_large", "message": f"Edit payload {payload_bytes} bytes exceeds 256 KiB cap; try structured ops or chunking."}
| import sys | ||
| import os | ||
| import re | ||
| import xml.etree.ElementTree as ET |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use defusedxml to parse untrusted JUnit XML.
xml.etree is vulnerable to XML attacks. Prefer defusedxml (with graceful fallback) to harden CI parsing.
@@
-import xml.etree.ElementTree as ET
+try:
+ from defusedxml import ElementTree as ET # safe XML parsing
+except ImportError: # fallback with warning
+ import xml.etree.ElementTree as ET
+ print("[mark_skipped] defusedxml not available; falling back to xml.etree (less safe)")
@@
- try:
- tree = ET.parse(path)
+ try:
+ tree = ET.parse(path)Also applies to: 62-65
🤖 Prompt for AI Agents
In .github/scripts/mark_skipped.py around line 15 (and also apply the same
change to lines 62-65), the script currently imports xml.etree.ElementTree which
is unsafe for parsing untrusted JUnit XML; update the import to try importing
defusedxml.ElementTree as ET and fall back to xml.etree.ElementTree if
defusedxml is not available, emitting a clear warning when falling back so CI
operators know parsing is less secure; ensure subsequent code uses ET unchanged
so behavior stays the same.
| .DS_Store* | ||
| # Unity test project lock files | ||
| TestProjects/UnityMCPTests/Packages/packages-lock.json |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Tighten macOS ignore and confirm Unity lockfile strategy
- Replace the wildcard to avoid unintentionally hiding files; optionally add AppleDouble ignores.
- If the test project is meant to float packages, OK. Otherwise, ignoring Packages/packages-lock.json can reduce determinism. Consider scoping the ignore to all test projects if that’s the intent.
Apply:
-.DS_Store*
+.DS_Store
+# macOS AppleDouble files
+._*
-# Unity test project lock files
-TestProjects/UnityMCPTests/Packages/packages-lock.json
+# Unity test project lock files (all test projects)
+TestProjects/**/Packages/packages-lock.jsonVerify Unity transients are ignored somewhere (root or per test project):
🏁 Script executed:
#!/bin/bash
# Look for common Unity transient dirs in any .gitignore
rg -n --glob '**/.gitignore' -C1 -e '^(Library|Temp|Obj|Logs|Build|UserSettings)/$'
# Check if other test projects also produce lockfiles that should be ignored
fd -a --glob 'TestProjects/**/Packages/packages-lock.json' || trueLength of output: 146
🏁 Script executed:
#!/bin/bash
# Search for Unity transient directory patterns in all .gitignore files
rg -n --glob '*.gitignore' -e 'Library/' -e 'Temp/' -e 'Obj/' -e 'Logs/' -e 'Build/' -e 'UserSettings/'
# List all Unity Packages lockfiles under TestProjects
find TestProjects -type f -path '*/Packages/packages-lock.json'Length of output: 213
Tighten macOS ignore patterns and cover Unity transients
- Rename
.DS_Store*to.DS_Storeand add._*for AppleDouble metadata. - Add common Unity transient folders at the root of
.gitignore:Library/ Temp/ Obj/ Logs/ Build/ UserSettings/ - Retain the existing
TestProjects/UnityMCPTests/Packages/packages-lock.jsonentry for today’s only test project; generalize toTestProjects/**/Packages/packages-lock.jsonif you plan to add more.
🤖 Prompt for AI Agents
In .gitignore around lines 36 to 38, tighten macOS and Unity ignores: replace
the broad `.DS_Store*` entry with a precise `.DS_Store` and add an `._*` pattern
to cover AppleDouble metadata, then add root-level Unity transient folders to
ignore (Library, Temp, Obj, Logs, Build, UserSettings) so Unity build artefacts
are excluded; keep the existing
TestProjects/UnityMCPTests/Packages/packages-lock.json entry (or broaden to
TestProjects/**/Packages/packages-lock.json if you expect more test projects).
| def start_dummy_server(greeting: bytes, respond_ping: bool = False): | ||
| """Start a minimal TCP server for handshake tests.""" | ||
| sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| sock.bind(("127.0.0.1", 0)) | ||
| sock.listen(1) | ||
| port = sock.getsockname()[1] | ||
| ready = threading.Event() | ||
|
|
||
| def _run(): | ||
| ready.set() | ||
| conn, _ = sock.accept() | ||
| conn.settimeout(1.0) | ||
| if greeting: | ||
| conn.sendall(greeting) | ||
| if respond_ping: | ||
| try: | ||
| # Read exactly n bytes helper | ||
| 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 | ||
|
|
||
| header = _read_exact(8) | ||
| if len(header) == 8: | ||
| length = struct.unpack(">Q", header)[0] | ||
| payload = _read_exact(length) | ||
| if payload == b'{"type":"ping"}': | ||
| resp = b'{"type":"pong"}' | ||
| conn.sendall(struct.pack(">Q", len(resp)) + resp) | ||
| except Exception: | ||
| pass | ||
| time.sleep(0.1) | ||
| try: | ||
| conn.close() | ||
| except Exception: | ||
| pass | ||
| finally: | ||
| sock.close() | ||
|
|
||
| threading.Thread(target=_run, daemon=True).start() | ||
| ready.wait() | ||
| return port | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden dummy server and frame reader for test robustness.
Enable SO_REUSEADDR and make _read_exact raise on early EOF to surface partial reads.
def start_dummy_server(greeting: bytes, respond_ping: bool = False):
@@
- sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+ sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+ sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
@@
- 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 before reading expected bytes")
+ buf += chunk
+ return buf📝 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 start_dummy_server(greeting: bytes, respond_ping: bool = False): | |
| """Start a minimal TCP server for handshake tests.""" | |
| sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | |
| sock.bind(("127.0.0.1", 0)) | |
| sock.listen(1) | |
| port = sock.getsockname()[1] | |
| ready = threading.Event() | |
| def _run(): | |
| ready.set() | |
| conn, _ = sock.accept() | |
| conn.settimeout(1.0) | |
| if greeting: | |
| conn.sendall(greeting) | |
| if respond_ping: | |
| try: | |
| # Read exactly n bytes helper | |
| 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 | |
| header = _read_exact(8) | |
| if len(header) == 8: | |
| length = struct.unpack(">Q", header)[0] | |
| payload = _read_exact(length) | |
| if payload == b'{"type":"ping"}': | |
| resp = b'{"type":"pong"}' | |
| conn.sendall(struct.pack(">Q", len(resp)) + resp) | |
| except Exception: | |
| pass | |
| time.sleep(0.1) | |
| try: | |
| conn.close() | |
| except Exception: | |
| pass | |
| finally: | |
| sock.close() | |
| threading.Thread(target=_run, daemon=True).start() | |
| ready.wait() | |
| return port | |
| def start_dummy_server(greeting: bytes, respond_ping: bool = False): | |
| """Start a minimal TCP server for handshake tests.""" | |
| sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | |
| # allow quick reuse of this address/port in tests | |
| sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | |
| sock.bind(("127.0.0.1", 0)) | |
| sock.listen(1) | |
| port = sock.getsockname()[1] | |
| ready = threading.Event() | |
| def _run(): | |
| ready.set() | |
| conn, _ = sock.accept() | |
| conn.settimeout(1.0) | |
| if greeting: | |
| conn.sendall(greeting) | |
| if respond_ping: | |
| try: | |
| # Read exactly n bytes helper | |
| def _read_exact(n: int) -> bytes: | |
| buf = b"" | |
| while len(buf) < n: | |
| chunk = conn.recv(n - len(buf)) | |
| if not chunk: | |
| # surface unexpected EOF immediately | |
| raise ConnectionError("EOF before reading expected bytes") | |
| buf += chunk | |
| return buf | |
| header = _read_exact(8) | |
| if len(header) == 8: | |
| length = struct.unpack(">Q", header)[0] | |
| payload = _read_exact(length) | |
| if payload == b'{"type":"ping"}': | |
| resp = b'{"type":"pong"}' | |
| conn.sendall(struct.pack(">Q", len(resp)) + resp) | |
| except Exception: | |
| pass | |
| time.sleep(0.1) | |
| try: | |
| conn.close() | |
| except Exception: | |
| pass | |
| finally: | |
| sock.close() | |
| threading.Thread(target=_run, daemon=True).start() | |
| ready.wait() | |
| return port |
🧰 Tools
🪛 Ruff (0.12.2)
63-64: try-except-pass detected, consider logging the exception
(S110)
63-63: Do not catch blind exception: Exception
(BLE001)
68-69: try-except-pass detected, consider logging the exception
(S110)
68-68: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In tests/test_transport_framing.py around lines 30 to 76, the dummy server
should set SO_REUSEADDR before bind and the _read_exact helper must raise on
early EOF instead of returning partial data; update the socket creation to call
sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) prior to
sock.bind(...), and change _read_exact to raise an appropriate exception (e.g.,
EOFError or ConnectionError) when conn.recv returns an empty bytes object before
the requested number of bytes have been read so callers can detect and handle
partial reads.
| if (directory.Contains(canonical)) | ||
| { | ||
| effectiveDir = directory.Replace(canonical, symlinkSeg); | ||
| var candidate = directory.Replace(canonical, symlinkSeg).Replace('\\', '/'); | ||
| if (System.IO.Directory.Exists(candidate)) | ||
| { | ||
| effectiveDir = candidate; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make canonical → symlink replacement case-insensitive.
Current Contains/Replace are case-sensitive and may miss common variations. Use IndexOf with OrdinalIgnoreCase and rebuild the string.
- if (directory.Contains(canonical))
- {
- var candidate = directory.Replace(canonical, symlinkSeg).Replace('\\', '/');
- if (System.IO.Directory.Exists(candidate))
- {
- effectiveDir = candidate;
- }
- }
+ int i = directory.IndexOf(canonical, StringComparison.OrdinalIgnoreCase);
+ if (i >= 0)
+ {
+ var candidate = (directory.Remove(i, canonical.Length)
+ .Insert(i, symlinkSeg))
+ .Replace('\\', '/');
+ if (System.IO.Directory.Exists(candidate))
+ {
+ effectiveDir = candidate;
+ }
+ }📝 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 (directory.Contains(canonical)) | |
| { | |
| effectiveDir = directory.Replace(canonical, symlinkSeg); | |
| var candidate = directory.Replace(canonical, symlinkSeg).Replace('\\', '/'); | |
| if (System.IO.Directory.Exists(candidate)) | |
| { | |
| effectiveDir = candidate; | |
| } | |
| } | |
| int i = directory.IndexOf(canonical, StringComparison.OrdinalIgnoreCase); | |
| if (i >= 0) | |
| { | |
| var candidate = (directory.Remove(i, canonical.Length) | |
| .Insert(i, symlinkSeg)) | |
| .Replace('\\', '/'); | |
| if (System.IO.Directory.Exists(candidate)) | |
| { | |
| effectiveDir = candidate; | |
| } | |
| } |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs around lines 66 to 73, the
current directory.Contains and Replace calls are case-sensitive; change to
perform a case-insensitive search using directory.IndexOf(canonical,
StringComparison.OrdinalIgnoreCase), and if index >= 0, rebuild the candidate
path by taking directory.Substring(0, index) + symlinkSeg +
directory.Substring(index + canonical.Length), then normalize directory
separators to '/' and check System.IO.Directory.Exists(candidate) before
assigning effectiveDir. Ensure you do not use the case-sensitive Replace or
Contains methods.
| private const ulong MaxFrameBytes = 64UL * 1024 * 1024; // 64 MiB hard cap for framed payloads | ||
| private const int FrameIOTimeoutMs = 30000; // Per-read timeout to avoid stalled clients | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a separate command-processing timeout.
Frames have a 30s read timeout, but a command can hang indefinitely (awaiting tcs). Add a command timeout constant.
- private const ulong MaxFrameBytes = 64UL * 1024 * 1024; // 64 MiB hard cap for framed payloads
- private const int FrameIOTimeoutMs = 30000; // Per-read timeout to avoid stalled clients
+ private const ulong MaxFrameBytes = 64UL * 1024 * 1024; // 64 MiB hard cap for framed payloads
+ private const int FrameIOTimeoutMs = 30000; // Per-read timeout to avoid stalled clients
+ private const int CommandTimeoutMs = 60000; // Per-command processing timeout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private const ulong MaxFrameBytes = 64UL * 1024 * 1024; // 64 MiB hard cap for framed payloads | |
| private const int FrameIOTimeoutMs = 30000; // Per-read timeout to avoid stalled clients | |
| private const ulong MaxFrameBytes = 64UL * 1024 * 1024; // 64 MiB hard cap for framed payloads | |
| private const int FrameIOTimeoutMs = 30000; // Per-read timeout to avoid stalled clients | |
| private const int CommandTimeoutMs = 60000; // Per-command processing timeout |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/MCPForUnityBridge.cs around lines 38–40, add a
dedicated command-processing timeout constant (e.g. private const int
CommandProcessingTimeoutMs = 60000) alongside MaxFrameBytes and
FrameIOTimeoutMs, and update the command-handling logic to use this timeout when
awaiting any TaskCompletionSource or long-running command: apply a timed wait
(or Task.WhenAny with Task.Delay(CommandProcessingTimeoutMs)) so commands that
exceed the timeout are aborted/failed, the TCS is completed with an
error/timeout result, and an appropriate log/error path is triggered to avoid
hanging indefinitely.
| elif op == "replace_range": | ||
| start_line = int(edit.get("startLine", 1)) | ||
| start_col = int(edit.get("startCol", 1)) | ||
| end_line = int(edit.get("endLine", start_line)) | ||
| end_col = int(edit.get("endCol", 1)) | ||
| replacement = edit.get("text", "") | ||
| lines = text.splitlines(keepends=True) | ||
| max_line = len(lines) + 1 # 1-based, exclusive end | ||
| if (start_line < 1 or end_line < start_line or end_line > max_line | ||
| or start_col < 1 or end_col < 1): | ||
| raise RuntimeError("replace_range out of bounds") | ||
| def index_of(line: int, col: int) -> int: | ||
| if line <= len(lines): | ||
| return sum(len(l) for l in lines[: line - 1]) + (col - 1) | ||
| return sum(len(l) for l in lines) | ||
| a = index_of(start_line, start_col) | ||
| b = index_of(end_line, end_col) | ||
| text = text[:a] + replacement + text[b:] |
There was a problem hiding this comment.
replace_range can cross line boundaries when columns exceed line length
index_of allows columns past EOL, shifting into subsequent lines and corrupting spans. Clamp or reject out-of-bounds columns per line.
elif op == "replace_range":
@@
- def index_of(line: int, col: int) -> int:
- if line <= len(lines):
- return sum(len(l) for l in lines[: line - 1]) + (col - 1)
- return sum(len(l) for l in lines)
+ def index_of(line: int, col: int) -> int:
+ if line <= len(lines):
+ line_len = len(lines[line - 1])
+ if col - 1 > line_len:
+ raise RuntimeError("replace_range column out of bounds")
+ return sum(len(l) for l in lines[: line - 1]) + (col - 1)
+ # end-exclusive sentinel line is allowed only when col == 1
+ if line == len(lines) + 1 and col == 1:
+ return sum(len(l) for l in lines)
+ raise RuntimeError("replace_range line out of bounds")📝 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.
| elif op == "replace_range": | |
| start_line = int(edit.get("startLine", 1)) | |
| start_col = int(edit.get("startCol", 1)) | |
| end_line = int(edit.get("endLine", start_line)) | |
| end_col = int(edit.get("endCol", 1)) | |
| replacement = edit.get("text", "") | |
| lines = text.splitlines(keepends=True) | |
| max_line = len(lines) + 1 # 1-based, exclusive end | |
| if (start_line < 1 or end_line < start_line or end_line > max_line | |
| or start_col < 1 or end_col < 1): | |
| raise RuntimeError("replace_range out of bounds") | |
| def index_of(line: int, col: int) -> int: | |
| if line <= len(lines): | |
| return sum(len(l) for l in lines[: line - 1]) + (col - 1) | |
| return sum(len(l) for l in lines) | |
| a = index_of(start_line, start_col) | |
| b = index_of(end_line, end_col) | |
| text = text[:a] + replacement + text[b:] | |
| elif op == "replace_range": | |
| start_line = int(edit.get("startLine", 1)) | |
| start_col = int(edit.get("startCol", 1)) | |
| end_line = int(edit.get("endLine", start_line)) | |
| end_col = int(edit.get("endCol", 1)) | |
| replacement = edit.get("text", "") | |
| lines = text.splitlines(keepends=True) | |
| max_line = len(lines) + 1 # 1-based, exclusive end | |
| if (start_line < 1 or end_line < start_line or end_line > max_line | |
| or start_col < 1 or end_col < 1): | |
| raise RuntimeError("replace_range out of bounds") | |
| def index_of(line: int, col: int) -> int: | |
| if line <= len(lines): | |
| line_len = len(lines[line - 1]) | |
| if col - 1 > line_len: | |
| raise RuntimeError("replace_range column out of bounds") | |
| return sum(len(l) for l in lines[: line - 1]) + (col - 1) | |
| # end-exclusive sentinel line is allowed only when col == 1 | |
| if line == len(lines) + 1 and col == 1: | |
| return sum(len(l) for l in lines) | |
| raise RuntimeError("replace_range line out of bounds") | |
| a = index_of(start_line, start_col) | |
| b = index_of(end_line, end_col) | |
| text = text[:a] + replacement + text[b:] |
🧰 Tools
🪛 Ruff (0.12.2)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Function definition does not bind loop variable lines
(B023)
62-62: Ambiguous variable name: l
(E741)
62-62: Function definition does not bind loop variable lines
(B023)
63-63: Ambiguous variable name: l
(E741)
63-63: Function definition does not bind loop variable lines
(B023)
| # Enforce .cs extension regardless of provided pattern | ||
| if p.suffix.lower() != ".cs": | ||
| continue | ||
| if pattern and not fnmatch.fnmatch(p.name, pattern): | ||
| continue |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pattern handling contradicts docs; respect provided glob instead of forcing .cs
Docs say pattern defaults to *.cs but can vary; code hard-enforces .cs regardless. Either enforce via docs or let pattern drive filtering (defaulting to *.cs when None).
- # Enforce .cs extension regardless of provided pattern
- if p.suffix.lower() != ".cs":
- continue
- if pattern and not fnmatch.fnmatch(p.name, pattern):
+ # Default to *.cs when pattern is None; otherwise respect caller's glob
+ eff_pattern = pattern or "*.cs"
+ if not fnmatch.fnmatch(p.name, eff_pattern):
continue📝 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.
| # Enforce .cs extension regardless of provided pattern | |
| if p.suffix.lower() != ".cs": | |
| continue | |
| if pattern and not fnmatch.fnmatch(p.name, pattern): | |
| continue | |
| # Default to *.cs when pattern is None; otherwise respect caller's glob | |
| eff_pattern = pattern or "*.cs" | |
| if not fnmatch.fnmatch(p.name, eff_pattern): | |
| continue |
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py around lines
153-157, the code currently forces files to have a .cs suffix before applying
the pattern which contradicts the docs; instead, if pattern is None set pattern
= "*.cs" and then use fnmatch on the filename (case-insensitive) to drive
filtering — remove the explicit p.suffix.lower() != ".cs" check, default pattern
to "*.cs" when not provided, and apply fnmatch.fnmatch(p.name.lower(),
pattern.lower()) so the provided glob controls which files are returned.
| try: | ||
| project = _resolve_project_root(project_root) | ||
| p = _resolve_safe_path_from_uri(uri, project) | ||
| if not p or not p.exists() or not p.is_file(): | ||
| return {"success": False, "error": f"Resource not found: {uri}"} | ||
|
|
||
| text = p.read_text(encoding="utf-8") | ||
| flags = re.MULTILINE | ||
| if ignore_case: | ||
| flags |= re.IGNORECASE |
There was a problem hiding this comment.
Allow only Assets/ reads in find_in_file to match security posture
read_resource enforces Assets/ containment; find_in_file does not. This allows regex scans anywhere under project root if a crafted URI is passed.
project = _resolve_project_root(project_root)
p = _resolve_safe_path_from_uri(uri, project)
if not p or not p.exists() or not p.is_file():
return {"success": False, "error": f"Resource not found: {uri}"}
+ try:
+ p.relative_to(project / "Assets")
+ except ValueError:
+ return {"success": False, "error": "Search restricted to Assets/"}📝 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.
| try: | |
| project = _resolve_project_root(project_root) | |
| p = _resolve_safe_path_from_uri(uri, project) | |
| if not p or not p.exists() or not p.is_file(): | |
| return {"success": False, "error": f"Resource not found: {uri}"} | |
| text = p.read_text(encoding="utf-8") | |
| flags = re.MULTILINE | |
| if ignore_case: | |
| flags |= re.IGNORECASE | |
| try: | |
| project = _resolve_project_root(project_root) | |
| p = _resolve_safe_path_from_uri(uri, project) | |
| if not p or not p.exists() or not p.is_file(): | |
| return {"success": False, "error": f"Resource not found: {uri}"} | |
| try: | |
| p.relative_to(project / "Assets") | |
| except ValueError: | |
| return {"success": False, "error": "Search restricted to Assets/"} | |
| text = p.read_text(encoding="utf-8") | |
| flags = re.MULTILINE | |
| if ignore_case: | |
| flags |= re.IGNORECASE |
| if heartbeat_count >= getattr(config, 'max_heartbeat_frames', 16) or time.monotonic() > deadline: | ||
| # Treat as empty successful response to match C# server behavior | ||
| logger.debug("Heartbeat threshold reached; returning empty response") | ||
| return b"" | ||
| continue | ||
| if payload_len > FRAMED_MAX: | ||
| raise ValueError(f"Invalid framed length: {payload_len}") | ||
| payload = self._read_exact(sock, payload_len) | ||
| logger.debug(f"Received framed response ({len(payload)} bytes)") | ||
| return payload |
There was a problem hiding this comment.
Framed receive returns empty payload on heartbeat threshold — breaks JSON decode
Returning b"" causes json.loads to fail in send_command. Treat as timeout (retryable) instead.
- if heartbeat_count >= getattr(config, 'max_heartbeat_frames', 16) or time.monotonic() > deadline:
- # Treat as empty successful response to match C# server behavior
- logger.debug("Heartbeat threshold reached; returning empty response")
- return b""
+ if heartbeat_count >= getattr(config, 'max_heartbeat_frames', 16) or time.monotonic() > deadline:
+ logger.debug("Heartbeat threshold reached; raising timeout for retry")
+ raise TimeoutError("No data frame before heartbeat threshold")📝 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 heartbeat_count >= getattr(config, 'max_heartbeat_frames', 16) or time.monotonic() > deadline: | |
| # Treat as empty successful response to match C# server behavior | |
| logger.debug("Heartbeat threshold reached; returning empty response") | |
| return b"" | |
| continue | |
| if payload_len > FRAMED_MAX: | |
| raise ValueError(f"Invalid framed length: {payload_len}") | |
| payload = self._read_exact(sock, payload_len) | |
| logger.debug(f"Received framed response ({len(payload)} bytes)") | |
| return payload | |
| if heartbeat_count >= getattr(config, 'max_heartbeat_frames', 16) or time.monotonic() > deadline: | |
| logger.debug("Heartbeat threshold reached; raising timeout for retry") | |
| raise TimeoutError("No data frame before heartbeat threshold") | |
| continue | |
| if payload_len > FRAMED_MAX: | |
| raise ValueError(f"Invalid framed length: {payload_len}") | |
| payload = self._read_exact(sock, payload_len) | |
| logger.debug(f"Received framed response ({len(payload)} bytes)") | |
| return payload |
🧰 Tools
🪛 Ruff (0.12.2)
143-143: Abstract raise to an inner function
(TRY301)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/unity_connection.py around lines 137 to
146, the code currently returns b"" when the heartbeat threshold is reached
which breaks JSON decoding; change this to signal a retryable timeout instead of
returning an empty payload — replace the return b"" with raising a timeout-like
exception (e.g., raise TimeoutError("Heartbeat threshold reached") or raise
socket.timeout(...)) so the caller can treat it as a retryable timeout, and
ensure send_command (or the caller) handles that exception as a retryable
condition.
|
I'll fix a few of these CodeRabbit comments, then probably merge tomorrow. If anyone wants me to wait so you can look at it, let me know. I feel fine getting it out there and fixing stuff if we get any user reports. I've been using the guts of it for a week and it seems to work fine overall, and passes the new Claude-powered Unity CI tests -- although a couple of the later ones could use a little extra love, mainly fail because of braces matching issues sometimes. But those tests seem to work fine in regular Claude Desktop or Claude Code or Cursor, etc. Just fail sometimes in the CI Claude version that's heavily tool-restricted and turn restricted for testing. E.g. here's from Cursor below. You get same from Claude Desktop and Claude Code. (Just drop the actual tests from 
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
1856-1934: UV resolver duplication persists; delegate to ServerInstaller to avoid drift.A shared resolver already exists (Helpers/ServerInstaller.FindUvPath). Keeping two implementations risks divergence (search paths, validation, timeouts). Make the local method a thin wrapper or remove it.
- private string FindUvPath() - { - string uvPath = null; - ... - return uvPath; - } + // Delegate to the shared implementation to keep logic unified + private string FindUvPath() => ServerInstaller.FindUvPath();I can follow up by removing IsValidUvInstallation/FindWindowsUvPath if they become unused. Want me to push that cleanup?
🧹 Nitpick comments (5)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (5)
725-731: Don’t assume macOS is case-insensitive; centralize path equality.Treating macOS as case-insensitive can misclassify on case-sensitive APFS volumes. Prefer a single shared helper (e.g., expose Helpers/ServerInstaller.PathsEqualSafe or a new PathUtil) to avoid drift and platform inconsistencies.
Would you like me to extract a shared internal helper and update call sites?
1138-1150: Good switch to ServerInstaller.FindUvPath(); also handle “uv” on PATH.This looks solid. One small robustness tweak: if existingCommand is just “uv”/“uv.exe” (non-rooted), ValidateUvBinarySafe currently rejects it due to File.Exists. Consider allowing PATH lookups there.
Apply this change in ValidateUvBinarySafe (see separate comment for exact diff).
1216-1269: Atomic write flow looks robust; add mkdir + unique backup.
- Ensure the config directory exists before writing the temp file.
- Use a unique backup name to avoid clobbering a prior backup on repeated runs.
Apply:
- // Robust atomic write without redundant backup or race on existence - string tmp = configPath + ".tmp"; - string backup = configPath + ".backup"; + // Robust atomic write without redundant backup or race on existence + string dir = System.IO.Path.GetDirectoryName(configPath); + if (!string.IsNullOrEmpty(dir)) System.IO.Directory.CreateDirectory(dir); + string tmp = configPath + ".tmp"; + string backup = configPath + "." + System.Guid.NewGuid().ToString("N") + ".bak";
1167-1183: macOS path remap: make the probe case-insensitive.The substring probe uses Ordinal; on macOS default FS is case-insensitive. Use OrdinalIgnoreCase to catch “/UnityMCP/” variants.
- int idx = norm.IndexOf("/.local/share/UnityMCP/", StringComparison.Ordinal); + int idx = norm.IndexOf("/.local/share/UnityMCP/", StringComparison.OrdinalIgnoreCase);
1027-1049: Let ValidateUvBinarySafe accept PATH-resolved “uv”.Currently it rejects non-rooted “uv”/“uv.exe” because File.Exists fails. Allow process exec when the path isn’t rooted; still guard rooted paths with File.Exists.
- try - { - if (string.IsNullOrEmpty(path) || !System.IO.File.Exists(path)) return false; + try + { + if (string.IsNullOrEmpty(path)) return false; + bool rooted = System.IO.Path.IsPathRooted(path); + if (rooted && !System.IO.File.Exists(path)) return false; var psi = new System.Diagnostics.ProcessStartInfo { FileName = path, Arguments = "--version",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
ServerInstaller(11-712)FindUvPath(523-686)
🔇 Additional comments (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
2146-2154: LGTM: macOS-aware config path selection.The macOS override to prefer macConfigPath (falling back to linuxConfigPath) matches ConfigureMcpClient and reduces false negatives.
…erver_version; prepare framing tests
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
758-776: Fix VerifyBridgePing to speak framed protocol (match strict FRAMING=1).Current probe sends raw “ping”, which the bridge no longer accepts. Read handshake, then send a framed payload “ping” and expect a framed JSON response.
- private static bool VerifyBridgePing(int port) + private static bool VerifyBridgePing(int port) { try { - using TcpClient c = new TcpClient(); - var task = c.ConnectAsync(IPAddress.Loopback, port); - if (!task.Wait(500)) return false; - using NetworkStream s = c.GetStream(); - byte[] ping = Encoding.UTF8.GetBytes("ping"); - s.Write(ping, 0, ping.Length); - s.ReadTimeout = 1000; - byte[] buf = new byte[256]; - int n = s.Read(buf, 0, buf.Length); - if (n <= 0) return false; - string resp = Encoding.UTF8.GetString(buf, 0, n); - return resp.Contains("pong", StringComparison.OrdinalIgnoreCase); + using var c = new TcpClient(); + if (!c.ConnectAsync(IPAddress.Loopback, port).Wait(1000)) return false; + using var s = c.GetStream(); + c.NoDelay = true; + s.ReadTimeout = 1500; + s.WriteTimeout = 1500; + // Read handshake (single line) + var hsBuf = new byte[256]; + int hsN = s.Read(hsBuf, 0, hsBuf.Length); + string hs = hsN > 0 ? Encoding.ASCII.GetString(hsBuf, 0, hsN) : ""; + if (hs.IndexOf("FRAMING=1", StringComparison.OrdinalIgnoreCase) < 0) return false; + // Send framed "ping" + byte[] payload = Encoding.UTF8.GetBytes("ping"); + Span<byte> hdr = stackalloc byte[8]; + // big-endian length + hdr[0] = (byte)(payload.Length >> 56); + hdr[1] = (byte)(payload.Length >> 48); + hdr[2] = (byte)(payload.Length >> 40); + hdr[3] = (byte)(payload.Length >> 32); + hdr[4] = (byte)(payload.Length >> 24); + hdr[5] = (byte)(payload.Length >> 16); + hdr[6] = (byte)(payload.Length >> 8); + hdr[7] = (byte)(payload.Length); + s.Write(hdr); + s.Write(payload, 0, payload.Length); + // Read framed response + byte[] lenBuf = new byte[8]; + if (s.Read(lenBuf, 0, 8) != 8) return false; + ulong respLen = ((ulong)lenBuf[0] << 56) | ((ulong)lenBuf[1] << 48) | ((ulong)lenBuf[2] << 40) | + ((ulong)lenBuf[3] << 32) | ((ulong)lenBuf[4] << 24) | ((ulong)lenBuf[5] << 16) | + ((ulong)lenBuf[6] << 8) | lenBuf[7]; + if (respLen == 0 || respLen > (64UL * 1024 * 1024)) return false; + byte[] respBuf = new byte[(int)respLen]; + int off = 0; + while (off < (int)respLen) + { + int r = s.Read(respBuf, off, (int)respLen - off); + if (r <= 0) return false; + off += r; + } + string resp = Encoding.UTF8.GetString(respBuf); + return resp.IndexOf("pong", StringComparison.OrdinalIgnoreCase) >= 0; } catch { return false; } }
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
467-470: Add a command-processing timeout to avoid hanging connections.This awaits tcs.Task indefinitely. Prior feedback requested a timeout; please add it.
- string response = await tcs.Task; + string response; + const int CommandTimeoutMs = 60000; + var completed = await Task.WhenAny(tcs.Task, Task.Delay(CommandTimeoutMs)); + if (completed != tcs.Task) + { + response = JsonConvert.SerializeObject(new + { + status = "error", + error = "Command timed out", + code = "timeout" + }); + } + else + { + response = tcs.Task.Result; + }Also declare the constant near FrameIOTimeoutMs:
- private const int FrameIOTimeoutMs = 30000; // Per-read timeout to avoid stalled clients + private const int FrameIOTimeoutMs = 30000; // Per-read timeout to avoid stalled clients + private const int CommandTimeoutMs = 60000; // Per-command processing timeout
🧹 Nitpick comments (3)
test_unity_socket_framing.py (2)
65-76: Tighten greeting/exception handling and make shebang consistent.
- Use narrower exceptions (socket.timeout) for greeting reads.
- Either remove the shebang or mark the file executable in git to satisfy EXE001.
- try: - greeting = s.recv(256) - except Exception: + try: + greeting = s.recv(256) + except socket.timeout: greeting = b""
48-64: Guard against oversize final payload (JSON expansion).filler_len caps raw filler but json.dumps can exceed MAX_FRAME. Validate body_bytes before send.
body_bytes = json.dumps(body, ensure_ascii=False).encode("utf-8") + if len(body_bytes) > MAX_FRAME: + raise RuntimeError(f"payload too large after JSON encoding: {len(body_bytes)} > {MAX_FRAME}")UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
628-673: Unify error responses with Response.Error to include machine-parsable code.Response.Error now adds a code field; use it for invalid JSON and empty command errors.
- var invalidJsonResponse = new - { - status = "error", - error = "Invalid JSON format", - receivedText = commandText.Length > 50 - ? commandText[..50] + "..." - : commandText, - }; - tcs.SetResult(JsonConvert.SerializeObject(invalidJsonResponse)); + var snippet = commandText.Length > 50 ? commandText[..50] + "..." : commandText; + var resp = Response.Error("invalid_json", new { receivedText = snippet, length = commandText.Length }); + tcs.SetResult(JsonConvert.SerializeObject(resp));And similarly for the empty-command case above.
📜 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 (8)
TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs(1 hunks)UnityMcpBridge/Editor/Helpers/PackageDetector.cs(1 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(8 hunks)UnityMcpBridge/Editor/Models/McpClient.cs(1 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs(14 hunks)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(5 hunks)UnityMcpBridge/UnityMcpServer~/src/server_version.txt(1 hunks)test_unity_socket_framing.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- UnityMcpBridge/UnityMcpServer~/src/server_version.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- UnityMcpBridge/Editor/Models/McpClient.cs
- TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs
- UnityMcpBridge/Editor/Tools/ManageScript.cs
🧰 Additional context used
🧬 Code graph analysis (3)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
UnityMcpBridge/Editor/Models/Command.cs (1)
Command(8-19)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
ServerInstaller(11-712)EnsureServerInstalled(21-109)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
ServerInstaller(11-712)FindUvPath(523-686)
🪛 Ruff (0.12.2)
test_unity_socket_framing.py
1-1: Shebang is present but file is not executable
(EXE001)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Consider moving this statement to an else block
(TRY300)
28-28: Do not catch blind exception: Exception
(BLE001)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Do not catch blind exception: Exception
(BLE001)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
555-572: LGTM: Framed read path and bounds checks are solid.Big‑endian length, size caps, and zero-length rejection look correct.
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (4)
1854-1857: UV resolver now delegates to shared ServerInstaller.FindUvPath — good.This avoids duplication and drift in platform logic.
1136-1149: Trusting existing uv when validated is fine; keep the validate step.Good use of ValidateUvBinarySafe and fallback to shared resolver.
1214-1267: Atomic write flow looks robust.Replace/Move fallbacks and cleanup handling are correct; approval.
1874-1881: macOS path selection honors macConfigPath — good alignment with McpClients.Works as intended; no action needed.
| // Framed I/O only; legacy mode removed | ||
| try | ||
| { | ||
| var ep = client.Client?.RemoteEndPoint?.ToString() ?? "unknown"; | ||
| Debug.Log($"<b><color=#2EA3FF>UNITY-MCP</color></b>: Client connected {ep}"); | ||
| } | ||
| catch { } | ||
| // Strict framing: always require FRAMING=1 and frame all I/O | ||
| try | ||
| { | ||
| client.NoDelay = true; | ||
| } | ||
| catch { } | ||
| try | ||
| { | ||
| string handshake = "WELCOME UNITY-MCP 1 FRAMING=1\n"; | ||
| byte[] handshakeBytes = System.Text.Encoding.ASCII.GetBytes(handshake); | ||
| using var cts = new CancellationTokenSource(FrameIOTimeoutMs); | ||
| #if NETSTANDARD2_1 || NET6_0_OR_GREATER | ||
| await stream.WriteAsync(handshakeBytes.AsMemory(0, handshakeBytes.Length), cts.Token).ConfigureAwait(false); | ||
| #else | ||
| await stream.WriteAsync(handshakeBytes, 0, handshakeBytes.Length, cts.Token).ConfigureAwait(false); | ||
| #endif | ||
| Debug.Log("<b><color=#2EA3FF>UNITY-MCP</color></b>: Sent handshake FRAMING=1 (strict)"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Debug.LogWarning($"<b><color=#2EA3FF>UNITY-MCP</color></b>: Handshake failed: {ex.Message}"); | ||
| return; // abort this client | ||
| } | ||
|
|
There was a problem hiding this comment.
Strict FRAMING=1 handshake breaks unframed “ping” probes; update all callers.
This server now requires framed I/O only. Unity editor’s VerifyBridgePing (in MCPForUnityEditorWindow.cs) still sends unframed “ping” and will fail under this handshake.
I’ve provided a concrete fix in MCPForUnityEditorWindow.VerifyBridgePing to speak framed ping. See that file’s comment for the patch.
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/MCPForUnityBridge.cs around lines 403-433 the server
now requires FRAMING=1 and will reject unframed probes; update all callers (at
least MCPForUnityEditorWindow.VerifyBridgePing) to use framed I/O. Change
VerifyBridgePing to perform the same framing as the bridge: after connecting and
receiving the handshake, send a framed "ping" message (use the same frame
encoding/length-prefix used elsewhere in the codebase and ASCII encoding), await
and parse the framed response with the same read routine, respect the same
cancellation timeout, and log success/failure; update any other probe/test
callers likewise so no unframed messages are sent to the strict FRAMING=1
server.
…ditorPrefs/DeleteKey + logging via delayCall
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (3)
35-37: Avoid potential editor jank: split heavy I/O from Unity API callsServerInstaller.EnsureServerInstalled() performs substantial file I/O (copying, deletes, version reads). Running it on the main thread during startup can hitch the Editor. Consider refactoring ServerInstaller to expose a pure I/O method (no EditorPrefs/Debug.*) and run that on a background Task, then marshal only Unity API work (prefs/logs/UI) back via delayCall.
Example (requires adding a pure-I/O method in ServerInstaller):
- // Ensure any UnityEditor API usage inside runs on the main thread - ServerInstaller.EnsureServerInstalled(); + // Offload heavy I/O; method must avoid Unity APIs (pure I/O only) + System.Threading.Tasks.Task.Run(() => + { + System.Exception workerEx = null; + try { ServerInstaller.EnsureServerInstalledIOOnly(); } catch (System.Exception ex2) { workerEx = ex2; } + EditorApplication.delayCall += () => + { + if (workerEx != null) Debug.LogException(workerEx); + try { EditorPrefs.SetBool(key, true); } catch { } + try { EditorPrefs.DeleteKey("MCPForUnity.ServerSrc"); } catch { } + try { EditorPrefs.DeleteKey("MCPForUnity.PythonDirOverride"); } catch { } + }; + });Support change in ServerInstaller (outside this file):
// New: no UnityEditor or Debug.* calls inside public static void EnsureServerInstalledIOOnly() { // Extract the pure file-system and process work from EnsureServerInstalled() // (copy/update server, version file writes, legacy cleanup, etc.) }
31-33: Simplify error handling and emit proper exception logsKeep only the captured exception and log it with Debug.LogException for clickable stack traces.
Apply:
- string error = null; - System.Exception capturedEx = null; + System.Exception capturedEx = null; @@ - error = ex.Message; - capturedEx = ex; + capturedEx = ex; @@ - if (!string.IsNullOrEmpty(error)) + if (capturedEx != null) { - Debug.LogWarning($"MCP for Unity: Auto-detect on load failed: {capturedEx}"); - // Alternatively: Debug.LogException(capturedEx); + Debug.LogException(capturedEx); }Alternative (if you prefer central logging): replace the log with McpLog.Warn/Error for consistency with ServerInstaller.
Also applies to: 40-41, 50-54
88-105: Reuse centralized legacy root detection to avoid driftLegacyRootsExist duplicates paths that appear to be managed by ServerInstaller.GetLegacyRootsForDetection(). Prefer reusing a single source of truth to avoid path drift across platforms.
If GetLegacyRootsForDetection() is public (or can be made internal+friend), consider:
- string home = System.Environment.GetFolderPath(System.Environment.SpecialFolder.UserProfile) ?? string.Empty; - string[] roots = - { - System.IO.Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer", "src"), - System.IO.Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer", "src") - }; - foreach (var r in roots) + foreach (var root in ServerInstaller.GetLegacyRootsForDetection()) { - try { if (System.IO.File.Exists(System.IO.Path.Combine(r, "server.py"))) return true; } catch { } + try + { + var src = System.IO.Path.Combine(root, "src"); + if (System.IO.File.Exists(System.IO.Path.Combine(src, "server.py"))) return true; + } + catch { } }If it’s private, consider adding a small public helper returning the same list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
ServerInstaller(11-712)EnsureServerInstalled(21-109)
🔇 Additional comments (2)
UnityMcpBridge/Editor/Helpers/PackageDetector.cs (2)
28-29: Main-thread marshalling fix looks goodMoving the flow into EditorApplication.delayCall addresses the unsafe background-thread Unity API usage flagged earlier. This is the right direction.
44-49: Unity API cleanup on the main thread is correctKeeping EditorPrefs.* deletes and the version flag set on the main thread with try/catch guards is appropriate here.
…ify-before-edit, and recommend anchors/structured edits
…alidators) and recommended practices
…xt_edits and script_apply_edits
TL;DR
This bad boy does a lot of stuff. More than one PR should. But...it kept growing and I couldn't stop it. I've tested it a lot so it should be pretty solid. Anyway: it hardens transport with explicit framing + timeouts/retries, upgrades the editing surface for Claude (micro‑edits and resource read/search), and adds an end‑to‑end natural‑language (NL) test harness that drives a live Unity instance from CI and reports JUnit/Markdown artifacts suitable for Actions. (+6,962/−741 over 47 files.)
Why this matters
Message integrity & throughput: Stable framed IO eliminates boundary bugs, enforces sane limits, and improves latency under load.
Claude‑first editing: Tools expose precise text/structured edits and resource reading so IDE-less clients like Claude can edit more easily and surgically. Does read → plan → apply → verify with preconditions, reducing churn and syntax breakage.
Strong NL Testing Infrastructure to Build on: The NL suite spins up a Unity bridge, verifies handshake/framing, runs and validates editing exercises, and publishes normalized JUnit + readable Markdown for quick review.
Addresses Claude having issues editing scripts #183 Unity Editor crashing #238 manage_script tool always bug #157 Unity MCP fails with "Invalid JSON format" error when creating C# scripts larger than ~2000 bytes. #136
Highlights (compared to
main)1) Robust framing of messages (with testing)
FRAMING=1handshake; non‑blocking script writes; TCPNoDelay; timeouts and retries on reads/writes. Caps frames at 64 MiB and rejects zero‑length frames.test_unity_socket_framing.py.2) Claude‑friendly editing tools + resource reading
apply_text_edits(with precondition hashes and atomic multi‑edit batches) andscript_apply_editsfor structured C# edits;validate_scriptto catch syntax issues before writes.list_resources,read_resource, andfind_in_fileenable read‑first plans and targeted diffs.manage_scriptpaths stay for compatibility while read/update/edit flavors are de‑emphasized.3) New end‑to‑end NL testing CI (client prompts → Unity → back)
…manage_editor, …list_resources, …read_resource, …apply_text_edits, …script_apply_edits, …validate_script, …find_in_file+ minimal Bash).FRAMING=1preamble before tests proceed.reports/junit-for-actions.xml) and a Markdown summary; usesmikepenz/action-junit-report@v5.Tests
CI
.github/workflows/claude-nl-suite.ymlor mini version; honorsJUNIT_OUT/MD_OUT; normalizes toreports/junit-for-actions.xml; publishes via action‑junit‑report v5; adds summary to the workflow job page.How to verify
A) In CI (recommended)
Run "Claude NL/T Full Suite (Unity live)" or “Claude NL Test Suite (mini)” on this branch. (Requires an
ANTHROPIC_API_KEYin your GH secrets)Confirm the steps:
FRAMING=1and the suite proceeds.reports/junit-for-actions.xmlis published and annotated by the JUnit action.B) Locally (quick check)
test_unity_socket_framing.pyassertions for boundary and preamble checks.nl-unity-suite-full.md, throw them in Claude, or Claude Code, and watch them cook. (May have to tell it not to worry about JUNIT tests or resetting the target script).Backwards compatibility
manage_scriptremains available; the NL suite and docs steer tooling toward micro‑edits and resource‑first reads. No public API breaks are expected for existing automations.Risks
FRAMING=1and proper lengths. The NL suite explicitly verifies this path.Follow‑ups
Change map (at a glance)
Summary by CodeRabbit
New Features
CI / Workflows
Documentation
Tests
Chores