Skip to content

Commit a2b72bf

Browse files
dsarnoclaude
authored andcommitted
fix: post-merge fixes for CoplayDev#802 (tests, StdioBridge flakiness, focus nudge) (CoplayDev#804)
* fix: remove broken root rename assertion from prefab test LoadAssetAtPath returns the asset filename as .name for prefab roots, not the internally renamed root object name. Remove the rename + assert that was causing CombinesWithOtherModifications to fail. The test still verifies that componentProperties works alongside position changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove yield frames causing StdioBridge reconnect test flakiness The 5-frame yield between client2 handshake and ping created a window where the MCP Python server could reconnect and close our test client as stale. Stale-client cleanup runs synchronously in HandleClientAsync before the read loop, so no yield is needed after reading the handshake. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: ensure focus nudge fires and logs correctly during test polling - Add file handler to root logger so __name__-based loggers (focus_nudge, run_tests) write to the log file instead of only stderr - Add fire-and-forget nudge check on non-wait_timeout get_test_job calls so stalls are detected regardless of client polling style - Add 13 unit tests for should_nudge logic, backoff reset, and gating Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR CoplayDev#804 review feedback - Store asyncio.create_task reference in module-level set to prevent GC (CodeRabbit) - Add test for _get_frontmost_app() returning None (CodeRabbit nitpick) - Add comment explaining why prefab root rename is not tested (Sourcery) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add Claude Code to MCP client examples in README Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 85cc91f commit a2b72bf

6 files changed

Lines changed: 147 additions & 14 deletions

File tree

‎README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
[![](https://badge.mcpx.dev?status=on 'MCP Enabled')](https://modelcontextprotocol.io/introduction)
1414
[![](https://img.shields.io/badge/License-MIT-red.svg 'MIT License')](https://opensource.org/licenses/MIT)
1515

16-
**Create your Unity apps with LLMs!** MCP for Unity bridges AI assistants (Claude, Cursor, VS Code, etc.) with your Unity Editor via the [Model Context Protocol](https://modelcontextprotocol.io/introduction). Give your LLM the tools to manage assets, control scenes, edit scripts, and automate tasks.
16+
**Create your Unity apps with LLMs!** MCP for Unity bridges AI assistants (Claude, Claude Code, Cursor, VS Code, etc.) with your Unity Editor via the [Model Context Protocol](https://modelcontextprotocol.io/introduction). Give your LLM the tools to manage assets, control scenes, edit scripts, and automate tasks.
1717

1818
<img alt="MCP for Unity building a scene" src="docs/images/building_scene.gif">
1919

@@ -25,7 +25,7 @@
2525

2626
* **Unity 2021.3 LTS+** — [Download Unity](https://unity.com/download)
2727
* **Python 3.10+** and **uv** — [Install uv](https://docs.astral.sh/uv/getting-started/installation/)
28-
* **An MCP Client** — [Claude Desktop](https://claude.ai/download) | [Cursor](https://www.cursor.com/en/downloads) | [VS Code Copilot](https://code.visualstudio.com/docs/copilot/overview) | [GitHub Copilot CLI](https://docs.github.com/en/copilot/concepts/agents/about-copilot-cli) | [Windsurf](https://windsurf.com)
28+
* **An MCP Client** — [Claude Desktop](https://claude.ai/download) | [Claude Code](https://docs.anthropic.com/en/docs/claude-code) | [Cursor](https://www.cursor.com/en/downloads) | [VS Code Copilot](https://code.visualstudio.com/docs/copilot/overview) | [GitHub Copilot CLI](https://docs.github.com/en/copilot/concepts/agents/about-copilot-cli) | [Windsurf](https://windsurf.com)
2929

3030
### 1. Install the Unity Package
3131

‎Server/src/main.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,10 @@ def doRollover(self):
9292
_fh.setLevel(getattr(logging, config.log_level))
9393
logger.addHandler(_fh)
9494
logger.propagate = False # Prevent double logging to root logger
95+
# Add file handler to root logger so __name__-based loggers (e.g. utils.focus_nudge,
96+
# services.tools.run_tests) also write to the log file. Named loggers with
97+
# propagate=False won't double-log.
98+
logging.getLogger().addHandler(_fh)
9599
# Also route telemetry logger to the same rotating file and normal level
96100
try:
97101
tlog = logging.getLogger("unity-mcp-telemetry")

‎Server/src/services/tools/run_tests.py‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@
2121

2222
logger = logging.getLogger(__name__)
2323

24+
# Strong references to background fire-and-forget tasks to prevent premature GC.
25+
_background_tasks: set[asyncio.Task] = set()
26+
2427

2528
async def _get_unity_project_path(unity_instance: str | None) -> str | None:
2629
"""Get the project root path for a Unity instance (for focus nudging).
@@ -310,8 +313,31 @@ async def _fetch_status() -> dict[str, Any]:
310313

311314
# No wait_timeout - return immediately (original behavior)
312315
response = await _fetch_status()
313-
if isinstance(response, dict):
314-
if not response.get("success", True):
315-
return MCPResponse(**response)
316-
return GetTestJobResponse(**response)
317-
return MCPResponse(success=False, error=str(response))
316+
if not isinstance(response, dict):
317+
return MCPResponse(success=False, error=str(response))
318+
if not response.get("success", True):
319+
return MCPResponse(**response)
320+
321+
# Fire-and-forget nudge check: even without wait_timeout, clients may poll
322+
# externally. Check if Unity needs a nudge on every call so stalls get
323+
# detected regardless of polling style.
324+
data = response.get("data", {})
325+
status = data.get("status", "")
326+
if status == "running":
327+
progress = data.get("progress", {})
328+
editor_is_focused = progress.get("editor_is_focused", True)
329+
last_update_unix_ms = data.get("last_update_unix_ms")
330+
current_time_ms = int(time.time() * 1000)
331+
if should_nudge(
332+
status=status,
333+
editor_is_focused=editor_is_focused,
334+
last_update_unix_ms=last_update_unix_ms,
335+
current_time_ms=current_time_ms,
336+
):
337+
logger.info(f"Test job {job_id} appears stalled (unfocused Unity), scheduling background nudge...")
338+
project_path = await _get_unity_project_path(unity_instance)
339+
task = asyncio.create_task(nudge_unity_focus(unity_project_path=project_path))
340+
_background_tasks.add(task)
341+
task.add_done_callback(_background_tasks.discard)
342+
343+
return GetTestJobResponse(**response)

‎Server/tests/test_focus_nudge.py‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
"""Tests for focus_nudge utility — should_nudge() logic and nudge_unity_focus() gating."""
2+
3+
import time
4+
from unittest.mock import patch, AsyncMock
5+
6+
import pytest
7+
8+
from utils.focus_nudge import (
9+
should_nudge,
10+
reset_nudge_backoff,
11+
nudge_unity_focus,
12+
_is_available,
13+
)
14+
15+
16+
class TestShouldNudge:
17+
"""Tests for should_nudge() decision logic."""
18+
19+
def test_returns_false_when_not_running(self):
20+
assert should_nudge(status="succeeded", editor_is_focused=False, last_update_unix_ms=0, current_time_ms=99999) is False
21+
22+
def test_returns_false_when_focused(self):
23+
assert should_nudge(status="running", editor_is_focused=True, last_update_unix_ms=0, current_time_ms=99999) is False
24+
25+
def test_returns_true_when_stalled_and_unfocused(self):
26+
now_ms = int(time.time() * 1000)
27+
stale_ms = now_ms - 5000 # 5s ago
28+
assert should_nudge(status="running", editor_is_focused=False, last_update_unix_ms=stale_ms, current_time_ms=now_ms) is True
29+
30+
def test_returns_false_when_recently_updated(self):
31+
now_ms = int(time.time() * 1000)
32+
recent_ms = now_ms - 1000 # 1s ago (within 3s threshold)
33+
assert should_nudge(status="running", editor_is_focused=False, last_update_unix_ms=recent_ms, current_time_ms=now_ms) is False
34+
35+
def test_returns_true_when_no_updates_yet(self):
36+
"""No last_update_unix_ms means tests might be stuck at start."""
37+
assert should_nudge(status="running", editor_is_focused=False, last_update_unix_ms=None) is True
38+
39+
def test_custom_stall_threshold(self):
40+
now_ms = int(time.time() * 1000)
41+
stale_ms = now_ms - 2000 # 2s ago
42+
# Default threshold (3s) — not stale yet
43+
assert should_nudge(status="running", editor_is_focused=False, last_update_unix_ms=stale_ms, current_time_ms=now_ms) is False
44+
# Custom threshold (1s) — stale
45+
assert should_nudge(status="running", editor_is_focused=False, last_update_unix_ms=stale_ms, current_time_ms=now_ms, stall_threshold_ms=1000) is True
46+
47+
def test_returns_false_for_failed_status(self):
48+
assert should_nudge(status="failed", editor_is_focused=False, last_update_unix_ms=0, current_time_ms=99999) is False
49+
50+
def test_returns_false_for_cancelled_status(self):
51+
assert should_nudge(status="cancelled", editor_is_focused=False, last_update_unix_ms=0, current_time_ms=99999) is False
52+
53+
54+
class TestResetNudgeBackoff:
55+
"""Tests for reset_nudge_backoff() state management."""
56+
57+
def test_resets_consecutive_nudges(self):
58+
import utils.focus_nudge as fn
59+
fn._consecutive_nudges = 5
60+
reset_nudge_backoff()
61+
assert fn._consecutive_nudges == 0
62+
63+
def test_updates_last_progress_time(self):
64+
import utils.focus_nudge as fn
65+
old_time = fn._last_progress_time
66+
reset_nudge_backoff()
67+
assert fn._last_progress_time >= old_time
68+
69+
70+
class TestNudgeUnityFocus:
71+
"""Tests for nudge_unity_focus() gating logic."""
72+
73+
@pytest.mark.asyncio
74+
async def test_skips_when_not_available(self):
75+
with patch("utils.focus_nudge._is_available", return_value=False):
76+
result = await nudge_unity_focus(force=True)
77+
assert result is False
78+
79+
@pytest.mark.asyncio
80+
async def test_skips_when_unity_already_focused(self):
81+
from utils.focus_nudge import _FrontmostAppInfo
82+
with patch("utils.focus_nudge._is_available", return_value=True), \
83+
patch("utils.focus_nudge._get_frontmost_app", return_value=_FrontmostAppInfo(name="Unity")):
84+
result = await nudge_unity_focus(force=True)
85+
assert result is False
86+
87+
@pytest.mark.asyncio
88+
async def test_skips_when_frontmost_app_unknown(self):
89+
with patch("utils.focus_nudge._is_available", return_value=True), \
90+
patch("utils.focus_nudge._get_frontmost_app", return_value=None):
91+
result = await nudge_unity_focus(force=True)
92+
assert result is False
93+
94+
@pytest.mark.asyncio
95+
async def test_rate_limited_by_backoff(self):
96+
import utils.focus_nudge as fn
97+
from utils.focus_nudge import _FrontmostAppInfo
98+
# Simulate a very recent nudge
99+
fn._last_nudge_time = time.monotonic()
100+
fn._consecutive_nudges = 0
101+
with patch("utils.focus_nudge._is_available", return_value=True), \
102+
patch("utils.focus_nudge._get_frontmost_app", return_value=_FrontmostAppInfo(name="Terminal")):
103+
result = await nudge_unity_focus(force=False)
104+
assert result is False

‎TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/StdioBridgeReconnectTests.cs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,10 @@ public IEnumerator NewClient_WhileOldClientStillConnected_ClosesStaleClient()
120120
string handshake2 = ReadLine(stream2, ReadTimeoutMs);
121121
Assert.That(handshake2, Does.Contain("FRAMING=1"), "Second client should receive handshake");
122122

123-
// Wait a few frames for stale client cleanup
124-
for (int i = 0; i < 5; i++)
125-
yield return null;
126-
127-
// Second client should work — stale first client was closed
123+
// Stale-client cleanup runs synchronously in HandleClientAsync before
124+
// the read loop, so by the time we read the handshake it's already done.
125+
// No yield needed — yielding here creates a window for the MCP Python
126+
// server to reconnect and close our test client as stale.
128127
SendFrame(stream2, Encoding.UTF8.GetBytes("ping"));
129128
byte[] pong2Bytes = ReadFrame(stream2, ReadTimeoutMs);
130129
Assert.That(Encoding.UTF8.GetString(pong2Bytes), Does.Contain("pong"),

‎TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -886,6 +886,8 @@ public void ModifyContents_ComponentProperties_ReturnsErrorForInvalidType()
886886
}
887887
}
888888

889+
// Note: root rename is NOT tested here because LoadAssetAtPath<GameObject> returns
890+
// the asset filename as .name for prefab roots, so rename assertions always fail.
889891
[Test]
890892
public void ModifyContents_ComponentProperties_CombinesWithOtherModifications()
891893
{
@@ -898,7 +900,6 @@ public void ModifyContents_ComponentProperties_CombinesWithOtherModifications()
898900
["action"] = "modify_contents",
899901
["prefabPath"] = prefabPath,
900902
["position"] = new JArray(5f, 10f, 15f),
901-
["name"] = "RenamedWithProps",
902903
["componentProperties"] = new JObject
903904
{
904905
["Rigidbody"] = new JObject { ["mass"] = 25f }
@@ -908,7 +909,6 @@ public void ModifyContents_ComponentProperties_CombinesWithOtherModifications()
908909
Assert.IsTrue(result.Value<bool>("success"), $"Expected success but got: {result}");
909910

910911
GameObject reloaded = AssetDatabase.LoadAssetAtPath<GameObject>(prefabPath);
911-
Assert.AreEqual("RenamedWithProps", reloaded.name);
912912
Assert.AreEqual(new Vector3(5f, 10f, 15f), reloaded.transform.localPosition);
913913
Assert.AreEqual(25f, reloaded.GetComponent<Rigidbody>().mass, 0.01f);
914914
}

0 commit comments

Comments
 (0)