From a21b62297e506d80d5e4fd970adeb981bfb0a5f1 Mon Sep 17 00:00:00 2001 From: Lou Garczynski Date: Tue, 1 Sep 2026 14:59:38 +0000 Subject: [PATCH 1/3] feat(refresh_unity): return console errors with the refresh Callers refresh to find out whether their code compiled. Across our multi-agent usage 501 of 618 refreshes were immediately followed by an identical read_console asking for errors, one wasted round trip each. Hand the errors back instead. console_errors is only filled when the call actually waited for readiness; otherwise the compile has not finished and the console would be stale. --- MCPForUnity/Editor/Tools/RefreshUnity.cs | 26 +++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/MCPForUnity/Editor/Tools/RefreshUnity.cs b/MCPForUnity/Editor/Tools/RefreshUnity.cs index e35237d80..a5acaca74 100644 --- a/MCPForUnity/Editor/Tools/RefreshUnity.cs +++ b/MCPForUnity/Editor/Tools/RefreshUnity.cs @@ -118,12 +118,36 @@ await WaitForUnityReadyAsync( refresh_triggered = refreshTriggered, compile_requested = compileRequested, resulting_state = resultingState, + console_errors = shouldWaitForReady ? ConsoleErrors() : null, hint = shouldWaitForReady - ? "Unity refresh completed; editor should be ready." + ? "Unity refresh completed; editor should be ready. console_errors holds what read_console would return." : "If Unity enters compilation/domain reload, poll the mcpforunity://editor/state resource until data.advice.ready_for_tools is true." }); } + // Callers refresh to learn whether the code compiled, then always read_console for errors + private static object ConsoleErrors() + { + try + { + var res = ReadConsole.HandleCommand(JObject.FromObject(new + { + action = "get", + types = new[] { "error" }, + count = 20, + format = "plain", + include_stacktrace = false, + })); + // A null Data would read as "compiled clean", so never let a failed read look like one + if (res is SuccessResponse ok) return ok.Data ?? new object[0]; + return new { error = (res as ErrorResponse)?.Error ?? "console_read_failed" }; + } + catch (Exception ex) + { + return new { error = $"console_read_failed: {ex.Message}" }; + } + } + private static Task WaitForUnityReadyAsync(TimeSpan timeout) { var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); From 4af1e1098a6b5aa9672ab3e2c7ecffcce40dbb01 Mon Sep 17 00:00:00 2001 From: Lou Garczynski Date: Wed, 2 Sep 2026 11:29:16 +0000 Subject: [PATCH 2/3] fix(refresh_unity): re-read the payload the reconnect path discards When compile="request" triggers a domain reload the connection closes mid-command, so the tool response is never received. The recovery path then returned a synthetic success carrying only {"recovered_from_disconnect": true}, throwing away whatever the tool would have reported. In our usage that is the common case, not the edge one: 474 of 618 refreshes take this path, so any data refresh_unity returns is invisible to the caller almost every time. The lost payload cannot be reconstructed, so ask again once the editor is ready. compile stays "none" on the re-read so it cannot trigger a second reload (#577), and a failed re-read falls back to the previous response rather than turning a recovered refresh into an error. --- Server/src/services/tools/refresh_unity.py | 32 +++++++++- .../test_refresh_unity_retry_recovery.py | 64 +++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/Server/src/services/tools/refresh_unity.py b/Server/src/services/tools/refresh_unity.py index 2c92831b9..b3065a90d 100644 --- a/Server/src/services/tools/refresh_unity.py +++ b/Server/src/services/tools/refresh_unity.py @@ -61,6 +61,35 @@ async def wait_for_editor_ready(ctx: Context, timeout_s: float = 30.0) -> tuple[ return (False, time.monotonic() - start) +async def _reread_payload_after_reconnect(unity_instance: str | None) -> dict[str, Any] | None: + """Fetch the payload the lost response would have carried. + + The reload closes the connection mid-command, so the original response cannot be + reconstructed - ask again once the editor is back. compile stays "none" so the + re-read cannot trigger a second reload (#577). + """ + try: + response = await unity_transport.send_with_unity_instance( + _legacy_conn.async_send_command_with_retry, + unity_instance, + "refresh_unity", + {"mode": "if_dirty", "scope": "all", + "compile": "none", "wait_for_ready": True}, + retry_on_reload=False, + ) + except Exception as e: + logger.warning( + "refresh_unity: could not re-read state after reconnect: %s", e) + return None + + payload = response if isinstance(response, dict) else ( + response.model_dump() if hasattr(response, "model_dump") else getattr(response, "__dict__", None)) + if not isinstance(payload, dict) or not payload.get("success"): + return None + data = payload.get("data") + return data if isinstance(data, dict) else None + + def is_reloading_rejection(resp: Any) -> bool: """True when Unity rejected a command because it thinks it is reloading. @@ -264,10 +293,11 @@ async def refresh_unity( pass if recovered_from_disconnect: + data = await _reread_payload_after_reconnect(unity_instance) or {} return MCPResponse( success=True, message="Refresh recovered after Unity disconnect/retry; editor is ready.", - data={"recovered_from_disconnect": True}, + data={**data, "recovered_from_disconnect": True}, ) return MCPResponse(**response_dict) if isinstance(response, dict) else response diff --git a/Server/tests/integration/test_refresh_unity_retry_recovery.py b/Server/tests/integration/test_refresh_unity_retry_recovery.py index 8a0a8c19d..6eb9329ea 100644 --- a/Server/tests/integration/test_refresh_unity_retry_recovery.py +++ b/Server/tests/integration/test_refresh_unity_retry_recovery.py @@ -41,3 +41,67 @@ async def fake_send_with_unity_instance(send_fn, unity_instance, command_type, p assert external_changes_scanner._states[inst].dirty is False + + +@pytest.mark.asyncio +async def test_reconnect_rereads_the_payload_it_lost(monkeypatch): + """The disconnect drops the tool payload, so console_errors must be fetched again.""" + from services.tools.refresh_unity import refresh_unity + + ctx = DummyContext() + await ctx.set_state("unity_instance", "UnityMCPTests@cc8756d4cce0805a") + + seen: list[dict] = [] + + async def fake_send_with_unity_instance(send_fn, unity_instance, command_type, params, **kwargs): + if command_type == "refresh_unity": + seen.append(params) + if params.get("compile") == "request": + return {"success": False, "error": "connection closed", "hint": "retry"} + return {"success": True, "data": { + "refresh_triggered": True, + "console_errors": ["Assets/Foo.cs(1,1): error CS0246: nope"], + }} + if command_type == "get_editor_state": + return {"success": True, "data": {"advice": {"ready_for_tools": True}}} + raise ValueError(f"Unexpected command: {command_type}") + + import services.tools.refresh_unity as refresh_mod + monkeypatch.setattr(refresh_mod.unity_transport, + "send_with_unity_instance", fake_send_with_unity_instance) + + resp = await refresh_unity(ctx, compile="request", wait_for_ready=True) + payload = resp.model_dump() if hasattr(resp, "model_dump") else resp + data = payload.get("data", {}) + + assert payload["success"] is True + assert data.get("recovered_from_disconnect") is True + assert data.get("console_errors") == [ + "Assets/Foo.cs(1,1): error CS0246: nope"] + assert seen[-1]["compile"] == "none", "the re-read must not trigger a second reload" + + +@pytest.mark.asyncio +async def test_reconnect_still_succeeds_when_the_reread_fails(monkeypatch): + """A failed re-read must not turn a recovered refresh into an error.""" + from services.tools.refresh_unity import refresh_unity + + ctx = DummyContext() + await ctx.set_state("unity_instance", "UnityMCPTests@cc8756d4cce0805a") + + async def fake_send_with_unity_instance(send_fn, unity_instance, command_type, params, **kwargs): + if command_type == "refresh_unity": + return {"success": False, "error": "connection closed", "hint": "retry"} + if command_type == "get_editor_state": + return {"success": True, "data": {"advice": {"ready_for_tools": True}}} + raise ValueError(f"Unexpected command: {command_type}") + + import services.tools.refresh_unity as refresh_mod + monkeypatch.setattr(refresh_mod.unity_transport, + "send_with_unity_instance", fake_send_with_unity_instance) + + resp = await refresh_unity(ctx, compile="request", wait_for_ready=True) + payload = resp.model_dump() if hasattr(resp, "model_dump") else resp + + assert payload["success"] is True + assert payload.get("data", {}).get("recovered_from_disconnect") is True From 1b77e2651f53f208051f7ee379a9b2ba97341c83 Mon Sep 17 00:00:00 2001 From: Lou Garczynski Date: Wed, 2 Sep 2026 13:44:22 +0000 Subject: [PATCH 3/3] fix(refresh_unity): skip the reconnect re-read when the caller opted out of waiting The re-read blocks until the editor is ready and asks for console_errors, both of which wait_for_ready=False explicitly declines. Recovery now returns the bare response in that case. --- Server/src/services/tools/refresh_unity.py | 6 ++-- .../test_refresh_unity_retry_recovery.py | 28 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/Server/src/services/tools/refresh_unity.py b/Server/src/services/tools/refresh_unity.py index b3065a90d..ca22dda26 100644 --- a/Server/src/services/tools/refresh_unity.py +++ b/Server/src/services/tools/refresh_unity.py @@ -293,11 +293,13 @@ async def refresh_unity( pass if recovered_from_disconnect: - data = await _reread_payload_after_reconnect(unity_instance) or {} + # The re-read waits for the editor, so a caller that asked not to wait keeps the + # bare response rather than a hidden stall and a console_errors it opted out of + data = await _reread_payload_after_reconnect(unity_instance) if wait_for_ready else None return MCPResponse( success=True, message="Refresh recovered after Unity disconnect/retry; editor is ready.", - data={**data, "recovered_from_disconnect": True}, + data={**(data or {}), "recovered_from_disconnect": True}, ) return MCPResponse(**response_dict) if isinstance(response, dict) else response diff --git a/Server/tests/integration/test_refresh_unity_retry_recovery.py b/Server/tests/integration/test_refresh_unity_retry_recovery.py index 6eb9329ea..02d9fe570 100644 --- a/Server/tests/integration/test_refresh_unity_retry_recovery.py +++ b/Server/tests/integration/test_refresh_unity_retry_recovery.py @@ -105,3 +105,31 @@ async def fake_send_with_unity_instance(send_fn, unity_instance, command_type, p assert payload["success"] is True assert payload.get("data", {}).get("recovered_from_disconnect") is True + + +@pytest.mark.asyncio +async def test_no_reread_when_the_caller_asked_not_to_wait(monkeypatch): + """wait_for_ready=False opts out of console_errors, so the re-read must not run.""" + from services.tools.refresh_unity import refresh_unity + + ctx = DummyContext() + await ctx.set_state("unity_instance", "UnityMCPTests@cc8756d4cce0805a") + + seen: list[dict] = [] + + async def fake_send_with_unity_instance(send_fn, unity_instance, command_type, params, **kwargs): + if command_type == "refresh_unity": + seen.append(params) + return {"success": False, "error": "connection closed", "hint": "retry"} + raise ValueError(f"Unexpected command: {command_type}") + + import services.tools.refresh_unity as refresh_mod + monkeypatch.setattr(refresh_mod.unity_transport, + "send_with_unity_instance", fake_send_with_unity_instance) + + resp = await refresh_unity(ctx, compile="request", wait_for_ready=False) + payload = resp.model_dump() if hasattr(resp, "model_dump") else resp + + assert payload["success"] is True + assert payload["data"] == {"recovered_from_disconnect": True} + assert len(seen) == 1, "the recovery path re-read despite wait_for_ready=False"