Fix: Unity Editor reload crash + debug-noise reduction #266
Conversation
…; gate ExecuteMenuItem logs behind debug flag
WalkthroughAdds cooperative cancellation and cancellation-aware framed async I/O to the MCP bridge, restructures listener/client handling and command routing, introduces a stress-testing CLI script (tools/stress_mcp.py) and duplicated README-DEV.md stress-test docs, enhances ManageGameObject error aggregation and tests, and replaces some Debug.Log calls with gated McpLog.Info. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Listener as TCP Listener
participant Bridge as MCPForUnityBridge
participant Queue as Command Queue
participant Handlers as Command Handlers
rect rgba(200,235,255,0.12)
Note over Bridge: Start() — create CancellationTokenSource, start ListenerLoopAsync(token), subscribe editor events
end
Client->>Listener: TCP connect
Listener-->>Client: "WELCOME UNITY-MCP 1 FRAMING=1"
loop Per-client interaction
Client->>Listener: Framed request (ping/command)
Listener->>Bridge: Accept & spawn HandleClientAsync(client, token)
Bridge->>Queue: Enqueue command
Queue->>Queue: Interlocked guard enter
Queue->>Handlers: ExecuteCommand(type, params)
Handlers-->>Queue: Result / Error
Queue-->>Client: Framed response
Queue->>Queue: Interlocked guard exit
end
rect rgba(255,230,200,0.12)
Note over Bridge: Shutdown / Reload
Bridge->>Bridge: Cancel CancellationTokenSource
Bridge->>Listener: Stop listener, await listenerTask briefly
Bridge->>Bridge: Unsubscribe editor events
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs (1)
30-31: Fix potential NRE and use culture-invariant lowercasing.
@params["action"]?.ToString().ToLower() ?? "execute"can NRE when"action"is missing because.ToLower()runs on a null string. Also preferToLowerInvariant().Apply:
- string action = @params["action"]?.ToString().ToLower() ?? "execute"; // Default action + string action = (@params["action"]?.ToString())?.ToLowerInvariant() ?? "execute"; // Default actionUnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
482-516: Fix cancellation not plumbed into reads; close active clients on Stop to prevent 30s linger during assembly reloads.Currently HandleClientAsync awaits ReadFrameAsUtf8Async without a CancellationToken, and ReadFrameAsUtf8Async/ReadExactAsync don’t get the bridge CTS propagated from Stop(). Accepted clients can linger up to FrameIOTimeoutMs (30s) across domain reloads, undermining the PR’s goal. Also, Stop() doesn’t forcibly close accepted client sockets.
Refactor to: (1) thread the token into ReadFrameAsUtf8Async → ReadExactAsync, (2) register/close TcpClients on Stop().
Apply these diffs:
- string commandText = await ReadFrameAsUtf8Async(stream, FrameIOTimeoutMs); + string commandText = await ReadFrameAsUtf8Async(stream, FrameIOTimeoutMs, token).ConfigureAwait(false);- private static async System.Threading.Tasks.Task<string> ReadFrameAsUtf8Async(NetworkStream stream, int timeoutMs) + private static async System.Threading.Tasks.Task<string> ReadFrameAsUtf8Async(NetworkStream stream, int timeoutMs, CancellationToken cancel) { - byte[] header = await ReadExactAsync(stream, 8, timeoutMs); + byte[] header = await ReadExactAsync(stream, 8, timeoutMs, cancel).ConfigureAwait(false); ulong payloadLen = ReadUInt64BigEndian(header); if (payloadLen > MaxFrameBytes) { throw new System.IO.IOException($"Invalid framed length: {payloadLen}"); } if (payloadLen == 0UL) throw new System.IO.IOException("Zero-length frames are not allowed"); if (payloadLen > int.MaxValue) { throw new System.IO.IOException("Frame too large for buffer"); } int count = (int)payloadLen; - byte[] payload = await ReadExactAsync(stream, count, timeoutMs); + byte[] payload = await ReadExactAsync(stream, count, timeoutMs, cancel).ConfigureAwait(false); return System.Text.Encoding.UTF8.GetString(payload); }Register/close clients:
private static readonly object lockObj = new(); private static readonly object startStopLock = new(); + private static readonly object clientsLock = new(); + private static readonly HashSet<TcpClient> activeClients = new();- using (client) - using (NetworkStream stream = client.GetStream()) - { + using (client) + using (NetworkStream stream = client.GetStream()) + { + lock (clientsLock) { activeClients.Add(client); } + try + { // Framed I/O only; legacy mode removed try { if (IsDebugEnabled()) { var ep = client.Client?.RemoteEndPoint?.ToString() ?? "unknown"; Debug.Log($"<b><color=#2EA3FF>UNITY-MCP</color></b>: Client connected {ep}"); } } catch { } // Strict framing: always require FRAMING=1 and frame all I/O try { client.NoDelay = true; } catch { } try { string handshake = "WELCOME UNITY-MCP 1 FRAMING=1\n"; byte[] handshakeBytes = System.Text.Encoding.ASCII.GetBytes(handshake); 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 if (IsDebugEnabled()) MCPForUnity.Editor.Helpers.McpLog.Info("Sent handshake FRAMING=1 (strict)", always: false); } catch (Exception ex) { if (IsDebugEnabled()) MCPForUnity.Editor.Helpers.McpLog.Warn($"Handshake failed: {ex.Message}"); return; // abort this client } @@ - } + } + finally + { + lock (clientsLock) { activeClients.Remove(client); } + } + }// Give the background loop a short window to exit without blocking the editor - if (toWait != null) + // Proactively close all active client sockets to unblock any pending reads + TcpClient[] toClose; + lock (clientsLock) + { + toClose = activeClients.ToArray(); + activeClients.Clear(); + } + foreach (var c in toClose) { try { c.Close(); } catch { } } + + if (toWait != null) { try { toWait.Wait(100); } catch { } }Also applies to: 540-585, 614-631, 350-395
494-516: Prevent main-thread inline continuations by using RunContinuationsAsynchronously and ConfigureAwait(false).Without RunContinuationsAsynchronously, SetResult in ProcessCommands can run the awaiting continuation inline on the Editor thread, risking sync I/O work on the main thread.
Apply this diff:
- TaskCompletionSource<string> tcs = new(); + var tcs = new TaskCompletionSource<string>(TaskCreationOptions.RunContinuationsAsynchronously); @@ - string response = await tcs.Task; + string response = await tcs.Task.ConfigureAwait(false);
198-207: Ensure isStarting is reset even if Start() throws.A thrown exception leaves isStarting = true, preventing future starts.
Apply this diff:
- isStarting = true; - // Attempt start; if it succeeds, remove the hook to avoid overhead - Start(); - isStarting = false; + isStarting = true; + try + { + // Attempt start; if it succeeds, remove the hook to avoid overhead + Start(); + } + finally + { + isStarting = false; + }
🧹 Nitpick comments (12)
UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs (2)
42-45: Unify logging via McpLog; gate the placeholder warning.
- Route logs through McpLog for consistent prefixing.
- Gate the “get_available_menus” placeholder under the debug toggle to avoid noise.
- Keep failure to execute as Warn; keep true errors as Error.
- Debug.LogWarning( - "[ExecuteMenuItem] 'get_available_menus' action is not fully implemented. Dynamically listing all menu items is complex." - ); + McpLog.Info( + "[ExecuteMenuItem] 'get_available_menus' is not fully implemented. Dynamically listing all menu items is complex.", + always: false + ); ... - Debug.LogWarning($"[ExecuteMenuItem] Failed (not found/disabled): '{menuPath}'"); + McpLog.Warn($"[ExecuteMenuItem] Failed (not found/disabled): '{menuPath}'"); ... - Debug.LogError($"[ExecuteMenuItem] Error executing '{menuPath}': {e}"); + McpLog.Error($"[ExecuteMenuItem] Error executing '{menuPath}': {e}");Also applies to: 113-113, 121-121
97-101: Optional: avoid interpolation cost when debug logs are disabled.Consider adding a lazy overload in McpLog to defer string formatting.
Outside this file (McpLog.cs), add:
public static void Info(Func<string> messageFactory, bool always = true) { if (!always && !IsDebugEnabled()) return; Debug.Log($"{Prefix} {messageFactory()}"); }Then here:
- McpLog.Info($"[ExecuteMenuItem] Request to execute menu: '{menuPath}'", always: false); + McpLog.Info(() => $"[ExecuteMenuItem] Request to execute menu: '{menuPath}'", always: false); ... - McpLog.Info($"[ExecuteMenuItem] Executed successfully: '{menuPath}'", always: false); + McpLog.Info(() => $"[ExecuteMenuItem] Executed successfully: '{menuPath}'", always: false);Also applies to: 106-107
README-DEV.md (1)
69-100: Stress-test docs — solid and actionable. Minor nits for clarity.
- Add a brief “Requirements” note (Python ≥ 3.10).
- Tighten a couple of list items with periods for consistency.
## MCP Bridge Stress Test @@ -An on-demand stress utility exercises the MCP bridge with multiple concurrent clients while triggering periodic asset refreshes and script reloads. +An on-demand stress utility exercises the MCP bridge with multiple concurrent clients while triggering periodic asset refreshes and script reloads. + +Requirements: Python 3.10+ and a Unity project with the MCP bridge active. @@ -Expected outcome: -- No Unity Editor crashes during reload churn -- Clients reconnect cleanly after reloads -- Script prints a JSON summary of request counts and disconnects +Expected outcome: +- No Unity Editor crashes during reload churn. +- Clients reconnect cleanly after reloads. +- Script prints a JSON summary of request counts and disconnects.tools/stress_mcp.py (4)
188-199: Add a manual port override.Discovery is great; a
--portoverride helps local/CI runs when discovery fails.@@ - ap = argparse.ArgumentParser(description="Stress test the Unity MCP bridge with concurrent clients and reload churn") + ap = argparse.ArgumentParser(description="Stress test the Unity MCP bridge with concurrent clients and reload churn") @@ ap.add_argument("--host", default="127.0.0.1") + ap.add_argument("--port", type=int, help="Override bridge port (skips discovery)") @@ - port = discover_port(args.project) + port = args.port if args.port else discover_port(args.project)
105-120: Prevent hangs and clean up sockets robustly.
- Wrap connect/handshake and reads with short timeouts to avoid hanging past --duration if the bridge stalls during reloads.
- Guard
writerinfinallyto avoidUnboundLocalErroron failed connects.@@ -async def client_loop(idx: int, host: str, port: int, stop_time: float, stats: dict): - reconnect_delay = 0.2 +async def client_loop(idx: int, host: str, port: int, stop_time: float, stats: dict): + reconnect_delay = 0.2 + conn_timeout = 3.0 + io_timeout = 5.0 while time.time() < stop_time: try: - reader, writer = await asyncio.open_connection(host, port) - await do_handshake(reader) + reader = writer = None + reader, writer = await asyncio.wait_for(asyncio.open_connection(host, port), timeout=conn_timeout) + await asyncio.wait_for(do_handshake(reader), timeout=io_timeout) # Send a quick ping first - await write_frame(writer, make_ping_frame()) - _ = await read_frame(reader) # ignore content + await write_frame(writer, make_ping_frame()) + _ = await asyncio.wait_for(read_frame(reader), timeout=io_timeout) # ignore content @@ - if r < 0.70: + if r < 0.70: # Ping await write_frame(writer, make_ping_frame()) - _ = await read_frame(reader) + _ = await asyncio.wait_for(read_frame(reader), timeout=io_timeout) stats["pings"] += 1 elif r < 0.90: # Lightweight menu execute: Assets/Refresh await write_frame(writer, make_execute_menu_item("Assets/Refresh")) - _ = await read_frame(reader) + _ = await asyncio.wait_for(read_frame(reader), timeout=io_timeout) stats["menus"] += 1 else: # Small manage_gameobject request (may legitimately error if target not found) await write_frame(writer, make_manage_gameobject_modify_dummy("__MCP_Stress_Object__")) - _ = await read_frame(reader) + _ = await asyncio.wait_for(read_frame(reader), timeout=io_timeout) stats["mods"] += 1 @@ - finally: - try: - writer.close() # type: ignore - await writer.wait_closed() # type: ignore - except Exception: - pass + finally: + try: + if writer is not None: + writer.close() + await writer.wait_closed() + except Exception: + passAlso applies to: 134-139, 146-151
171-179: Also timeout the churn task’s one-off RPC.Avoids blocking if the bridge handshake/response stalls mid-reload.
- try: - reader, writer = await asyncio.open_connection(host, port) - await do_handshake(reader) - await write_frame(writer, make_execute_menu_item("Assets/Refresh")) - _ = await read_frame(reader) + try: + reader, writer = await asyncio.wait_for(asyncio.open_connection(host, port), timeout=3.0) + await asyncio.wait_for(do_handshake(reader), timeout=5.0) + await write_frame(writer, make_execute_menu_item("Assets/Refresh")) + _ = await asyncio.wait_for(read_frame(reader), timeout=5.0) writer.close() await writer.wait_closed()
105-105: Silence linters or use the params.
idxandproject_pathare currently unused. Either prefix with_or use them (e.g., random seeding or logging).-async def client_loop(idx: int, host: str, port: int, stop_time: float, stats: dict): +async def client_loop(_idx: int, host: str, port: int, stop_time: float, stats: dict): @@ -async def reload_churn_task(project_path: str, stop_time: float, unity_file: str | None, host: str, port: int): +async def reload_churn_task(_project_path: str, stop_time: float, unity_file: str | None, host: str, port: int):Also applies to: 153-153
UnityMcpBridge/Editor/MCPForUnityBridge.cs (5)
669-674: Throttle heartbeat writes; 0.5s file I/O on the main thread is aggressive.Recommend ≥2.0s cadence (or move writes to a background task) to reduce Editor stalls on slow filesystems.
Apply this diff:
- nextHeartbeatAt = EditorApplication.timeSinceStartup + 0.5f; + nextHeartbeatAt = EditorApplication.timeSinceStartup + 2.0f;- nextHeartbeatAt = now + 0.5f; + nextHeartbeatAt = now + 2.0f;Also applies to: 336-340
664-783: Time-slice command processing to avoid long Editor frames under load.Processing the entire snapshot can monopolize the main thread. Add a small per-frame budget and cap the batch size.
Apply this diff:
- foreach (var item in work) + // Time/volume budget to prevent Editor stalls + double budgetDeadline = EditorApplication.timeSinceStartup + 0.010f; // ~10ms/frame + int processed = 0; + foreach (var item in work) { @@ - } + processed++; + if (processed >= 256 || EditorApplication.timeSinceStartup >= budgetDeadline) + { + break; + } + }
410-413: NetworkStream.ReceiveTimeout doesn’t affect async reads.This line is ineffective with ReadAsync; your ReadExactAsync already enforces timeouts. Remove to avoid confusion.
Apply this diff:
- // Set longer receive timeout to prevent quick disconnections - client.ReceiveTimeout = 60000; // 60 seconds + // Timeout is enforced by ReadExactAsync; ReceiveTimeout does not affect async reads.
398-405: Use Volatile.Read on isRunning checks (optional).Reads happen on background threads; while CTS and socket closure already unblock, using volatile improves intent/visibility.
Apply this diff:
- while (isRunning && !token.IsCancellationRequested) + while (System.Threading.Volatile.Read(ref isRunning) && !token.IsCancellationRequested) @@ - while (isRunning && !token.IsCancellationRequested) + while (System.Threading.Volatile.Read(ref isRunning) && !token.IsCancellationRequested)And optionally:
- private static bool isRunning = false; + private static volatile bool isRunning = false;Also applies to: 477-479
892-901: Avoid double ToString() calls and substring range ops in params summary.Minor allocation/CPU reduction; more robust for short strings.
Apply this diff:
- .Select(static p => - $"{p.Name}: {p.Value?.ToString()?[..Math.Min(20, p.Value?.ToString()?.Length ?? 0)]}" - ) + .Select(static p => { + var s = p.Value?.ToString() ?? string.Empty; + var len = Math.Min(20, s.Length); + return $"{p.Name}: {s.Substring(0, len)}"; + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
README-DEV.md(1 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(9 hunks)UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs(1 hunks)tools/stress_mcp.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs (1)
UnityMcpBridge/Editor/Helpers/McpLog.cs (2)
McpLog(6-30)Info(15-19)
tools/stress_mcp.py (1)
tests/test_script_tools.py (1)
run(146-157)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
UnityMcpBridge/Editor/Helpers/PortManager.cs (1)
IsDebugEnabled(19-23)
🪛 Ruff (0.12.2)
tools/stress_mcp.py
1-1: Shebang is present but file is not executable
(EXE001)
39-40: try-except-pass detected, consider logging the exception
(S110)
39-39: Do not catch blind exception: Exception
(BLE001)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
58-58: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Unused function argument: idx
(ARG001)
117-117: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
141-141: Do not catch blind exception: Exception
(BLE001)
149-150: try-except-pass detected, consider logging the exception
(S110)
149-149: Do not catch blind exception: Exception
(BLE001)
153-153: Unused function argument: project_path
(ARG001)
179-180: try-except-pass detected, consider logging the exception
(S110)
179-179: Do not catch blind exception: Exception
(BLE001)
182-183: try-except-pass detected, consider logging the exception
(S110)
182-182: Do not catch blind exception: Exception
(BLE001)
🪛 LanguageTool
README-DEV.md
[grammar] ~77-~77: There might be a mistake here.
Context: ...~/.unity-mcp/unity-mcp-status-*.json). - Sends a mix of framed ping, `execute_m...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...s to run (default 60) Expected outcome: - No Unity Editor crashes during reload ch...
(QB_NEW_EN)
🔇 Additional comments (4)
UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs (1)
99-101: Gated debug logs — LGTM.Switching to McpLog.Info(..., always: false) reduces console noise while keeping traceability.
Also applies to: 106-107
UnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
254-318: Nice socket hardening.ReuseAddress, ExclusiveAddressUse false on Windows, and Linger(RST) minimize TIME_WAIT—good call for rapid reload churn.
909-924: Assembly reload flow looks safe.beforeAssemblyReload → Stop() with minimal work; afterAssemblyReload writes lightweight heartbeat and schedules deferred init. Aligns with PR goals.
330-336: Defensive event (re)subscription is solid.Unsubscribe-then-subscribe pattern avoids duplicates across Start/Stop cycles.
…l (immediate refresh, precondition SHA, EOF edits); revert to manage_script.read for compatibility
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
tools/stress_mcp.py (8)
1-1: Shebang vs executabilityEither make the file executable in git or drop the shebang to avoid EXE001 noise.
Apply one of:
-#!/usr/bin/env python3or ensure
chmod +x tools/stress_mcp.pyin the repo.
19-39: Make port discovery robust across OS and status schema variantsNormalize paths before comparing and fall back to common key names (
port,bridge_port) to improve hit rate on Windows and mixed environments.Apply:
def discover_port(project_path: str | None) -> int: # Default bridge port if nothing found default_port = 6400 files = find_status_files() + def _norm(p: str | os.PathLike | None) -> str: + if not p: + return "" + return os.path.normcase(os.path.abspath(str(p))).replace("\\", "/") + project_norm = _norm(project_path) for f in files: try: - data = json.loads(f.read_text()) - port = int(data.get("unity_port", 0) or 0) - proj = data.get("project_path") or "" + data = json.loads(f.read_text()) + port = int(data.get("unity_port") or data.get("port") or data.get("bridge_port") or 0) + proj = data.get("project_path") or "" + proj_norm = _norm(proj) if project_path: # Match status for the given project if possible - if proj and project_path in proj: - if 0 < port < 65536: - return port + if proj_norm and project_norm and (proj_norm.startswith(project_norm) or project_norm in proj_norm): + if 0 < port < 65536: + return port else: if 0 < port < 65536: return port except Exception: pass return default_port
41-49: Use readexactly for simpler, faster fixed-length readsLet asyncio handle partial reads and EOF semantics.
async def read_exact(reader: asyncio.StreamReader, n: int) -> bytes: - buf = b"" - while len(buf) < n: - chunk = await reader.read(n - len(buf)) - if not chunk: - raise ConnectionError("Connection closed while reading") - buf += chunk - return buf + return await reader.readexactly(n)
66-71: Bound handshake wait to avoid hangsGuard the initial
readline()with a timeout; surface a clear error.async def do_handshake(reader: asyncio.StreamReader) -> None: # Server sends a single line handshake: "WELCOME UNITY-MCP 1 FRAMING=1\n" - line = await reader.readline() - if not line or b"WELCOME UNITY-MCP" not in line: - raise ConnectionError(f"Unexpected handshake from server: {line!r}") + try: + line = await asyncio.wait_for(reader.readline(), timeout=5.0) + except asyncio.TimeoutError: + raise ConnectionError("Handshake timeout") # noqa: TRY003 + if not line or b"WELCOME UNITY-MCP" not in line: + raise ConnectionError(f"Unexpected handshake from server: {line!r}") # noqa: TRY003
83-116: Tighten connection cleanup and cancellationInitialize
writerto avoid NameError and propagate task cancellation cleanly.async def client_loop(idx: int, host: str, port: int, stop_time: float, stats: dict): reconnect_delay = 0.2 while time.time() < stop_time: try: - reader, writer = await asyncio.open_connection(host, port) + writer: asyncio.StreamWriter | None = None + reader, writer = await asyncio.open_connection(host, port) await do_handshake(reader) # Send a quick ping first await write_frame(writer, make_ping_frame()) _ = await read_frame(reader) # ignore content @@ - except (ConnectionError, OSError, asyncio.IncompleteReadError): + except (ConnectionError, OSError, asyncio.IncompleteReadError): stats["disconnects"] += 1 await asyncio.sleep(reconnect_delay) reconnect_delay = min(reconnect_delay * 1.5, 2.0) continue - except Exception: + except asyncio.CancelledError: + raise + except Exception: stats["errors"] += 1 await asyncio.sleep(0.2) continue finally: - try: - writer.close() # type: ignore - await writer.wait_closed() # type: ignore - except Exception: - pass + if writer is not None: + try: + writer.close() + await writer.wait_closed() + except Exception: + pass
225-228: Account for swallowed outer exceptionsIncrement a counter so silent exceptions are visible in the summary.
- except Exception: - pass + except Exception: + stats["errors"] = stats.get("errors", 0) + 1
230-241: Add explicit --port overrideUseful when status discovery is unavailable or multiple bridges run.
ap = argparse.ArgumentParser(description="Stress test the Unity MCP bridge with concurrent clients and reload churn") ap.add_argument("--host", default="127.0.0.1") ap.add_argument("--project", default=str(Path(__file__).resolve().parents[1] / "TestProjects" / "UnityMCPTests")) ap.add_argument("--unity-file", default=str(Path(__file__).resolve().parents[1] / "TestProjects" / "UnityMCPTests" / "Assets" / "Scripts" / "LongUnityScriptClaudeTest.cs")) ap.add_argument("--clients", type=int, default=10) ap.add_argument("--duration", type=int, default=60) + ap.add_argument("--port", type=int, help="Bridge TCP port (skips auto-discovery if set)") args = ap.parse_args() - port = discover_port(args.project) + port = args.port or discover_port(args.project) stop_time = time.time() + max(10, args.duration)
242-244: Prune unused counters and pre-init apply tallies
menus/modsaren’t updated; initializeapplies/apply_errorsfor consistent output.- stats = {"pings": 0, "menus": 0, "mods": 0, "disconnects": 0, "errors": 0} + stats = {"pings": 0, "applies": 0, "apply_errors": 0, "disconnects": 0, "errors": 0}README-DEV.md (3)
78-79: Minor wording: “keepalives” → “keep‑alives”Standard hyphenation improves readability.
-- Sends lightweight framed `ping` keepalives to maintain concurrency. +- Sends lightweight framed `ping` keep-alives to maintain concurrency.
93-98: Document the optional --port flag (if adopted)Add to Flags to mirror the CLI change.
Flags: - `--project` Unity project path (auto-detected to the included test project by default) - `--unity-file` C# file to edit (defaults to the long test script) - `--clients` number of concurrent clients (default 10) - `--duration` seconds to run (default 60) + - `--port` override the TCP port (skips auto-discovery)
106-117: Add Windows path noteCall out that the tool normalizes paths but the target file must be under “Assets/”.
### Notes and troubleshooting - Immediate vs debounced: @@ - Transient failures: - Occasional `apply_errors` often indicate the connection reloaded mid-reply. Edits still typically apply; the loop continues on the next iteration. + - Windows paths: + - The tool normalizes backslashes, but the target script must reside under `Assets/` within the Unity project for edits to apply.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README-DEV.md(1 hunks)tools/stress_mcp.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T01:01:11.889Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.889Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
README-DEV.md
🪛 LanguageTool
README-DEV.md
[grammar] ~77-~77: There might be a mistake here.
Context: ...~/.unity-mcp/unity-mcp-status-*.json). - Sends lightweight framed ping keepaliv...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...he current file contents to avoid drift. - Uses EOF insertion to avoid header/`usin...
(QB_NEW_EN)
[grammar] ~107-~107: There might be a mistake here.
Context: ...roubleshooting - Immediate vs debounced: - The tool sets `options.refresh = "immedi...
(QB_NEW_EN)
[grammar] ~119-~119: There might be a mistake here.
Context: ...or requirements and runtime variability. - Optionally run it as a manual workflow o...
(QB_NEW_EN)
🪛 Ruff (0.12.2)
tools/stress_mcp.py
1-1: Shebang is present but file is not executable
(EXE001)
36-37: try-except-pass detected, consider logging the exception
(S110)
36-36: Do not catch blind exception: Exception
(BLE001)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Unused function argument: idx
(ARG001)
106-106: Do not catch blind exception: Exception
(BLE001)
114-115: try-except-pass detected, consider logging the exception
(S110)
114-114: Do not catch blind exception: Exception
(BLE001)
118-118: Unused function argument: project_path
(ARG001)
133-134: try-except-pass detected, consider logging the exception
(S110)
133-133: Do not catch blind exception: Exception
(BLE001)
166-166: Do not catch blind exception: Exception
(BLE001)
218-218: Do not catch blind exception: Exception
(BLE001)
222-222: Do not catch blind exception: Exception
(BLE001)
225-226: try-except-pass detected, consider logging the exception
(S110)
225-225: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
tools/stress_mcp.py (2)
11-17: LGTM: simple, deterministic status-file discoverySorted-most-recent-first is appropriate here.
171-183: No change needed; indexing aligns with server spec
The client usesend_line = len(lines) + 1for a 1-based exclusive EOF range, matchingapply_text_edits’ documented 1-indexed coordinates.README-DEV.md (1)
69-121: LGTM: clear, actionable stress-test documentationGood coverage of purpose, usage, expected outcomes, and CI guidance.
…ManageGameObject; test hardening
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (1)
817-841: Flatten aggregation; avoid silent catch, dedupe, and freeze output.The approach is fine, but:
- Catch-all without logging can hide serialization/shape issues.
- Prefer typed checks over ToString() to avoid accidental non-string entries.
- Return an array (immutable) and dedupe to keep payload tidy.
Apply this diff:
- // Aggregate flattened error strings to make tests/API assertions simpler - var aggregatedErrors = new System.Collections.Generic.List<string>(); - foreach (var errorObj in componentErrors) - { - try - { - var dataProp = errorObj?.GetType().GetProperty("data"); - var dataVal = dataProp?.GetValue(errorObj); - if (dataVal != null) - { - var errorsProp = dataVal.GetType().GetProperty("errors"); - var errorsEnum = errorsProp?.GetValue(dataVal) as System.Collections.IEnumerable; - if (errorsEnum != null) - { - foreach (var item in errorsEnum) - { - var s = item?.ToString(); - if (!string.IsNullOrEmpty(s)) aggregatedErrors.Add(s); - } - } - } - } - catch { } - } + // Aggregate flattened error strings to make tests/API assertions simpler + var aggregatedErrors = new List<string>(); + foreach (var errorObj in componentErrors) + { + try + { + var data = errorObj?.GetType().GetProperty("data")?.GetValue(errorObj); + var errsObj = data?.GetType().GetProperty("errors")?.GetValue(data); + if (errsObj is IEnumerable<string> errs) + { + foreach (var s in errs) if (!string.IsNullOrEmpty(s)) aggregatedErrors.Add(s); + } + else if (errsObj is System.Collections.IEnumerable any) + { + foreach (var item in any) + { + if (item is string s && !string.IsNullOrEmpty(s)) aggregatedErrors.Add(s); + } + } + } + catch (Exception) + { + // Intentionally ignore to avoid noisy logs during stress runs. + // Consider logging behind a debug toggle if needed. + } + } + var errorsOut = aggregatedErrors.Count > 0 ? aggregatedErrors.Distinct().ToArray() : Array.Empty<string>(); return Response.Error( $"One or more component property operations failed on '{targetGo.name}'.", - new { componentErrors = componentErrors, errors = aggregatedErrors } + new { componentErrors, errors = errorsOut } );Also applies to: 844-845
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (2)
265-289: Make assertions simpler and more robust via JObject inspection.This avoids reflection boilerplate and keeps tests resilient to anonymous type implementations.
Apply this diff:
- // Harden: verify structured error response with failures list contains both invalid fields - var successProp = result.GetType().GetProperty("success"); - Assert.IsNotNull(successProp, "Result should expose 'success' property"); - Assert.IsFalse((bool)successProp.GetValue(result), "Result.success should be false for partial failure"); - - var dataProp = result.GetType().GetProperty("data"); - Assert.IsNotNull(dataProp, "Result should include 'data' with errors"); - var dataVal = dataProp.GetValue(result); - Assert.IsNotNull(dataVal, "Result.data should not be null"); - var errorsProp = dataVal.GetType().GetProperty("errors"); - Assert.IsNotNull(errorsProp, "Result.data should include 'errors' list"); - var errorsEnum = errorsProp.GetValue(dataVal) as System.Collections.IEnumerable; - Assert.IsNotNull(errorsEnum, "errors should be enumerable"); - - bool foundRotatoin = false; - bool foundInvalidProp = false; - foreach (var err in errorsEnum) - { - string s = err?.ToString() ?? string.Empty; - if (s.Contains("rotatoin")) foundRotatoin = true; - if (s.Contains("invalidProp")) foundInvalidProp = true; - } - Assert.IsTrue(foundRotatoin, "errors should mention the misspelled 'rotatoin' property"); - Assert.IsTrue(foundInvalidProp, "errors should mention the 'invalidProp' property"); + // Harden: verify structured error response with failures list contains both invalid fields + var jobj = JObject.FromObject(result); + Assert.IsFalse(jobj["success"]?.Value<bool>() ?? true, "Result.success should be false for partial failure"); + var errorsArr = jobj["data"]?["errors"] as JArray; + Assert.IsNotNull(errorsArr, "Result.data should include 'errors' list"); + bool foundRotatoin = false, foundInvalidProp = false; + foreach (var err in errorsArr) + { + var s = err?.ToString() ?? string.Empty; + if (s.Contains("rotatoin")) foundRotatoin = true; + if (s.Contains("invalidProp")) foundInvalidProp = true; + } + Assert.IsTrue(foundRotatoin, "errors should mention the misspelled 'rotatoin' property"); + Assert.IsTrue(foundInvalidProp, "errors should mention the 'invalidProp' property");
336-357: Mirror the same JObject-based checks for the exception path.Keeps test style consistent and concise.
Apply this diff:
- // Harden: verify structured error response contains velocity failure - var successProp2 = result.GetType().GetProperty("success"); - Assert.IsNotNull(successProp2, "Result should expose 'success' property"); - Assert.IsFalse((bool)successProp2.GetValue(result), "Result.success should be false when an exception occurs for a property"); - - var dataProp2 = result.GetType().GetProperty("data"); - Assert.IsNotNull(dataProp2, "Result should include 'data' with errors"); - var dataVal2 = dataProp2.GetValue(result); - Assert.IsNotNull(dataVal2, "Result.data should not be null"); - var errorsProp2 = dataVal2.GetType().GetProperty("errors"); - Assert.IsNotNull(errorsProp2, "Result.data should include 'errors' list"); - var errorsEnum2 = errorsProp2.GetValue(dataVal2) as System.Collections.IEnumerable; - Assert.IsNotNull(errorsEnum2, "errors should be enumerable"); - - bool foundVelocityError = false; - foreach (var err in errorsEnum2) - { - string s = err?.ToString() ?? string.Empty; - if (s.Contains("velocity")) { foundVelocityError = true; break; } - } - Assert.IsTrue(foundVelocityError, "errors should include a message referencing 'velocity'"); + // Harden: verify structured error response contains velocity failure + var jobj2 = JObject.FromObject(result); + Assert.IsFalse(jobj2["success"]?.Value<bool>() ?? true, "Result.success should be false when an exception occurs"); + var errorsArr2 = jobj2["data"]?["errors"] as JArray; + Assert.IsNotNull(errorsArr2, "Result.data should include 'errors' list"); + bool foundVelocityError = false; + foreach (var err in errorsArr2) + { + var s = err?.ToString() ?? string.Empty; + if (s.Contains("velocity")) { foundVelocityError = true; break; } + } + Assert.IsTrue(foundVelocityError, "errors should include a message referencing 'velocity'");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs(2 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(15 hunks)UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs(2 hunks)UnityMcpBridge/Editor/Tools/ManageGameObject.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- UnityMcpBridge/Editor/MCPForUnityBridge.cs
- UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Error(41-60)
… with project-root fallback
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tools/stress_mcp.py (1)
118-141: Windows ‘Assets/…’ derivation fix: good catch, now robust across separators.This resolves the earlier backslash bug by using Path.parts and as_posix.
🧹 Nitpick comments (6)
tools/stress_mcp.py (6)
1-1: Make the script executable or drop the shebang.Ruff flagged EXE001. Either mark the file as executable or remove the shebang to silence the warning.
You can mark it executable in git:
# run in shell, not a code change git update-index --chmod=+x tools/stress_mcp.py
19-38: Normalize project-path matching for robustness across OSes.Substring matching (
project_path in proj) can mis-match and is case-sensitive on Windows. Resolve both sides and compare canonical strings (or equality) first.Apply:
def discover_port(project_path: str | None) -> int: # Default bridge port if nothing found default_port = 6400 + pp = Path(project_path).resolve() if project_path else None files = find_status_files() for f in files: try: data = json.loads(f.read_text()) port = int(data.get("unity_port", 0) or 0) proj = data.get("project_path") or "" if project_path: # Match status for the given project if possible - if proj and project_path in proj: - if 0 < port < 65536: - return port + if proj and pp: + try: + pj = Path(proj) + if not pj.is_absolute(): + pj = pj.resolve() + except Exception: + pj = Path(proj) + if str(pj) == str(pp) or str(pp) in str(pj): + if 0 < port < 65536: + return port else: if 0 < port < 65536: return port except Exception: pass return default_port
146-148: Prefer Path.as_posix() for dir path normalization.Slightly cleaner than manual replace.
Apply:
- name_base = Path(relative).stem - dir_path = str(Path(relative).parent).replace('\\', '/') + name_base = Path(relative).stem + dir_path = Path(relative).parent.as_posix()
233-235: Stop swallowing all exceptions silently; gate minimal diagnostics behind a debug flag.Replace bare
except Exception: passwith a debug log to aid triage without spamming CI.Apply:
- except Exception: - pass + except Exception as e: + dlog(f"[churn] loop error: {e!r}")Repeat similarly for other broad catches if feasible (Ruff BLE001/S110).
238-245: Add CLI flags to control debug and timeouts; plumb into globals.This lets you tune behavior without editing code.
Apply:
async def main(): ap = argparse.ArgumentParser(description="Stress test the Unity MCP bridge with concurrent clients and reload churn") ap.add_argument("--host", default="127.0.0.1") ap.add_argument("--project", default=str(Path(__file__).resolve().parents[1] / "TestProjects" / "UnityMCPTests")) ap.add_argument("--unity-file", default=str(Path(__file__).resolve().parents[1] / "TestProjects" / "UnityMCPTests" / "Assets" / "Scripts" / "LongUnityScriptClaudeTest.cs")) ap.add_argument("--clients", type=int, default=10) ap.add_argument("--duration", type=int, default=60) + ap.add_argument("--timeout", type=float, default=2.0) + ap.add_argument("--debug", action="store_true") args = ap.parse_args() - port = discover_port(args.project) + # Configure globals + global TIMEOUT, DEBUG + TIMEOUT = args.timeout + DEBUG = DEBUG or args.debug + + port = discover_port(args.project) stop_time = time.time() + max(10, args.duration) @@ - print(json.dumps({"port": port, "stats": stats}, indent=2)) + print(json.dumps({"port": port, "stats": stats}, indent=2))Also applies to: 247-249, 261-261
150-177: Reduce repetition: factor a tiny request helper (connect → handshake → send → recv → close).This will DRY up the read/apply blocks and centralize timeout/cleanup.
Add this helper near other I/O helpers:
async def send_request(host: str, port: int, payload: dict) -> bytes: reader, writer = await asyncio.wait_for(asyncio.open_connection(host, port), timeout=TIMEOUT) try: await asyncio.wait_for(do_handshake(reader), timeout=TIMEOUT) await write_frame(writer, json.dumps(payload).encode("utf-8")) return await asyncio.wait_for(read_frame(reader), timeout=TIMEOUT) finally: writer.close() await writer.wait_closed()Then replace the duplicated blocks with
resp = await send_request(host, port, read_payload)andresp = await send_request(host, port, apply_payload).Also applies to: 213-232
📜 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)
tools/stress_mcp.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
tools/stress_mcp.py
1-1: Shebang is present but file is not executable
(EXE001)
36-37: try-except-pass detected, consider logging the exception
(S110)
36-36: Do not catch blind exception: Exception
(BLE001)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Unused function argument: idx
(ARG001)
106-106: Do not catch blind exception: Exception
(BLE001)
114-115: try-except-pass detected, consider logging the exception
(S110)
114-114: Do not catch blind exception: Exception
(BLE001)
141-141: Do not catch blind exception: Exception
(BLE001)
174-174: Do not catch blind exception: Exception
(BLE001)
226-226: Do not catch blind exception: Exception
(BLE001)
230-230: Do not catch blind exception: Exception
(BLE001)
233-234: try-except-pass detected, consider logging the exception
(S110)
233-233: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
tools/stress_mcp.py (3)
11-16: Status-file scan logic looks good.Sorted newest-first and graceful empty-dir handling are solid.
73-75: Ping payload shape: confirm expected echo vs. JSON.If the server expects JSON frames for everything,
b"ping"might be logged as noise. If it accepts raw “ping”, all good. Just double-check against the bridge.
179-186: No changes needed—ManageScript uses 1-based indexing and accepts (len(lines)+1, 1) as EOF. TryIndexFromLineCol maps 1-based line/col to the end-of-file correctly.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tools/stress_mcp.py (1)
94-118: Nice: timeouts, jitter, and safe writer cleanup are in place.
The previous feedback about enforcing per-IO timeouts, desynchronizing clients with jitter, and guarding writer close has been addressed correctly.Also applies to: 187-206, 267-271
🧹 Nitpick comments (6)
tools/stress_mcp.py (6)
22-28: Harden status file discovery against races on stat().
Sorting directly by p.stat() can explode if a file is deleted between glob and stat. Build a safe list first.Apply:
def find_status_files() -> list[Path]: home = Path.home() status_dir = Path(os.environ.get("UNITY_MCP_STATUS_DIR", home / ".unity-mcp")) if not status_dir.exists(): return [] - return sorted(status_dir.glob("unity-mcp-status-*.json"), key=lambda p: p.stat().st_mtime, reverse=True) + files_with_mtime: list[tuple[float, Path]] = [] + for p in status_dir.glob("unity-mcp-status-*.json"): + try: + files_with_mtime.append((p.stat().st_mtime, p)) + except OSError: + continue + return [p for _, p in sorted(files_with_mtime, key=lambda t: t[0], reverse=True)]
52-68: Consider centralizing read timeouts in I/O primitives.
You already wrap read_frame/handshake at call sites. As an optional cleanup, add atimeout: float | None = TIMEOUTparam toread_exact/read_frameand internally useasyncio.wait_forto reduce duplication and prevent future call sites from forgetting timeouts.
77-82: Validate handshake parameters (FRAMING=1) and decode once.
Stricter validation aids early protocol mismatch detection.Apply:
-async def do_handshake(reader: asyncio.StreamReader) -> None: - # Server sends a single line handshake: "WELCOME UNITY-MCP 1 FRAMING=1\n" - line = await reader.readline() - if not line or b"WELCOME UNITY-MCP" not in line: - raise ConnectionError(f"Unexpected handshake from server: {line!r}") +async def do_handshake(reader: asyncio.StreamReader) -> None: + # Server sends: "WELCOME UNITY-MCP 1 FRAMING=1\n" + raw = await reader.readline() + txt = raw.decode("utf-8", "ignore").strip() + if not txt.startswith("WELCOME UNITY-MCP"): + raise ConnectionError(f"Unexpected handshake from server: {txt!r}") + if "FRAMING=1" not in txt: + raise ConnectionError(f"Server does not advertise FRAMING=1: {txt!r}")
217-222: Simplify writer cleanup guard.
writeris always defined in these scopes; thelocals()check is redundant.Apply:
- if 'writer' in locals() and writer is not None: + if writer is not None: try: writer.close() await writer.wait_closed() except Exception: passAlso applies to: 286-291
295-296: Don’t silently swallow outer-loop exceptions; log under DEBUG.
Helps diagnose rare churn errors without noisy consoles by default.Apply:
- except Exception: - pass + except Exception as e: + dlog(f"[reload_churn_task] outer loop error: {e!r}")
313-314: Stabilize stats schema and drop unused keys.
Initialize fields you later use (applies,apply_errors) and remove unused (menus,mods) to keep output predictable.Apply:
- stats = {"pings": 0, "menus": 0, "mods": 0, "disconnects": 0, "errors": 0} + stats = {"pings": 0, "applies": 0, "apply_errors": 0, "disconnects": 0, "errors": 0}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/stress_mcp.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tools/stress_mcp.py (1)
tests/test_script_tools.py (1)
run(146-157)
🪛 Ruff (0.12.2)
tools/stress_mcp.py
1-1: Shebang is present but file is not executable
(EXE001)
47-48: try-except-pass detected, consider logging the exception
(S110)
47-47: Do not catch blind exception: Exception
(BLE001)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
81-81: Avoid specifying long messages outside the exception class
(TRY003)
113-113: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
121-121: Do not catch blind exception: Exception
(BLE001)
131-132: try-except-pass detected, consider logging the exception
(S110)
131-131: Do not catch blind exception: Exception
(BLE001)
146-146: Do not catch blind exception: Exception
(BLE001)
179-179: Do not catch blind exception: Exception
(BLE001)
213-213: Do not catch blind exception: Exception
(BLE001)
215-215: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
221-222: try-except-pass detected, consider logging the exception
(S110)
221-221: Do not catch blind exception: Exception
(BLE001)
279-281: try-except-pass detected, consider logging the exception
(S110)
279-279: Do not catch blind exception: Exception
(BLE001)
282-282: Do not catch blind exception: Exception
(BLE001)
284-284: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
290-291: try-except-pass detected, consider logging the exception
(S110)
290-290: Do not catch blind exception: Exception
(BLE001)
295-296: try-except-pass detected, consider logging the exception
(S110)
295-295: Do not catch blind exception: Exception
(BLE001)
| @@ -0,0 +1,333 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Shebang is fine; ensure script is executable or drop it.
Either set the executable bit in git (preferred) or remove the shebang and invoke via python -m.
Run to check and fix:
🏁 Script executed:
#!/bin/bash
# Check exec bit
if [ -x tools/stress_mcp.py ]; then
echo "OK: tools/stress_mcp.py is executable"
else
echo "Set executable bit (git-tracked):"
echo " git update-index --chmod=+x tools/stress_mcp.py && git commit -m 'chore: make stress_mcp executable'"
fiLength of output: 330
Make tools/stress_mcp.py executable
tools/stress_mcp.py currently isn’t marked executable. Run:
git update-index --chmod=+x tools/stress_mcp.py && git commit -m "chore: make stress_mcp executable"
🧰 Tools
🪛 Ruff (0.12.2)
1-1: Shebang is present but file is not executable
(EXE001)
🤖 Prompt for AI Agents
In tools/stress_mcp.py around lines 1 to 1, the script file is not marked
executable; update the git index to set the executable bit and commit that
change. Run git update-index --chmod=+x tools/stress_mcp.py to mark it
executable and then git commit -m "chore: make stress_mcp executable" (or stage
and commit via your usual workflow) so the file is executable in the repository.
| def discover_port(project_path: str | None) -> int: | ||
| # Default bridge port if nothing found | ||
| default_port = 6400 | ||
| files = find_status_files() | ||
| for f in files: | ||
| try: | ||
| data = json.loads(f.read_text()) | ||
| port = int(data.get("unity_port", 0) or 0) | ||
| proj = data.get("project_path") or "" | ||
| if project_path: | ||
| # Match status for the given project if possible | ||
| if proj and project_path in proj: | ||
| if 0 < port < 65536: | ||
| return port | ||
| else: | ||
| if 0 < port < 65536: | ||
| return port | ||
| except Exception: | ||
| pass | ||
| return default_port | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make port discovery more robust (alt keys, normalized project match, debug on failures).
Supports older/newer status schemas and avoids substring false-positives.
Apply:
def discover_port(project_path: str | None) -> int:
# Default bridge port if nothing found
default_port = 6400
- files = find_status_files()
- for f in files:
- try:
- data = json.loads(f.read_text())
- port = int(data.get("unity_port", 0) or 0)
- proj = data.get("project_path") or ""
- if project_path:
- # Match status for the given project if possible
- if proj and project_path in proj:
- if 0 < port < 65536:
- return port
- else:
- if 0 < port < 65536:
- return port
- except Exception:
- pass
+ files = find_status_files()
+ proj_norm = Path(project_path).resolve() if project_path else None
+ for f in files:
+ try:
+ data = json.loads(f.read_text())
+ # Accept multiple key spellings
+ port_val = data.get("unity_port") or data.get("port") or data.get("unityPort") or 0
+ port = int(port_val) if port_val else 0
+ proj_raw = (data.get("project_path") or data.get("projectPath") or "").strip()
+ if not (0 < port < 65536):
+ continue
+ if not proj_norm:
+ return port
+ try:
+ proj_path = Path(proj_raw).resolve() if proj_raw else None
+ except Exception:
+ proj_path = None
+ # Prefer exact normalized match; fall back to string contains as last resort
+ if proj_path and proj_path == proj_norm:
+ return port
+ if proj_raw and str(proj_norm) in proj_raw:
+ return port
+ except Exception as e:
+ dlog(f"[discover_port] ignoring status file {f}: {e!r}")
return default_portTo ensure key names align with the bridge, scan the repo for where status files are written:
🏁 Script executed:
#!/bin/bash
rg -nC2 -e 'unity-mcp-status' -e 'project_path' -e 'unity_port' -e 'projectPath' -e 'unityPort' --type-add 'cs:*.cs' --type csLength of output: 8419
Normalize project paths and add debug logging
- Use pathlib.Path.resolve() on both the status file’s project_path and the input project_path for exact matching instead of plain substring checks.
- Replace the bare except: pass with a call to dlog (or similar) to report any failures reading or parsing status files.
🧰 Tools
🪛 Ruff (0.12.2)
47-48: try-except-pass detected, consider logging the exception
(S110)
47-47: Do not catch blind exception: Exception
(BLE001)
| while time.time() < stop_time: | ||
| try: | ||
| if path and path.exists(): | ||
| # Determine files to touch this cycle | ||
| targets: list[Path] | ||
| if storm_count and storm_count > 1 and candidates: | ||
| k = min(max(1, storm_count), len(candidates)) | ||
| targets = random.sample(candidates, k) | ||
| else: | ||
| targets = [path] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable churn when unity_file is omitted (use candidate pool).
Currently, no edits occur without --unity-file despite candidates being built. Use the candidates list as the source of truth.
Apply:
- if path and path.exists():
+ if candidates:
# Determine files to touch this cycle
targets: list[Path]
- if storm_count and storm_count > 1 and candidates:
+ if storm_count and storm_count > 1 and candidates:
k = min(max(1, storm_count), len(candidates))
targets = random.sample(candidates, k)
else:
- targets = [path]
+ targets = [path if path else random.choice(candidates)]📝 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.
| while time.time() < stop_time: | |
| try: | |
| if path and path.exists(): | |
| # Determine files to touch this cycle | |
| targets: list[Path] | |
| if storm_count and storm_count > 1 and candidates: | |
| k = min(max(1, storm_count), len(candidates)) | |
| targets = random.sample(candidates, k) | |
| else: | |
| targets = [path] | |
| while time.time() < stop_time: | |
| try: | |
| if candidates: | |
| # Determine files to touch this cycle | |
| targets: list[Path] | |
| if storm_count and storm_count > 1 and candidates: | |
| k = min(max(1, storm_count), len(candidates)) | |
| targets = random.sample(candidates, k) | |
| else: | |
| targets = [path if path else random.choice(candidates)] |
🤖 Prompt for AI Agents
In tools/stress_mcp.py around lines 152 to 161, the loop only selects targets
when path exists so churn is disabled if --unity-file is omitted even though
candidates were built; change the target-selection logic so that if a valid path
exists use it, otherwise fall back to using the candidates pool (respecting
storm_count and bounds) — i.e., set targets from random.sample(candidates, k) or
candidates[0]/[...], and ensure targets is always defined when candidates is
non-empty so touches/edits run even without a unity_file.
…emetry * commit '3e83f993bfe632034bf7302d4319e3cd16353eb8': Improved ci prompt testing suite (CoplayDev#270) chore: bump version to 3.3.2 Fix: Unity Editor reload crash + debug-noise reduction (CoplayDev#266) Revise README for improved clarity and organization docs: install uv via official installer (curl/winget) Update README.md docs: fix Windows uv path to use WinGet shim, keep macOS AppSupport symlink path docs: update README.md with improved installation paths, documentation, and logo fix: Update README installation paths to match ServerInstaller.cs
This PR hardens the Unity bridge to survive assembly reloads without native/mono crashes and reduces console noise during stress:
MCPForUnityBridge:ProcessCommandsbeforeAssemblyReloadtools/stress_mcp.pyand docs to run an on-demand multi-client/reload churn stressWhy it matters:
Notes:
Aimed at fixing #238, maybe helps with #257
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests