v9 pre-release pruning #528
Conversation
…ault to Color.white
Tested everything, works like a charm
We know what it's managing
We also update the namespace for ManageVFX
…plementation Merged EditorStateV2 into EditorState, making get_editor_state the canonical resource. Updated Unity C# to use EditorStateCache directly. Enhanced Python implementation with advice/staleness enrichment, external changes detection, and instance ID inference. Removed duplicate EditorStateV2 resource and legacy fallback mapping.
Added strongly-typed Pydantic models for EditorStateV2 schema in Python and corresponding C# classes with JsonProperty attributes. Updated C# to serialize using typed classes instead of anonymous objects. Python now validates the editor state payload before returning it, catching schema mismatches early.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @msanatan, your pull request is larger than the review limit of 300000 diff characters
📝 WalkthroughWalkthroughReplaced many unity:// URIs with mcpforunity://, consolidated editor snapshotting into a typed EditorStateCache, introduced granular GameObject and VFX editor tooling, converted test runs to a job-based API with polling, tightened transport timeouts, and added parsing/validation helpers (VectorParsing, ParamCoercion) and many Unity editor helpers. Changes
Sequence Diagram(s)sequenceDiagram
%% High-level flow for ManageGameObject command (new modular flow)
participant Client
participant Server
participant Resource as "ManageGameObject Resource"
participant Editor as "Editor Tool (GameObjectHandlers)"
participant Unity as "Unity Editor"
Client->>Server: HTTP/MCP call (mcpforunity://tools/manage_gameobject)
Server->>Resource: validate & route command
Resource->>Editor: invoke GameObjectHandlers.Create/Modify/Delete/...
Editor->>Unity: scene operations (instantiate/prefab/transform/components/Undo)
Unity-->>Editor: operation result (success/data or error)
Editor-->>Resource: serialize SuccessResponse / ErrorResponse
Resource-->>Server: deliver response
Server-->>Client: HTTP/MCP response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 9
🤖 Fix all issues with AI agents
In @MCPForUnity/Editor/Services/EditorStateCache.cs:
- Around line 156-169: The C# model EditorStateV2Assets is missing the fields
external_changes_dirty_since_unix_ms and external_changes_last_cleared_unix_ms
present on the Python side; add nullable long properties named
ExternalChangesDirtySinceUnixMs and ExternalChangesLastClearedUnixMs (with
matching JsonProperty names "external_changes_dirty_since_unix_ms" and
"external_changes_last_cleared_unix_ms") to EditorStateV2Assets and update
BuildSnapshot to populate and persist these properties when constructing the
EditorStateV2Assets instance so the serialized schema matches the server model.
In @MCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.cs:
- Around line 21-40: The message construction is using reflection on the
anonymous object in deletedObjects to get the name; instead capture the first
deleted name when building deletedObjects (e.g., store firstName = goName when
adding the first item) or use a concrete type for entries instead of anonymous
objects, then use that firstName (or the typed object's name property) in the
ternary that builds message for SuccessResponse; update the message expression
that references deletedObjects[0].GetType().GetProperty("name").GetValue(...) to
use the stored firstName (or typed entry.name) and keep deletedObjects as the
payload for SuccessResponse.
In @MCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.cs:
- Around line 59-62: The warning in ManageGameObject.Duplicate (McpLog.Warn
call) is misleading because when the requested parent token isn't found the
duplicate stays at root (from Instantiate) rather than "keeping original
parent"; update the warning message to accurately state that the parent token
wasn't found and the duplicate remains at root level (include parentToken in the
message for context) so logs reflect actual behavior.
In @MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs:
- Around line 190-214: The empty catch in the reflection loop that builds
aggregatedErrors from componentErrors hides exceptions; update the catch to log
the exception and context (e.g., include which errorObj/type caused it) using
UnityEngine.Debug.LogException or Debug.LogError so failures in
dataProp/errorsProp reflection are visible, then continue processing; reference
the loop handling componentErrors, the reflection variables
dataProp/dataVal/errorsProp/errorsEnum and the aggregatedErrors list when adding
the logging and preserving existing behavior.
In @MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs:
- Around line 47-54: The switch in the Control method that switches on the
action string (cases "play","stop","pause","restart","clear") lacks a default
branch; update the switch inside the Control method in ParticleControl.cs to add
a default case that handles unknown action values by returning or throwing an
appropriate error response (e.g., log an error and return a failure status or
throw an ArgumentException) so callers receive a clear failure for unrecognized
actions; ensure the default references the same response mechanism used by
Control (or the method's return type) and include the unknown action value in
the message for debugging.
In @MCPForUnity/Editor/Tools/Vfx/ParticleRead.cs:
- Around line 31-47: ParticleRead.GetInfo currently reads MinMaxCurve fields by
using .constant only, losing the MinMaxCurve.mode needed by the write side
(ParticleWrite.SetMain/SetEmission and ParticleCommon.ParseMinMaxCurve); update
GetInfo to include the curve mode and corresponding data for each MinMaxCurve
(startLifetime, startSpeed, startSize, gravityModifier, and
emission.rateOverTime): include a mode field (e.g., Constant,
TwoConstants/RandomBetweenConstants, Curve) plus either constant value, min/max
values, or serialized AnimationCurve + multiplier so consumers can reconstruct
via ParticleCommon.ParseMinMaxCurve when writing.
In @MCPForUnity/Editor/Tools/Vfx/ParticleWrite.cs:
- Around line 1-11: Remove the unused using directive for the
MCPForUnity.Editor.Tools.GameObjects namespace at the top of ParticleWrite.cs;
keep the existing MCPForUnity.Editor.Helpers import (which provides
ObjectResolver) and ensure there are no references to any types from
MCPForUnity.Editor.Tools.GameObjects (e.g., check within the ParticleWrite class
and related methods) before deleting the using line to avoid compilation errors.
🧹 Nitpick comments (25)
Server/src/services/tools/find_in_file.py (1)
16-65: Consider extracting_split_urito a shared module.This function is duplicated verbatim in
manage_script.py(lines 14-63). Consider extracting it to a common utility module to reduce duplication and ensure consistent behavior across both tools.Server/src/services/tools/manage_vfx.py (1)
42-43: Consider iterable unpacking for cleaner list construction.The static analysis tool (Ruff RUF005) suggests using iterable unpacking instead of concatenation. This is slightly more idiomatic and avoids creating intermediate lists.
♻️ Suggested refactor
-ALL_ACTIONS = ["ping"] + PARTICLE_ACTIONS + \ - VFX_ACTIONS + LINE_ACTIONS + TRAIL_ACTIONS +ALL_ACTIONS = ["ping", *PARTICLE_ACTIONS, *VFX_ACTIONS, *LINE_ACTIONS, *TRAIL_ACTIONS]Server/src/services/tools/batch_execute.py (1)
45-46: LGTM - Improved readability.The reformatted error messages improve readability without changing functionality.
Optional: Consider using
TypeErrorfor type validation.Lines 56-57 and 69-70 validate parameter types. Using
TypeErrorinstead ofValueErrorwould more precisely signal the nature of the error:♻️ Optional refactor
for index, command in enumerate(commands): if not isinstance(command, dict): - raise ValueError( + raise TypeError( f"Command at index {index} must be an object with 'tool' and 'params' keys")if not isinstance(params, dict): - raise ValueError( + raise TypeError( f"Command '{tool_name}' must specify parameters as an object/dict")Also applies to: 56-57, 63-64, 69-70
Server/src/transport/plugin_hub.py (1)
51-58: Optional: Annotate mutable class attributes withClassVar.Static analysis flags that mutable class attributes like
_FAST_FAIL_COMMANDS,_connections,_pending, etc. should be annotated withtyping.ClassVarto clarify they are shared class-level state rather than instance attributes. This is a pre-existing pattern and not introduced by this formatting change, so it can be addressed separately if desired.Example annotation
+from typing import ClassVar + class PluginHub(WebSocketEndpoint): ... - _FAST_FAIL_COMMANDS: set[str] = { + _FAST_FAIL_COMMANDS: ClassVar[set[str]] = { "read_console", "get_editor_state", "ping"} _registry: PluginRegistry | None = None - _connections: dict[str, WebSocket] = {} - _pending: dict[str, dict[str, Any]] = {} - _lock: asyncio.Lock | None = None - _loop: asyncio.AbstractEventLoop | None = None + _connections: ClassVar[dict[str, WebSocket]] = {} + _pending: ClassVar[dict[str, dict[str, Any]]] = {} + _lock: ClassVar[asyncio.Lock | None] = None + _loop: ClassVar[asyncio.AbstractEventLoop | None] = NoneMCPForUnity/Editor/Tools/ManageMaterial.cs (1)
98-112: Verify instruction detection logic for texture handling.The condition
(obj.ContainsKey("find") || obj.ContainsKey("method"))is loose. Per theObjectResolver.Resolvedocs, "find" is mandatory while "method" is optional. An object with only "method" would fail resolution and return null, falling through toMaterialOps.TrySetShaderProperty, which may not be the intended behavior.Consider tightening this to check only
obj.ContainsKey("find"):if (value is JObject obj && obj.ContainsKey("find"))That said, the graceful null check (
if (tex != null && ...)) mitigates the risk. This is safe as-is but could be clearer semantically.MCPForUnity/Editor/Tools/GameObjects/ComponentResolver.cs (2)
58-60: Cache key may be inefficient for large property lists.The cache key concatenates all available properties, which could create very long keys and impact dictionary lookup performance for components with many properties. Consider using a hash of the property list instead.
♻️ Proposed optimization
-var cacheKey = $"{userInput.ToLowerInvariant()}:{string.Join(",", availableProperties)}"; +var propertiesHash = string.Join(",", availableProperties).GetHashCode(); +var cacheKey = $"{userInput.ToLowerInvariant()}:{propertiesHash}";
75-75: Unbounded static cache.
PropertySuggestionCachegrows without limit during an editor session. For typical editor usage this is acceptable, but consider adding a size limit or LRU eviction if this becomes a concern.MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs (1)
390-393: UnusedinputSerializerparameter.The
inputSerializerparameter is passed toConvertJTokenToTypebut is never used—the method delegates directly toPropertyConversion.ConvertToType(token, targetType). This parameter should either be removed or utilized for consistency.♻️ Option 1: Remove unused parameter
-private static object ConvertJTokenToType(JToken token, Type targetType, JsonSerializer inputSerializer) +private static object ConvertJTokenToType(JToken token, Type targetType) { return PropertyConversion.ConvertToType(token, targetType); }Then update all call sites to remove the
inputSerializerargument.♻️ Option 2: Use the serializer if custom conversion is needed
If the serializer is intended for future use or specific edge cases, consider using it:
private static object ConvertJTokenToType(JToken token, Type targetType, JsonSerializer inputSerializer) { - return PropertyConversion.ConvertToType(token, targetType); + return token.ToObject(targetType, inputSerializer); }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
427-432: Consider adding a brief comment explaining the empty catch.The empty catch block is acceptable for test cleanup, but a brief inline comment would clarify intent for future maintainers.
💡 Suggested improvement
// Clean up the created tag - try { UnityEditorInternal.InternalEditorUtility.RemoveTag(testTag); } catch { } + try { UnityEditorInternal.InternalEditorUtility.RemoveTag(testTag); } catch { /* Tag may already be removed or not exist */ }Server/tests/integration/test_refresh_unity_retry_recovery.py (1)
29-36: Consider renaming the mock function for clarity.The function
fake_get_editor_state_v2retains the "v2" suffix but now mocksget_editor_state(notget_editor_state_v2). Consider renaming it tofake_get_editor_stateto avoid confusion.♻️ Suggested rename
- async def fake_get_editor_state_v2(_ctx): + async def fake_get_editor_state(_ctx): return MCPResponse(success=True, data={"advice": {"ready_for_tools": True}}) import services.tools.refresh_unity as refresh_mod monkeypatch.setattr(refresh_mod.unity_transport, "send_with_unity_instance", fake_send_with_unity_instance) import services.resources.editor_state as es_mod - monkeypatch.setattr(es_mod, "get_editor_state", fake_get_editor_state_v2) + monkeypatch.setattr(es_mod, "get_editor_state", fake_get_editor_state)Server/tests/integration/test_editor_state_v2_contract.py (1)
14-14: Remove unnecessarynoqadirective.The
noqa: F401directive on line 14 appears to be unused according to static analysis. Consider removing it unless there's a specific linter configuration that requires it.♻️ Proposed fix
- import services.resources.editor_state # noqa: F401 + import services.resources.editor_stateMCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.cs (1)
24-30: The null check is likely unnecessary.
FindObjectsInternalfilters or conditionally adds GameObjects in all branches. Theby_componentcase explicitly filters nulls withWhere(go => go != null), and other cases use conditional checks (if (obj != null)) before adding to results. Since the method's logic prevents null entries from being added to the results list, the defensive null check at line 24 can be removed.MCPForUnity/Editor/Tools/GameObjects/ManageGameObject.cs (2)
1-1: Consider removing #nullable disable.The
#nullable disabledirective can hide potential null reference issues and makes the code less safe. Given that the file performs parameter validation and uses null checks, consider enabling nullable reference types and making nullability explicit.If nullable annotations are enabled project-wide, removing this directive would improve type safety:
♻️ Consider enabling nullable reference types
-#nullable disable using System; using System.Collections.Generic;Then ensure proper nullable annotations on parameters and fields (e.g.,
string?for optional fields).
38-47: Document the name→target aliasing behavior in user-facing documentation.The aliasing of
nametotargetfor "modify" actions is tested (seeModify_WithNameAlias_UsesNameAsTarget()test), but it's not documented in the tool's schema or README. While similar aliasing patterns exist elsewhere in the codebase (e.g.,FindGameObjects,ManagePrefabs) without apparent issues, users need to understand that providingnamewill populatetargetso they aren't surprised by the behavior when inspecting parameter objects or composing downstream requests.MCPForUnity/Editor/Tools/Vfx/TrailControl.cs (1)
24-27: Consider addingUndo.RecordObjectbeforeAddPositionfor consistency.The
Clearmethod records an undo operation, butEmitdoes not. For consistency with other trail write operations (e.g.,TrailWrite.SetTime,TrailWrite.SetWidth), consider recording the undo state before modifying the trail:#if UNITY_2021_1_OR_NEWER Vector3 pos = ManageVfxCommon.ParseVector3(@params["position"]); + Undo.RecordObject(tr, "Emit Trail Position"); tr.AddPosition(pos);Note: If
AddPositionmodifies purely runtime state that isn't serialized, undo may not be meaningful here, in which case this is fine as-is.MCPForUnity/Editor/Tools/Vfx/TrailWrite.cs (1)
26-41: Handle case where no width properties were actually changed.If
@paramscontains no recognized width properties (nowidth,startWidth,endWidth,widthCurve, orwidthMultiplier), thechangeslist will be empty, and the message will be"Updated: ". Consider returning a no-op success message or validating that at least one property was provided.This same pattern appears in
SetColor(lines 43-58) andSetProperties(lines 66-88).Example improvement
EditorUtility.SetDirty(tr); - return new { success = true, message = $"Updated: {string.Join(", ", changes)}" }; + return changes.Count > 0 + ? new { success = true, message = $"Updated: {string.Join(", ", changes)}" } + : new { success = true, message = "No properties to update" };MCPForUnity/Editor/Tools/Vfx/ManageVFX.cs (2)
312-323: Emptycatchblocks suppress errors silently in template search.Lines 321-322 swallow exceptions during template file search. Similar pattern at lines 452-453 in
VFXListTemplates. While this prevents crashes during file enumeration, it could hide permission or path issues.Consider logging warnings:
- catch { } + catch (Exception ex) + { + McpLog.Warn($"[ManageVFX] Error searching for templates: {ex.Message}"); + }
458-466: Consider using a typed class instead of anonymous types with dynamic casts.Line 461 uses
((dynamic)t).pathto access properties of anonymous objects in thetemplateslist. While functional, this bypasses compile-time type checking.A small DTO class would provide better type safety:
private class TemplateInfo { public string name; public string path; public string source; }MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs (1)
76-80: Burst array expansion may leave uninitialized data beforeGetBurstscall.The array
burstsis created with sizeidx + 1, butGetBurstsonly fills the firstidxelements (indices 0 to idx-1). This works because you then assignbursts[idx] = burst, but the code reads slightly confusingly. Consider reordering for clarity:var existingBursts = new ParticleSystem.Burst[idx]; emission.GetBursts(existingBursts); var bursts = new ParticleSystem.Burst[idx + 1]; Array.Copy(existingBursts, bursts, idx); bursts[idx] = burst;However, the current implementation is functionally correct since Unity's
GetBurstswrites up to the array length or burst count (whichever is smaller).MCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.cs (2)
100-108: Redundant name validation after early return.The name validation at lines 100-108 is unreachable because lines 17-21 already return an error if
nameis null or empty. This dead code can be removed.♻️ Suggested cleanup
try { PrimitiveType type = (PrimitiveType)Enum.Parse(typeof(PrimitiveType), primitiveType, true); newGo = GameObject.CreatePrimitive(type); - if (!string.IsNullOrEmpty(name)) - { - newGo.name = name; - } - else - { - UnityEngine.Object.DestroyImmediate(newGo); - return new ErrorResponse("'name' parameter is required when creating a primitive."); - } + newGo.name = name; createdNewObject = true; }
120-128: Similar redundant check for empty GameObject creation.The check at line 122-125 is also unreachable due to the early validation at lines 17-21.
♻️ Suggested cleanup
else { - if (string.IsNullOrEmpty(name)) - { - return new ErrorResponse("'name' parameter is required for 'create' action when not instantiating a prefab or creating a primitive."); - } newGo = new GameObject(name); createdNewObject = true; }MCPForUnity/Editor/Tools/Vfx/LineCreate.cs (1)
36-39: Duplicated perpendicular axis calculation.The logic for computing perpendicular axes from a normal vector is duplicated between
CreateCircleandCreateArc. Consider extracting this into a shared helper method within the class or inManageVfxCommon.♻️ Suggested extraction
+ private static (Vector3 right, Vector3 forward) GetPerpendicularAxes(Vector3 normal) + { + Vector3 right = Vector3.Cross(normal, Vector3.forward); + if (right.sqrMagnitude < 0.001f) right = Vector3.Cross(normal, Vector3.up); + right = right.normalized; + Vector3 forward = Vector3.Cross(right, normal).normalized; + return (right, forward); + }Also applies to: 68-71
MCPForUnity/Editor/Tools/Vfx/LineWrite.cs (1)
66-98: Consider guarding against empty changes in the response message.If no recognized width/color properties are present in
@params,changesremains empty and the message becomes"Updated: ". While not incorrect, it could confuse callers.🔧 Optional: Return early or clarify when no changes applied
EditorUtility.SetDirty(lr); - return new { success = true, message = $"Updated: {string.Join(", ", changes)}" }; + return new { success = true, message = changes.Count > 0 ? $"Updated: {string.Join(", ", changes)}" : "No width properties updated" };Server/src/services/resources/editor_state.py (2)
136-170: Consider logging or narrowing broad exception handlers.Multiple
except Exceptionblocks silently swallow all errors, reducing observability. At minimum, logging at DEBUG level would aid troubleshooting transport/discovery issues.🔧 Example: Add debug logging
+import logging + +logger = logging.getLogger(__name__) + async def _infer_single_instance_id(ctx: Context) -> str | None: ... try: transport = unity_transport._current_transport() - except Exception: + except Exception as e: + logger.debug("Failed to determine transport: %s", e) transport = None
254-285: Avoid silenttry-except-pass—log or propagate partial failures.Lines 284-285 catch all exceptions during external-changes detection and discard them. If this scanner fails, the caller has no indication. Consider logging the exception or including a flag in the response.
🔧 Proposed: Log instead of silent pass
+import logging + +logger = logging.getLogger(__name__) + except Exception: - pass + logger.debug("External changes detection failed", exc_info=True)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (101)
MCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Resources/Editor/EditorState.csMCPForUnity/Editor/Resources/Editor/EditorStateV2.csMCPForUnity/Editor/Resources/Scene/GameObjectResource.csMCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Tools/FindGameObjects.csMCPForUnity/Editor/Tools/GameObjects.metaMCPForUnity/Editor/Tools/GameObjects/ComponentResolver.csMCPForUnity/Editor/Tools/GameObjects/ComponentResolver.cs.metaMCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.csMCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs.metaMCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.csMCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.cs.metaMCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.cs.metaMCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.cs.metaMCPForUnity/Editor/Tools/GameObjects/GameObjectHandlers.csMCPForUnity/Editor/Tools/GameObjects/GameObjectHandlers.cs.metaMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs.metaMCPForUnity/Editor/Tools/GameObjects/GameObjectMoveRelative.csMCPForUnity/Editor/Tools/GameObjects/GameObjectMoveRelative.cs.metaMCPForUnity/Editor/Tools/GameObjects/ManageGameObject.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObject.cs.metaMCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs.metaMCPForUnity/Editor/Tools/GetTestJob.csMCPForUnity/Editor/Tools/JsonUtil.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageVFX.csMCPForUnity/Editor/Tools/RefreshUnity.csMCPForUnity/Editor/Tools/RunTestsAsync.csMCPForUnity/Editor/Tools/Vfx.metaMCPForUnity/Editor/Tools/Vfx/LineCreate.csMCPForUnity/Editor/Tools/Vfx/LineCreate.cs.metaMCPForUnity/Editor/Tools/Vfx/LineRead.csMCPForUnity/Editor/Tools/Vfx/LineRead.cs.metaMCPForUnity/Editor/Tools/Vfx/LineWrite.csMCPForUnity/Editor/Tools/Vfx/LineWrite.cs.metaMCPForUnity/Editor/Tools/Vfx/ManageVFX.csMCPForUnity/Editor/Tools/Vfx/ManageVFX.cs.metaMCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.csMCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.cs.metaMCPForUnity/Editor/Tools/Vfx/ParticleCommon.csMCPForUnity/Editor/Tools/Vfx/ParticleCommon.cs.metaMCPForUnity/Editor/Tools/Vfx/ParticleControl.csMCPForUnity/Editor/Tools/Vfx/ParticleControl.cs.metaMCPForUnity/Editor/Tools/Vfx/ParticleRead.csMCPForUnity/Editor/Tools/Vfx/ParticleRead.cs.metaMCPForUnity/Editor/Tools/Vfx/ParticleWrite.csMCPForUnity/Editor/Tools/Vfx/ParticleWrite.cs.metaMCPForUnity/Editor/Tools/Vfx/TrailControl.csMCPForUnity/Editor/Tools/Vfx/TrailControl.cs.metaMCPForUnity/Editor/Tools/Vfx/TrailRead.csMCPForUnity/Editor/Tools/Vfx/TrailRead.cs.metaMCPForUnity/Editor/Tools/Vfx/TrailWrite.csMCPForUnity/Editor/Tools/Vfx/TrailWrite.cs.metaREADME.mdServer/src/main.pyServer/src/routes/__init__.pyServer/src/services/custom_tool_service.pyServer/src/services/resources/editor_state.pyServer/src/services/resources/editor_state_v2.pyServer/src/services/resources/gameobject.pyServer/src/services/state/external_changes_scanner.pyServer/src/services/tools/batch_execute.pyServer/src/services/tools/debug_request_context.pyServer/src/services/tools/find_gameobjects.pyServer/src/services/tools/find_in_file.pyServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/manage_scriptable_object.pyServer/src/services/tools/manage_shader.pyServer/src/services/tools/manage_vfx.pyServer/src/services/tools/preflight.pyServer/src/services/tools/read_console.pyServer/src/services/tools/refresh_unity.pyServer/src/services/tools/run_tests.pyServer/src/services/tools/script_apply_edits.pyServer/src/services/tools/set_active_instance.pyServer/src/services/tools/test_jobs.pyServer/src/services/tools/utils.pyServer/src/transport/legacy/port_discovery.pyServer/src/transport/plugin_hub.pyServer/src/transport/unity_instance_middleware.pyServer/src/transport/unity_transport.pyServer/tests/integration/test_editor_state_v2_contract.pyServer/tests/integration/test_refresh_unity_retry_recovery.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs
💤 Files with no reviewable changes (11)
- MCPForUnity/Editor/Tools/GetTestJob.cs
- MCPForUnity/Editor/Resources/Editor/EditorStateV2.cs
- MCPForUnity/Editor/Tools/RefreshUnity.cs
- MCPForUnity/Editor/Resources/Scene/GameObjectResource.cs
- MCPForUnity/Editor/Tools/JsonUtil.cs
- MCPForUnity/Editor/Tools/ManageVFX.cs
- MCPForUnity/Editor/Tools/FindGameObjects.cs
- MCPForUnity/Editor/Tools/ManageComponents.cs
- Server/src/services/resources/editor_state_v2.py
- MCPForUnity/Editor/Tools/RunTestsAsync.cs
- MCPForUnity/Editor/Tools/ManageGameObject.cs
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Tools/Vfx/ParticleRead.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.csMCPForUnity/Editor/Tools/GameObjects/ComponentResolver.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs.metaMCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.cs.metaMCPForUnity/Editor/Tools/GameObjects/ComponentResolver.cs.metaMCPForUnity/Editor/Services/EditorStateCache.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.csMCPForUnity/Editor/Resources/Editor/EditorState.csMCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csMCPForUnity/Editor/Tools/GameObjects.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageAsset.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.csMCPForUnity/Editor/Tools/GameObjects/ComponentResolver.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs.metaMCPForUnity/Editor/Tools/GameObjects/ComponentResolver.cs.metaMCPForUnity/Editor/Services/EditorStateCache.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.csMCPForUnity/Editor/Resources/Editor/EditorState.csMCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csMCPForUnity/Editor/Tools/Vfx/ParticleCommon.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.csMCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObject.csMCPForUnity/Editor/Tools/GameObjects/GameObjectHandlers.csMCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs.metaMCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csMCPForUnity/Editor/Tools/GameObjects/GameObjectMoveRelative.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
📚 Learning: 2025-12-29T15:23:17.871Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/GameObjects/ComponentResolver.csMCPForUnity/Editor/Tools/Vfx/ParticleRead.csMCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Resources/Editor/EditorState.csMCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Tools/Vfx/LineRead.csMCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.csMCPForUnity/Editor/Tools/Vfx/ManageVFX.csMCPForUnity/Editor/Tools/Vfx/LineWrite.csMCPForUnity/Editor/Tools/Vfx/TrailControl.csMCPForUnity/Editor/Tools/Vfx/ParticleCommon.csMCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.csMCPForUnity/Editor/Tools/Vfx/LineCreate.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObject.csMCPForUnity/Editor/Tools/GameObjects/GameObjectHandlers.csMCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csMCPForUnity/Editor/Tools/Vfx/ParticleControl.csMCPForUnity/Editor/Tools/Vfx/TrailRead.csMCPForUnity/Editor/Tools/GameObjects/GameObjectMoveRelative.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.csMCPForUnity/Editor/Tools/Vfx/ParticleWrite.csMCPForUnity/Editor/Tools/Vfx/TrailWrite.cs
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/transport/unity_transport.pyServer/src/services/tools/refresh_unity.py
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csMCPForUnity/Editor/Tools/ManageMaterial.csServer/src/services/tools/refresh_unity.py
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.cs.metaMCPForUnity/Editor/Tools/GameObjects.meta
🧬 Code graph analysis (25)
Server/src/services/tools/manage_vfx.py (2)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-114)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)
MCPForUnity/Editor/Services/EditorStateCache.cs (1)
Server/src/services/resources/editor_state.py (11)
EditorStateV2Unity(16-21)EditorStateV2Editor(36-39)EditorStateV2Activity(42-45)EditorStateV2Compilation(48-54)EditorStateV2Assets(63-69)EditorStateV2Tests(78-84)EditorStateV2Transport(87-89)EditorStateV2PlayMode(24-27)EditorStateV2ActiveScene(30-33)EditorStateV2Refresh(57-60)EditorStateV2LastRun(72-75)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (2)
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
DeleteAsset(495-523)TestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.cs (1)
CleanupEmptyParentFolders(103-137)
MCPForUnity/Editor/Resources/Editor/EditorState.cs (1)
MCPForUnity/Editor/Services/EditorStateCache.cs (1)
EditorStateCache(225-253)
MCPForUnity/Editor/Tools/Vfx/LineRead.cs (3)
MCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.cs (3)
GameObject(16-17)ManageVfxCommon(7-21)Vector3(10-10)MCPForUnity/Editor/Tools/Vfx/ParticleRead.cs (1)
GetInfo(9-73)MCPForUnity/Editor/Tools/Vfx/TrailRead.cs (1)
GetInfo(14-47)
MCPForUnity/Editor/Tools/Vfx/ManageVFX.cs (4)
MCPForUnity/Editor/Tools/Vfx/ParticleRead.cs (2)
ParticleRead(7-74)GetInfo(9-73)MCPForUnity/Editor/Tools/Vfx/LineRead.cs (2)
GetInfo(15-50)LineRead(7-51)MCPForUnity/Editor/Tools/Vfx/TrailRead.cs (2)
GetInfo(14-47)TrailRead(6-48)MCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.cs (7)
GameObject(16-17)ManageVfxCommon(7-21)Vector4(11-11)Vector3(10-10)Color(9-9)Gradient(12-12)AnimationCurve(13-14)
Server/src/services/tools/preflight.py (2)
Server/src/services/resources/editor_state.py (1)
get_editor_state(220-305)Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
MCPForUnity/Editor/Tools/Vfx/TrailControl.cs (2)
MCPForUnity/Editor/Tools/Vfx/TrailRead.cs (2)
TrailRenderer(8-12)TrailRead(6-48)MCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.cs (2)
Vector3(10-10)ManageVfxCommon(7-21)
MCPForUnity/Editor/Tools/Vfx/ParticleCommon.cs (2)
MCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.cs (4)
GameObject(16-17)ManageVfxCommon(7-21)AnimationCurve(13-14)Color(9-9)MCPForUnity/Editor/Tools/BatchExecute.cs (1)
JArray(153-161)
Server/src/services/tools/manage_scene.py (1)
Server/src/services/tools/utils.py (2)
coerce_int(64-78)coerce_bool(12-25)
MCPForUnity/Editor/Tools/ManageMaterial.cs (4)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (5)
UnityEngine(32-105)UnityEngine(170-199)Texture(146-153)ObjectResolver(13-200)Material(134-141)MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (2)
UnityEngine(311-353)Color(102-111)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
JObject(98-149)MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
Color(345-395)
MCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.cs (1)
MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs (3)
List(31-179)GameObject(15-29)ManageGameObjectCommon(13-209)
MCPForUnity/Editor/Tools/GameObjects/ManageGameObject.cs (6)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Warn(43-46)Error(48-51)MCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.cs (2)
GameObjectCreate(13-308)Handle(15-307)MCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.cs (2)
Handle(12-43)GameObjectDelete(10-44)MCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.cs (2)
Handle(12-84)GameObjectDuplicate(10-85)MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs (2)
Handle(15-235)GameObjectModify(13-236)MCPForUnity/Editor/Tools/GameObjects/GameObjectMoveRelative.cs (2)
Handle(12-79)GameObjectMoveRelative(10-118)
MCPForUnity/Editor/Tools/GameObjects/GameObjectHandlers.cs (5)
MCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.cs (2)
GameObjectCreate(13-308)Handle(15-307)MCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.cs (2)
Handle(12-43)GameObjectDelete(10-44)MCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.cs (2)
Handle(12-84)GameObjectDuplicate(10-85)MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs (2)
Handle(15-235)GameObjectModify(13-236)MCPForUnity/Editor/Tools/GameObjects/GameObjectMoveRelative.cs (2)
Handle(12-79)GameObjectMoveRelative(10-118)
Server/src/services/custom_tool_service.py (1)
Server/tests/integration/test_helpers.py (1)
warning(39-40)
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs (5)
MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs (3)
GameObject(15-29)Type(195-208)List(31-179)MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
Component(23-73)MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)NormalizePropertyName(170-199)MCPForUnity/Editor/Helpers/PropertyConversion.cs (2)
PropertyConversion(14-91)ConvertToType(22-44)
MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs (1)
MCPForUnity/Editor/Tools/Vfx/ParticleCommon.cs (4)
ParticleSystem(8-12)ParticleSystem(14-50)ParticleSystem(52-85)ParticleCommon(6-86)
MCPForUnity/Editor/Tools/Vfx/TrailRead.cs (3)
MCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.cs (2)
GameObject(16-17)ManageVfxCommon(7-21)MCPForUnity/Editor/Tools/Vfx/LineRead.cs (1)
GetInfo(15-50)MCPForUnity/Editor/Tools/ManageVFX.cs (1)
TrailSetProperties(1649-1674)
Server/src/services/tools/refresh_unity.py (1)
Server/src/services/resources/editor_state.py (2)
get_editor_state(220-305)_infer_single_instance_id(128-171)
MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs (2)
MCPForUnity/Editor/Tools/GameObjects/ComponentResolver.cs (5)
List(29-47)List(53-73)List(81-112)ComponentResolver(15-141)TryResolve(21-24)MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)
Server/src/services/resources/gameobject.py (3)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-114)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/services/custom_tool_service.py (1)
_normalize_response(228-260)
Server/tests/integration/test_editor_state_v2_contract.py (1)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
Server/src/services/tools/find_in_file.py (1)
Server/src/services/tools/manage_script.py (1)
_split_uri(15-64)
Server/src/services/tools/manage_material.py (1)
Server/src/services/tools/utils.py (2)
parse_json_payload(28-61)normalize_properties(99-131)
MCPForUnity/Editor/Tools/Vfx/TrailWrite.cs (3)
MCPForUnity/Editor/Tools/Vfx/TrailRead.cs (2)
TrailRenderer(8-12)TrailRead(6-48)MCPForUnity/Editor/Helpers/RendererHelpers.cs (6)
RendererHelpers(13-166)ApplyWidthProperties(75-91)ApplyColorProperties(104-120)SetRendererMaterial(130-145)ApplyLineTrailProperties(151-164)ApplyCommonRendererProperties(20-45)MCPForUnity/Editor/Tools/Vfx/ManageVfxCommon.cs (1)
ManageVfxCommon(7-21)
🪛 Ruff (0.14.10)
Server/src/services/tools/manage_vfx.py
42-43: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
Server/src/transport/plugin_hub.py
51-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
456-457: Avoid specifying long messages outside the exception class
(TRY003)
Server/src/services/tools/batch_execute.py
45-46: Avoid specifying long messages outside the exception class
(TRY003)
56-57: Prefer TypeError exception for invalid type
(TRY004)
56-57: Avoid specifying long messages outside the exception class
(TRY003)
63-64: Avoid specifying long messages outside the exception class
(TRY003)
69-70: Prefer TypeError exception for invalid type
(TRY004)
69-70: Avoid specifying long messages outside the exception class
(TRY003)
Server/tests/integration/test_editor_state_v2_contract.py
14-14: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
27-27: Unused function argument: send_fn
(ARG001)
27-27: Unused function argument: unity_instance
(ARG001)
27-27: Unused function argument: params
(ARG001)
27-27: Unused function argument: kwargs
(ARG001)
Server/src/services/resources/editor_state.py
128-128: Unused function argument: ctx
(ARG001)
138-138: Do not catch blind exception: Exception
(BLE001)
155-155: Do not catch blind exception: Exception
(BLE001)
169-169: Do not catch blind exception: Exception
(BLE001)
179-179: Do not catch blind exception: Exception
(BLE001)
284-285: try-except-pass detected, consider logging the exception
(S110)
284-284: Do not catch blind exception: Exception
(BLE001)
297-297: Do not catch blind exception: Exception
(BLE001)
| case "by_path": | ||
| Transform foundTransform = rootSearchObject | ||
| ? rootSearchObject.transform.Find(searchTerm) | ||
| : GameObject.Find(searchTerm)?.transform; | ||
| if (foundTransform != null) | ||
| results.Add(foundTransform.gameObject); | ||
| break; |
There was a problem hiding this comment.
GameObject.Find ignores inactive objects, contradicting searchInactive flag.
When searchMethod is "by_path" and searchInactive is true, calling GameObject.Find(searchTerm) will still only return active GameObjects. This inconsistency may cause unexpected behavior when users expect to find inactive objects by path.
Consider using a manual hierarchy traversal that respects the searchInactive flag, or document this limitation.
…ation Merged run_tests_async into run_tests, making async job-based execution the default behavior. Removed synchronous blocking test execution. Updated RunTests.cs to start test jobs immediately and return job_id for polling. Changed TestJobManager methods to internal visibility. Updated README to reflect single run_tests_async tool. Python implementation now uses async job pattern exclusively.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Server/src/transport/plugin_hub.py:
- Line 149: PluginHub references an undefined class attribute FAST_FAIL_TIMEOUT
causing AttributeError; add a FAST_FAIL_TIMEOUT constant to the PluginHub class
alongside the other timeout definitions (e.g., define FAST_FAIL_TIMEOUT = 5.0 or
set it equal to FAST_TIMEOUT) so read_console/get_editor_state/ping can use it
without raising at runtime.
🧹 Nitpick comments (3)
MCPForUnity/Editor/Tools/RunTests.cs (1)
34-53: Consider logging parse failures for includeDetails/includeFailedTests.The silent exception handling (lines 36-53) for boolean parameter parsing is defensive, but could hide malformed input that might indicate client-side issues. Since these are optional flags with sensible defaults, the current approach is acceptable, but consider adding a debug log on parse failure to aid troubleshooting.
🔍 Optional: Add debug logging for parse failures
try { var includeDetailsToken = @params?["includeDetails"]; if (includeDetailsToken != null && bool.TryParse(includeDetailsToken.ToString(), out var parsedIncludeDetails)) { includeDetails = parsedIncludeDetails; } var includeFailedTestsToken = @params?["includeFailedTests"]; if (includeFailedTestsToken != null && bool.TryParse(includeFailedTestsToken.ToString(), out var parsedIncludeFailedTests)) { includeFailedTests = parsedIncludeFailedTests; } } -catch +catch (Exception ex) { - // ignore parse failures + McpLog.Debug($"[RunTests] Failed to parse boolean flags: {ex.Message}"); + // Use defaults }Server/src/transport/plugin_hub.py (2)
51-52: Formatting improvement looks good.The multiline format improves readability. Optionally, consider annotating this class attribute with
typing.ClassVarto clarify it's a class-level variable.📝 Optional: Add ClassVar annotation
- _FAST_FAIL_COMMANDS: set[str] = { + _FAST_FAIL_COMMANDS: ClassVar[set[str]] = { "read_console", "get_editor_state", "ping"}Add the import at the top:
from typing import Any, ClassVar
452-453: Consider moving exception message to the exception class.The exception message is clear, but static analysis suggests defining default messages within the exception class itself for better maintainability.
♻️ Optional refactor
Define a default message in the
NoUnitySessionErrorclass:class NoUnitySessionError(RuntimeError): - """Raised when no Unity plugins are available.""" + """Raised when no Unity plugins are available.""" + + def __init__(self, message: str = "No Unity plugins are currently connected"): + super().__init__(message)Then simplify the raise statement:
- raise NoUnitySessionError( - "No Unity plugins are currently connected") + raise NoUnitySessionError()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
MCPForUnity/Editor/Services/TestJobManager.csMCPForUnity/Editor/Tools/RunTests.csMCPForUnity/Editor/Tools/RunTests.cs.metaMCPForUnity/Editor/Tools/RunTestsAsync.csREADME.mdServer/src/services/tools/run_tests.pyServer/src/services/tools/test_jobs.pyServer/src/transport/plugin_hub.pyServer/tests/integration/test_run_tests_async.pyServer/tests/integration/test_run_tests_busy_semantics.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs
💤 Files with no reviewable changes (3)
- MCPForUnity/Editor/Tools/RunTestsAsync.cs
- Server/tests/integration/test_run_tests_busy_semantics.py
- Server/src/services/tools/test_jobs.py
✅ Files skipped from review due to trivial changes (1)
- MCPForUnity/Editor/Tools/RunTests.cs.meta
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/TestJobManager.csMCPForUnity/Editor/Tools/RunTests.cs
🧬 Code graph analysis (3)
Server/src/services/tools/run_tests.py (5)
Server/src/services/tools/preflight.py (1)
preflight(27-110)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)MCPForUnity/Editor/Services/TestJobManager.cs (1)
TestJobFailure(20-24)Server/tests/integration/test_helpers.py (1)
error(46-47)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-114)
MCPForUnity/Editor/Tools/RunTests.cs (4)
MCPForUnity/Editor/Tools/ManageEditor.cs (2)
McpForUnityTool(13-377)HandleCommand(25-127)MCPForUnity/Editor/Services/TestRunnerService.cs (3)
Task(34-59)Task(61-144)Task(351-379)MCPForUnity/Editor/Resources/Tests/GetTests.cs (4)
Task(18-36)Task(46-74)ModeParser(77-105)TryParse(79-104)MCPForUnity/Editor/Resources/Editor/Windows.cs (1)
HandleCommand(16-57)
Server/tests/integration/test_run_tests_async.py (2)
Server/src/services/tools/run_tests.py (2)
run_tests(97-155)get_test_job(165-191)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
🪛 Ruff (0.14.10)
Server/src/transport/plugin_hub.py
51-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
452-453: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (11)
MCPForUnity/Editor/Services/TestJobManager.cs (1)
419-419: LGTM: Appropriate encapsulation improvement.The visibility reduction from
public statictointernal staticfor bothGetJobandToSerializablealigns well with the PR's shift toward internal test job orchestration. These methods are now accessed only through the newrun_tests/get_test_jobworkflow.Also applies to: 431-431
MCPForUnity/Editor/Tools/RunTests.cs (2)
55-76: LGTM: Clean job-based execution model.The immediate return with
job_idand status metadata is the correct pattern for async job management. The normalization of "already in progress" errors to a structured response withretry_after_mshint (lines 70-73) provides good client guidance.
78-125: LGTM: Robust filter parameter handling.The
GetFilterOptionsimplementation correctly handles both string and array inputs for filter parameters, with appropriate null/whitespace filtering. The localParseStringArrayfunction is well-structured and handles edge cases properly.Server/src/services/tools/run_tests.py (3)
43-88: LGTM: Well-structured Pydantic models for async job lifecycle.The new data models provide strong typing for the job-based workflow. The use of optional fields and nested structures (e.g.,
TestJobProgresswithinGetTestJobData) properly represents the asynchronous job lifecycle states.
97-155: LGTM: Clean async job start implementation.The refactored
run_testsproperly:
- Uses preflight checks to prevent concurrent test runs (line 116)
- Handles string/list parameter coercion elegantly with
_coerce_string_list(lines 120-128)- Employs walrus operator for concise parameter building (lines 131-142)
- Returns typed
RunTestsStartResponsefor immediate job acknowledgment
165-191: LGTM: Straightforward job polling endpoint.The
get_test_jobimplementation correctly polls job status byjob_idand returns typedGetTestJobResponse. The parameter handling and response wrapping are consistent with therun_testspattern.Server/tests/integration/test_run_tests_async.py (2)
8-31: LGTM: Test correctly updated for new run_tests API.The test properly validates:
- Import from
services.tools.run_tests(line 8)- Monkeypatch on the new module path (lines 17-19)
- Command type "run_tests" (line 27)
- Parameter forwarding to Unity transport (lines 28-30)
35-52: LGTM: get_test_job polling test properly structured.The test validates the new polling endpoint with correct:
- Import path (line 36)
- Monkeypatch setup (lines 45-47)
- Command type assertion for "get_test_job" (line 50)
- Job ID parameter forwarding (lines 51-52)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs (2)
17-49: Reflection-based testing is appropriate but fragile.The use of reflection to set
TestJobManager._currentJobId(lines 19-30) is necessary for testing the busy-state behavior without triggering actual test runs. The cleanup in thefinallyblock (lines 45-48) properly restores state.However, be aware that reflection-based tests are fragile and will break if internal field names change. Based on learnings, this approach is acceptable in MCPForUnity's testing strategy.
Consider adding a comment documenting the fragility:
// NOTE: This test uses reflection to manipulate internal state. // If TestJobManager refactoring breaks this test, consider exposing // a test-only method to simulate busy state.
52-63: LGTM: Clean validation of invalid mode handling.The test properly validates that an invalid mode parameter returns an
ErrorResponsewith an appropriate error message (line 62). The synchronous.GetAwaiter().GetResult()pattern (line 57) correctly handles the non-asyncTask<object>return type.Server/src/transport/plugin_hub.py (1)
122-123: Formatting improvements enhance readability.The multiline reformatting of conditionals, dictionary assignments, and function calls improves code readability without changing behavior.
Also applies to: 182-183, 373-374, 486-487, 489-490, 506-507
It should reduce conflicts with other Unity MCPs that users try, and to comply with Unity's requests regarding use of their company and product name
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.claude/prompts/nl-unity-suite-t.md (1)
272-272: Update template example to reflect the new canonical URI scheme.The template example at line 272 still references the old
unity://path/...scheme, but the canonical URI format established at line 40 now mandatesmcpforunity://path/Assets/.... Update the example to reflect the new scheme for consistency.💡 Proposed fix
- Edit via unity://path/... succeeded. + Edit via mcpforunity://path/Assets/Scripts/LongUnityScriptClaudeTest.cs succeeded..claude/prompts/nl-gameobject-suite.md (1)
6-6: AllowedTools missingread_resourcetool used in tests GO-2, GO-3, GO-7.Lines 54, 62, and 97 call
mcp__UnityMCP__read_resource, but this tool is not declared in the AllowedTools list. The test suite will fail at runtime when these test cases attempt to execute.Add the missing tool to the AllowedTools declaration:
-AllowedTools: Write,mcp__UnityMCP__manage_editor,mcp__UnityMCP__manage_gameobject,mcp__UnityMCP__find_gameobjects,mcp__UnityMCP__manage_components,mcp__UnityMCP__manage_scene,mcp__UnityMCP__read_console +AllowedTools: Write,mcp__UnityMCP__manage_editor,mcp__UnityMCP__manage_gameobject,mcp__UnityMCP__find_gameobjects,mcp__UnityMCP__manage_components,mcp__UnityMCP__manage_scene,mcp__UnityMCP__read_resource,mcp__UnityMCP__read_console
🤖 Fix all issues with AI agents
In @Server/src/services/resources/editor_state.py:
- Around line 128-172: Function _infer_single_instance_id declares an unused
ctx: Context parameter—remove the ctx parameter from the function signature and
all internal/type hints so the function becomes async def
_infer_single_instance_id() -> str | None, then update every call site that
passes a Context (the reviewed call site that previously invoked
_infer_single_instance_id(ctx) must be changed to call
_infer_single_instance_id() instead); while here the broad excepts are
acceptable for best-effort behavior, consider narrowing them
(ImportError/AttributeError) if you want more precise error handling.
In @Server/src/services/resources/menu_items.py:
- Around line 14-18: The decorator call using mcp_for_unity_resource has a typo
in the uri argument: change the duplicated scheme string
"mcpformcpforunity://menu-items" to the correct "mcpforunity://menu-items" in
the mcp_for_unity_resource(...) invocation so the resource URI matches the rest
of the project.
In @Server/src/services/resources/tests.py:
- Line 24: The URI strings in the mcp_for_unity_resource decorators are
misspelled with a duplicated prefix ("mcpformcpforunity://"); update the uri
values used in the @mcp_for_unity_resource decorators (e.g., the one annotating
get_tests and the other at the second occurrence) to use the correct scheme
"mcpforunity://" (so "mcpformcpforunity://tests" → "mcpforunity://tests" and
similarly for the other decorator).
In @Server/src/transport/plugin_hub.py:
- Line 149: The class references cls.FAST_FAIL_TIMEOUT in methods like
read_console, get_editor_state and ping but that attribute is missing; add a
FAST_FAIL_TIMEOUT class attribute alongside the other timeout constants in the
PluginHub class (e.g. define FAST_FAIL_TIMEOUT as a numeric timeout in seconds
consistent with the existing TIMEOUT/STARTUP_TIMEOUT values) so
cls.FAST_FAIL_TIMEOUT resolves and fast-fail commands use the intended timeout.
🧹 Nitpick comments (4)
Server/src/transport/plugin_hub.py (2)
51-52: Consider annotating mutable class attribute withClassVar.Mutable class attributes like
_FAST_FAIL_COMMANDSshould be annotated withtyping.ClassVarto clarify that they are class-level attributes and prevent accidental instance-level overrides.♻️ Proposed refactor
+from typing import Any, ClassVar + class PluginHub(WebSocketEndpoint): """Manages persistent WebSocket connections to Unity plugins.""" encoding = "json" KEEP_ALIVE_INTERVAL = 15 SERVER_TIMEOUT = 30 COMMAND_TIMEOUT = 30 # Fast-path commands should never block the client for long; return a retry hint instead. # This helps avoid the Cursor-side ~30s tool-call timeout when Unity is compiling/reloading # or is throttled while unfocused. - _FAST_FAIL_COMMANDS: set[str] = { + _FAST_FAIL_COMMANDS: ClassVar[set[str]] = { "read_console", "get_editor_state", "ping"}
452-453: Consider defining error messages as exception class attributes.Static analysis suggests moving the error message to the
NoUnitySessionErrorclass definition for better reusability and consistency. However, this is a minor refactoring opportunity since the message is only used once.♻️ Optional refactor
Define the message on the exception class:
class NoUnitySessionError(RuntimeError): """Raised when no Unity plugins are available.""" DEFAULT_MESSAGE = "No Unity plugins are currently connected"Then use it:
- raise NoUnitySessionError( - "No Unity plugins are currently connected") + raise NoUnitySessionError(NoUnitySessionError.DEFAULT_MESSAGE)Server/tests/integration/test_editor_state_v2_contract.py (1)
27-27: Consider prefixing unused parameters with underscore.The unused parameters
send_fn,unity_instance,params, andkwargsare intentionally present to match the expected signature for the monkeypatch. Consider prefixing them with underscore to signal they're deliberately unused and silence the linter.♻️ Optional refactoring to clarify intent
- async def fake_send_with_unity_instance(send_fn, unity_instance, command_type, params, **kwargs): + async def fake_send_with_unity_instance(_send_fn, _unity_instance, command_type, _params, **_kwargs): # Minimal stub payload for v2 resource tests. The server layer should enrich with staleness/advice. assert command_type == "get_editor_state"Server/src/services/resources/editor_state.py (1)
254-285: Consider logging the exception in the external changes block.The
try-except-passat lines 284-285 silently swallows all exceptions during external changes enrichment. While this prevents external change tracking failures from breaking the entire state response, it makes debugging difficult if issues occur.🔧 Recommended improvement
except Exception: - pass + # External changes enrichment is optional; failures shouldn't break editor_state. + # Log at debug level if observability is needed in the future. + passOr, if logging infrastructure is available:
- except Exception: - pass + except Exception as e: + # External changes enrichment is optional; log and continue. + logger.debug(f"Failed to enrich external changes for instance {instance_id}: {e}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
.claude/prompts/nl-gameobject-suite.md.claude/prompts/nl-unity-suite-nl.md.claude/prompts/nl-unity-suite-t.md.github/scripts/mark_skipped.pyMCPForUnity/Editor/Tools/ManageScript.csREADME-zh.mdREADME.mdServer/src/main.pyServer/src/services/registry/resource_registry.pyServer/src/services/resources/active_tool.pyServer/src/services/resources/custom_tools.pyServer/src/services/resources/editor_state.pyServer/src/services/resources/gameobject.pyServer/src/services/resources/layers.pyServer/src/services/resources/menu_items.pyServer/src/services/resources/prefab_stage.pyServer/src/services/resources/project_info.pyServer/src/services/resources/selection.pyServer/src/services/resources/tags.pyServer/src/services/resources/tests.pyServer/src/services/resources/unity_instances.pyServer/src/services/resources/windows.pyServer/src/services/tools/execute_custom_tool.pyServer/src/services/tools/find_gameobjects.pyServer/src/services/tools/find_in_file.pyServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/script_apply_edits.pyServer/src/services/tools/set_active_instance.pyServer/src/transport/legacy/unity_connection.pyServer/src/transport/plugin_hub.pyServer/tests/integration/test_edit_normalization_and_noop.pyServer/tests/integration/test_edit_strict_and_warnings.pyServer/tests/integration/test_editor_state_v2_contract.pyServer/tests/integration/test_gameobject_resources.pyServer/tests/integration/test_get_sha.pyServer/tests/integration/test_manage_script_uri.pyServer/tests/integration/test_script_tools.pyServer/tests/integration/test_validate_script_summary.pydocs/README-DEV-zh.mddocs/v8_NEW_NETWORKING_SETUP.md
✅ Files skipped from review due to trivial changes (5)
- .claude/prompts/nl-unity-suite-nl.md
- Server/src/services/tools/manage_components.py
- Server/tests/integration/test_gameobject_resources.py
- Server/src/services/registry/resource_registry.py
- Server/src/services/resources/unity_instances.py
🚧 Files skipped from review as they are similar to previous changes (3)
- Server/src/services/tools/set_active_instance.py
- Server/src/services/tools/script_apply_edits.py
- Server/src/main.py
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
.claude/prompts/nl-unity-suite-t.mdServer/tests/integration/test_script_tools.pydocs/v8_NEW_NETWORKING_SETUP.mdMCPForUnity/Editor/Tools/ManageScript.csServer/src/services/resources/project_info.pyREADME-zh.mdServer/src/services/tools/manage_script.py
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
.claude/prompts/nl-unity-suite-t.mdServer/tests/integration/test_script_tools.pydocs/v8_NEW_NETWORKING_SETUP.mdMCPForUnity/Editor/Tools/ManageScript.csREADME-zh.md
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
.claude/prompts/nl-unity-suite-t.mddocs/v8_NEW_NETWORKING_SETUP.mdServer/src/services/resources/custom_tools.pyMCPForUnity/Editor/Tools/ManageScript.csServer/src/services/resources/project_info.pyServer/src/services/tools/execute_custom_tool.pyREADME-zh.mdServer/src/services/resources/gameobject.pyREADME.md
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
.claude/prompts/nl-unity-suite-t.mdMCPForUnity/Editor/Tools/ManageScript.csdocs/README-DEV-zh.mdREADME.md
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
.claude/prompts/nl-unity-suite-t.mdServer/tests/integration/test_script_tools.pydocs/v8_NEW_NETWORKING_SETUP.mdServer/src/services/resources/custom_tools.pyMCPForUnity/Editor/Tools/ManageScript.csdocs/README-DEV-zh.mdServer/src/services/resources/layers.pyREADME-zh.mdREADME.md
📚 Learning: 2025-12-29T15:23:17.871Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Applied to files:
.claude/prompts/nl-unity-suite-t.mddocs/v8_NEW_NETWORKING_SETUP.mdREADME-zh.md
📚 Learning: 2025-12-19T21:09:44.740Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 468
File: Server/DOCKER_OVERVIEW.md:22-22
Timestamp: 2025-12-19T21:09:44.740Z
Learning: In the Unity MCP project, the Docker workflow (.github/workflows/publish-docker.yml) intentionally uses the DOCKER_USERNAME secret instead of hardcoding "msanatan" to allow users who fork the repository to publish to their own Docker Hub repositories, while the documentation (Server/DOCKER_OVERVIEW.md) correctly references the official image location "msanatan/mcp-for-unity-server" for end users.
Applied to files:
.claude/prompts/nl-unity-suite-t.mdREADME-zh.md
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/services/resources/menu_items.pydocs/v8_NEW_NETWORKING_SETUP.mdServer/src/services/resources/custom_tools.py.claude/prompts/nl-gameobject-suite.mdServer/src/services/resources/selection.pyMCPForUnity/Editor/Tools/ManageScript.csServer/src/services/resources/tests.pyServer/src/services/resources/tags.pyServer/src/services/resources/project_info.pyServer/src/services/resources/prefab_stage.pyServer/src/services/resources/layers.pyServer/src/services/tools/execute_custom_tool.pyServer/src/services/resources/active_tool.pyREADME-zh.mdServer/src/services/resources/gameobject.pyREADME.mdServer/src/services/resources/windows.py
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
Server/src/services/resources/menu_items.pydocs/v8_NEW_NETWORKING_SETUP.mdServer/src/services/resources/custom_tools.pyServer/src/services/resources/selection.pyServer/src/services/resources/project_info.pyServer/src/services/tools/execute_custom_tool.pyServer/src/services/resources/active_tool.pyREADME-zh.mdServer/src/services/tools/find_gameobjects.py
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
docs/v8_NEW_NETWORKING_SETUP.md.claude/prompts/nl-gameobject-suite.md
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/ManageScript.cs
📚 Learning: 2025-11-27T21:09:35.011Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 401
File: MCPForUnity/Editor/Clients/Configurators/VSCodeConfigurator.cs:10-18
Timestamp: 2025-11-27T21:09:35.011Z
Learning: VS Code GitHub Copilot MCP configuration supports mcp.json files placed directly in the Code/User directory: %APPDATA%\Code\User\mcp.json on Windows, ~/Library/Application Support/Code/User/mcp.json on macOS, and ~/.config/Code/User/mcp.json on Linux. This is in addition to workspace-scoped .vscode/mcp.json files.
Applied to files:
README-zh.md
🧬 Code graph analysis (7)
Server/tests/integration/test_script_tools.py (1)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
Server/src/services/resources/tests.py (1)
Server/src/services/registry/resource_registry.py (1)
mcp_for_unity_resource(10-43)
Server/tests/integration/test_get_sha.py (2)
Server/src/services/tools/manage_script.py (1)
get_sha(622-645)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
Server/tests/integration/test_validate_script_summary.py (2)
Server/src/services/tools/manage_script.py (1)
validate_script(463-500)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
Server/src/services/tools/find_in_file.py (1)
Server/src/services/tools/manage_script.py (1)
_split_uri(15-64)
Server/src/services/resources/editor_state.py (6)
Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)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-114)Server/src/transport/plugin_hub.py (1)
get_sessions(217-231)Server/src/services/resources/project_info.py (1)
get_project_info(30-39)Server/src/services/state/external_changes_scanner.py (2)
set_project_root(51-54)update_and_get(173-241)
Server/tests/integration/test_editor_state_v2_contract.py (2)
Server/src/services/registry/resource_registry.py (1)
get_registered_resources(46-48)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
🪛 LanguageTool
docs/README-DEV-zh.md
[uncategorized] ~206-~206: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：保守"地"响应
Context: ... - 当 include_properties=true 时，会启用保守的响应大小预算（约 ~250KB JSON 文本），返回条数可能少于 `p...
(wb4)
🪛 markdownlint-cli2 (0.18.1)
README-zh.md
214-214: Bare URL used
(MD034, no-bare-urls)
216-216: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
225-225: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
226-226: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
227-227: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
446-446: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Ruff (0.14.10)
Server/src/services/resources/editor_state.py
128-128: Unused function argument: ctx
(ARG001)
138-138: Do not catch blind exception: Exception
(BLE001)
155-155: Do not catch blind exception: Exception
(BLE001)
169-169: Do not catch blind exception: Exception
(BLE001)
179-179: Do not catch blind exception: Exception
(BLE001)
284-285: try-except-pass detected, consider logging the exception
(S110)
284-284: Do not catch blind exception: Exception
(BLE001)
297-297: Do not catch blind exception: Exception
(BLE001)
Server/src/transport/plugin_hub.py
51-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
452-453: Avoid specifying long messages outside the exception class
(TRY003)
Server/tests/integration/test_editor_state_v2_contract.py
14-14: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
27-27: Unused function argument: send_fn
(ARG001)
27-27: Unused function argument: unity_instance
(ARG001)
27-27: Unused function argument: params
(ARG001)
27-27: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (41)
Server/src/services/tools/find_gameobjects.py (1)
17-43: LGTM! Clean URL scheme migration and formatting improvements.The changes successfully migrate from
unity://tomcpforunity://URL scheme as intended in the PR objectives. The formatting adjustments (multi-line Literal type, Annotated parameter wrapping, docstring whitespace) improve readability without affecting runtime behavior..github/scripts/mark_skipped.py (1)
27-27: LGTM! Pattern updated to match the new URI scheme.The pattern correctly reflects the migration from
unity://tomcpforunity://throughout the codebase, ensuring that unknown-resource errors using the new scheme are properly classified as environmental failures and marked as skipped.Server/src/transport/legacy/unity_connection.py (1)
587-587: LGTM! URI scheme updated correctly.The error message now references
mcpforunity://instancesinstead of the legacyunity://scheme, aligning with the PR-wide migration to avoid naming conflicts..claude/prompts/nl-gameobject-suite.md (1)
54-54: Confirm URI scheme migration is complete and correct.All resource URIs have been migrated from
unity://tomcpforunity://scheme, which aligns with the PR objectives. The changes are consistent across test specs (GO-2, GO-3, GO-7) and the Tool Reference section (lines 134–136).Verify that the new
mcpforunity://URIs are registered server-side and match the corresponding handler implementations. If you have access to the server-side resource registration code, confirm that these URI patterns are properly defined.Also applies to: 62-62, 97-97, 134-136
Server/src/services/resources/active_tool.py (1)
33-37: LGTM! URI scheme migration applied correctly.The URI scheme has been updated from
unity://tomcpforunity://as intended. The internal command identifier and external API name remain unchanged, ensuring compatibility with the C# side and a clean migration path.docs/README-DEV-zh.md (2)
1-360: Verify URI scheme documentation reflects mcpforunity:// migration.The AI summary notes that resource URIs were changed from
unity://tomcpforunity://throughout the codebase. While the provided documentation appears to focus on development workflows rather than API references, you may want to verify that any examples or references to editor-state endpoints or tool resources elsewhere in the repo (or in complementary documentation like README-DEV.md) have been consistently updated to reflect the new URI scheme.
206-206: Fix Chinese grammar in line 206.Per LanguageTool, the phrase "保守的响应大小预算" should be "保守地响应大小预算" to maintain the correct grammatical structure for a verb modifier (adjective/adverb + 地 + verb).
📝 Proposed grammar correction
- - 当 `include_properties=true` 时，会启用保守的响应大小预算（约 **~250KB** JSON 文本），返回条数可能少于 `page_size`；使用 `next_cursor` 继续。 + - 当 `include_properties=true` 时，会启用保守地响应大小预算（约 **~250KB** JSON 文本），返回条数可能少于 `page_size`；使用 `next_cursor` 继续。Likely an incorrect or invalid review comment.
Server/src/services/resources/tags.py (1)
17-17: LGTM—URI scheme migration completed correctly.The URI update from
unity://tomcpforunity://project/tagsaligns with the PR objectives to avoid naming conflicts. Documentation (README.md and README-zh.md) has been updated to reflect the new URI scheme. The internal command identifier and external API name remain unchanged, ensuring proper decoupling.Server/src/services/tools/find_in_file.py (1)
16-29: URI scheme migration is complete and verified.The change from
unity://path/tomcpforunity://path/has been successfully applied across the codebase. All callers—including test files and related tool modules (manage_script.py,script_apply_edits.py)—have been updated to use the newmcpforunity://scheme. No remaining references to the oldunity://path/scheme exist.MCPForUnity/Editor/Tools/ManageScript.cs (2)
2640-2641: LGTM! URI parsing logic correctly updated.The
SanitizeAssetsPathmethod now strips the newmcpforunity://path/prefix. The substring length calculation is correct (19 characters), and the case-insensitive comparison is appropriate for URI scheme matching.
285-285: URI scheme migration is complete and consistent.All response URI constructions have been properly updated from
unity://path/tomcpforunity://path/across get_sha, create, read, update, apply_text_edits (both cases), and edit actions. The SanitizeAssetsPath function correctly handles the new scheme by checking and stripping themcpforunity://path/prefix. No lingeringunity://references remain in the file.Also applies to: 375-375, 404-404, 484-484, 707-707, 808-808, 1378-1378, 1444-1444
Server/src/transport/plugin_hub.py (1)
420-420: URI migration looks good.The updates from
unity://instancestomcpforunity://instancesare consistent with the PR objectives to avoid naming conflicts and align with Unity's requests.Also applies to: 441-441
Server/tests/integration/test_get_sha.py (1)
42-53: LGTM! URI scheme migration properly applied.The test correctly updates both the mock response data and the test input to use the new
mcpforunity://URI scheme, maintaining consistency between the mocked Unity response and the test invocation.Server/tests/integration/test_validate_script_summary.py (1)
55-55: LGTM! Test updated for new URI scheme.The test invocation correctly uses the
mcpforunity://URI scheme.Server/tests/integration/test_script_tools.py (4)
71-71: LGTM! Test updated for new URI scheme.
99-104: LGTM! Sequential edits test updated consistently.Both test invocations correctly use the
mcpforunity://URI scheme.
135-135: LGTM! Options forwarding test updated.
168-168: LGTM! Multi-span test updated.Server/tests/integration/test_manage_script_uri.py (1)
52-52: LGTM! URI parsing test updated for new scheme.The test correctly verifies that the
mcpforunity://URI scheme is properly parsed into name (MyScript) and path (Assets/Scripts) components.Server/src/services/resources/custom_tools.py (1)
25-34: LGTM! Resource URI and error messaging updated consistently.Both the resource decorator URI and the error message reference correctly use the new
mcpforunity://scheme. The error message appropriately directs users tomcpforunity://instancesfor resolving the active Unity instance.docs/v8_NEW_NETWORKING_SETUP.md (1)
189-189: LGTM: Documentation correctly updated to reflect new URI scheme.The URI update from
unity://custom-toolstomcpforunity://custom-toolsaligns with the broader migration to avoid naming conflicts with Unity and other MCPs.Server/src/services/resources/windows.py (1)
34-34: LGTM: Clean URI migration.The resource URI has been correctly updated to the new
mcpforunity://scheme while maintaining backward compatibility through unchanged internal command routing.Server/src/services/resources/selection.py (1)
42-42: LGTM: URI scheme updated consistently.The resource endpoint has been migrated to
mcpforunity://editor/selectionwhile preserving the internal command identifier and function behavior.Server/src/services/resources/prefab_stage.py (1)
26-26: LGTM: Prefab stage resource URI updated.The endpoint migration to
mcpforunity://editor/prefab-stageis consistent with the broader URI scheme change across all resources.Server/src/services/tools/manage_script.py (2)
15-64: LGTM: URI parsing correctly extended to support new scheme.The
_split_urifunction has been properly updated to handlemcpforunity://path/URIs alongside existingfile://and plain path formats. The prefix stripping logic is correct, and the normalized path flows through the same validation and Assets-relative path extraction as before.
89-89: LGTM: Parameter annotations consistently document new URI scheme.All script management tools that accept a
uriparameter (apply_text_edits,delete_script,validate_script,get_sha) have been updated to document support for themcpforunity://path/Assets/...format alongside existing schemes. This provides clear guidance for MCP clients and developers.Also applies to: 437-437, 465-465, 624-624
README-zh.md (2)
79-96: LGTM! Resource listings updated correctly.The resource URIs have been properly migrated to the
mcpforunity://scheme, and the descriptions align with the expanded public API catalog mentioned in the PR objectives.
367-376: Good addition: Performance guidance for batch_execute.The before/after examples clearly demonstrate the performance benefits of using
batch_executefor multiple operations. This is helpful user-facing documentation.Server/src/services/tools/execute_custom_tool.py (1)
24-27: LGTM! Error message updated to match new URI scheme.The error message correctly guides users to
mcpforunity://instances, aligning with the PR's URI migration fromunity://tomcpforunity://.Server/tests/integration/test_edit_normalization_and_noop.py (1)
55-58: LGTM! Test URIs updated consistently.All test cases correctly use the new
mcpforunity://URI scheme, maintaining test coverage while aligning with the PR's URI migration objectives.Also applies to: 82-85, 111-116, 163-167
Server/src/services/resources/project_info.py (1)
25-29: LGTM! Resource URI updated to new scheme.The URI correctly migrates from
unity://tomcpforunity://, aligning with the PR's objective to avoid naming conflicts. The internal command identifier remains unchanged, which is the correct approach per project conventions.Server/tests/integration/test_edit_strict_and_warnings.py (1)
47-47: LGTM! URI scheme migration is correct.The URI scheme updates from
unity://tomcpforunity://align with the project-wide migration described in the PR objectives.Also applies to: 80-80
Server/src/services/resources/layers.py (1)
16-16: LGTM! URI scheme migration is correct.The URI scheme update from
unity://project/layerstomcpforunity://project/layersis consistent with the project-wide migration to avoid conflicts with other MCPs and Unity naming.Server/src/services/resources/gameobject.py (1)
5-7: LGTM! URI scheme migration and documentation updates are correct.All GameObject resource URIs and documentation examples have been correctly updated from
unity://scene/gameobject/...tomcpforunity://scene/gameobject/.... The changes are consistent and comprehensive.Also applies to: 43-43, 66-83, 130-130, 171-171, 217-217
Server/tests/integration/test_editor_state_v2_contract.py (1)
11-11: LGTM! Editor state migration to unified resource is correct.The test correctly migrates from v2-specific to the canonical
mcpforunity://editor/stateresource. The import on line 14 properly triggers decorator registration (thenoqa: F401comment is intentional and should remain), and the resource lookup and invocation logic have been appropriately updated.Also applies to: 13-14, 18-25, 29-29, 45-45
README.md (1)
58-72: Documentation updates look excellent.The tool and resource documentation changes are clear, consistent, and well-aligned with the PR objectives:
- New tools (
find_in_file,debug_request_context,apply_text_edits,script_apply_edits,validate_script,create_script,delete_script,get_sha) are properly documented with concise descriptions- URI scheme migration from
unity://tomcpforunity://is consistently applied across all resources- The
run_teststool description accurately reflects the async/polling modelAlso applies to: 81-97
Server/src/services/resources/editor_state.py (5)
1-13: LGTM! Imports are clean and purposeful.All new imports support the refactored editor state infrastructure (external changes scanning, transport integration, timing utilities, type hints).
16-116: Well-structured data models for the v2 schema.The EditorStateV2* models provide a clear, strongly-typed contract for the editor state snapshot. The use of optional fields throughout is appropriate for a flexible state representation that may be partially populated depending on Unity's current state.
119-126: LGTM! Utility functions are simple and correct.Both helpers serve clear purposes:
_now_unix_ms()provides consistent timestamp formatting, and_in_pytest()prevents discovery side effects during testing.
174-212: Solid readiness assessment logic.The function correctly computes staleness, identifies blocking conditions (compilation, domain reload, tests, asset refresh), and provides actionable advice. The 2-second staleness threshold is well-documented and reasonable for editor polling scenarios.
215-305: Well-structured editor state handler with good error handling.The refactored
get_editor_statefunction properly:
- Migrates to the
mcpforunity://editor/stateURI scheme- Ensures Unity instance identification (explicit or inferred)
- Enriches state with server-side external changes detection
- Validates the payload against the typed EditorStateV2Data schema
- Returns structured errors on validation failure
The overall design is solid and handles edge cases appropriately.
We now differentiate whether it's HTTP local or remote
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Server/src/transport/plugin_hub.py (1)
48-55: LGTM – Previous critical issue resolved.The
FAST_FAIL_TIMEOUTconstant is now properly defined, resolving theAttributeErrorflagged in the previous review. The comment clearly explains the rationale for the short timeout.Optional: Consider annotating
_FAST_FAIL_COMMANDSwithClassVarto satisfy the static analysis hint (RUF012) and make the intent explicit that this is a class-level constant not meant to be overridden per-instance.♻️ Optional type annotation fix
+from typing import Any, ClassVar -from typing import Any ... - _FAST_FAIL_COMMANDS: set[str] = { + _FAST_FAIL_COMMANDS: ClassVar[set[str]] = { "read_console", "get_editor_state", "ping"}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Dependencies/DependencyManager.csMCPForUnity/Editor/MenuItems/MCPForUnityMenu.csServer/src/transport/plugin_hub.pytools/prepare_unity_asset_store_release.py
💤 Files with no reviewable changes (1)
- tools/prepare_unity_asset_store_release.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Editor/Dependencies/DependencyManager.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Dependencies/DependencyManager.csMCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Editor/Dependencies/DependencyManager.cs
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs
🪛 Ruff (0.14.10)
Server/src/transport/plugin_hub.py
54-55: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
455-456: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (10)
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs (1)
23-23: LGTM! Clear improvement to menu label.The addition of "Local" to the Setup Window menu item improves clarity and aligns with the PR's objective to differentiate local vs remote setup workflows.
MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
139-139: LGTM! Consistent with menu item update.The message correctly references the updated "Local Setup Window" menu path, maintaining consistency with the MenuItem change in MCPForUnityMenu.cs.
Server/src/transport/plugin_hub.py (8)
125-126: LGTM – formatting only.
148-156: LGTM – cleaner timeout resolution.Using the class constant
cls.FAST_FAIL_TIMEOUTinstead of repeated environment variable lookups improves consistency and maintainability.
185-186: LGTM – formatting only.
376-381: LGTM – proper fallback in exception path.
421-424: LGTM – URI scheme updated consistently.The error messages now correctly reference
mcpforunity://instances, aligning with the PR objective to migrate all resource URIs to themcpforunityscheme to avoid conflicts.Also applies to: 442-445
455-456: LGTM – clear error message.The static analysis hint (TRY003) suggests moving the message into the exception class, but this is a minor stylistic preference. The current approach is readable and the message is appropriately concise.
489-497: LGTM – consistent environment variable handling.The multiline formatting and exception handling pattern matches the approach used elsewhere in the file.
509-510: LGTM – formatting only.
…ypes Added SerializeAnimationCurve and SerializeMinMaxCurve helper methods to properly serialize Unity's curve types. Updated GetInfo to use these helpers for startLifetime, startSpeed, startSize, gravityModifier, and rateOverTime instead of only reading constant values.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/refresh_unity.py (1)
86-91: Add logging to the broad exception handler.The broad
except Exceptionblock silently swallows all errors, which can hide real issues such as bugs in theeditor_statemodule, network problems, or configuration errors. While the cleanup may be best-effort, completely silent failures make debugging very difficult.Add at least debug-level logging to provide visibility into why cleanup failed.
🔍 Proposed fix to add logging
+import logging + +logger = logging.getLogger(__name__) + ... # After readiness is restored, clear any external-dirty flag for this instance so future tools can proceed cleanly. try: inst = unity_instance or await editor_state._infer_single_instance_id(ctx) if inst: external_changes_scanner.clear_dirty(inst) - except Exception: - pass + except Exception as e: + logger.debug(f"Failed to clear dirty flag after refresh: {e}")
🤖 Fix all issues with AI agents
In @MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs:
- Around line 94-107: In GameObjectModify.cs update the layer validation in the
block that reads layerName and calls LayerMask.NameToLayer: remove the
unnecessary "Default" special-case from the condition `if (layerId == -1 &&
layerName != "Default")` so it simply checks `if (layerId == -1)` and returns
the ErrorResponse on invalid names; keep the subsequent `if (layerId != -1 &&
targetGo.layer != layerId)` branch that sets `targetGo.layer` and `modified =
true` unchanged.
In @MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs:
- Around line 60-85: In AddBurst, the int→short casts for minCount and maxCount
can overflow if JSON provides values > short.MaxValue or negative; parse the
ints into local int variables first (e.g., parsedMin, parsedMax), validate or
clamp them to the valid short range (and ensure non-negative for burst counts),
then safely cast to short (or fail/return an error if out-of-range) before
constructing the ParticleSystem.Burst; update references to minCount and
maxCount in the method accordingly to avoid overflow and unexpected behavior.
In @Server/src/services/tools/refresh_unity.py:
- Line 87: The code is calling the private function
editor_state._infer_single_instance_id(ctx); change this to use a public API:
either rename the function to editor_state.infer_single_instance_id (remove the
leading underscore) or add a new public wrapper function (e.g.,
infer_single_instance_id) in the editor_state module that delegates to the
existing implementation, then update the call here to use inst = unity_instance
or await editor_state.infer_single_instance_id(ctx) so no private member is
accessed directly.
- Line 16: You are calling the private helper
editor_state._infer_single_instance_id(ctx) and silently swallowing all
exceptions; replace the private call with a public API (either use an existing
public function in editor_state or add a new exported function like
infer_single_instance_id(ctx)) and update the call site to use that public
function, and change the try/except to not catch Exception blindly—catch
specific exceptions you expect (e.g., ValueError or RuntimeError), or log the
exception via the module logger and re-raise if unrecoverable; ensure the
subsequent clear_dirty() is only called when the public infer function succeeds.
In @Server/src/services/tools/run_tests.py:
- Around line 190-195: The function currently constructs
GetTestJobResponse(**response) but returns the raw dict, breaking the declared
return type (GetTestJobResponse | MCPResponse); replace the final successful
branch to return the typed object instead of the dict (i.e., return
GetTestJobResponse(**response)), and/or update the function's return annotation
to dict | MCPResponse if you intend to return raw dicts; ensure any redundant
GetTestJobResponse(...) construction is removed or used and that imports/types
remain consistent.
- Around line 151-158: The function currently validates the payload by calling
RunTestsStartResponse(**response) but then returns the raw dict, breaking the
declared return type RunTestsStartResponse | MCPResponse; change the
implementation to return the constructed typed object (the RunTestsStartResponse
instance) instead of the raw dict (i.e., replace the discarded
RunTestsStartResponse(**response) call with returning that instance), or if raw
dicts must be preserved update the function signature to dict | MCPResponse and
adjust callers; reference RunTestsStartResponse, MCPResponse and the local
response variable when making the fix.
🧹 Nitpick comments (3)
Server/src/services/tools/run_tests.py (1)
120-128: Consider raising TypeError for unexpected input types.The
_coerce_string_listhelper silently returnsNonewhenvalueis neitherNone,str, norlist(line 128). This could hide programming errors where an unexpected type (e.g.,int,dict) is passed.♻️ Proposed fix
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 + raise TypeError(f"Expected str, list, or None; got {type(value).__name__}")MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs (1)
70-70: Simplify the LINQ Contains call.The fully qualified
System.Linq.Enumerable.Containsis unnecessarily verbose. Sinceusing System.Linq;is typically included at the top of the file, you can simplify this.♻️ Proposed simplification
- if (tagToSet != "Untagged" && !System.Linq.Enumerable.Contains(InternalEditorUtility.tags, tagToSet)) + if (tagToSet != "Untagged" && !InternalEditorUtility.tags.Contains(tagToSet))Server/src/services/tools/refresh_unity.py (1)
74-80: Consider simplifying the nested state extraction logic.The defensive extraction with multiple
isinstancechecks and nested.get()calls is correct but quite complex. While this might be intentional during the migration to handle various response formats, consider simplifying once the migration is complete.♻️ Potential simplification
- state_resp = await editor_state.get_editor_state(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: + state_resp = await editor_state.get_editor_state(ctx) + state = state_resp.model_dump() if hasattr(state_resp, "model_dump") else state_resp + ready = state.get("data", {}).get("advice", {}).get("ready_for_tools", False) if isinstance(state, dict) else False + if ready is True: break
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
MCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csMCPForUnity/Editor/Tools/Vfx/ParticleControl.csMCPForUnity/Editor/Tools/Vfx/ParticleRead.csMCPForUnity/Editor/Tools/Vfx/ParticleWrite.csServer/src/services/tools/refresh_unity.pyServer/src/services/tools/run_tests.py
🚧 Files skipped from review as they are similar to previous changes (3)
- MCPForUnity/Editor/Tools/Vfx/ParticleWrite.cs
- MCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.cs
- MCPForUnity/Editor/Tools/Vfx/ParticleRead.cs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/services/tools/refresh_unity.py
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
Server/src/services/tools/refresh_unity.py
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Services/EditorStateCache.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csMCPForUnity/Editor/Tools/Vfx/ParticleControl.cs
🧬 Code graph analysis (3)
Server/src/services/tools/run_tests.py (3)
Server/src/services/tools/preflight.py (1)
preflight(27-110)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-114)
MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs (3)
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs (5)
Type(395-408)GameObjectComponentHelpers(16-409)RemoveComponentInternal(85-118)AddComponentInternal(18-83)SetComponentPropertiesInternal(120-172)MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Info(37-41)Warn(43-46)MCPForUnity/Editor/Tools/ManageEditor.cs (1)
AddTag(168-191)
MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs (3)
MCPForUnity/Editor/Tools/Vfx/ParticleCommon.cs (4)
ParticleSystem(8-12)ParticleSystem(14-50)ParticleSystem(52-85)ParticleCommon(6-86)MCPForUnity/Editor/Tools/Vfx/LineWrite.cs (1)
Clear(126-137)MCPForUnity/Editor/Tools/Vfx/TrailControl.cs (1)
Clear(9-17)
🔇 Additional comments (21)
MCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.cs (1)
1-84: Well-structured duplication handler with proper Unity Editor integration.The implementation correctly:
- Registers undo for the created object before modifications
- Handles all parent scenarios (inherit, explicit root, specified parent)
- Marks the scene dirty for proper save behavior
- Returns comprehensive response data
The past review comment about the misleading warning message has been addressed (line 61 now correctly states the object will remain at root level).
MCPForUnity/Editor/Services/EditorStateCache.cs (3)
36-229: Well-structured typed model for editor state snapshot.The introduction of strongly-typed nested classes with
JsonPropertyattributes is a solid improvement over anonymous objects. This provides:
- Compile-time type safety
- Clear schema documentation
- Better IDE support and refactoring capabilities
The
sealedmodifier on all classes is appropriate for internal DTOs.One minor observation:
EditorStateV2LastRun.Counts(line 219) is typed asobject, which loses type safety. If the counts structure is known, consider defining a concrete type. However, this is acceptable if the structure varies or is currently undefined.
378-391: Placeholder values in Assets section are acceptable but may need future attention.The
ExternalChangesDirty,ExternalChangesLastSeenUnixMs,ExternalChangesDirtySinceUnixMs,ExternalChangesLastClearedUnixMs, andRefreshfields are currently hardcoded tofalse/null. Similarly,Transportfields (lines 408-412) and someTestsfields are placeholders.This is fine for the current pre-release work. Consider adding TODO comments or tracking these for future implementation if external change detection and transport status are needed features.
418-429: Thread-safe snapshot retrieval with defensive cloning.The
GetSnapshotmethod correctly:
- Acquires a lock before accessing cached state
- Rebuilds the snapshot if
_cachedis null (defensive fallback)- Returns a deep clone to prevent external mutation of the cached object
This is a sound implementation for a cache that may be accessed from multiple contexts.
MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs (3)
10-38: LGTM! EnableModule follows best practices.The method correctly handles null checks, records undo, provides a default case for unknown modules, and marks the object dirty. The underscore stripping at line 22 provides flexible input handling for module names.
40-58: Previous review concern addressed - default case now present.The default case at line 54 now handles unknown actions appropriately, addressing the concern from the previous review. The method correctly omits Undo/SetDirty since these operations control runtime particle preview behavior rather than modifying serialized state.
87-99: LGTM! ClearBursts follows best practices.The method correctly handles the particle system lookup, records undo, clears the bursts, marks the object dirty, and provides clear feedback about the operation.
Server/src/services/tools/run_tests.py (2)
1-16: LGTM!The updated docstring and imports appropriately reflect the transition to an async job-based workflow.
43-87: LGTM!The new data models are well-structured and provide comprehensive typing for the async job lifecycle, supporting detailed progress tracking and flexible result inclusion.
MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs (11)
15-21: LGTM!The target resolution and error handling are appropriate and follow the established patterns in the codebase.
23-24: LGTM!Proper undo recording for both transform and GameObject properties ensures changes can be reverted.
28-33: LGTM!Name modification logic is straightforward and correct.
35-56: LGTM!The parent modification logic includes proper validation for circularity and null handling, preventing hierarchy loops and supporting both parenting and unparenting operations.
58-63: LGTM!SetActive modification is correctly implemented.
65-92: LGTM!The tag modification logic includes dynamic tag creation, which provides good user experience by automatically creating missing tags rather than failing. Error handling for both tag creation and assignment is appropriate.
109-127: LGTM!Transform modification logic correctly uses local transform properties and includes change detection to avoid unnecessary modifications.
129-142: LGTM!Component removal logic uses early return on first error, ensuring all-or-nothing semantics for the removal operations.
144-166: LGTM!Component addition logic supports both simple string type names and objects with properties, providing flexibility in the API. Error handling follows the same early-return pattern as component removal.
168-188: LGTM!The component property setting logic intentionally accumulates errors rather than using early return, allowing partial success when updating multiple components. This design provides better UX compared to the all-or-nothing approach used for add/remove operations.
225-237: LGTM!The success response logic appropriately distinguishes between no-op operations and actual modifications, providing clear feedback to the caller. Including serialized GameObject data in both cases allows verification of the final state.
Server/src/services/tools/refresh_unity.py (1)
29-34: LGTM! Parameter formatting improves readability.The multiline formatting of type annotations makes the function signature more readable without changing functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Server/src/services/resources/editor_state.py (3)
123-125: Consider consolidating duplicate_in_pytest()helper.This function duplicates the implementation in
Server/src/services/tools/preflight.py(lines 10-13). Consider importing from a shared utility module to maintain a single source of truth.♻️ Proposed refactor
+from services.tools.preflight import _in_pytest + -def _in_pytest() -> bool: - # Avoid instance-discovery side effects during the Python integration test suite. - return bool(os.environ.get("PYTEST_CURRENT_TEST"))
285-286: Consider logging exceptions in external changes integration.The bare
try-except-passsilently swallows all exceptions during external changes detection. While graceful degradation is appropriate here, logging the exception would aid debugging when external change tracking fails unexpectedly.♻️ Proposed improvement
except Exception: - pass + ctx.error("Failed to integrate external changes detection", exc_info=True)
256-269: Optional: Cache project root for performance.The function calls
get_project_info()on everyget_editor_state()invocation to retrieve the project root. Since the project root is unlikely to change during an editor session, consider caching it per instance to reduce overhead if this resource is polled frequently.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/src/services/resources/editor_state.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
🧬 Code graph analysis (1)
Server/src/services/resources/editor_state.py (6)
Server/src/services/registry/resource_registry.py (1)
mcp_for_unity_resource(10-43)Server/src/transport/legacy/unity_connection.py (2)
async_send_command_with_retry(838-870)get_unity_connection_pool(652-665)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-114)Server/src/transport/plugin_hub.py (2)
PluginHub(41-558)get_sessions(220-234)Server/src/services/state/external_changes_scanner.py (2)
set_project_root(51-54)update_and_get(173-241)
🪛 Ruff (0.14.10)
Server/src/services/resources/editor_state.py
139-139: Do not catch blind exception: Exception
(BLE001)
156-156: Do not catch blind exception: Exception
(BLE001)
170-170: Do not catch blind exception: Exception
(BLE001)
180-180: Do not catch blind exception: Exception
(BLE001)
285-286: try-except-pass detected, consider logging the exception
(S110)
285-285: Do not catch blind exception: Exception
(BLE001)
298-298: Do not catch blind exception: Exception
(BLE001)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
It's the default, there's no old version
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs (1)
94-107: Optional: Remove redundant condition at line 102.The check
layerId != -1at line 102 is redundant since lines 98-101 already return early iflayerId == -1. The condition can be simplified to justif (targetGo.layer != layerId).♻️ Simplified condition
int layerId = LayerMask.NameToLayer(layerName); if (layerId == -1) { return new ErrorResponse($"Invalid layer specified: '{layerName}'. Use a valid layer name."); } - if (layerId != -1 && targetGo.layer != layerId) + if (targetGo.layer != layerId) { targetGo.layer = layerId; modified = true; }Server/tests/integration/test_run_tests_async.py (1)
6-34: Consider renaming test function for clarity.The test function is named
test_run_tests_async_forwards_paramsbut now tests therun_testsfunction (the "_async" suffix is no longer part of the function name being tested). While the test logic is correct and properly validates parameter forwarding and the typed response objects, renaming totest_run_tests_forwards_paramswould improve clarity.♻️ Suggested rename
-async def test_run_tests_async_forwards_params(monkeypatch): +async def test_run_tests_forwards_params(monkeypatch):Server/src/services/resources/editor_state.py (1)
254-284: Consider logging exceptions from external changes scanner.The external changes scanner integration (lines 254-284) is wrapped in
try/except: pass, which silently suppresses all errors. While it's appropriate that external enrichment failures shouldn't break the main response, the complete silence makes debugging difficult.💡 Optional: Add debug logging
except Exception: - pass + await ctx.debug("External changes scanner enrichment failed; continuing without external change data")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
MCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csMCPForUnity/Editor/Tools/Vfx/ParticleControl.csServer/src/services/resources/editor_state.pyServer/src/services/tools/refresh_unity.pyServer/src/services/tools/run_tests.pyServer/tests/integration/test_run_tests_async.py
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/services/tools/refresh_unity.py
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
Server/src/services/tools/refresh_unity.py
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Services/EditorStateCache.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csMCPForUnity/Editor/Tools/Vfx/ParticleControl.cs
🧬 Code graph analysis (4)
Server/src/services/tools/refresh_unity.py (2)
Server/src/services/resources/editor_state.py (2)
get_editor_state(219-304)infer_single_instance_id(128-170)Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
Server/src/services/tools/run_tests.py (5)
Server/src/services/tools/preflight.py (1)
preflight(27-110)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)MCPForUnity/Editor/Services/TestJobManager.cs (1)
TestJobFailure(20-24)Server/tests/integration/test_helpers.py (1)
error(46-47)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-114)
Server/tests/integration/test_run_tests_async.py (2)
Server/src/services/tools/run_tests.py (2)
run_tests(97-155)get_test_job(165-191)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs (1)
MCPForUnity/Editor/Tools/Vfx/ParticleCommon.cs (4)
ParticleSystem(8-12)ParticleSystem(14-50)ParticleSystem(52-85)ParticleCommon(6-86)
🪛 Ruff (0.14.10)
Server/src/services/resources/editor_state.py
137-137: Do not catch blind exception: Exception
(BLE001)
154-154: Do not catch blind exception: Exception
(BLE001)
168-168: Do not catch blind exception: Exception
(BLE001)
178-178: Do not catch blind exception: Exception
(BLE001)
283-284: try-except-pass detected, consider logging the exception
(S110)
283-283: Do not catch blind exception: Exception
(BLE001)
296-296: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (22)
MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs (4)
10-38: LGTM! Module toggle implementation is solid.The method correctly handles module enabling/disabling with proper null checks, undo support, and the default case for unknown modules. The underscore removal normalization (line 22) is a nice touch for input flexibility.
40-58: Default case properly handles unknown actions.The Control method now includes a default case (line 54) that returns an appropriate error for unrecognized actions. The implementation correctly handles all common playback controls with proper parameter handling.
60-87: Integer overflow safely handled with Math.Clamp.The AddBurst method now correctly prevents integer overflow (lines 71-72) by parsing to
int, clamping to the validshortrange, then casting. This addresses the previous concern about values exceedingshort.MaxValue. The burst array manipulation and parameter handling are implemented correctly.
89-101: LGTM! Burst clearing is correctly implemented.The method properly clears all bursts with undo support and dirty marking. Storing the count before clearing (line 96) allows for an informative success message.
MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs (3)
35-56: LGTM: Excellent defensive checks for parent modification.The parent modification logic properly handles null/empty values for unparenting, and the loop detection using
IsChildOfat lines 47-49 prevents hierarchy cycles. The use ofSetParentwithworldPositionStays=trueis appropriate for preserving world-space positioning.
190-223: LGTM: Improved error aggregation with exception logging.The error aggregation logic now properly logs exceptions during reflection-based error extraction (lines 213-216), ensuring that failures in the aggregation process are visible while allowing the method to continue processing remaining errors. This provides good visibility into any issues with the error handling logic itself.
15-238: LGTM: Well-structured GameObject modification handler.The overall implementation demonstrates solid design:
- Comprehensive parameter handling with appropriate validation
- Proper Undo recording before modifications
- Defensive checks (null handling, hierarchy loop detection, tag/layer validation)
- Contextual error messages
- Distinction between early-return errors (add/remove components) and collected errors (property setting) allows for appropriate failure modes
- Clean separation of concerns using helper classes
Server/src/services/tools/run_tests.py (4)
1-16: LGTM! Clear documentation and well-organized imports.The module docstring accurately describes the new job-based workflow, and all imports are appropriate for the async test runner implementation.
43-88: LGTM! Well-structured data models for the job-based workflow.The Pydantic models properly define the async test job lifecycle with appropriate typing and optional fields. The snake_case naming follows Python conventions and will be correctly serialized/deserialized.
97-155: LGTM! Job-start workflow correctly implemented.The function properly:
- Validates preconditions via preflight (no tests running, no compilation, assets refreshed if dirty)
- Normalizes filter parameters with the
_coerce_string_listhelper- Sends the command asynchronously via unified transport
- Returns typed
RunTestsStartResponseobjects (correctly addresses previous type annotation concerns)The camelCase parameter keys (
testNames,groupNames, etc.) appropriately match Unity/C# naming conventions.
158-191: LGTM! Polling endpoint properly implemented.The function correctly:
- Omits preflight checks (appropriate for read-only polling)
- Forwards job_id and detail flags to Unity
- Returns typed
GetTestJobResponseobjects (correctly addresses previous type annotation concerns)The consistent response-handling pattern across both endpoints improves maintainability.
Server/tests/integration/test_run_tests_async.py (1)
36-56: LGTM! Test correctly validates the polling endpoint.The test properly verifies that
get_test_jobforwards the job_id parameter and returns typed response objects with the expected structure. The attribute-based assertions correctly match the new API surface.MCPForUnity/Editor/Services/EditorStateCache.cs (2)
36-230: LGTM! Well-structured typed snapshot model.The nested class hierarchy provides a clear, type-safe representation of the editor state schema. The JsonProperty attributes correctly map C# PascalCase properties to snake_case JSON keys, ensuring compatibility with the Python side.
The inclusion of
ExternalChangesDirtySinceUnixMsandExternalChangesLastClearedUnixMsfields (lines 167-171) addresses the schema mismatch flagged in previous reviews.
334-415: LGTM! Clean transition to typed snapshot construction.The
BuildSnapshotmethod now constructs a strongly-typedEditorStateSnapshotgraph, which improves type safety and makes the serialization contract explicit. The initialization logic correctly populates all fields, including the newly addedExternalChangesDirtySinceUnixMsandExternalChangesLastClearedUnixMs(lines 383-384).Note: A previous review flagged that
Activity.SinceUnixMs(line 366) always equals the current observation time rather than tracking when the activity phase started. This is marked as addressed in past comments, so no further action is needed here unless the team decides to implement phase-transition tracking.Server/src/services/resources/editor_state.py (5)
16-117: LGTM! Comprehensive and well-structured Pydantic models.The model hierarchy (EditorStateUnity, EditorStateEditor, EditorStateActivity, etc.) provides clear schema definitions that align with the C# counterparts in EditorStateCache.cs. The use of optional fields (
| None = None) and proper type hints ensures flexibility and type safety.The
counts: Anyfield (line 75) inEditorStateLastRunis appropriate for handling variable test result structures.
119-126: LGTM! Clean utility helpers.
_now_unix_ms()provides consistent timestamp generation, and_in_pytest()prevents unintended side effects during test execution. Both are well-implemented and appropriately scoped.
128-170: LGTM! Public API correctly addresses past review concerns.The function is now public (line 128, no underscore prefix), which properly addresses previous feedback about exposing this functionality. The implementation handles both transport types with appropriate fallback behavior.
The broad
except Exceptionhandlers (lines 137, 154, 168) are flagged by static analysis but are appropriate here—this is a best-effort inference helper that should never crash the caller. Each transport path is isolated to prevent one failure mode from affecting the other.
173-211: LGTM! Clear and well-documented enrichment logic.The function correctly computes staleness and readiness advice based on editor state. The 2-second staleness threshold (line 183) is conservative and appropriate for detecting unfocused editors, as noted in the comment.
The blocking reasons logic (lines 190-200) comprehensively covers compilation, domain reload, test execution, asset refresh, and staleness—providing useful guidance for retry behavior.
288-302: LGTM! Robust validation with clear error reporting.The validation logic (lines 288-302) properly handles both Pydantic v1 (
parse_obj) and v2 (model_validate) APIs, and returns a detailed error response when validation fails. This ensures schema mismatches are caught and reported clearly to callers.Server/src/services/tools/refresh_unity.py (3)
16-16: LGTM! Clean migration to the new editor_state module.The import (line 16) and parameter formatting (lines 29-34) correctly reflect the transition from
editor_state_v2to the unifiededitor_statemodule. The multi-line parameter annotations improve readability.Also applies to: 29-34
74-83: LGTM! Robust readiness polling implementation.The polling logic correctly calls the new
editor_state.get_editor_state(ctx)API and defensively extracts theadvice.ready_for_toolsflag through multiple levels of dictionary access. Themodel_dump()fallback to direct dict access handles both Pydantic versions gracefully.
87-91: LGTM! Properly uses public API.Line 87 correctly calls
editor_state.infer_single_instance_id(ctx), which is now a public function (as confirmed ineditor_state.pyline 128). This addresses previous review feedback about accessing private functions.The broad exception handler (lines 90-91) is appropriate here—external changes cleanup is a best-effort operation that should never break the main flow.
#8) * Add local test harness for fast developer iteration Scripts for running the NL/T/GO test suites locally against a GUI Unity Editor, complementing the CI workflows in .github/workflows/. Benefits: - 10-100x faster than CI (no Docker startup) - Real-time Unity console debugging - Single test execution for rapid iteration - Auto-detects HTTP vs stdio transport Usage: ./scripts/local-test/setup.sh # One-time setup ./scripts/local-test/quick-test.sh NL-0 # Run single test ./scripts/local-test/run-nl-suite-local.sh # Full suite See scripts/local-test/README.md for details. Also updated .gitignore to: - Allow scripts/local-test/ to be tracked - Ignore generated artifacts (reports/*.xml, .claude/local/, .unity-mcp/) * Fix issue CoplayDev#525: Save dirty scenes for all test modes Move SaveDirtyScenesIfNeeded() call outside the PlayMode conditional so EditMode tests don't get blocked by Unity's "Save Scene" modal dialog. This prevents MCP from timing out when running EditMode tests with unsaved scene changes. * fix: add missing FAST_FAIL_TIMEOUT constant in PluginHub The FAST_FAIL_TIMEOUT class attribute was referenced on line 149 but never defined, causing AttributeError on every ping attempt. This error was silently caught by the broad 'except Exception' handler, causing all fast-fail commands (read_console, get_editor_state, ping) to fail after 6 seconds of retries with 'ping not answered' error. Added FAST_FAIL_TIMEOUT = 10 to define a 10-second timeout for fast-fail commands, matching the intent of the existing fast-fail infrastructure. * feat(ScriptableObject): enhance dry-run validation for AnimationCurve and Quaternion Dry-run validation now validates value formats, not just property existence: - AnimationCurve: Validates structure ({keys:[...]} or direct array), checks each keyframe is an object, validates numeric fields (time, value, inSlope, outSlope, inWeight, outWeight) and integer fields (weightedMode) - Quaternion: Validates array length (3 for Euler, 4 for raw) or object structure ({x,y,z,w} or {euler:[x,y,z]}), ensures all components are numeric Refactored shared validation helpers into appropriate locations: - ParamCoercion: IsNumericToken, ValidateNumericField, ValidateIntegerField - VectorParsing: ValidateAnimationCurveFormat, ValidateQuaternionFormat Added comprehensive XML documentation clarifying keyframe field defaults (all default to 0 except as noted). Added 5 new dry-run validation tests covering valid and invalid formats for both AnimationCurve and Quaternion properties. * test: fix integration tests after merge - test_refresh_unity_retry_recovery: Mock now handles both refresh_unity and get_editor_state commands (refresh_unity internally calls get_editor_state when wait_for_ready=True) - test_run_tests_async_forwards_params: Mock response now includes required 'mode' field for RunTestsStartResponse Pydantic validation - test_get_test_job_forwards_job_id: Updated to handle GetTestJobResponse as Pydantic model instead of dict (use model_dump() for assertions) * Update warning message to apply to all test modes Follow-up to PR CoplayDev#527: Since SaveDirtyScenesIfNeeded() now runs for all test modes, update the warning message to say 'tests' instead of 'PlayMode tests'. * feat(run_tests): add wait_timeout to get_test_job to avoid client loop detection When polling for test completion, MCP clients like Cursor can detect the repeated get_test_job calls as 'looping' and terminate the agent. Added wait_timeout parameter that makes the server wait internally for tests to complete (polling Unity every 2s) before returning. This dramatically reduces client-side tool calls from 10-20 down to 1-2, avoiding loop detection. Usage: get_test_job(job_id='xxx', wait_timeout=30) - Returns immediately if tests complete within timeout - Returns current status if timeout expires (client can call again) - Recommended: 30-60 seconds * fix: use Pydantic attribute access in test_run_tests_async for merge compatibility * revert: remove local test harness - will be submitted in separate PR --------- Co-authored-by: Scott Jennings <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Server/src/services/tools/run_tests.py (1)
195-221: Server-side polling implementation is correct.The wait_timeout logic properly polls Unity at 2-second intervals, respects the deadline, and returns immediately on terminal states ("succeeded", "failed", "cancelled"). The use of
asyncio.get_event_loop().time()for deadline calculations is acceptable.💡 Optional: Make poll interval configurable
The
poll_interval = 2.0is hardcoded (line 197). If future use cases require different polling frequencies, consider making it a function parameter with a sensible default.wait_timeout: Annotated[int | None, "If set, wait up to this many seconds for tests to complete before returning. " "Reduces polling frequency and avoids client-side loop detection. " "Recommended: 30-60 seconds. Returns immediately if tests complete sooner."] = None, + poll_interval: Annotated[float, + "Polling interval in seconds when using wait_timeout"] = 2.0, ) -> GetTestJobResponse | MCPResponse:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
MCPForUnity/Editor/Dependencies/DependencyManager.cs.metaMCPForUnity/Editor/Dependencies/Models/DependencyCheckResult.cs.metaMCPForUnity/Editor/Dependencies/Models/DependencyStatus.cs.metaMCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs.metaMCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs.metaMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs.metaMCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs.metaMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Services/TestRunnerService.csMCPForUnity/Editor/Setup/SetupWindowService.cs.metaMCPForUnity/Editor/Tools/ManageScriptableObject.csServer/src/services/tools/run_tests.pyServer/tests/integration/test_refresh_unity_retry_recovery.pyServer/tests/integration/test_run_tests_async.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectStressTests.cs
✅ Files skipped from review due to trivial changes (6)
- MCPForUnity/Editor/Dependencies/Models/DependencyStatus.cs.meta
- MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs.meta
- MCPForUnity/Editor/Dependencies/DependencyManager.cs.meta
- MCPForUnity/Editor/Setup/SetupWindowService.cs.meta
- MCPForUnity/Editor/Dependencies/Models/DependencyCheckResult.cs.meta
- MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs.meta
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs.metaMCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs.meta
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs.meta
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Services/TestRunnerService.csMCPForUnity/Editor/Helpers/VectorParsing.cs
🧬 Code graph analysis (6)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
MCPForUnity/Editor/Helpers/VectorParsing.cs (6)
AnimationCurve(415-463)AnimationCurve(471-474)VectorParsing(12-729)ValidateAnimationCurveFormat(490-553)Quaternion(152-213)ValidateQuaternionFormat(571-660)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectStressTests.cs (3)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
HandleCommand(40-75)TestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.cs (1)
JObject(21-25)MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)
MCPForUnity/Editor/Services/TestRunnerService.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)
Server/src/services/tools/run_tests.py (2)
Server/src/services/tools/preflight.py (1)
preflight(27-110)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-114)
MCPForUnity/Editor/Helpers/VectorParsing.cs (1)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (4)
ParamCoercion(11-259)ValidateNumericField(178-192)ValidateIntegerField(202-216)IsNumericToken(165-168)
Server/tests/integration/test_run_tests_async.py (2)
Server/src/services/tools/run_tests.py (2)
run_tests(98-156)get_test_job(166-229)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
🪛 Ruff (0.14.10)
Server/tests/integration/test_refresh_unity_retry_recovery.py
30-30: Avoid specifying long messages outside the exception class
(TRY003)
Server/tests/integration/test_run_tests_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)
42-42: Unused function argument: send_fn
(ARG001)
42-42: Unused function argument: unity_instance
(ARG001)
42-42: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (23)
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs.meta (1)
2-2: LGTM! GUID regeneration addresses the reported conflict.The GUID change resolves Issue #524 as stated in the PR objectives. The Unity-generated GUID replaces the previous value, ensuring unique asset identification across the project.
MCPForUnity/Editor/Dependencies/PlatformDetectors/IPlatformDetector.cs.meta (1)
2-2: GUID regeneration is correct and safe for this interface.The GUID change correctly addresses the conflict mentioned in issue #524. Since
IPlatformDetectoris a pure C# interface with no serialized asset references (only used in code likeDependencyManager.cs), changing the GUID poses no risk of breaking asset references. No further verification needed.Server/tests/integration/test_refresh_unity_retry_recovery.py (1)
25-30: LGTM! Clean refactoring of the mock strategy.The updated mock correctly simulates the retry-disconnect scenario and handles both
refresh_unityandget_editor_statecommands with appropriate responses. The branching logic is clear and the test validation at lines 35-41 confirms the expected behavior.The static analysis hint about the error message on line 30 is a nitpick—in test code, descriptive error messages are valuable for debugging when unexpected commands are encountered.
MCPForUnity/Editor/Services/TestRunnerService.cs (2)
109-111: LGTM! Fixes Issue #525.Calling
SaveDirtyScenesIfNeeded()for all test modes (not just PlayMode) correctly prevents modal save dialogs from blocking EditMode tests, as referenced in the comment.
333-333: LGTM! Consistent with broader scope.The warning message correctly uses "tests" (generic) instead of "PlayMode tests" (specific), since the function now handles both EditMode and PlayMode.
Server/src/services/tools/run_tests.py (3)
44-88: LGTM! Well-structured data models.The new Pydantic models for the async job lifecycle (RunTestsStartData, GetTestJobData, TestJobProgress, TestJobFailure) are cleanly defined with appropriate Optional fields and proper inheritance from MCPResponse.
152-156: LGTM! Type safety issue resolved.The response handling correctly returns typed
RunTestsStartResponseorMCPResponseobjects instead of raw dicts, addressing the type annotation inconsistency flagged in previous reviews.
224-229: LGTM! Consistent typed response handling.The immediate return path (no wait_timeout) correctly returns typed
GetTestJobResponseorMCPResponse, consistent with the run_tests implementation.Server/tests/integration/test_run_tests_async.py (2)
7-33: LGTM! Test correctly updated for new module structure.The test properly reflects the migration from
services.tools.test_jobstoservices.tools.run_tests, with correct command type ("run_tests"), updated monkeypatch target, and proper attribute-based access to the typed response object.
37-56: LGTM! Test correctly updated for new module structure.The test properly reflects the migration to
services.tools.run_testswith correct monkeypatch target and attribute-based response access.MCPForUnity/Editor/Tools/ManageScriptableObject.cs (3)
391-434: Enhanced dry-run validation for complex types looks good.The validation logic correctly delegates to
VectorParsing.ValidateAnimationCurveFormatandValidateQuaternionFormatfor early format feedback during dry-run. The implementation properly handles:
- Null/non-null value tokens
- Success/failure result branching with appropriate messages
- Property type and array info in the response
Consider extending dry-run validation to other complex types (Vector2/3/4, Color) in a follow-up if clients would benefit from early format feedback for those as well.
1094-1119: Comprehensive XML documentation for AnimationCurve parsing.The updated documentation clearly describes:
- Supported input formats (wrapped, direct array, null)
- All keyframe fields with their defaults
- The defaulting behavior for missing fields
This will be valuable for consumers of the API.
1204-1225: Clear documentation for Quaternion input formats.The documentation comprehensively covers:
- All supported input formats (Euler array, raw quaternion array, object formats)
- Format detection rules based on array length
- Null handling (identity quaternion)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (1)
159-216: Well-designed validation helpers for dry-run support.The new validation methods are clean and follow good patterns:
IsNumericTokenis a simple, reusable predicateValidateNumericFieldandValidateIntegerFieldcorrectly treat absent/null fields as valid (will use defaults)- Error messages include the actual token type for debugging
These integrate well with the
VectorParsingvalidators.MCPForUnity/Editor/Helpers/VectorParsing.cs (4)
272-279: Good API design for ParseColorOrDefault overloads.Splitting into two overloads provides both convenience (parameterless returns
Color.white) and flexibility (explicit default). This is a clean pattern.
394-412: Documentation correctly notes default value inconsistency.The note at lines 405 and 410-411 appropriately calls out that
valuedefaults to 1 here (for ManageVFX) but to 0 inManageScriptableObject. This transparency is helpful.
475-553: Thorough AnimationCurve format validation.The validation logic correctly:
- Handles wrapped
{ keys: [...] }and direct array formats- Validates each keyframe has numeric fields using
ParamCoercion.ValidateNumericField- Validates
weightedModeas integer- Provides informative success/error messages
The success message correctly states defaults for the
ManageScriptableObjectcontext where this validator is used.
555-660: Comprehensive Quaternion format validation.The validator covers all supported formats:
- Euler angles array
[x, y, z]- Raw quaternion array
[x, y, z, w]- Object with
eulerarray- Object with
x, y, z, wfieldsEach branch properly validates numeric types and provides clear error messages when validation fails.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectStressTests.cs (5)
734-793: Good test coverage for valid AnimationCurve dry-run validation.The test correctly validates that:
- A properly formatted AnimationCurve passes dry-run validation
- The success message includes descriptive content about the curve format
The test pattern (create, dry-run, assert) is consistent with other tests in the file.
795-852: Effective negative test for AnimationCurve format validation.The test verifies that invalid input (non-numeric
timefield) is caught during dry-run with a clear, actionable error message. The assertion checks for key terms in the error message without being overly brittle.
854-905: Good test for valid Quaternion Euler format.The test confirms that a 3-element Euler angles array passes dry-run validation with an appropriate descriptive message.
907-958: Appropriate negative test for Quaternion array length.The test verifies that an incorrect array length (2 elements) is caught with a clear error message explaining the valid options.
960-1011: Good test for non-numeric Quaternion value detection.The test confirms that string values in numeric positions (e.g.,
"ninety"instead of90) are caught during dry-run validation with a helpful error message.
2 participants
As we're making going to a major bump, I've made some changes on the code side that hopefully makes it easier to maintain, mostly because of less duplication + smaller files:
ParseColorOrDefaultunity://editor/stateto remain consistent with other editor resourcesmcpforunityinstead ofunity- this should avoid conflicts with other MCPs, and avoid issues with Unity themselves 😅Summary by CodeRabbit
New Features
API Updates
Behavioral Changes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.