Detect external package changes and harden refresh retry #120
Conversation
|
Warning Rate limit exceeded@dsarno has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 46 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 selected for processing (1)
WalkthroughAdds a v2 editor-state pipeline (Unity cache + server enrichment), external-changes scanning, preflight gating and refresh tooling with readiness/polling, async test-job orchestration (start/poll), TestJobManager/TestRun tooling, many synchronous AssetDatabase refreshes, and version/docs bump to 8.6.0. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server
participant Preflight as Preflight Guard
participant State as get_editor_state_v2
participant Unity as Unity Transport
participant Tool as Tool (e.g., manage_scene)
Client->>Server: call tool
Server->>Preflight: preflight(wait_for_no_compile=True, refresh_if_dirty=True)
Preflight->>State: get_editor_state_v2
State->>Unity: send v2 or legacy editor_state command
Unity-->>State: editor state
State-->>Preflight: enriched state (advice/staleness)
alt assets dirty & refresh_if_dirty
Preflight->>Server: refresh_unity (best-effort)
Server->>Unity: refresh_unity (with retry)
Unity-->>Server: response (may hint "retry")
Server->>State: poll get_editor_state_v2 until advice.ready_for_tools
end
alt blocked (compiling/tests)
Preflight-->>Server: MCPResponse(busy, retry_after_ms)
Server-->>Client: busy response
else ready
Server->>Tool: proceed to execute
Tool->>Unity: send tool command
Unity-->>Server: result
Server-->>Client: result
end
sequenceDiagram
actor User
participant Server
participant Refresh as refresh_unity
participant Unity as Unity Transport
participant State as get_editor_state_v2
participant Scanner as ExternalChangesScanner
User->>Server: refresh_unity(wait_for_ready=True,...)
Server->>Unity: send_command_with_retry(refresh_unity params)
alt response with retry hint
Unity-->>Server: {success:false, hint:"retry"}
Server->>State: poll get_editor_state_v2 until advice.ready_for_tools
State->>Unity: fetch state
else success
Unity-->>Server: success
end
alt ready detected
Server->>Scanner: clear_dirty(instance_id)
Scanner-->>Server: OK
Server-->>User: SuccessResponse (recovered_from_disconnect if applicable)
else timeout/error
Server-->>User: Error/timeout MCPResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 4
🧹 Nitpick comments (13)
MCPForUnity/Editor/Services/TestRunStatus.cs (1)
19-37: Consider consistent brace style in lock statements.The property getters use
lock (LockObj) return _field;without braces, whileMarkStartedandMarkFinisheduse braces within their lock blocks. While both are valid C#, consistent use of braces improves readability.🔎 Suggested refactor for consistency
public static bool IsRunning { - get { lock (LockObj) return _isRunning; } + get { lock (LockObj) { return _isRunning; } } } public static TestMode? Mode { - get { lock (LockObj) return _mode; } + get { lock (LockObj) { return _mode; } } } public static long? StartedUnixMs { - get { lock (LockObj) return _startedUnixMs; } + get { lock (LockObj) { return _startedUnixMs; } } } public static long? FinishedUnixMs { - get { lock (LockObj) return _finishedUnixMs; } + get { lock (LockObj) { return _finishedUnixMs; } } }MCPForUnity/Editor/Tools/RefreshUnity.cs (2)
36-39: Empty catch block silently swallows exceptions.Catching all exceptions without at least logging makes debugging difficult. Consider logging a warning or being more specific about what parse failures are expected.
🔎 Proposed improvement
catch { - // ignore parse failures + // Ignore parse failures for wait_for_ready; default to false is acceptable. + // Uncomment for debugging: McpLog.Debug("[RefreshUnity] Failed to parse wait_for_ready"); }
61-65: Consider adding a comment clarifying the intentional no-op.The empty block for
scope == "scripts"is intentional but may confuse future maintainers. Adding a brief explanation of why the refresh is skipped would improve clarity.🔎 Proposed improvement
if (string.Equals(scope, "scripts", StringComparison.OrdinalIgnoreCase)) { // For scripts, requesting compilation is usually the meaningful action. - // We avoid a heavyweight full refresh by default. + // We avoid a heavyweight full refresh by default; the caller should set + // compile="request" to trigger script compilation instead. }Server/tests/integration/test_refresh_unity_registration.py (1)
9-9: Consider removing the unusednoqadirective.Per static analysis, the
F401rule (unused imports) is not enabled, making thenoqacomment unnecessary. The import is intentionally side-effectual for registration, which the preceding comment already explains.🔎 Proposed fix
# Import the specific module to ensure it registers its decorator without disturbing global registry state. - import services.tools.refresh_unity # noqa: F401 + import services.tools.refresh_unityServer/tests/integration/test_editor_state_v2_contract.py (1)
15-16: Consider removing the unnecessarynoqadirective.The import is for side effects (triggering resource registration via decorator). Ruff indicates
F401is not enabled in your configuration, so thenoqadirective is unused.🔎 Proposed fix
# Import the v2 module to ensure it registers its decorator without disturbing global registry state. - import services.resources.editor_state_v2 # noqa: F401 + import services.resources.editor_state_v2Server/src/services/tools/preflight.py (1)
46-52: Consider adding debug-level logging for swallowed exceptions.The broad exception handling is intentional for resilience—preflight should not block tools on unexpected errors. However, logging at debug level would aid troubleshooting without affecting normal operation.
🔎 Proposed improvement
+ import logging + logger = logging.getLogger(__name__) + try: from services.resources.editor_state_v2 import get_editor_state_v2 state_resp = await get_editor_state_v2(ctx) state = state_resp.model_dump() if hasattr(state_resp, "model_dump") else state_resp except Exception: # If we cannot determine readiness, fall back to proceeding (tools already contain retry logic). + logger.debug("Preflight: failed to get editor state, proceeding", exc_info=True) return NoneServer/src/services/tools/refresh_unity.py (1)
55-69: Consider logging when readiness polling times out.The wait loop exits silently after 60s if readiness is never achieved. While proceeding is reasonable (best-effort wait), a warning log would help diagnose situations where Unity remains stuck.
🔎 Proposed improvement
+ import logging + logger = logging.getLogger(__name__) + if wait_for_ready: timeout_s = 60.0 start = time.monotonic() from services.resources.editor_state_v2 import get_editor_state_v2 while time.monotonic() - start < timeout_s: state_resp = await get_editor_state_v2(ctx) state = state_resp.model_dump() if hasattr(state_resp, "model_dump") else state_resp data = (state or {}).get("data") if isinstance(state, dict) else None advice = (data or {}).get("advice") if isinstance(data, dict) else None if isinstance(advice, dict) and advice.get("ready_for_tools") is True: break await asyncio.sleep(0.25) + else: + logger.warning("refresh_unity: readiness polling timed out after %.1fs", timeout_s)Server/src/services/resources/editor_state_v2.py (4)
15-16: Minor duplication withexternal_changes_scanner._now_unix_ms.This helper duplicates the same function in
external_changes_scanner.py. Consider extracting to a shared utility module if more timestamp helpers are needed, but acceptable as-is for module isolation.
24-31: Unusedctxparameter.The
ctxparameter is declared but never used in this function. Either remove it if not needed, or document why it's reserved for future use.🔎 Proposed fix
If
ctxis not needed:-async def _infer_single_instance_id(ctx: Context) -> str | None: +async def _infer_single_instance_id() -> str | None:Then update the call site at line 231:
- inferred = await _infer_single_instance_id(ctx) + inferred = await _infer_single_instance_id()
32-66: Consider debug logging for inference failures.The broad exception handling is appropriate for best-effort inference, but silent failures can make transport issues hard to diagnose. Consider adding debug-level logging before returning
None.🔎 Example
import logging logger = logging.getLogger(__name__) # Then in each except block: except Exception as e: logger.debug("Failed to infer instance ID via HTTP: %s", e) return None
237-265: Add logging to the external changes integration block.The
try-except-passpattern silently swallows all errors from external changes scanning. While best-effort is appropriate here, logging at debug level would aid troubleshooting when filesystem scanning unexpectedly fails.🔎 Proposed fix
+import logging +logger = logging.getLogger(__name__) except Exception: - # Best-effort; do not fail readiness resource if filesystem scan can't run. - pass + # Best-effort; do not fail readiness resource if filesystem scan can't run. + logger.debug("External changes scan failed", exc_info=True)Server/src/services/state/external_changes_scanner.py (2)
123-129: Consider narrowing the exception type or adding logging.The bare
except Exceptionat line 126 will catch both JSON parsing errors and unexpected I/O errors. While defensive, this could mask issues. Consider catchingjson.JSONDecodeErrorspecifically or adding debug logging.🔎 Proposed fix
try: raw = manifest_path.read_text(encoding="utf-8") doc = json.loads(raw) - except Exception: + except (OSError, json.JSONDecodeError): st.extra_roots = [] st.manifest_last_mtime_ns = mtime_ns return []
213-216: Add logging for manifest resolution failures.The
try-except-passsilently swallows errors from_resolve_manifest_extra_roots. While non-fatal, logging would help diagnose issues with local package detection.🔎 Proposed fix
+import logging +logger = logging.getLogger(__name__) try: paths.extend(self._resolve_manifest_extra_roots(root, st)) - except Exception: - pass + except Exception: + logger.debug("Failed to resolve manifest extra roots for %s", project_root, exc_info=True)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
MCPForUnity/Editor/Dependencies/Models.metaMCPForUnity/Editor/Resources/Editor/EditorStateV2.csMCPForUnity/Editor/Resources/Editor/EditorStateV2.cs.metaMCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Services/EditorStateCache.cs.metaMCPForUnity/Editor/Services/TestRunStatus.csMCPForUnity/Editor/Services/TestRunStatus.cs.metaMCPForUnity/Editor/Services/TestRunnerService.csMCPForUnity/Editor/Tools/RefreshUnity.csMCPForUnity/Editor/Tools/RefreshUnity.cs.metaMCPForUnity/package.jsonServer/README.mdServer/pyproject.tomlServer/src/services/resources/editor_state_v2.pyServer/src/services/state/external_changes_scanner.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/preflight.pyServer/src/services/tools/refresh_unity.pyServer/src/services/tools/run_tests.pyServer/tests/integration/test_editor_state_v2_contract.pyServer/tests/integration/test_external_changes_scanner.pyServer/tests/integration/test_refresh_unity_registration.pyServer/tests/integration/test_refresh_unity_retry_recovery.pyServer/tests/integration/test_run_tests_busy_semantics.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:19:59.258Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 85
File: tests/test_telemetry_queue_worker.py:33-37
Timestamp: 2025-10-21T05:19:59.258Z
Learning: In the unity-mcp repository, pyproject.toml is located in MCPForUnity/UnityMcpServer~/src/, not at the repository root. When testing code that imports telemetry.py, the working directory should be changed to SRC (MCPForUnity/UnityMcpServer~/src/) so that telemetry.py's get_package_version() can find pyproject.toml via relative path.
Applied to files:
Server/pyproject.toml
🧬 Code graph analysis (10)
MCPForUnity/Editor/Resources/Editor/EditorStateV2.cs (1)
MCPForUnity/Editor/Services/EditorStateCache.cs (3)
JObject(90-213)JObject(215-226)EditorStateCache(34-62)
Server/src/services/tools/manage_asset.py (1)
Server/src/services/tools/preflight.py (1)
preflight(27-105)
MCPForUnity/Editor/Services/TestRunnerService.cs (1)
MCPForUnity/Editor/Services/TestRunStatus.cs (3)
TestRunStatus(10-59)MarkStarted(39-48)MarkFinished(50-58)
Server/tests/integration/test_refresh_unity_retry_recovery.py (3)
Server/src/services/state/external_changes_scanner.py (1)
ExternalChangesState(21-31)Server/tests/integration/test_helpers.py (3)
DummyContext(16-55)set_state(49-51)model_dump(10-13)Server/src/services/tools/refresh_unity.py (1)
refresh_unity(20-88)
Server/tests/integration/test_editor_state_v2_contract.py (2)
Server/tests/integration/test_helpers.py (2)
DummyContext(16-55)model_dump(10-13)Server/tests/integration/test_run_tests_busy_semantics.py (1)
fake_send_with_unity_instance(14-20)
Server/src/services/resources/editor_state_v2.py (5)
Server/src/transport/legacy/unity_connection.py (2)
async_send_command_with_retry(753-785)get_unity_connection_pool(652-665)Server/src/services/state/external_changes_scanner.py (4)
_now_unix_ms(11-12)_in_pytest(15-17)set_project_root(51-54)update_and_get(172-240)Server/src/services/tools/preflight.py (1)
_in_pytest(11-14)Server/src/transport/unity_transport.py (2)
_current_transport(23-25)send_with_unity_instance(79-113)Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
Server/src/services/tools/manage_gameobject.py (1)
Server/src/services/tools/preflight.py (1)
preflight(27-105)
Server/tests/integration/test_refresh_unity_registration.py (1)
Server/src/services/tools/refresh_unity.py (1)
refresh_unity(20-88)
MCPForUnity/Editor/Tools/RefreshUnity.cs (2)
MCPForUnity/Editor/Services/EditorStateCache.cs (2)
JObject(90-213)JObject(215-226)MCPForUnity/Editor/Services/TestRunStatus.cs (1)
TestRunStatus(10-59)
Server/src/services/tools/manage_scene.py (1)
Server/src/services/tools/preflight.py (1)
preflight(27-105)
🪛 Ruff (0.14.10)
Server/src/services/tools/preflight.py
50-50: Do not catch blind exception: Exception
(BLE001)
69-71: try-except-pass detected, consider logging the exception
(S110)
69-69: Do not catch blind exception: Exception
(BLE001)
100-100: Do not catch blind exception: Exception
(BLE001)
Server/src/services/tools/refresh_unity.py
78-79: try-except-pass detected, consider logging the exception
(S110)
78-78: Do not catch blind exception: Exception
(BLE001)
Server/tests/integration/test_run_tests_busy_semantics.py
14-14: Unused function argument: send_fn
(ARG001)
14-14: Unused function argument: unity_instance
(ARG001)
14-14: Unused function argument: params
(ARG001)
14-14: Unused function argument: kwargs
(ARG001)
Server/src/services/state/external_changes_scanner.py
126-126: Do not catch blind exception: Exception
(BLE001)
215-216: try-except-pass detected, consider logging the exception
(S110)
215-215: Do not catch blind exception: Exception
(BLE001)
Server/tests/integration/test_refresh_unity_retry_recovery.py
25-25: Unused function argument: send_fn
(ARG001)
25-25: Unused function argument: unity_instance
(ARG001)
25-25: Unused function argument: params
(ARG001)
25-25: Unused function argument: kwargs
(ARG001)
Server/tests/integration/test_editor_state_v2_contract.py
16-16: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
26-26: Unused function argument: send_fn
(ARG001)
26-26: Unused function argument: unity_instance
(ARG001)
26-26: Unused function argument: params
(ARG001)
26-26: Unused function argument: kwargs
(ARG001)
Server/src/services/resources/editor_state_v2.py
24-24: Unused function argument: ctx
(ARG001)
34-34: Do not catch blind exception: Exception
(BLE001)
50-50: Do not catch blind exception: Exception
(BLE001)
64-64: Do not catch blind exception: Exception
(BLE001)
146-146: Do not catch blind exception: Exception
(BLE001)
263-265: try-except-pass detected, consider logging the exception
(S110)
263-263: Do not catch blind exception: Exception
(BLE001)
Server/tests/integration/test_refresh_unity_registration.py
9-9: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (40)
Server/README.md (1)
72-72: LGTM! Documentation version reference updated correctly.The version bump to v8.6.0 aligns with the package version changes across the project.
MCPForUnity/package.json (1)
3-3: LGTM! Package version bumped consistently.The version bump to 8.6.0 is synchronized with the server-side version in pyproject.toml and the documentation reference in README.md.
MCPForUnity/Editor/Services/EditorStateCache.cs.meta (1)
1-11: Standard Unity meta file.This is standard Unity-generated metadata for the EditorStateCache.cs file. All fields contain expected default values.
Server/src/services/tools/manage_asset.py (2)
15-15: Good addition: preflight import for readiness checks.Importing the preflight guard enables stability checks before asset operations.
51-53: Excellent safeguard: preflight check before asset operations.The preflight guard with
wait_for_no_compile=Trueandrefresh_if_dirty=Trueis a smart addition that:
- Prevents operations during compilation/domain reload
- Ensures external asset changes are imported first
- Reduces flaky timeouts and stale reads
The best-effort nature (falls through on errors) maintains backward compatibility while adding resilience. The default 30-second timeout is reasonable for asset operations.
Server/pyproject.toml (1)
3-3: LGTM! Server version synchronized with Unity package.The version bump to 8.6.0 maintains consistency with MCPForUnity/package.json and properly reflects the new v2 editor state architecture features introduced in this PR.
MCPForUnity/Editor/Services/TestRunStatus.cs (1)
39-58: LGTM! Thread-safe state management is correct.The locking strategy properly guards all state mutations and reads. The state transitions in
MarkStartedandMarkFinishedcorrectly manage the test run lifecycle.MCPForUnity/Editor/Services/EditorStateCache.cs (3)
64-88: LGTM! Throttling and thread-safe updates are well implemented.The throttling logic correctly reduces overhead while still capturing compilation edge transitions. The
ForceUpdatemethod properly guards the cached snapshot with a lock to ensure thread safety across editor callbacks.
113-133: Activity phase prioritization is logical and comprehensive.The phase detection correctly prioritizes: tests → compiling → domain reload → asset import → playmode transition → idle. This provides clear semantics for downstream preflight and readiness checks.
215-226: Defensive cloning prevents external mutation of cached state.Returning a deep clone ensures callers cannot accidentally mutate the cached snapshot, maintaining thread safety guarantees.
MCPForUnity/Editor/Tools/RefreshUnity.cs (2)
128-170: LGTM - Async polling pattern is well-structured.The
WaitForUnityReadyAsyncimplementation properly:
- Uses
TaskCreationOptions.RunContinuationsAsynchronouslyto avoid deadlocks- Wraps the tick handler in try-catch to ensure delegate cleanup on exceptions
- Checks all relevant readiness conditions matching
EditorStateCachelogic- Nudges the editor loop to handle throttled updates
92-126: LGTM - Response handling is comprehensive.The wait logic handles timeout and general exceptions appropriately, and the success response provides actionable hints for callers based on whether
wait_for_readywas used.MCPForUnity/Editor/Services/TestRunnerService.cs (3)
96-97: Good placement of status tracking before Execute.Marking the run as started before
_testRunnerApi.Execute(settings)ensures the readiness snapshot reflects the busy state even if the execution throws synchronously. This aligns well with the early-return guard inRefreshUnity.HandleCommand.
118-129: LGTM - Proper cleanup on startup failure.The catch block correctly clears the test run status before releasing the lock and rethrowing, ensuring the readiness snapshot doesn't get stuck in a "running" state when the test run fails to start.
172-183: LGTM - Status correctly cleared on run completion.The
RunFinishedcallback properly marks the test run as finished after completing the task, ensuring the status is cleared even if the run result processing has issues downstream.MCPForUnity/Editor/Tools/RefreshUnity.cs.meta (1)
1-11: LGTM - Standard Unity meta file.This is a correctly structured Unity asset metadata file for the new
RefreshUnity.cssource file.MCPForUnity/Editor/Services/TestRunStatus.cs.meta (1)
1-11: LGTM - Standard Unity meta file.This is a correctly structured Unity asset metadata file for the new
TestRunStatus.cssource file.Server/tests/integration/test_run_tests_busy_semantics.py (2)
14-20: Unused mock arguments are acceptable here.The static analysis flags for unused arguments (
send_fn,unity_instance,params,kwargs) are false positives. The mock function signature must match the realsend_with_unity_instancesignature for the monkeypatch to work correctly. The assertion oncommand_typeat line 15 validates the call is made with the expected command.
24-35: LGTM - Comprehensive busy-state contract validation.The test properly validates:
- Failure indication (
success: False)- Retry hint presence (
hint: "retry")- Reason categorization (
tests_runningorbusy)- Minimum backoff (
retry_after_ms >= 500)This ensures callers can reliably detect and handle busy states.
MCPForUnity/Editor/Resources/Editor/EditorStateV2.cs.meta (1)
1-11: LGTM - Standard Unity meta file.This is a correctly structured Unity asset metadata file for the new
EditorStateV2.csresource file.Server/tests/integration/test_refresh_unity_registration.py (1)
4-13: LGTM - Simple and effective registration test.The test correctly triggers module import to register the tool via decorators, then validates presence in the registry. This pattern ensures the tool is properly wired up without testing implementation details.
MCPForUnity/Editor/Resources/Editor/EditorStateV2.cs (1)
1-27: LGTM! Clean resource handler implementation.The implementation correctly retrieves the cached v2 snapshot and wraps it in appropriate response types. Exception handling is adequate for surfacing errors to clients. The
@paramsparameter is unused but likely required by the MCP resource handler contract.Server/tests/integration/test_refresh_unity_retry_recovery.py (1)
10-44: Well-structured integration test for disconnect recovery.The test effectively validates the critical recovery path when Unity disconnects with a retry hint. It correctly:
- Seeds the external changes scanner with dirty state
- Mocks transport to simulate a disconnect with
hint="retry"- Mocks editor state to indicate readiness
- Verifies both the response payload and the side effect (dirty flag cleared)
The unused arguments in
fake_send_with_unity_instance(flagged by static analysis) are intentional—the mock must match the real function's signature.Server/src/services/tools/run_tests.py (2)
61-63: Good preflight integration.The preflight guard correctly gates test execution with
requires_no_tests=Trueto prevent overlapping test runs. The early return pattern is clean and avoids unnecessary work before the guard passes.
106-118: Well-structured busy-state response.The busy-state detection for in-progress test runs is properly implemented:
- Case-insensitive matching handles variations in error messages
- The structured response with
error="tests_running"andhint="retry"enables clients to handle this gracefully- The
retry_after_ms: 5000provides actionable guidanceServer/tests/integration/test_editor_state_v2_contract.py (1)
48-56: Solid contract validation.The assertions comprehensively verify the v2 contract structure including the required fields (
schema_version,observed_at_unix_ms,sequence,advice,staleness). This ensures the resource meets client expectations for readiness polling.Server/tests/integration/test_external_changes_scanner.py (2)
6-43: Comprehensive test for dirty state lifecycle.The test properly validates the scanner's core behavior:
- Establishes a baseline scan (not dirty)
- Touches a file with a future timestamp to trigger dirty detection
- Verifies the dirty flag and associated timestamps
- Confirms clear_dirty resets state appropriately
The
monkeypatch.delenv("PYTEST_CURRENT_TEST")is correctly used to enable the scanner during this unit-style test.
46-84: Good coverage for external package root detection.This test validates an important edge case: detecting changes in external packages referenced via
file:dependencies inmanifest.json. The test correctly:
- Creates a package outside the project root
- Declares it in manifest.json with a relative
file:path- Verifies that touching the external package triggers dirty state
Server/src/services/tools/preflight.py (2)
11-14: Appropriate test environment detection.Using
PYTEST_CURRENT_TESTto detect the test environment is a reasonable approach. This allows preflight to be a no-op during integration tests that mock transports.
79-101: Compilation wait loop is well-structured.The bounded wait with periodic state refresh is a solid pattern. The 0.25s polling interval balances responsiveness with avoiding excessive requests. Returning
Noneon state refresh failures allows tools to proceed rather than hang.One minor note: the import inside the loop (
from services.resources.editor_state_v2 import get_editor_state_v2) could be hoisted outside for slight efficiency, though the impact is negligible.Server/src/services/tools/refresh_unity.py (2)
44-53: Good retry detection heuristics.The recovery logic correctly identifies retryable scenarios by checking both the explicit
hint="retry"and the presence of "disconnected" in the error message. This handles both explicit Unity signaling and transport-level disconnects.
71-79: Silent exception handling is acceptable for cleanup.The dirty flag clearing is non-critical cleanup, so swallowing exceptions here is reasonable. The instance inference and clearing are best-effort operations that shouldn't fail the overall refresh.
Server/src/services/resources/editor_state_v2.py (3)
19-21: LGTM!The pytest guard follows the same pattern used in
external_changes_scanner.pyand correctly prevents instance-discovery side effects during tests.
69-138: LGTM!The legacy-to-v2 mapping is well-structured with appropriate defaults. The fallback handling for wrapped vs unwrapped responses (line 76) and safe type coercions are correct.
141-179: LGTM!The advice computation logic is clear and correct. The 2000ms staleness threshold is well-documented. The blocking reasons aggregation properly captures all states that should prevent tool execution.
Server/src/services/state/external_changes_scanner.py (5)
1-17: LGTM!Clean imports and utility helpers. The
_in_pytestguard correctly disables filesystem scanning during integration tests.
20-31: LGTM!Well-structured dataclass with appropriate defaults. The separation of timing fields (
last_scan_unix_ms,dirty_since_unix_ms, etc.) and the manifest cache (extra_roots,manifest_last_mtime_ns) supports efficient incremental scanning.
34-62: LGTM!The scanner configuration with configurable
scan_interval_ms(1500ms) andmax_entries(20000) provides sensible bounds on I/O overhead. Theclear_dirtymethod correctly resets the mtime baseline to force re-establishment on the next scan.
64-104: LGTM!The recursive mtime scan is well-optimized:
- Prunes heavy Unity directories (
Library,Temp, etc.) to avoid I/O on generated content- Entry limit prevents runaway scans on large projects
- Graceful
OSErrorhandling for inaccessible files- Cross-platform case-insensitive directory matching
226-244: Thread safety consideration for concurrent access.The scanner mutates state (
st.dirty,st.last_seen_mtime_ns, etc.) without synchronization. Ifupdate_and_getis called concurrently from multiple asyncio tasks for the sameinstance_id, there could be race conditions. Consider whether this is a concern in your usage pattern.If concurrent access is expected, a per-instance lock could be added:
import threading def __init__(self, ...): self._locks: dict[str, threading.Lock] = {} def _get_lock(self, instance_id: str) -> threading.Lock: return self._locks.setdefault(instance_id, threading.Lock())
| assets = new | ||
| { | ||
| is_updating = EditorApplication.isUpdating, | ||
| external_changes_dirty = false, | ||
| external_changes_last_seen_unix_ms = (long?)null, | ||
| refresh = new | ||
| { | ||
| is_refresh_in_progress = false, | ||
| last_refresh_requested_unix_ms = (long?)null, | ||
| last_refresh_finished_unix_ms = (long?)null | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for code that updates external_changes_dirty or external_changes_last_seen
rg -n -C3 'external_changes_dirty|external_changes_last_seen' --type=csRepository: dsarno/unity-mcp
Length of output: 820
🏁 Script executed:
#!/bin/bash
# Search for refresh field updates
rg -n -C3 'is_refresh_in_progress|last_refresh_requested_unix_ms|last_refresh_finished_unix_ms' --type=csRepository: dsarno/unity-mcp
Length of output: 916
🏁 Script executed:
#!/bin/bash
# Search for methods that create or modify the state snapshot
rg -n 'CreateSnapshot|GetSnapshot|SetSnapshot' --type=cs -B2 -A5Repository: dsarno/unity-mcp
Length of output: 1474
🏁 Script executed:
#!/bin/bash
# Search for external changes scanner or related logic
rg -n 'external.*change|scanner' --type=cs -iRepository: dsarno/unity-mcp
Length of output: 285
These fields are hardcoded placeholders with no update mechanism in the codebase.
Verification confirms that external_changes_dirty, external_changes_last_seen_unix_ms, and all refresh tracking fields are hardcoded at initialization and are never updated anywhere in the repository. There is no external changes scanner implementation and no logic to populate these fields. Either the feature is incomplete or these fields are intentional stubs for future implementation.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Services/EditorStateCache.cs around lines 180 to 191,
several asset-tracking fields are hardcoded placeholders
(external_changes_dirty, external_changes_last_seen_unix_ms, and the refresh
tracking fields) but there is no scanner or update logic populating them; either
implement the missing update mechanism or remove/mark these fields as TODO. Add
a background or event-driven external-changes detector (or hook into Unity's
file/asset change callbacks) that updates external_changes_dirty and
external_changes_last_seen_unix_ms, and implement refresh state transitions (set
is_refresh_in_progress, last_refresh_requested_unix_ms and
last_refresh_finished_unix_ms) around the refresh workflow; alternatively, if
the feature is not ready, remove these fields or clearly annotate them as
TODO/stubs and ensure no code consumes them to avoid misleading state. Ensure
timestamps use consistent Unix ms conversion and all updates occur on the main
thread or are synchronized appropriately.
| # If Unity returns a structured retry hint or error, surface it directly. | ||
| if isinstance(response, dict) and not response.get("success", True): | ||
| return MCPResponse(**response) | ||
|
|
||
| # If v2 is unavailable (older plugin), fall back to legacy get_editor_state and map. | ||
| if not (isinstance(response, dict) and isinstance(response.get("data"), dict) and response["data"].get("schema_version")): | ||
| legacy = await unity_transport.send_with_unity_instance( | ||
| async_send_command_with_retry, | ||
| unity_instance, | ||
| "get_editor_state", | ||
| {}, | ||
| ) | ||
| if isinstance(legacy, dict) and not legacy.get("success", True): | ||
| return MCPResponse(**legacy) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find MCPResponse class definition
rg -n "class MCPResponse" --type py -A 15Repository: dsarno/unity-mcp
Length of output: 1058
🏁 Script executed:
#!/bin/bash
# Search for MCPResponse imports and usage
rg -n "MCPResponse" --type py -l | head -20Repository: dsarno/unity-mcp
Length of output: 939
🏁 Script executed:
#!/bin/bash
# Check for Pydantic model configuration in the file containing MCPResponse
fd . --type f -name "*.py" | xargs rg -l "MCPResponse" | head -5Repository: dsarno/unity-mcp
Length of output: 539
🏁 Script executed:
#!/bin/bash
# Get the full MCPResponse definition with proper context
cat -n Server/src/models/models.py | head -50Repository: dsarno/unity-mcp
Length of output: 1985
🏁 Script executed:
#!/bin/bash
# Check for Pydantic version and imports
rg -n "pydantic|ConfigDict|model_config" Server/src/models/models.py -B 2 -A 2Repository: dsarno/unity-mcp
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Check project dependencies for Pydantic version
cat pyproject.toml | grep -i pydanticRepository: dsarno/unity-mcp
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Find pyproject.toml in Server directory
find . -name "pyproject.toml" -type fRepository: dsarno/unity-mcp
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Find requirements files
find . -name "requirements*.txt" -type fRepository: dsarno/unity-mcp
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for Pydantic version specification anywhere
rg -n "pydantic.*==" --type toml --type txtRepository: dsarno/unity-mcp
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Read Server/pyproject.toml to find Pydantic version
cat Server/pyproject.tomlRepository: dsarno/unity-mcp
Length of output: 1853
🏁 Script executed:
#!/bin/bash
# Double-check MCPResponse for any ConfigDict or model_config we might have missed
rg -n "MCPResponse" Server/src/models/models.py -A 20 -B 5Repository: dsarno/unity-mcp
Length of output: 819
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or error handling related to MCPResponse validation
rg -n "MCPResponse\|ValidationError" --type py -l | head -10Repository: dsarno/unity-mcp
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check how responses are actually constructed from Unity in the code
rg -n "\*\*response" Server/src/services/resources/editor_state_v2.py -B 3 -A 1Repository: dsarno/unity-mcp
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Search for test fixtures or mock responses to see what fields Unity actually returns
rg -n "success.*message.*error\|MCPResponse\|mock.*response" --type py Server/tests/ -B 2 -A 2 | head -80Repository: dsarno/unity-mcp
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check integration tests for actual response patterns
cat Server/tests/integration/test_refresh_unity_retry_recovery.py 2>/dev/null | head -100Repository: dsarno/unity-mcp
Length of output: 1925
🏁 Script executed:
#!/bin/bash
# Look at how responses from Unity are structured elsewhere in the codebase
rg -n "isinstance.*dict.*response\|response\[.*\]" Server/src/services/ --type py -B 1 -A 1 | head -60Repository: dsarno/unity-mcp
Length of output: 42
Add explicit extra field handling to MCPResponse model.
Lines 200 and 211 unpack response dicts directly: MCPResponse(**response). With Pydantic v2 (specified in pyproject.toml as >=2.12.5), the default behavior is extra="forbid", which will raise a ValidationError if Unity returns any undocumented response fields. Add model_config = ConfigDict(extra="ignore") to the MCPResponse class definition in Server/src/models/models.py to gracefully handle unexpected fields from the Unity plugin.
🤖 Prompt for AI Agents
In Server/src/services/resources/editor_state_v2.py around lines 198 to 211, the
code calls MCPResponse(**response) which will raise a Pydantic v2
ValidationError if Unity returns extra unexpected fields; update the MCPResponse
model in Server/src/models/models.py to allow unknown fields by adding
model_config = ConfigDict(extra="ignore") (import ConfigDict from pydantic) so
unpacking arbitrary response dicts succeeds, then run tests/flake to ensure no
import or typing issues.
Greptile SummaryThis PR implements a comprehensive editor readiness system (v2) with external change detection and hardened refresh retry logic. Major Changes:
Key Benefits:
Testing: Strong test coverage including contract validation, external changes scanning (with file: dependencies), and disconnect recovery scenarios. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Server as Python Server
participant ESV2 as editor_state_v2.py
participant Scanner as external_changes_scanner
participant Preflight as preflight.py
participant Tool as Tool (manage_asset/gameobject/scene)
participant Refresh as refresh_unity.py
participant Unity as Unity Editor
participant Cache as EditorStateCache.cs
participant RefreshTool as RefreshUnity.cs
Note over Unity,Cache: Unity-side caching (new)
Unity->>Cache: EditorApplication.update events
Cache->>Cache: BuildSnapshot() every 250ms
Cache->>Cache: Lock & store v2 state
Note over Client,Server: Client polls readiness
Client->>Server: get resource: editor_state_v2
Server->>ESV2: get_editor_state_v2(ctx)
ESV2->>Unity: send "get_editor_state_v2"
Unity->>Cache: GetSnapshot()
Cache-->>Unity: Cached v2 snapshot
Unity-->>ESV2: v2 state data
ESV2->>Scanner: update_and_get(instance_id)
Scanner->>Scanner: Scan Assets/Packages mtime
Scanner-->>ESV2: external_changes_dirty status
ESV2->>ESV2: _enrich_advice_and_staleness()
ESV2-->>Server: MCPResponse with advice & staleness
Server-->>Client: ready_for_tools, blocking_reasons
Note over Client,Tool: Client invokes tool when ready
Client->>Server: call tool: manage_asset
Server->>Tool: manage_asset(ctx, params)
Tool->>Preflight: preflight(wait_for_no_compile, refresh_if_dirty)
Preflight->>ESV2: get_editor_state_v2(ctx)
ESV2-->>Preflight: Check external_changes_dirty=true
alt External changes detected
Preflight->>Refresh: refresh_unity(mode="if_dirty", wait_for_ready=True)
Refresh->>Unity: send "refresh_unity"
Unity->>RefreshTool: HandleCommand()
RefreshTool->>RefreshTool: AssetDatabase.Refresh()
RefreshTool->>RefreshTool: WaitForUnityReadyAsync()
alt Unity disconnects during refresh
Unity-->>Refresh: {success: false, hint: "retry"}
Refresh->>Refresh: Poll editor_state_v2 until ready
Refresh->>ESV2: get_editor_state_v2(ctx)
ESV2-->>Refresh: ready_for_tools=true
end
RefreshTool-->>Unity: Refresh complete
Unity-->>Refresh: Success response
Refresh->>Scanner: clear_dirty(instance_id)
Refresh-->>Preflight: Refresh succeeded
end
Preflight->>ESV2: Check compilation status
alt Still compiling
Preflight->>Preflight: Wait loop (max 30s)
Preflight->>ESV2: Poll editor_state_v2
ESV2-->>Preflight: is_compiling status
end
Preflight-->>Tool: None (proceed) or busy MCPResponse
alt Preflight blocks
Tool-->>Server: MCPResponse busy/retry
Server-->>Client: Error with retry_after_ms
else Preflight allows
Tool->>Unity: send tool command
Unity-->>Tool: Tool result
Tool-->>Server: Success response
Server-->>Client: Tool result
end
|
… stall prevention - Add TestRunnerNoThrottle.cs: Sets editor to 'No Throttling' mode during test runs with SessionState persistence across domain reload - Add run_tests_async and get_test_job tools for non-blocking test execution - Add TestJobManager for async test job tracking with progress monitoring - Add ForceSynchronousImport to all AssetDatabase.Refresh() calls to prevent stalls - Mark DomainReloadResilienceTests as [Explicit] with documentation explaining the test infrastructure limitation (internal coroutine waits vs MCP socket polling) - MCP workflow is unaffected - socket messages provide external stimulus that keeps Unity responsive even when backgrounded
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
MCPForUnity/Editor/Services/EditorStateCache.cs (1)
183-194: The placeholder fields (external_changes_dirty,external_changes_last_seen_unix_ms, andrefreshfields) were already flagged in a previous review as hardcoded with no update mechanism. This remains unaddressed but has already been noted.
🧹 Nitpick comments (6)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (1)
60-76: Cleanup logic is correct but slightly redundant.The
TearDownlogic individually deletes_createdAssetPath(lines 60-63) before deleting_runRoot(lines 73-76). Since all test assets are created under_runRoot, the individual deletion of_createdAssetPathis redundant—deleting_runRootrecursively handles it. However, this explicit approach is defensive and ensures clarity, so it's a reasonable trade-off for maintainability.Note: The materials (
_matAPath,_matBPath) correctly require individual deletion since they're created at theTempRootlevel, not under_runRoot.MCPForUnity/Editor/Services/TestRunnerService.cs (1)
266-293: Consider returningnullinstead of0on error to distinguish "unknown" from "zero tests".When an exception occurs during traversal, returning
0conflates "error occurred" with "no leaf tests found." SinceRunStartedalready handlesnullas the unknown case (line 182), this helper should returnnullon failure for consistency.🔎 Proposed fix
- private static int CountLeafTests(ITestAdaptor node) + private static int? CountLeafTests(ITestAdaptor node) { if (node == null) { - return 0; + return null; } if (!node.HasChildren) { return 1; } int total = 0; try { foreach (var child in node.Children) { - total += CountLeafTests(child); + var childCount = CountLeafTests(child); + if (!childCount.HasValue) + { + return null; + } + total += childCount.Value; } } catch { // If Unity changes the adaptor behavior, treat it as "unknown total". - return 0; + return null; } return total; }MCPForUnity/Editor/Services/EditorStateCache.cs (1)
168-173:since_unix_msalways equalsobserved_at_unix_ms, which may not reflect actual phase start time.The
activity.since_unix_msfield is set to_observedUnixMs(the current snapshot time), but for meaningful phase tracking, it should capture when the current phase actually started (e.g., when compilation began).This would require tracking phase transition timestamps separately, which could be deferred to a future iteration if phase duration tracking is not immediately needed.
MCPForUnity/Editor/Tools/RunTestsAsync.cs (1)
70-72: String-based exception detection is fragile.Checking
ex.Message.IndexOf("already in progress", ...)is brittle and could break if the exception message wording changes. Consider catching a specific exception type or adding a dedicated exception class.🔎 Proposed approach
Define a custom exception type (e.g.,
TestRunAlreadyInProgressException) inTestJobManager.StartJoband catch it explicitly:catch (TestRunAlreadyInProgressException) { return Task.FromResult<object>(new ErrorResponse("tests_running", new { reason = "tests_running", retry_after_ms = 5000 })); } catch (Exception ex) { return Task.FromResult<object>(new ErrorResponse($"Failed to start test job: {ex.Message}")); }Server/src/services/tools/test_jobs.py (1)
64-66: Consider extracting duplicated response handling logic.Both
run_tests_asyncandget_test_jobhave identical response normalization logic. This could be extracted to a helper function for DRY compliance.🔎 Proposed helper
def _normalize_response(response: Any) -> dict[str, Any] | MCPResponse: if isinstance(response, dict) and not response.get("success", True): return MCPResponse(**response) return response if isinstance(response, dict) else MCPResponse(success=False, error=str(response)).model_dump()Then use
return _normalize_response(response)in both functions.Also applies to: 90-92
MCPForUnity/Editor/Services/TestJobManager.cs (1)
297-316: Clarify:Succeededmeans job completion, not test pass rate.The status is set to
Succeededregardless of test failures. This is likely intentional (the job completed, even if tests failed), but consider adding a brief comment to clarify thatSucceededindicates successful job completion, with actual test results available inResult.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
MCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Services/TestJobManager.csMCPForUnity/Editor/Services/TestJobManager.cs.metaMCPForUnity/Editor/Services/TestRunnerNoThrottle.csMCPForUnity/Editor/Services/TestRunnerNoThrottle.cs.metaMCPForUnity/Editor/Services/TestRunnerService.csMCPForUnity/Editor/Tools/GetTestJob.csMCPForUnity/Editor/Tools/GetTestJob.cs.metaMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/ManageScript.csMCPForUnity/Editor/Tools/ManageShader.csMCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csMCPForUnity/Editor/Tools/RefreshUnity.csMCPForUnity/Editor/Tools/RunTestsAsync.csMCPForUnity/Editor/Tools/RunTestsAsync.cs.metaServer/src/services/tools/test_jobs.pyServer/tests/integration/test_test_jobs_async.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/DomainReloadResilienceTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs
✅ Files skipped from review due to trivial changes (4)
- MCPForUnity/Editor/Services/TestJobManager.cs.meta
- MCPForUnity/Editor/Tools/GetTestJob.cs.meta
- MCPForUnity/Editor/Tools/RunTestsAsync.cs.meta
- MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs.meta
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:19:59.258Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 85
File: tests/test_telemetry_queue_worker.py:33-37
Timestamp: 2025-10-21T05:19:59.258Z
Learning: In the unity-mcp repository, pyproject.toml is located in MCPForUnity/UnityMcpServer~/src/, not at the repository root. When testing code that imports telemetry.py, the working directory should be changed to SRC (MCPForUnity/UnityMcpServer~/src/) so that telemetry.py's get_package_version() can find pyproject.toml via relative path.
Applied to files:
Server/src/services/tools/test_jobs.py
🧬 Code graph analysis (6)
MCPForUnity/Editor/Tools/RunTestsAsync.cs (3)
MCPForUnity/Editor/Tools/GetTestJob.cs (2)
McpForUnityTool(11-51)HandleCommand(14-50)MCPForUnity/Editor/Resources/Tests/GetTests.cs (2)
ModeParser(77-105)TryParse(79-104)MCPForUnity/Editor/Services/TestJobManager.cs (3)
TestJobManager(49-571)TestJobManager(64-68)StartJob(234-295)
MCPForUnity/Editor/Tools/GetTestJob.cs (2)
MCPForUnity/Editor/Services/TestJobManager.cs (3)
TestJobManager(49-571)TestJobManager(64-68)ToSerializable(422-460)MCPForUnity/Editor/Services/TestRunnerService.cs (3)
ToSerializable(454-482)ToSerializable(531-542)ToSerializable(573-585)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinition.cs (1)
ManageScriptableObjectTestDefinition(14-23)MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
HandleCommand(42-77)
Server/tests/integration/test_test_jobs_async.py (2)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)Server/src/services/tools/test_jobs.py (2)
run_tests_async(17-66)get_test_job(70-92)
MCPForUnity/Editor/Services/EditorStateCache.cs (3)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Error(48-51)MCPForUnity/Editor/Services/TestRunStatus.cs (1)
TestRunStatus(10-59)MCPForUnity/Editor/Services/TestJobManager.cs (2)
TestJobManager(49-571)TestJobManager(64-68)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/DomainReloadResilienceTests.cs (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (1)
TearDown(56-90)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs (1)
TearDown(46-70)
🪛 Ruff (0.14.10)
Server/tests/integration/test_test_jobs_async.py
12-12: Unused function argument: send_fn
(ARG001)
12-12: Unused function argument: unity_instance
(ARG001)
12-12: Unused function argument: kwargs
(ARG001)
39-39: Unused function argument: send_fn
(ARG001)
39-39: Unused function argument: unity_instance
(ARG001)
39-39: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (43)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (4)
19-20: Excellent per-run isolation pattern.The introduction of unique per-run directories (
_runRoot) with GUID-based naming effectively eliminates reimport churn that would occur from deleting and recreating the entireTempRootbetween tests. The computed_nestedFolderpath provides good test coverage for deep folder hierarchies.Also applies to: 31-36
93-121: Well-focused test for nested folder creation.Splitting this concern into its own test improves clarity and makes test failures easier to diagnose. The assertions thoroughly validate:
- Nested folder structure is created (line 114)
- Asset placement under the correct hierarchy (line 115)
- Asset file extension (line 116)
- Asset can be loaded with the correct type (lines 119-120)
123-160: Good separation of concerns with lightweight path strategy.Using
_runRootinstead of_nestedFolderfor the patch test (line 131) is a sensible optimization—it keeps this test focused on patch correctness without the overhead of nested folder creation. The comment on line 130 clearly explains the rationale.The patch validation assertions (lines 157-159) cover private fields, inherited fields, and nested struct properties, providing comprehensive test coverage.
170-170: Correct usage of per-run isolation.Updating this test to use
_runRootmaintains consistency with the new isolation pattern.MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
585-597: Synchronous refresh after prefab folder creation is reasonable and consistentForcing a synchronous AssetDatabase.Refresh right after creating the directory ensures the asset DB definitely sees the new folder before
SaveAsPrefabAssetAndConnect, which matches the rest of the PR’s “deterministic refresh” story. Given this path is user/tool‑triggered (not hot‑loop), the extra blocking refresh is acceptable.MCPForUnity/Editor/Tools/ManageScene.cs (1)
220-226: Synchronous refresh for new/updated scenes and screenshots aligns with deterministic editor stateUsing
AssetDatabase.Refresh(ImportAssetOptions.ForceSynchronousImport)after scene creation, scene save, and screenshot capture makes the new/changed assets immediately visible to tooling that inspects the project state right after these operations. Given these are explicit editor commands (not per‑frame), the extra blocking cost is an acceptable tradeoff for the more predictable state your v2 editor‑state and refresh workflows need.Also applies to: 363-369, 382-423
MCPForUnity/Editor/Tools/ManageScript.cs (1)
975-985: Immediate refresh after moving script to trash keeps script state consistent with other flowsCalling
AssetDatabase.Refresh(ImportAssetOptions.ForceSynchronousImport)right afterMoveAssetToTrashmakes the deletion visible to the asset DB and compilation pipeline immediately, matching the synchronous import semantics you use for apply_text_edits and structured edits. This keeps downstream editor‑state / refresh tooling from briefly seeing stale scripts.MCPForUnity/Editor/Tools/ManageAsset.cs (2)
180-184: LGTM! Synchronous refresh ensures directory recognition.The forced synchronous import after directory creation ensures Unity's AssetDatabase recognizes the new folder before subsequent asset operations, reducing timing-related failures.
864-874: LGTM! Consistent synchronous refresh pattern.The forced synchronous import ensures Unity recognizes the new directory immediately, maintaining consistency with the pattern applied at line 183.
MCPForUnity/Editor/Tools/ManageShader.cs (3)
89-106: LGTM! Synchronous refresh after directory creation.The forced synchronous import ensures Unity recognizes the new shader directory before proceeding with create/update operations.
173-187: LGTM! Ensures shader is immediately available.The synchronous refresh after shader creation ensures Unity fully imports and recognizes the shader, making it immediately available for subsequent operations like
Shader.Find().
241-255: LGTM! Consistent synchronous refresh on shader update.The forced synchronous import ensures shader changes are immediately reflected in Unity's AssetDatabase, maintaining consistency with the CreateShader pattern.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/DomainReloadResilienceTests.cs (4)
15-24: LGTM! Clear documentation of test infrastructure limitation.The updated documentation clearly explains that these tests are marked
[Explicit]due to test infrastructure limitations (internal Unity coroutine waits stall when backgrounded), not issues with the MCP workflow itself. This helps maintainers understand the context.
29-38: LGTM! Synchronous refresh in test setup.The forced synchronous import ensures the temp directory is fully recognized by Unity before tests execute, preventing timing-related failures.
40-63: Excellent! Critical synchronous refresh in TearDown.The forced synchronous refresh at the end of TearDown is critical for preventing compilation from running into subsequent tests. The clear comment explains this prevents stalling other tests, which is especially important given these tests intentionally trigger domain reloads.
85-87: LGTM! Synchronous refresh after script creation in tests.The forced synchronous imports after script file creation ensure Unity immediately starts compilation and recognizes the new scripts. This is correct for these domain reload resilience tests, which need deterministic timing for their assertions.
Also applies to: 177-178, 225-226
MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs (1)
214-228: LGTM! Consistent synchronous refresh pattern for prefab directories.The forced synchronous import ensures Unity recognizes the prefab directory immediately before attempting to save prefab assets, maintaining consistency with the pattern applied across ManageAsset.cs and ManageShader.cs.
MCPForUnity/Editor/Tools/GetTestJob.cs (3)
14-20: LGTM! Flexible parameter name handling.Supporting both
job_idandjobIdparameter names provides good flexibility for API consumers while maintaining clear error messaging.
22-40: LGTM! Lenient boolean parsing with safe defaults.The boolean parsing is intentionally lenient (silently ignoring parse failures) and defaults to
falsefor both flags. This provides good API robustness for optional parameters.
42-50: LGTM! Clean delegation to TestJobManager.The tool appropriately delegates job retrieval and serialization to
TestJobManager, maintaining good separation of concerns. Error handling for unknown job IDs is clear and user-friendly.MCPForUnity/Editor/Services/TestRunnerService.cs (3)
96-97: LGTM - Early state marking ensures consistency.Marking the test run as started before
_testRunnerApi.Execute()ensures the readiness snapshot reflects the busy state even if callbacks are delayed. This is a good defensive pattern.
170-183: LGTM - Robust callback handling with fallback.The try-catch around leaf test counting with a null fallback on failure is appropriate since the adaptor API may change. This ensures the run proceeds even if counting fails.
229-260: LGTM - Defensive failure detection with nested error handling.The nested try-catch for outcome parsing and the outer try-catch for reporting isolate adaptor quirks from the main test result collection. The string-based failure detection (
"failed"or"error") handles NUnit's adaptor states appropriately.MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs (2)
72-87: LGTM - Idempotent settings capture.The
AreSettingsCaptured()guard prevents overwriting the original settings ifApplyNoThrottlingis called multiple times (e.g., after domain reload recovery).
102-116: LGTM - Reflection fallback is appropriately guarded.Using reflection on an internal Unity method is fragile, but the empty catch ensures the feature degrades gracefully if Unity's internals change.
Server/tests/integration/test_test_jobs_async.py (1)
12-15: Unused parameters in mock functions are intentional and correct.The static analysis hints about unused
send_fn,unity_instance, andkwargsparameters are false positives. These mock functions must match the signature of the realsend_with_unity_instancefunction to work correctly withmonkeypatch.setattr.Also applies to: 39-42
MCPForUnity/Editor/Services/EditorStateCache.cs (3)
35-63: LGTM - Robust initialization with event subscriptions.The static constructor correctly subscribes to editor lifecycle events and handles initialization failures gracefully. The domain reload tracking via
beforeAssemblyReload/afterAssemblyReloadensures accurate state across reloads.
65-81: LGTM - Smart throttling with edge-triggered updates.The throttle logic correctly bypasses the interval check when compilation state changes, ensuring timestamps remain meaningful while reducing overhead during idle periods.
219-230: LGTM - Thread-safe snapshot access with defensive cloning.The lock ensures thread-safety and
DeepClone()prevents callers from mutating the cached state. The null check with rebuild is a good defensive measure.MCPForUnity/Editor/Tools/RunTestsAsync.cs (2)
78-124: LGTM - Robust filter options parsing.The
ParseStringArrayhelper correctly handles both single strings and arrays, with proper null/empty filtering. Returningnullwhen no filters are specified avoids unnecessary allocations.
34-53: No action needed. TheincludeDetailsandincludeFailedTestsparameters are correctly handled. These flags control result serialization (used byToSerializable), not job execution. The pattern is: inRunTestsAsync, they're parsed and echoed in the response so the client knows what preferences were set; when the client later callsGetTestJob, it resends these same flags, whichGetTestJobthen passes toToSerializableto format results appropriately. Passing them toStartJobwould be incorrect since job execution parameters are separate from display preferences.MCPForUnity/Editor/Tools/RefreshUnity.cs (2)
129-171: LGTM - Well-structured async polling with proper cleanup.The
WaitForUnityReadyAsyncimplementation correctly:
- Removes itself from
updateon all exit paths (completion, timeout, exception)- Uses
TaskCompletionSourcewithRunContinuationsAsynchronouslyto avoid deadlocks- Includes a
QueuePlayerLoopUpdatenudge for throttled editors
41-48: LGTM - Early exit prevents refresh during test runs.Returning early with retry guidance when tests are running prevents disrupting the test run with asset database refreshes.
Server/src/services/tools/test_jobs.py (2)
16-66: LGTM - Well-structured async test runner with proper preflight.The function correctly:
- Gates on preflight checks before starting tests
- Coerces filter parameters to list format with whitespace handling
- Only includes optional params when they have meaningful values
69-92: LGTM - Clean job polling implementation.The
get_test_jobfunction is straightforward with proper parameter handling and response normalization.MCPForUnity/Editor/Services/TestJobManager.cs (8)
1-44: LGTM!The data structures are well-defined.
TestJobcaptures comprehensive state for tracking async test runs including progress, failures, and timing information.
46-84: LGTM!Good use of
SessionStatefor domain reload survival. Thread-safe property access with consistent locking on shared state.
86-232: LGTM!Persistence logic is well-designed with best-effort semantics. The separation of taking a snapshot under lock and serializing outside the lock is correct. Legacy key migration is a nice touch.
277-295: LGTM!Good defensive pattern: the task continuation acts as a safety net while the primary finalization happens via
RunFinishedcallback.FinalizeFromTaskcorrectly checks if the job was already finalized before modifying state.
318-408: LGTM!Progress callbacks are well-implemented with consistent locking, early exits for missing jobs, and proper state persistence. The failure cap is correctly enforced.
410-460: LGTM!Clean serialization with comprehensive diagnostic fields (
stuck_suspected,editor_is_focused,blocked_reason). This will be helpful for debugging stalled test runs.
462-507: LGTM!Good diagnostic helpers. The stuck detection with a 60-second threshold and the blocked reason categorization will help identify why tests may be stalling.
509-571: LGTM!
FinalizeFromTaskcorrectly guards against double finalization by checking if the job is stillRunning. The exception unwrapping viaGetBaseException()properly handlesAggregateExceptionfrom tasks.
| static TestRunnerNoThrottle() | ||
| { | ||
| try | ||
| { | ||
| var api = ScriptableObject.CreateInstance<TestRunnerApi>(); | ||
| api.RegisterCallbacks(new TestCallbacks()); | ||
|
|
||
| // Check if recovering from domain reload during an active test run | ||
| if (IsTestRunActive()) | ||
| { | ||
| McpLog.Info("[TestRunnerNoThrottle] Recovered from domain reload - reapplying No Throttling."); | ||
| ApplyNoThrottling(); | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| McpLog.Warn($"[TestRunnerNoThrottle] Failed to register callbacks: {e}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
TestRunnerApi instance is created but never disposed.
The TestRunnerApi created on line 43 is a ScriptableObject that should be disposed to avoid resource leaks. Consider storing a reference and implementing cleanup, or use a static lifetime pattern with HideFlags.HideAndDontSave.
🔎 Proposed fix
+ private static TestRunnerApi _api;
+
static TestRunnerNoThrottle()
{
try
{
- var api = ScriptableObject.CreateInstance<TestRunnerApi>();
- api.RegisterCallbacks(new TestCallbacks());
+ _api = ScriptableObject.CreateInstance<TestRunnerApi>();
+ _api.hideFlags = HideFlags.HideAndDontSave;
+ _api.RegisterCallbacks(new TestCallbacks());
// Check if recovering from domain reload during an active test run
if (IsTestRunActive())📝 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.
| static TestRunnerNoThrottle() | |
| { | |
| try | |
| { | |
| var api = ScriptableObject.CreateInstance<TestRunnerApi>(); | |
| api.RegisterCallbacks(new TestCallbacks()); | |
| // Check if recovering from domain reload during an active test run | |
| if (IsTestRunActive()) | |
| { | |
| McpLog.Info("[TestRunnerNoThrottle] Recovered from domain reload - reapplying No Throttling."); | |
| ApplyNoThrottling(); | |
| } | |
| } | |
| catch (Exception e) | |
| { | |
| McpLog.Warn($"[TestRunnerNoThrottle] Failed to register callbacks: {e}"); | |
| } | |
| } | |
| private static TestRunnerApi _api; | |
| static TestRunnerNoThrottle() | |
| { | |
| try | |
| { | |
| _api = ScriptableObject.CreateInstance<TestRunnerApi>(); | |
| _api.hideFlags = HideFlags.HideAndDontSave; | |
| _api.RegisterCallbacks(new TestCallbacks()); | |
| // Check if recovering from domain reload during an active test run | |
| if (IsTestRunActive()) | |
| { | |
| McpLog.Info("[TestRunnerNoThrottle] Recovered from domain reload - reapplying No Throttling."); | |
| ApplyNoThrottling(); | |
| } | |
| } | |
| catch (Exception e) | |
| { | |
| McpLog.Warn($"[TestRunnerNoThrottle] Failed to register callbacks: {e}"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs around lines 39-57, the
TestRunnerApi ScriptableObject created in the static constructor is never stored
or cleaned up; store it in a private static TestRunnerApi field, set
api.hideFlags = HideFlags.HideAndDontSave to avoid accidental scene persistence,
and register a cleanup callback (e.g., AssemblyReloadEvents.beforeAssemblyReload
or EditorApplication.quitting) that calls DestroyImmediate on the stored
instance and nulls the field so the ScriptableObject is properly disposed on
domain reload/editor quit; ensure any registration of callbacks uses the stored
api instance.
| if (string.Equals(scope, "scripts", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| // For scripts, requesting compilation is usually the meaningful action. | ||
| // We avoid a heavyweight full refresh by default. | ||
| } | ||
| else | ||
| { | ||
| AssetDatabase.Refresh(ImportAssetOptions.ForceUpdate | ImportAssetOptions.ForceSynchronousImport); | ||
| refreshTriggered = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The scripts scope branch is a no-op with only a comment.
When scope is "scripts", lines 61-65 do nothing, then the fallback at lines 79-86 triggers a full refresh anyway (since refreshTriggered is false and scope is not "all"). This seems unintended. If scripts-only refresh should skip asset refresh, the fallback condition needs adjustment.
🔎 Proposed fix to skip fallback refresh for scripts scope
- if (string.Equals(scope, "all", StringComparison.OrdinalIgnoreCase) && !refreshTriggered)
+ if (!refreshTriggered && !string.Equals(scope, "scripts", StringComparison.OrdinalIgnoreCase))
{Or, if scripts scope should trigger compilation by default:
if (string.Equals(scope, "scripts", StringComparison.OrdinalIgnoreCase))
{
// For scripts, requesting compilation is usually the meaningful action.
- // We avoid a heavyweight full refresh by default.
+ CompilationPipeline.RequestScriptCompilation();
+ compileRequested = true;
}Committable suggestion skipped: line range outside the PR's diff.
| async def test_run_tests_async_forwards_params(monkeypatch): | ||
| from services.tools.test_jobs import run_tests_async | ||
|
|
||
| captured = {} | ||
|
|
||
| async def fake_send_with_unity_instance(send_fn, unity_instance, command_type, params, **kwargs): | ||
| captured["command_type"] = command_type | ||
| captured["params"] = params | ||
| return {"success": True, "data": {"job_id": "abc123", "status": "running"}} | ||
|
|
||
| import services.tools.test_jobs as mod | ||
| monkeypatch.setattr(mod.unity_transport, "send_with_unity_instance", fake_send_with_unity_instance) | ||
|
|
||
| resp = await run_tests_async( | ||
| DummyContext(), | ||
| mode="EditMode", | ||
| test_names="MyNamespace.MyTests.TestA", | ||
| include_details=True, | ||
| ) | ||
| assert captured["command_type"] == "run_tests_async" | ||
| assert captured["params"]["mode"] == "EditMode" | ||
| assert captured["params"]["testNames"] == ["MyNamespace.MyTests.TestA"] | ||
| assert captured["params"]["includeDetails"] is True | ||
| assert resp["success"] is True |
There was a problem hiding this comment.
Test may fail due to unmocked preflight dependency.
The run_tests_async function calls preflight(ctx, ...) before the transport call. Without mocking preflight, the test may fail or have unintended side effects. Consider mocking the preflight to return a non-MCPResponse value.
🔎 Proposed fix
@pytest.mark.asyncio
async def test_run_tests_async_forwards_params(monkeypatch):
from services.tools.test_jobs import run_tests_async
captured = {}
async def fake_send_with_unity_instance(send_fn, unity_instance, command_type, params, **kwargs):
captured["command_type"] = command_type
captured["params"] = params
return {"success": True, "data": {"job_id": "abc123", "status": "running"}}
+ async def fake_preflight(ctx, **kwargs):
+ return None # Non-MCPResponse indicates success
+
import services.tools.test_jobs as mod
monkeypatch.setattr(mod.unity_transport, "send_with_unity_instance", fake_send_with_unity_instance)
+ monkeypatch.setattr(mod, "preflight", fake_preflight)
resp = await run_tests_async(
DummyContext(),🧰 Tools
🪛 Ruff (0.14.10)
12-12: Unused function argument: send_fn
(ARG001)
12-12: Unused function argument: unity_instance
(ARG001)
12-12: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
In Server/tests/integration/test_test_jobs_async.py around lines 7 to 30, the
test calls run_tests_async which invokes preflight(ctx, ...); since preflight is
not mocked the test may fail or produce side effects—mock preflight via
monkeypatch to return a simple non-MCPResponse (e.g. a dict or None) before
calling run_tests_async so the preflight path is bypassed; place the
monkeypatch.setattr for services.tools.test_jobs.preflight near where
unity_transport.send_with_unity_instance is patched and ensure the fake
preflight signature matches the real one so the test continues to assert the
forwarded params.
- Remove unused Newtonsoft.Json.Linq import from TestJobManager - Add throttling to SessionState persistence (once per second) to reduce overhead - Critical job state changes (start/finish) still persist immediately - Fix duplicate XML summary tag in DomainReloadResilienceTests
- Add run_tests_async and get_test_job to main README tools list - Document background stall limitation for domain reload tests in DEV readme
Run [Explicit] domain_reload tests in their own job using -testCategory
Combines into single job with two test steps to reuse cached Library
- Fix TOCTOU race in TestJobManager.StartJob (single lock scope for check-and-set) - Store TestRunnerApi reference with HideAndDontSave to prevent GC/serialization issues
- run_tests_async is now marked as preferred for long-running suites - run_tests description notes it blocks and suggests async alternative
…sults - manage_asset, manage_gameobject, manage_scene now check preflight return value and propagate busy/retry signals to clients (fixes Sourcery #1) - TestJobManager.FinalizeCurrentJobFromRunFinished now sets job status to Failed when resultPayload.Failed > 0, not always Succeeded (fixes Sourcery #2)
When 'Force fresh server install' is enabled, uvx uses --no-cache --refresh which rebuilds the package and takes significantly longer to start. - Increase timeout from 10s to 45s when dev mode is enabled - Add informative log message explaining the longer startup time - Show actual timeout value in warning message
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs (1)
42-61: Missing cleanup callback forTestRunnerApidisposal.While the static field and
HideFlags.HideAndDontSavehave been added (addressing part of the earlier feedback), there's still no cleanup callback to callDestroyImmediate(_api)on editor quit or assembly reload. Without this, the ScriptableObject may leak.Consider adding a cleanup hook:
AssemblyReloadEvents.beforeAssemblyReload += () => { if (_api != null) { ScriptableObject.DestroyImmediate(_api); _api = null; } };
🧹 Nitpick comments (2)
MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs (1)
106-120: Silent failure is acceptable, but consider debug logging.The silent catch is reasonable for production robustness. However, if the reflection fails (e.g., Unity API changes in a future version), debugging could be difficult.
Consider adding a debug log when the method isn't found:
if (method == null) { McpLog.Debug("[TestRunnerNoThrottle] UpdateInteractionModeSettings not found - interaction mode may not apply immediately."); }README.md (1)
19-19: Minor: Missing period after "etc" in American English.Static analysis suggests adding a period after "etc" for consistency with American English conventions:
(Claude, Cursor, Antigravity, VS Code, etc.)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/images/unity-mcp-ui-v8.6.pngis excluded by!**/*.png
📒 Files selected for processing (11)
.github/workflows/unity-tests.ymlMCPForUnity/Editor/Services/TestJobManager.csMCPForUnity/Editor/Services/TestRunnerNoThrottle.csREADME.mdServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/run_tests.pyServer/src/services/tools/test_jobs.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/DomainReloadResilienceTests.csdocs/README-DEV.md
🚧 Files skipped from review as they are similar to previous changes (1)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/DomainReloadResilienceTests.cs
🧰 Additional context used
🧬 Code graph analysis (5)
Server/src/services/tools/manage_scene.py (2)
Server/src/services/tools/preflight.py (1)
preflight(27-105)Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Info(37-41)Warn(43-46)
Server/src/services/tools/manage_asset.py (2)
Server/src/services/tools/preflight.py (1)
preflight(27-105)Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
Server/src/services/tools/test_jobs.py (4)
Server/src/services/tools/preflight.py (1)
preflight(27-105)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(753-785)Server/src/services/tools/run_tests.py (1)
_coerce_string_list(66-74)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)
Server/src/services/tools/run_tests.py (1)
Server/src/services/tools/preflight.py (1)
preflight(27-105)
🪛 LanguageTool
README.md
[style] ~19-~19: In American English, abbreviations like “etc.” require a period.
Context: ... (Claude, Cursor, Antigravity, VS Code, etc) to interact directly with your Unity E...
(ETC_PERIOD)
🔇 Additional comments (15)
docs/README-DEV.md (1)
340-348: Well-documented workaround for a real OS-level limitation.The new section clearly explains the macOS background-threading throttle that can stall domain reload tests, provides three practical workarounds, and reassures developers that the core MCP workflow is unaffected. This aligns well with the PR's investment in test tooling and refresh resilience, and will save developers debugging time.
Server/src/services/tools/manage_gameobject.py (2)
11-11: LGTM! Import added correctly.The import for
preflightis properly added and supports the new preflight validation logic.
96-98: Excellent! Critical issue resolved.The preflight return value is now properly captured and handled. When Unity is busy (compiling, running tests, etc.), the function correctly returns the gate response early, preventing operations during unsafe states. This addresses the previously flagged critical issue.
Server/src/services/tools/manage_scene.py (1)
9-9: Excellent fix: Critical issue resolved.The preflight return value is now properly captured and checked. When Unity is busy (gate is non-None), the function correctly returns the busy/retry signal, preventing the tool from executing prematurely. This addresses the critical issue raised in the previous review.
Also applies to: 44-46
.github/workflows/unity-tests.yml (1)
67-71: AI summary inconsistency: Upload step still present.The AI-generated summary states "Removed the Upload test results step from the workflow," but this upload-artifact step is still present in the code. The actual code takes precedence.
MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs (4)
63-74: LGTM!Clean helper abstractions over
SessionStatefor persisting throttle state across domain reloads.
76-91: LGTM!The guard
AreSettingsCaptured()correctly prevents overwriting previously saved settings during domain reload recovery.
93-104: LGTM!Restoration logic correctly clears both state flags after restoring the original throttling settings.
122-137: LGTM!Clean implementation of
ICallbackswith appropriate lifecycle hooks.Server/src/services/tools/run_tests.py (2)
61-63: LGTM: Preflight gating prevents invalid test runs.The preflight check with
requires_no_tests=Trueensures Unity is ready and no tests are already running before starting a new test run. This aligns well with the PR's goal of hardening retry logic.
106-118: LGTM: Structured error handling prevents retry loops.The specialized handling for "test run is already in progress" errors returns a structured
MCPResponsewith retry guidance instead of a generic failure. This prevents clients from misinterpreting the error and entering retry loops (addresses issue CoplayDev#503 mentioned in comments).MCPForUnity/Editor/Services/TestJobManager.cs (3)
269-278: LGTM: TOCTOU race condition resolved.The previous review comment flagged a TOCTOU race between the check and set of
_currentJobId. This has been properly addressed by combining the check (line 272) and registration (lines 276-277) within a single lock scope, ensuring atomicity. Well done.
190-242: LGTM: Persistence throttling prevents overhead.The throttling logic (lines 194-198) limits persistence to once per second for non-critical updates while allowing forced persistence for critical state changes. This is a smart optimization for large test runs that would otherwise trigger frequent SessionState writes.
471-500: LGTM: Comprehensive blocked reason detection.The blocked reason logic checks the most common causes of test run delays (editor unfocused, compilation, asset import) and provides actionable feedback to clients. The editor focus check (line 484) is particularly valuable for catching OS-level throttling of background Unity processes.
Server/src/services/tools/manage_asset.py (1)
15-15: LGTM!The import is correctly added and used in the preflight guard below.
| # Run domain reload tests first (they're [Explicit] so need explicit category) | ||
| - name: Run domain reload tests | ||
| uses: game-ci/unity-test-runner@v4 | ||
| id: domain-tests | ||
| env: | ||
| UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} | ||
| UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} | ||
| UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} | ||
| with: | ||
| projectPath: ${{ matrix.projectPath }} | ||
| unityVersion: ${{ matrix.unityVersion }} | ||
| testMode: ${{ matrix.testMode }} | ||
| customParameters: -testCategory domain_reload | ||
|
|
There was a problem hiding this comment.
Domain reload test artifacts are not uploaded.
The new domain reload test step produces artifacts via steps.domain-tests.outputs.artifactsPath, but these are not captured by the upload step (Line 67-71), which only uploads steps.tests.outputs.artifactsPath. Consider uploading both sets of test results for complete CI visibility.
🔎 Proposed fix to upload domain reload test artifacts
Add a separate upload step for domain reload tests:
customParameters: -testCategory domain_reload
+ - uses: actions/upload-artifact@v4
+ if: always()
+ with:
+ name: Test results for ${{ matrix.testMode }} (domain reload)
+ path: ${{ steps.domain-tests.outputs.artifactsPath }}
+
- name: Run tests
uses: game-ci/unity-test-runner@v4🤖 Prompt for AI Agents
.github/workflows/unity-tests.yml around lines 41 to 54: the domain reload test
step (id: domain-tests) produces artifacts but they aren’t uploaded; update the
workflow to capture and upload those artifacts as well by adding an
upload-artifacts step that references steps.domain-tests.outputs.artifactsPath
(either add a new upload step for domain-tests or modify the existing upload
step to upload both steps.tests.outputs.artifactsPath and
steps.domain-tests.outputs.artifactsPath), ensuring the step uses the same
name/path pattern and runs after the domain-tests step.
| # Best-effort guard: if Unity is compiling/reloading or known external changes are pending, | ||
| # wait/refresh to avoid stale reads and flaky timeouts. | ||
| gate = await preflight(ctx, wait_for_no_compile=True, refresh_if_dirty=True) | ||
| if gate is not None: | ||
| return gate.model_dump() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of manage_asset function
rg -nP --type=py -C3 '\bmanage_asset\s*\(' -g '!**/manage_asset.py' | head -100Repository: dsarno/unity-mcp
Length of output: 4818
🏁 Script executed:
#!/bin/bash
# Check for test coverage of the preflight integration with manage_asset
rg -nP --type=py -C2 'preflight|manage_asset' -g '**/test_*.py' -g '**/*_test.py' | head -100Repository: dsarno/unity-mcp
Length of output: 8210
🏁 Script executed:
#!/bin/bash
# Verify MCPResponse class definition and model_dump() method
rg -nP --type=py -A10 'class MCPResponse' | head -50Repository: dsarno/unity-mcp
Length of output: 736
🏁 Script executed:
#!/bin/bash
# Check the preflight function signature and return type
rg -nP --type=py -B2 -A8 'async def preflight' | head -50Repository: dsarno/unity-mcp
Length of output: 735
🏁 Script executed:
#!/bin/bash
# Read lines 40-70 of manage_asset.py to see full context around lines 51-55
sed -n '40,70p' Server/src/services/tools/manage_asset.py | cat -nRepository: dsarno/unity-mcp
Length of output: 2123
🏁 Script executed:
#!/bin/bash
# Check if any tests verify gate != None behavior (preflight returning a response)
rg -nP --type=py 'preflight.*None|gate.*None' Server/tests/Repository: dsarno/unity-mcp
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for test cases that might mock preflight to return a non-None value
rg -nP --type=py -B5 -A5 'preflight.*return|mock.*preflight' Server/tests/Repository: dsarno/unity-mcp
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for any mocking or testing of preflight function itself
rg -nP --type=py 'preflight' Server/tests/ | head -50Repository: dsarno/unity-mcp
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if manage_asset tests stub/mock the preflight call
rg -nP --type=py -B10 -A10 'async def test_manage_asset' Server/tests/integration/ | head -100Repository: dsarno/unity-mcp
Length of output: 1709
🏁 Script executed:
#!/bin/bash
# Check what happens in manage_asset when preflight is called - does it get mocked in tests?
cat -n Server/tests/integration/test_script_tools.py | sed -n '175,220p'Repository: dsarno/unity-mcp
Length of output: 1494
🏁 Script executed:
#!/bin/bash
# Final check: Look at the full manage_asset function signature and return type
sed -n '1,50p' Server/src/services/tools/manage_asset.py | cat -nRepository: dsarno/unity-mcp
Length of output: 3179
🏁 Script executed:
#!/bin/bash
# Verify if there are any test files specifically for preflight behavior
find Server/tests -type f -name '*preflight*' 2>/dev/nullRepository: dsarno/unity-mcp
Length of output: 42
Add test coverage for the preflight gate response path.
The preflight guard implementation is correct and follows a clean early-exit pattern. The gate.model_dump() call properly returns a dict[str, Any] as required, and the parameters (wait_for_no_compile=True, refresh_if_dirty=True) are appropriate for asset operations.
However, the existing tests (test_script_tools.py, test_manage_asset_param_coercion.py, test_manage_asset_json_parsing.py) all mock async_send_command_with_retry and never verify the case where preflight returns a non-None gate response. Add a test case that mocks preflight to return an MCPResponse and confirms the function returns gate.model_dump() without executing the asset operation logic.
🤖 Prompt for AI Agents
In Server/src/services/tools/manage_asset.py around lines 51-55, add a unit test
that covers the early-exit preflight gate path by mocking preflight to return a
populated MCPResponse and asserting the function returns gate.model_dump()
without performing asset operations; specifically, patch preflight to return an
MCPResponse instance (with known fields), call the async entry point (e.g.,
manage_asset) with a test ctx, assert the return equals the
MCPResponse.model_dump() value, and also assert that
async_send_command_with_retry (and any other downstream operation mocks) were
not called; implement this using pytest/asyncio and monkeypatch or
unittest.mock.patch and place the test alongside existing manage_asset tests (or
in a new test_manage_asset_preflight.py).
| def _coerce_string_list(value) -> list[str] | None: | ||
| if value is None: | ||
| return None | ||
| if isinstance(value, str): | ||
| return [value] if value.strip() else None | ||
| if isinstance(value, list): | ||
| result = [str(v).strip() for v in value if v and str(v).strip()] | ||
| return result if result else None | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: Extract _coerce_string_list to a shared utility.
This helper function is duplicated from Server/src/services/tools/run_tests.py (lines 65-73). Consider extracting it to a shared utility module (e.g., services/tools/utils.py) to maintain DRY principles and ensure consistent behavior across tools.
🔎 Suggested refactor
Create or update Server/src/services/tools/utils.py:
def coerce_string_list(value) -> list[str] | None:
"""Convert string or list input to a list of non-empty strings."""
if value is None:
return None
if isinstance(value, str):
return [value] if value.strip() else None
if isinstance(value, list):
result = [str(v).strip() for v in value if v and str(v).strip()]
return result if result else None
return NoneThen import and use in both files:
+from services.tools.utils import coerce_string_list as _coerce_string_list
- def _coerce_string_list(value) -> list[str] | None:
- if value is None:
- return None
- if isinstance(value, str):
- return [value] if value.strip() else None
- if isinstance(value, list):
- result = [str(v).strip() for v in value if v and str(v).strip()]
- return result if result else None
- return NoneCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Server/src/services/tools/test_jobs.py around lines 33-41, the
_coerce_string_list helper is duplicated from run_tests.py; extract it to a
shared module Server/src/services/tools/utils.py as coerce_string_list (preserve
the exact behavior and signature), remove the duplicate functions from both
test_jobs.py and run_tests.py, and update both files to import
coerce_string_list from services.tools.utils; ensure the new utility uses the
same type hints and behavior, update any references, and run lint/tests to
confirm no import or behavior regressions.
| if isinstance(response, dict) and not response.get("success", True): | ||
| return MCPResponse(**response) | ||
| return response if isinstance(response, dict) else MCPResponse(success=False, error=str(response)).model_dump() |
There was a problem hiding this comment.
Inconsistent return handling and type annotation mismatch.
The return logic is inconsistent:
- Line 65: Returns
MCPResponse(**response)(an MCPResponse instance) - Line 66: Returns either
response(a dict) orMCPResponse(...).model_dump()(also a dict)
The return type annotation dict[str, Any] | MCPResponse suggests both types are valid returns, but the actual code path on line 66 always returns a dict, never an MCPResponse instance. This creates confusion about the actual contract.
Consider standardizing: either always return MCPResponse instances (removing .model_dump()), or always return dicts (calling .model_dump() on line 65 as well).
Same issue exists in get_test_job at lines 90-92.
🤖 Prompt for AI Agents
In Server/src/services/tools/test_jobs.py around lines 64-66 and similarly in
get_test_job around lines 90-92, the function mixes returning MCPResponse
instances and dicts which contradicts the declared return type; standardize
returns by choosing one form: either always return MCPResponse instances (remove
.model_dump() calls) or always return dicts (call .model_dump() on the
MCPResponse returned on line 65 and the analogous line in get_test_job). Update
both code paths so both branches return the same type and adjust the function
return annotation if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
630-644: Timeout adjustment for dev mode looks good; consider extracting timeout constants.The implementation correctly addresses the hardened retry objective by extending the timeout to 45 seconds when dev mode is enabled. The logging is helpful and the dynamic timeout is properly used in both the method call and warning message.
💡 Consider extracting timeout constants for maintainability
The timeout values (45s and 10s) are currently magic numbers. Extracting them as named constants would improve maintainability and make the intent clearer:
+ private static readonly TimeSpan DevModeServerStartupTimeout = TimeSpan.FromSeconds(45); + private static readonly TimeSpan DefaultServerStartupTimeout = TimeSpan.FromSeconds(10); + private async Task TryAutoStartSessionAfterHttpServerAsync() { // ... // Dev mode uses --no-cache --refresh which causes uvx to rebuild the package, taking significantly longer. bool devModeEnabled = false; try { devModeEnabled = EditorPrefs.GetBool(EditorPrefKeys.DevModeForceServerRefresh, false); } catch { } - var startupTimeout = devModeEnabled ? TimeSpan.FromSeconds(45) : TimeSpan.FromSeconds(10); + var startupTimeout = devModeEnabled ? DevModeServerStartupTimeout : DefaultServerStartupTimeout;Note: The empty
catchblock on line 632 is consistent with similar patterns in the codebase (e.g.,McpLog.ReadDebugPreference). As this is reading a feature flag with a safe default fallback, the defensive approach is reasonable, though logging the exception could aid diagnostics if EditorPrefs access issues occur.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Info(37-41)Warn(43-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: nl-suite
Apply same logic as FinalizeCurrentJobFromRunFinished: check result.Failed > 0 to correctly mark jobs as Failed when tests fail, even in the fallback path when RunFinished callback is not delivered.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.