🔧 Consolidate Shared Services Across MCP Tools #125
Conversation
WalkthroughAdds multiple Unity Editor helper libraries (type resolution, object resolution, property conversion, component ops, pagination, serializer), refactors editor tools to use them, standardizes responses, tightens server detection and Python tooling, updates tests, CI workflows, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Tool as Editor Tool
participant ObjectResolver as ObjectResolver
participant AssetDB as AssetDatabase
participant GameLookup as GameObjectLookup
participant TypeResolver as UnityTypeResolver
Tool->>ObjectResolver: Resolve(instruction, targetType)
ObjectResolver->>ObjectResolver: parse instruction (find/method/component)
alt asset target
ObjectResolver->>AssetDB: Load by path or GUID
AssetDB-->>ObjectResolver: asset or null
else scene target
ObjectResolver->>GameLookup: FindByTarget(find, method)
GameLookup-->>ObjectResolver: GameObject
alt component requested
ObjectResolver->>TypeResolver: ResolveComponent(name)
TypeResolver-->>ObjectResolver: Type or error
ObjectResolver-->>Tool: component instance or null
end
end
ObjectResolver-->>Tool: resolved object
sequenceDiagram
participant Editor as ManageComponents
participant Resolver as UnityTypeResolver
participant Ops as ComponentOps
participant Undo as Unity Undo System
participant Conv as PropertyConversion
Editor->>Resolver: ResolveComponent(typeName)
Resolver-->>Editor: Type
Editor->>Ops: AddComponent(target, Type, out error)
Ops->>Ops: validate & check physics conflicts
Ops->>Undo: Undo.AddComponent(target, Type)
Undo-->>Ops: Component instance
Ops->>Conv: Convert properties (JToken -> target type)
Conv-->>Ops: converted value or error
Ops->>Component: set fields/properties via reflection
Ops-->>Editor: result / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Greptile SummaryThis PR successfully consolidates shared functionality across MCP tools into reusable helper classes, eliminating ~860 lines of duplicate code while improving architecture and maintainability. Key Improvements:
Testing:
Backwards Compatibility: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Tool as ManageGameObject/Components/Material
participant Resolver as UnityTypeResolver
participant ObjRes as ObjectResolver
participant CompOps as ComponentOps
participant PropConv as PropertyConversion
participant Serializer as UnityJsonSerializer
participant Unity as Unity Engine
Client->>Tool: manage_gameobject(action, params)
alt Type Resolution Needed
Tool->>Resolver: ResolveComponent("Rigidbody")
Resolver->>Resolver: Check cache
alt Cache Hit
Resolver-->>Tool: Type (cached)
else Cache Miss
Resolver->>Resolver: Search Player assemblies
Resolver->>Resolver: Fallback to Editor assemblies
Resolver->>Resolver: Cache result
Resolver-->>Tool: Type
end
end
alt Object Resolution Needed
Tool->>ObjRes: Resolve<GameObject>(instruction)
ObjRes->>ObjRes: Check if asset or scene object
alt Asset Type
ObjRes->>Unity: AssetDatabase.LoadAssetAtPath()
Unity-->>ObjRes: Asset
else Scene Object
ObjRes->>Unity: GameObjectLookup.FindByTarget()
Unity-->>ObjRes: GameObject
end
ObjRes-->>Tool: Resolved Object
end
alt Component Operation
Tool->>CompOps: AddComponent(gameObject, type)
CompOps->>CompOps: Validate type
CompOps->>CompOps: CheckPhysicsConflict()
CompOps->>Unity: Undo.AddComponent()
Unity-->>CompOps: Component
CompOps->>CompOps: ApplyDefaultValues()
CompOps-->>Tool: Component
end
alt Property Conversion
Tool->>PropConv: ConvertToType(jtoken, targetType)
PropConv->>Serializer: Get Instance
Serializer-->>PropConv: JsonSerializer with Unity converters
PropConv->>Serializer: ToObject(targetType)
Serializer->>Serializer: Apply Vector3/Color/etc converters
Serializer-->>PropConv: Converted value
PropConv-->>Tool: Converted value
end
Tool->>Unity: Apply changes with Undo support
Unity-->>Tool: Success
Tool-->>Client: Response (SuccessResponse/ErrorResponse)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Server/src/services/tools/manage_script.py (1)
510-519: Fix documentation: 'update' is not a valid action.Line 515-516 documents
contentsas "C# code for 'create'/'update'" but theactionparameter at line 512 only acceptsLiteral['create', 'read', 'delete']. The mention of 'update' is misleading.🔎 Proposed fix
contents: Annotated[str, "Contents of the script to create", - "C# code for 'create'/'update'"] | None = None, + "C# code for 'create' action"] | None = None,MCPForUnity/Editor/Tools/ManageAsset.cs (1)
992-1000: Consider logging conversion failures or using a result wrapper to distinguish failure from intentional null.
TryConvertToTypereturnsnullon conversion failure (line 59-68), but the null check at lines 992-1000 and 1004-1016 cannot distinguish between intentional null assignment and conversion failure. For value types, this is particularly problematic: when a token is null,ConvertToTypelogs a warning and returns a default value; when conversion fails, the failure is silently suppressed with no logging. This inconsistency can mask conversion errors. Consider either logging failures explicitly inTryConvertToTypeor using a wrapper type (e.g.,Result<object>) to signal success/failure state distinctly from null semantics.
🤖 Fix all issues with AI Agents
In @MCPForUnity/Editor/Helpers/ComponentOps.cs:
- Around line 132-214: SetProperty currently treats a null return from
PropertyConversion.ConvertToType as equivalent to a valid null assignment and
falls through to the generic "not found" error when conversion fails; update
SetProperty (the property/field/serialized-field branches that call
PropertyConversion.ConvertToType) to detect when convertedValue is null while
the input JToken is not JTokenType.Null and set a clear conversion error and
return false (e.g., "Failed to convert value for property/field '<propertyName>'
to type '<TargetType>'"); do this for the PropertyInfo branch, the FieldInfo
branch, and the non-public serialized FieldInfo branch so conversion failures
are reported immediately rather than falling through to the not-found message.
In @MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs:
- Around line 1-31: UnityJsonSerializer.Instance currently registers converters
for Vector2/3, Quaternion, Color, Rect, Bounds and UnityEngineObject but not
Vector4 (which is used in MaterialOps.cs and ManageScriptableObject.cs); add a
Vector4Converter to the Converters list (and implement Vector4Converter in
MCPForUnity.Runtime.Serialization mirroring the Vector3Converter pattern if
custom handling is required) and optionally register Matrix4x4Converter as well
if Matrix4x4 values need custom serialization; update the Instance
initialization to include new Vector4Converter (and Matrix4x4Converter if
chosen).
In @MCPForUnity/Editor/Tools/ManageComponents.cs:
- Around line 96-101: When setting properties on a newly added component you
must record the component for undo before applying changes: call
UnityEditor.Undo.RecordObject(newComponent, "Modify Component Properties") (or
Undo.RegisterCompleteObjectUndo) immediately before invoking
SetPropertiesOnComponent(newComponent, properties) in ManageComponents.cs so
property modifications are included in the undo stack; ensure this runs only
when properties != null && properties.HasValues and keeps the existing
AddComponent/ComponentOps.AddComponent undo behavior intact.
In @MCPForUnity/Editor/Tools/ManageScene.cs:
- Line 399: Replace the Unity 2023+ API call
UnityEngine.Object.FindObjectsByType<Camera>(FindObjectsSortMode.None) with the
Unity 2021-compatible UnityEngine.Object.FindObjectsOfType<Camera>() in
ManageScene (the code around the cams variable); remove the FindObjectsSortMode
argument so the project compiles against Unity 2021.3.45f2.
In @Server/src/services/tools/manage_shader.py:
- Around line 15-18: Remove the global destructiveHint flag from the
ToolAnnotations in the Manage Shader tool (the annotations=ToolAnnotations block
with title "Manage Shader") so the safe read action is not marked destructive;
instead, update the tool description/docstring to explicitly state which
sub-actions (create/update/delete) are destructive and which (read) are
read-only to match the non-destructive pattern used by read_console.py.
In @TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs:
- Around line 23-26: The BlendXHash and BlendYHash constants are initialized
with "BlendX"/"BlendY" but the animator parameters are actually named
"reachX"/"reachY"; update the Animator.StringToHash calls for BlendXHash and
BlendYHash to use "reachX" and "reachY" respectively, and also update any other
occurrences where those hashes are defined or used (e.g., the code block around
the BlendXHash/BlendYHash declarations and the usage site around lines 107-112)
so the animator.SetFloat/Set... calls reference the corrected hashes.
🧹 Nitpick comments (29)
Server/src/services/tools/manage_asset.py (1)
19-45: Consider extracting_normalize_propertiesto a shared utility module.The
_normalize_propertiesfunction is duplicated across at least three files (manage_asset.py, manage_material.py, and manage_components.py) with nearly identical implementations. Given the PR's stated goal of consolidating shared services and removing duplicate code, this helper should be extracted to a shared location (e.g.,services/tools/utils.pyor the newPropertyConversionhelper mentioned in the PR objectives).🔎 Proposed refactor to eliminate duplication
Move this function to
Server/src/services/tools/utils.pyand import it in all three files:In
Server/src/services/tools/utils.py, add:def normalize_properties(value: Any) -> tuple[dict[str, Any] | None, str | None]: """ Robustly normalize properties parameter to a dict. Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None. Consistent across manage_asset, manage_components, and manage_material. """ if value is None: return None, None if isinstance(value, dict): return value, None if isinstance(value, str): if value in ("[object Object]", "undefined", "null", ""): return None, f"properties received invalid value: '{value}'. Expected a JSON object like {{\"key\": value}}" parsed = parse_json_payload(value) if isinstance(parsed, dict): return parsed, None return None, f"properties must be a JSON object (dict), got string that parsed to {type(parsed).__name__}" return None, f"properties must be a dict or JSON string, got {type(value).__name__}"Then in this file:
-def _normalize_properties(value: Any) -> tuple[dict[str, Any] | None, str | None]: - """ - Robustly normalize properties parameter to a dict. - Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None. - Consistent with manage_components and manage_material normalizers. - """ - if value is None: - return None, None - - # Already a dict - return as-is - if isinstance(value, dict): - return value, None - - # Try parsing as string - if isinstance(value, str): - # Check for obviously invalid values from serialization bugs - if value in ("[object Object]", "undefined", "null", ""): - return None, f"properties received invalid value: '{value}'. Expected a JSON object like {{\"key\": value}}" - - parsed = parse_json_payload(value) - if isinstance(parsed, dict): - return parsed, None - - return None, f"properties must be a JSON object (dict), got string that parsed to {type(parsed).__name__}" - - return None, f"properties must be a dict or JSON string, got {type(value).__name__}" +from services.tools.utils import parse_json_payload, coerce_int, normalize_propertiesApply similar changes to
manage_material.pyandmanage_components.py.Server/src/services/tools/execute_menu_item.py (1)
29-29: Consider removing the historical comment.The comment "Removed session_state import" references a past change that's no longer visible in the current code. Historical notes are better captured in commit messages rather than inline comments, which can confuse future readers.
🔎 Proposed cleanup
- # Get active instance from session state - # Removed session_state import + # Get active instance from context unity_instance = get_unity_instance_from_context(ctx).github/workflows/claude-nl-suite.yml (3)
799-821: Missing post-retry assertion for GO tests.After the T retry step, there's a "Re-assert T coverage (post-retry)" step (lines 740-752) that fails the workflow immediately if T fragments are still missing. The GO retry lacks an equivalent step.
While the final CI gate will catch missing GO tests, adding a parallel assertion step after the GO retry would provide earlier, clearer failure messages consistent with the T pass flow.
Proposed addition after GO retry step
- name: Re-assert GO coverage (post-retry) if: always() shell: bash run: | set -euo pipefail missing=() for id in GO-0 GO-1 GO-2 GO-3 GO-4 GO-5 GO-6 GO-7 GO-8 GO-9 GO-10; do [[ -s "reports/${id}_results.xml" ]] || missing+=("$id") done if (( ${#missing[@]} )); then echo "::error::Still missing GO fragments: ${missing[*]}" exit 1 fi
827-842: Consider extending this assertion to include GO tests.This step only validates T fragments, but GO tests are now part of the suite. For consistency, consider either:
- Renaming to clarify it's T-only, or
- Extending to also check GO-0 through GO-10
The CI gate at the end handles both, so this is optional.
1230-1238: GO tests will sort incorrectly in markdown output.The
order_keyfunction doesn't handle GO tests explicitly, so they fall through to string comparison. This causes GO-10 to sort between GO-1 and GO-2 (string "10" < "2").Proposed fix for proper numeric ordering
def order_key(n: str): if n.startswith('NL-'): try: return (0, int(n.split('-')[1])) except: return (0, 999) if n.startswith('T-') and len(n) > 2: return (1, ord(n[2])) + if n.startswith('GO-'): + try: + return (2, int(n.split('-')[1])) + except: + return (2, 999) - return (2, n) + return (3, n)MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
271-283: Excellent orphaned session detection logic.The defensive check correctly identifies when an HTTP Local session believes it's active but the underlying server is no longer running, and automatically cleans up the orphaned state. The local
isRunning = falseassignment ensures the UI updates consistently.Optional: Consider tracking the cleanup task
The fire-and-forget pattern (
_ = EndOrphanedSessionAsync()) works here since the method has proper error handling, but for improved observability you could optionally track the task:- _ = EndOrphanedSessionAsync(); + var cleanupTask = EndOrphanedSessionAsync(); + // Optionally log or monitor cleanupTask.Status for diagnosticsThis is a minor code quality suggestion, not a requirement.
Server/src/services/custom_tool_service.py (1)
270-278: Add runtime deprecation warning or remove the function.The
compute_project_idfunction is marked as deprecated in its docstring but still remains callable without any runtime warning. If this function is truly no longer used (as stated), consider either:
- Adding a runtime deprecation warning using
warnings.warn()- Removing the function entirely if there are no remaining callers
Documentation-only deprecation can lead to continued usage by developers who don't read docstrings carefully.
🔎 Proposed fix to add runtime deprecation warning
+import warnings + def compute_project_id(project_name: str, project_path: str) -> str: """ DEPRECATED: Computes a SHA256-based project ID. This function is no longer used as of the multi-session fix. Unity instances now use their native project_hash (SHA1-based) for consistency across stdio and WebSocket transports. """ + warnings.warn( + "compute_project_id is deprecated and will be removed in a future version. " + "Unity instances now use their native project_hash.", + DeprecationWarning, + stacklevel=2 + ) combined = f"{project_name}:{project_path}" return sha256(combined.encode("utf-8")).hexdigest().upper()[:16]Server/src/services/tools/manage_material.py (1)
50-70: Duplicated_normalize_propertiesacross multiple files.This helper is nearly identical to the implementations in
manage_components.py(lines 15-39) andmanage_asset.py(lines 18-43). Consider extracting to the sharedutils.pymodule alongsideparse_json_payloadandcoerce_int.Example: Extract to Server/src/services/tools/utils.py
def normalize_properties(value: Any) -> tuple[dict[str, Any] | None, str | None]: """ Normalize properties parameter to a dict. Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None. """ if value is None: return None, None if isinstance(value, dict): return value, None if isinstance(value, str): if value in ("[object Object]", "undefined", "null", ""): return None, f"properties received invalid value: '{value}'. Expected a JSON object" parsed = parse_json_payload(value) if isinstance(parsed, dict): return parsed, None return None, f"properties must parse to a dict, got {type(parsed).__name__}" return None, f"properties must be a dict or JSON string, got {type(value).__name__}"MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
890-896: Consider usingResolveScriptableObjectinstead ofResolveAny.The
UnityTypeResolverclass (lines 104-110 inUnityTypeResolver.cs) provides aResolveScriptableObject(typeName)method that constrains resolution toScriptableObjecttypes. UsingResolveAnyhere means invalid types (non-ScriptableObject) will be resolved and only rejected later at line 117. Using the constrained method would provide cleaner semantics and potentially better error messages for ambiguous type names.Proposed change
/// <summary> - /// Resolves a type by name. Delegates to UnityTypeResolver.ResolveAny(). + /// Resolves a ScriptableObject type by name. Delegates to UnityTypeResolver. /// </summary> private static Type ResolveType(string typeName) { - return Helpers.UnityTypeResolver.ResolveAny(typeName); + return Helpers.UnityTypeResolver.ResolveScriptableObject(typeName); }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs (1)
58-61: Consider extracting sharedToJObjecthelper.The
ToJObjecthelper method is duplicated across multiple test files (seeManageMaterialPropertiesTests.cs,ManageMaterialReproTests.cs,ManageMaterialStressTests.cs,MaterialParameterToolTests.cs). Consider extracting to a shared test utilities class.MCPForUnity/Editor/Tools/ManageScene.cs (1)
632-664: LGTM: Lightweight component metadata addition.The addition of
componentTypesprovides useful metadata for hierarchy browsing without the overhead of full component serialization. The implementation correctly handles null components and uses type names only.The empty
catchblock silently swallows all exceptions. While this may be intentional for resilience in metadata collection, consider logging exceptions during development to catch unexpected issues:Optional: Add debug logging to catch block
catch { } +catch (Exception ex) +{ + // Silently continue, but log in debug builds + #if UNITY_EDITOR + Debug.LogWarning($"[ManageScene] Error collecting component types for {go.name}: {ex.Message}"); + #endif +}Server/src/services/tools/find_gameobjects.py (1)
47-51: Consider validating search_term before preflight.The
search_termvalidation occurs afterpreflight(), which may perform I/O operations. Moving the validation earlier would fail fast on invalid input without incurring preflight overhead.🔎 Proposed change
async def find_gameobjects( ... ) -> dict[str, Any]: ... unity_instance = get_unity_instance_from_context(ctx) + if not search_term: + return { + "success": False, + "message": "Missing required parameter 'search_term'. Specify what to search for." + } + gate = await preflight(ctx, wait_for_no_compile=True, refresh_if_dirty=True) if gate is not None: return gate.model_dump() - if not search_term: - return { - "success": False, - "message": "Missing required parameter 'search_term'. Specify what to search for." - }Server/src/services/tools/manage_components.py (2)
16-40: Consider extracting_normalize_propertiesto a shared utility.This function is duplicated nearly identically across
manage_components.py,manage_material.py, andmanage_asset.py(per relevant code snippets). Extracting it toservices/tools/utils.pywould reduce duplication and ensure consistent behavior.🔎 Proposed refactor
Move to
Server/src/services/tools/utils.py:def normalize_properties(value: Any) -> tuple[dict[str, Any] | None, str | None]: """ Robustly normalize properties parameter to a dict. Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None. """ if value is None: return None, None if isinstance(value, dict): return value, None if isinstance(value, str): if value in ("[object Object]", "undefined", "null", ""): return None, f"properties received invalid value: '{value}'. Expected a JSON object" parsed = parse_json_payload(value) if isinstance(parsed, dict): return parsed, None return None, f"properties must be a JSON object (dict), got string that parsed to {type(parsed).__name__}" return None, f"properties must be a dict or JSON string, got {type(value).__name__}"Then import in all three files:
from services.tools.utils import normalize_properties
130-138: Clarify mutually exclusive property parameters.The
set_propertyaction accepts bothproperty/value(single property) andproperties(dict). Both can be provided simultaneously. Consider documenting whether one takes precedence or if they're merged, or add validation to prevent ambiguity.🔎 Option: Add validation for conflicting inputs
if action == "set_property": + has_single = property is not None and value is not None + has_multi = properties is not None + if has_single and has_multi: + # Could merge, or reject ambiguity + pass # Current behavior: both are sent to Unity if property and value is not None: params["property"] = property params["value"] = value if properties: params["properties"] = propertiesServer/src/services/tools/manage_editor.py (1)
46-57: Consider removing placeholder comments.Lines 52-55 contain commented-out placeholder code that appears to be leftover from development. These don't affect functionality but could be cleaned up for clarity.
🔎 Proposed cleanup
params = { "action": action, "waitForCompletion": wait_for_completion, "toolName": tool_name, # Corrected parameter name to match C# "tagName": tag_name, # Pass tag name "layerName": layer_name, # Pass layer name - # Add other parameters based on the action being performed - # "width": width, - # "height": height, - # etc. }MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (1)
150-185: Consider caching player assembly names to improve performance.
CompilationPipeline.GetAssemblies(AssembliesType.Player)is called in bothFindCandidates(line 157-159) andPreferPlayer(line 197-199). For repeated type lookups, caching this set could reduce overhead.🔎 Proposed optimization
+ private static HashSet<string> _cachedPlayerAsmNames; + private static HashSet<string> GetPlayerAssemblyNames() + { +#if UNITY_EDITOR + if (_cachedPlayerAsmNames == null) + { + _cachedPlayerAsmNames = new HashSet<string>( + CompilationPipeline.GetAssemblies(AssembliesType.Player).Select(a => a.name), + StringComparer.Ordinal); + } + return _cachedPlayerAsmNames; +#else + return new HashSet<string>(); +#endif + } + private static List<Type> FindCandidates(string query, Type requiredBaseType) { bool isShort = !query.Contains('.'); var loaded = AppDomain.CurrentDomain.GetAssemblies(); #if UNITY_EDITOR // Names of Player (runtime) script assemblies - var playerAsmNames = new HashSet<string>( - CompilationPipeline.GetAssemblies(AssembliesType.Player).Select(a => a.name), - StringComparer.Ordinal); + var playerAsmNames = GetPlayerAssemblyNames();Note: The cache would need invalidation on domain reload, which Unity handles automatically for static fields.
.github/workflows/claude-gameobject-suite.yml (1)
476-477: Silent exception handling in fragment parsing may hide issues.The
except Exception: passon line 476-477 silently ignores parsing errors. Consider logging a warning to aid debugging when fragments are malformed.🔎 Suggested improvement
for frag in fragments: try: froot = ET.parse(frag).getroot() if localname(froot.tag) == 'testcase': suite.append(froot) added += 1 except Exception: - pass + print(f"Warning: Could not parse fragment {frag}")TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (1)
148-158: Test logic may have issues with object tracking after Unity destroys objects.Lines 157-158 attempt to remove objects from
testObjectsbased on whether they're deleted, buttarget1Deletedandtarget2Deletedare booleans. The conditionif (!target1Deleted)followed bytestObjects.Remove(target1)will try to remove the destroyed object reference, which should work but is a bit confusing. Consider inverting the logic for clarity.🔎 Suggested clarification
// Clean up only surviving objects from tracking - if (!target1Deleted) testObjects.Remove(target1); - if (!target2Deleted) testObjects.Remove(target2); + // Remove destroyed objects from tracking (they're already gone) + if (target1Deleted) testObjects.Remove(target1); + if (target2Deleted) testObjects.Remove(target2);Actually, the original logic is correct - we want to remove surviving objects that will be cleaned up in TearDown. The destroyed ones are already gone. The comment could be clearer.
MCPForUnity/Editor/Helpers/ObjectResolver.cs (2)
51-58: Asset path detection usesStartsWith("Assets/")which may miss normalized paths.The condition
findTerm.StartsWith("Assets/")won't match paths with backslashes on Windows or paths that have been normalized differently. The AI summary mentionsAssetPathUtility.NormalizeSeparatorsis available.🔎 Proposed fix
// --- Asset Search --- // If the target is an asset type, try AssetDatabase first + string normalizedPath = AssetPathUtility.NormalizeSeparators(findTerm); if (IsAssetType(targetType) || - (typeof(GameObject).IsAssignableFrom(targetType) && findTerm.StartsWith("Assets/"))) + (typeof(GameObject).IsAssignableFrom(targetType) && normalizedPath.StartsWith("Assets/"))) { - UnityEngine.Object asset = TryLoadAsset(findTerm, targetType); + UnityEngine.Object asset = TryLoadAsset(normalizedPath, targetType);
153-164:IsAssetTypemay miss some common asset types.Consider adding
Mesh,PhysicMaterial,PhysicsMaterial2D,RuntimeAnimatorController, andAvatarfor more complete coverage. However, this is a minor enhancement since the current list covers the most common cases.🔎 Extended asset types
private static bool IsAssetType(Type type) { return typeof(Material).IsAssignableFrom(type) || typeof(Texture).IsAssignableFrom(type) || typeof(ScriptableObject).IsAssignableFrom(type) || type.FullName?.StartsWith("UnityEngine.U2D") == true || typeof(AudioClip).IsAssignableFrom(type) || typeof(AnimationClip).IsAssignableFrom(type) || typeof(Font).IsAssignableFrom(type) || typeof(Shader).IsAssignableFrom(type) || - typeof(ComputeShader).IsAssignableFrom(type); + typeof(ComputeShader).IsAssignableFrom(type) || + typeof(Mesh).IsAssignableFrom(type) || + typeof(PhysicMaterial).IsAssignableFrom(type); }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (3)
23-26: Consider making batch size constants configurable or documenting rationale.The batch size constants (10, 50, 100) are hardcoded. While reasonable for stress tests, consider adding a brief comment explaining why these specific values were chosen, as future maintainers may want to adjust them for different CI environments.
54-58: Potential null reference if result is a non-JObject type.
JObject.FromObject(result)can fail ifresultcontains types that Json.NET cannot serialize. Consider adding error handling or a try-catch.🔎 Proposed defensive wrapper
private static JObject ToJObject(object result) { if (result == null) return new JObject(); + if (result is JObject jobj) return jobj; + try + { + return JObject.FromObject(result); + } + catch (Exception ex) + { + Debug.LogWarning($"[ToJObject] Failed to convert result: {ex.Message}"); + return new JObject { ["error"] = ex.Message }; + } - return result as JObject ?? JObject.FromObject(result); }
86-88: Performance threshold assertions may be flaky in CI.The 5000ms threshold could easily be exceeded on slower CI runners or under load. Consider using
[Category("Performance")]to allow selective skipping, or increase the threshold with a safety margin.🔎 Proposed approach
+ [Category("Performance")] [Test] public void BulkCreate_SmallBatch_AllSucceed() { // ... - Assert.Less(sw.ElapsedMilliseconds, 5000, "Bulk create took too long"); + // Use generous threshold for CI variability + Assert.Less(sw.ElapsedMilliseconds, 10000, "Bulk create took too long (CI threshold)"); }MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
148-155: Exception message may expose internal details.Catching a generic exception and returning
e.Messageis acceptable, but ensure that color parsing exceptions don't leak sensitive implementation details to API consumers.
487-490: Redundant path validation after NormalizePath.
NormalizePathcallsAssetPathUtility.SanitizeAssetPathwhich should handle theAssets/prefix. If it doesn't guarantee this, the error message "normalization failed" is misleading. Consider fixingNormalizePathto guarantee the prefix instead.🔎 Proposed fix in NormalizePath
private static string NormalizePath(string path) { if (string.IsNullOrEmpty(path)) return path; // Normalize separators and ensure Assets/ root path = AssetPathUtility.SanitizeAssetPath(path); + // Ensure Assets/ prefix + if (!path.StartsWith("Assets/")) + { + path = "Assets/" + path; + } + // Ensure .mat extension if (!path.EndsWith(".mat", StringComparison.OrdinalIgnoreCase)) { path += ".mat"; } return path; }MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
285-292: ApplyDefaultValues is extensible but sparse.Currently only handles Light, but the pattern is clear for future additions. Consider documenting that this is intentionally minimal or adding a TODO for other common defaults.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
238-254: Test captures behavior without asserting specific outcome.
Modify_NameToEmpty_HandlesGracefullyonly asserts that a result is returned, not the specific behavior. This is appropriate for documenting current behavior before redesign, but consider adding a comment about expected future behavior.🔎 Suggested documentation
[Test] public void Modify_NameToEmpty_HandlesGracefully() { int instanceID = testObjects[0].GetInstanceID(); var p = new JObject { ["action"] = "modify", ["target"] = instanceID, ["searchMethod"] = "by_id", ["name"] = "" // Empty name }; var result = ManageGameObject.HandleCommand(p); - // Capture current behavior - may reject or allow empty name + // Capture current behavior - may reject or allow empty name + // TODO: After API redesign, this should return an error (empty names are invalid) Assert.IsNotNull(result, "Should return a result"); }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (2)
213-226: Test captures current behavior without strict assertion.
Create_InvalidPrimitiveType_HandlesGracefullyonly asserts a result is returned. This is appropriate for baseline tests, but consider documenting the expected future behavior (error vs. empty object).
454-477: Response structure test for name could be more precise.The fallback
data.ToString().Contains("TestReturnedName")is very loose. Consider being more specific about where the name should appear in the response.🔎 Suggested stricter assertion
// Check name is in response var nameValue = data["name"]?.ToString() ?? data["Name"]?.ToString(); - Assert.IsTrue(!string.IsNullOrEmpty(nameValue) || data.ToString().Contains("TestReturnedName"), - "Response should include name"); + Assert.IsNotNull(nameValue, "Response data should include 'name' field"); + Assert.AreEqual("TestReturnedName", nameValue, "Response should include correct name");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (88)
.claude/prompts/nl-gameobject-suite.md.claude/prompts/nl-unity-suite-nl.md.claude/prompts/nl-unity-suite-t.md.github/workflows/claude-gameobject-suite.yml.github/workflows/claude-nl-suite.yml.gitignoreMCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/ComponentOps.cs.metaMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Helpers/GameObjectLookup.cs.metaMCPForUnity/Editor/Helpers/ObjectResolver.csMCPForUnity/Editor/Helpers/ObjectResolver.cs.metaMCPForUnity/Editor/Helpers/Pagination.csMCPForUnity/Editor/Helpers/Pagination.cs.metaMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Helpers/ParamCoercion.cs.metaMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Helpers/PropertyConversion.cs.metaMCPForUnity/Editor/Helpers/UnityJsonSerializer.csMCPForUnity/Editor/Helpers/UnityJsonSerializer.cs.metaMCPForUnity/Editor/Helpers/UnityTypeResolver.csMCPForUnity/Editor/Helpers/UnityTypeResolver.cs.metaMCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Helpers/VectorParsing.cs.metaMCPForUnity/Editor/Resources/Scene.metaMCPForUnity/Editor/Resources/Scene/GameObjectResource.csMCPForUnity/Editor/Resources/Scene/GameObjectResource.cs.metaMCPForUnity/Editor/Tools/FindGameObjects.csMCPForUnity/Editor/Tools/FindGameObjects.cs.metaMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageComponents.cs.metaMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csREADME.mdServer/src/services/custom_tool_service.pyServer/src/services/resources/gameobject.pyServer/src/services/tools/batch_execute.pyServer/src/services/tools/debug_request_context.pyServer/src/services/tools/execute_custom_tool.pyServer/src/services/tools/execute_menu_item.pyServer/src/services/tools/find_gameobjects.pyServer/src/services/tools/find_in_file.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_prefabs.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/manage_scriptable_object.pyServer/src/services/tools/manage_shader.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/tests/integration/conftest.pyServer/tests/integration/test_find_gameobjects.pyServer/tests/integration/test_gameobject_resources.pyServer/tests/integration/test_manage_asset_json_parsing.pyServer/tests/integration/test_manage_components.pyTestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.csTestProjects/UnityMCPTests/Assets/Scripts/TestScriptableObjectInstance.asset.metaTestProjects/UnityMCPTests/Assets/Temp.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-04T00:02:45.818Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 60
File: TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef.meta:1-7
Timestamp: 2025-09-04T00:02:45.818Z
Learning: Unity assembly definition (.asmdef) files use name-based references by default, not GUID references. When an .asmdef file references another assembly, it uses the target assembly's name (from the "name" field) in the "references" array, and Unity automatically resolves this to the appropriate GUID at build time. Direct GUID references are not required for standard Unity assembly references.
Applied to files:
MCPForUnity/Editor/Helpers/GameObjectLookup.cs.metaMCPForUnity/Editor/Helpers/ObjectResolver.cs.meta
📚 Learning: 2025-09-04T00:02:45.818Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 60
File: TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef.meta:1-7
Timestamp: 2025-09-04T00:02:45.818Z
Learning: Unity assembly definition (.asmdef) files use name-based references by default, not GUID references. When an .asmdef file references another assembly, it uses the target assembly's name (from the "name" field) in the "references" array, and Unity automatically resolves this to the appropriate GUID at build time. Direct GUID references are not required for standard Unity assembly references. Name-based references are the standard and recommended approach.
Applied to files:
MCPForUnity/Editor/Helpers/ObjectResolver.cs.meta
🧬 Code graph analysis (27)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
UnityEngine(1931-1934)
Server/src/services/tools/find_gameobjects.py (4)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/services/tools/utils.py (2)
coerce_bool(11-24)coerce_int(63-77)Server/src/services/tools/preflight.py (1)
preflight(27-105)
Server/tests/integration/test_find_gameobjects.py (2)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)Server/src/services/tools/find_gameobjects.py (1)
find_gameobjects(19-84)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
List(1130-1289)List(1976-1994)List(2000-2021)List(2029-2064)MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
List(219-253)
MCPForUnity/Editor/Helpers/Pagination.cs (1)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (3)
ParamCoercion(11-200)CoerceInt(19-45)T(137-157)
Server/tests/integration/test_gameobject_resources.py (2)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)Server/src/services/resources/gameobject.py (3)
get_gameobject(134-149)get_gameobject_components(175-201)get_gameobject_component(221-243)
MCPForUnity/Editor/Helpers/VectorParsing.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (8)
Vector3(998-1032)Vector3(1079-1097)Vector3(1831-1846)Vector2(1847-1860)Quaternion(1861-1874)Color(1875-1888)Rect(1889-1902)Bounds(1903-1920)
MCPForUnity/Editor/Resources/Scene/GameObjectResource.cs (5)
MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
HandleCommand(24-70)MCPForUnity/Editor/Tools/ManageScene.cs (3)
HandleCommand(90-191)GameObject(577-622)GetGameObjectPath(681-699)MCPForUnity/Editor/Helpers/ParamCoercion.cs (4)
ParamCoercion(11-200)CoerceInt(19-45)CoerceBool(53-82)CoerceString(121-128)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
GameObject(57-64)GameObject(69-72)GameObjectLookup(15-312)GetGameObjectPath(296-311)MCPForUnity/Editor/Helpers/GameObjectSerializer.cs (2)
GameObjectSerializer(17-557)GetComponentData(122-429)
Server/src/services/tools/manage_components.py (6)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/services/tools/utils.py (1)
parse_json_payload(27-60)Server/src/services/tools/preflight.py (1)
preflight(27-105)Server/src/services/tools/manage_asset.py (1)
_normalize_properties(19-44)Server/src/services/tools/manage_material.py (1)
_normalize_properties(50-69)
MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (7)
UnityEngine(1931-1934)Type(1941-1955)TryResolve(1968-1971)List(1130-1289)List(1976-1994)List(2000-2021)List(2029-2064)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (3)
Type(288-291)List(82-86)List(96-135)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (4)
Type(103-108)Type(113-118)Type(123-128)UnityTypeResolver(18-215)
MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (7)
Vector3Converter(11-34)Vector2Converter(36-56)QuaternionConverter(58-84)ColorConverter(86-112)RectConverter(114-140)BoundsConverter(142-161)UnityEngineObjectConverter(225-326)
MCPForUnity/Editor/Tools/ManageComponents.cs (4)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (3)
UnityEngine(31-101)UnityEngine(166-195)GameObject(106-125)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)CoerceString(121-128)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
GameObject(57-64)GameObject(69-72)Type(288-291)GameObjectLookup(15-312)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (4)
Type(103-108)Type(113-118)Type(123-128)UnityTypeResolver(18-215)
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (3)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (3)
UnityEngine(31-101)UnityEngine(166-195)GameObject(106-125)MCPForUnity/Editor/Tools/ManageGameObject.cs (8)
UnityEngine(1931-1934)GameObject(1102-1125)List(1130-1289)List(1976-1994)List(2000-2021)List(2029-2064)IEnumerable(1292-1305)Type(1941-1955)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (7)
List(150-185)IEnumerable(187-192)IEnumerable(194-207)Type(103-108)Type(113-118)Type(123-128)UnityTypeResolver(18-215)
MCPForUnity/Editor/Tools/ManageAsset.cs (3)
MCPForUnity/Editor/Helpers/MaterialOps.cs (2)
MaterialOps(12-396)ApplyProperties(17-155)MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-30)MCPForUnity/Editor/Helpers/PropertyConversion.cs (2)
PropertyConversion(13-100)TryConvertToType(59-69)
Server/src/services/tools/manage_gameobject.py (1)
Server/src/services/tools/utils.py (2)
parse_json_payload(27-60)coerce_int(63-77)
MCPForUnity/Editor/Tools/ManageMaterial.cs (3)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (4)
Material(130-137)ObjectResolver(12-196)Texture(142-149)GameObject(106-125)MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-30)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (2)
GameObject(57-64)GameObject(69-72)
Server/src/services/tools/manage_asset.py (3)
Server/src/services/tools/manage_material.py (1)
_normalize_properties(50-69)Server/src/services/tools/manage_components.py (1)
_normalize_properties(16-40)Server/src/services/tools/utils.py (1)
parse_json_payload(27-60)
Server/tests/integration/test_manage_components.py (2)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)Server/src/services/tools/manage_components.py (1)
manage_components(46-156)
Server/src/services/resources/gameobject.py (2)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (3)
MCPForUnity/Editor/Tools/ManageGameObject.cs (6)
List(1130-1289)List(1976-1994)List(2000-2021)List(2029-2064)GameObject(1102-1125)HandleCommand(30-122)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
List(82-86)List(96-135)GameObject(57-64)GameObject(69-72)MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
HandleCommand(24-70)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (5)
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
List(82-86)List(96-135)GameObject(57-64)GameObject(69-72)MCPForUnity/Editor/Tools/ManageComponents.cs (2)
GameObject(263-287)HandleCommand(27-64)MCPForUnity/Editor/Tools/ManageScene.cs (2)
GameObject(577-622)HandleCommand(90-191)MCPForUnity/Editor/Helpers/VectorParsing.cs (3)
Vector3(18-51)Vector3(56-59)Quaternion(108-169)MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
HandleCommand(24-70)
Server/src/services/tools/manage_material.py (5)
Server/src/services/tools/utils.py (2)
parse_json_payload(27-60)coerce_int(63-77)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/services/tools/manage_asset.py (1)
_normalize_properties(19-44)Server/src/services/tools/manage_components.py (1)
_normalize_properties(16-40)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (3)
MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)MCPForUnity/Editor/Tools/ManageComponents.cs (1)
AddComponent(68-116)MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
Component(23-73)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (3)
MCPForUnity/Editor/Tools/ManageGameObject.cs (3)
UnityEngine(1931-1934)Type(1941-1955)GameObject(1102-1125)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
Type(288-291)GameObject(57-64)GameObject(69-72)GameObjectLookup(15-312)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (3)
Type(103-108)Type(113-118)Type(123-128)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (4)
MCPForUnity/Editor/Tools/ManageGameObject.cs (5)
GameObject(1102-1125)HandleCommand(30-122)Vector3(998-1032)Vector3(1079-1097)Vector3(1831-1846)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (2)
GameObject(57-64)GameObject(69-72)MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
HandleCommand(24-70)MCPForUnity/Editor/Helpers/VectorParsing.cs (2)
Vector3(18-51)Vector3(56-59)
Server/src/services/tools/debug_request_context.py (1)
Server/src/transport/unity_instance_middleware.py (1)
get_unity_instance_middleware(22-31)
🪛 Ruff (0.14.10)
Server/src/services/tools/find_gameobjects.py
83-83: Do not catch blind exception: Exception
(BLE001)
Server/tests/integration/test_find_gameobjects.py
17-17: Unused function argument: kwargs
(ARG001)
54-54: Unused function argument: cmd
(ARG001)
54-54: Unused function argument: kwargs
(ARG001)
89-89: Unused function argument: cmd
(ARG001)
89-89: Unused function argument: kwargs
(ARG001)
128-128: Unused function argument: cmd
(ARG001)
128-128: Unused function argument: kwargs
(ARG001)
155-155: Unused function argument: cmd
(ARG001)
155-155: Unused function argument: kwargs
(ARG001)
181-181: Unused function argument: cmd
(ARG001)
181-181: Unused function argument: kwargs
(ARG001)
Server/tests/integration/test_gameobject_resources.py
20-20: Unused function argument: kwargs
(ARG001)
58-58: Unused function argument: kwargs
(ARG001)
97-97: Unused function argument: cmd
(ARG001)
97-97: Unused function argument: kwargs
(ARG001)
135-135: Unused function argument: cmd
(ARG001)
135-135: Unused function argument: kwargs
(ARG001)
173-173: Unused function argument: kwargs
(ARG001)
210-210: Unused function argument: cmd
(ARG001)
210-210: Unused function argument: params
(ARG001)
210-210: Unused function argument: kwargs
(ARG001)
235-235: Unused function argument: cmd
(ARG001)
235-235: Unused function argument: params
(ARG001)
235-235: Unused function argument: kwargs
(ARG001)
Server/src/services/tools/manage_components.py
155-155: Do not catch blind exception: Exception
(BLE001)
Server/tests/integration/test_manage_components.py
17-17: Unused function argument: kwargs
(ARG001)
52-52: Unused function argument: cmd
(ARG001)
52-52: Unused function argument: kwargs
(ARG001)
79-79: Unused function argument: cmd
(ARG001)
79-79: Unused function argument: kwargs
(ARG001)
109-109: Unused function argument: cmd
(ARG001)
109-109: Unused function argument: kwargs
(ARG001)
137-137: Unused function argument: cmd
(ARG001)
137-137: Unused function argument: kwargs
(ARG001)
164-164: Unused function argument: cmd
(ARG001)
164-164: Unused function argument: kwargs
(ARG001)
194-194: Unused function argument: cmd
(ARG001)
194-194: Unused function argument: kwargs
(ARG001)
221-221: Unused function argument: cmd
(ARG001)
221-221: Unused function argument: kwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @Server/src/services/tools/manage_script.py:
- Line 516: The parameter annotation for contents is using two descriptive
strings (Annotated[str, "Contents of the script to create", "C# code for
'create' action"]) which is inconsistent with the rest of the file; replace the
dual annotations with a single descriptive annotation such as Annotated[str,
"Contents of the script to create"] (or move the language-specific note into the
docstring/comment) and ensure any other similar parameters follow the same
single-string pattern to keep annotations consistent; update the signature where
the parameter name contents appears (and related docstrings) accordingly.
🧹 Nitpick comments (4)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
163-189: LGTM! Consistent with existing converter patterns.The
Vector4Convertercorrectly follows the established pattern used byVector2Converter,Vector3Converter, andQuaternionConverterin this file. The serialization and deserialization logic is straightforward and correct.Optional: For consistency with the more defensive
Matrix4x4Converterimplementation (lines 232-247), you could consider using null-safe accessors inReadJson:🔎 Optional defensive null handling
public override Vector4 ReadJson(JsonReader reader, Type objectType, Vector4 existingValue, bool hasExistingValue, JsonSerializer serializer) { JObject jo = JObject.Load(reader); return new Vector4( - (float)jo["x"], - (float)jo["y"], - (float)jo["z"], - (float)jo["w"] + jo["x"]?.Value<float>() ?? 0f, + jo["y"]?.Value<float>() ?? 0f, + jo["z"]?.Value<float>() ?? 0f, + jo["w"]?.Value<float>() ?? 0f ); }This would prevent
NullReferenceExceptionon malformed input, though the current approach is acceptable given the existing converters use the same pattern.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (2)
510-524: Consider using pattern matching for safer iteration.The direct cast
foreach (JObject comp in components)will throwInvalidCastExceptionif any element isn't aJObject. While this would indicate a serialization bug, using pattern matching provides clearer diagnostics.🔎 Suggested improvement
- foreach (JObject comp in components) + foreach (var compToken in components) { + Assert.IsTrue(compToken is JObject, $"Expected JObject but got {compToken?.Type}"); + var comp = (JObject)compToken; var typeName = comp["typeName"]?.Value<string>();
578-582: Consider hiding marker component from Unity's Add Component menu.Since this marker is only for test isolation, hiding it from the Unity editor menu prevents accidental usage in production scenes.
🔎 Suggested improvement
/// <summary> /// Marker component used for isolating component-based searches to objects created by this test fixture. /// </summary> + [UnityEngine.AddComponentMenu("")] // Hide from Add Component menu public sealed class GameObjectAPIStressTestMarker : MonoBehaviour { }MCPForUnity/Editor/Helpers/ObjectResolver.cs (1)
62-69: Consider makingincludeInactiveconfigurable.The scene search hardcodes
includeInactive: false. If callers need to resolve inactive GameObjects, they'd need to modify this or useGameObjectLookupdirectly. Consider adding an optional parameter toResolveor accepting it from the instruction:bool includeInactive = instruction["includeInactive"]?.ToObject<bool>() ?? false;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/claude-gameobject-suite.yml.github/workflows/claude-nl-suite.ymlMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/ObjectResolver.csMCPForUnity/Editor/Helpers/UnityJsonSerializer.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Runtime/Serialization/UnityTypeConverters.csServer/src/services/tools/execute_menu_item.pyServer/src/services/tools/find_gameobjects.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/manage_shader.pyServer/src/services/tools/utils.pyTestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs
- Server/src/services/tools/manage_shader.py
- Server/src/services/tools/manage_editor.py
- MCPForUnity/Editor/Tools/ManageScene.cs
🧰 Additional context used
🧬 Code graph analysis (8)
MCPForUnity/Editor/Tools/ManageComponents.cs (6)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (3)
UnityEngine(31-104)UnityEngine(169-198)GameObject(109-128)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
JObject(98-149)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)CoerceString(121-128)MCPForUnity/Editor/Helpers/ComponentOps.cs (5)
RemoveComponent(82-122)SetProperty(132-223)Component(23-73)ComponentOps(14-302)List(228-262)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (6)
GameObject(57-64)GameObject(69-72)Type(288-291)List(82-86)List(96-135)GameObjectLookup(15-312)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (5)
Type(103-108)Type(113-118)Type(123-128)UnityTypeResolver(18-215)List(150-185)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (3)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (3)
UnityEngine(31-104)UnityEngine(169-198)GameObject(109-128)MCPForUnity/Editor/Tools/ManageComponents.cs (3)
GameObject(265-289)HandleCommand(27-64)AddComponent(68-118)MCPForUnity/Editor/Tools/ManageScene.cs (2)
GameObject(578-623)HandleCommand(90-191)
Server/src/services/tools/find_gameobjects.py (5)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/services/tools/utils.py (2)
coerce_bool(11-24)coerce_int(63-77)Server/src/services/tools/preflight.py (1)
preflight(27-105)Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
Server/src/services/tools/manage_asset.py (1)
Server/src/services/tools/utils.py (3)
parse_json_payload(27-60)coerce_int(63-77)normalize_properties(80-112)
Server/src/services/tools/manage_material.py (3)
Server/src/services/tools/utils.py (3)
parse_json_payload(27-60)coerce_int(63-77)normalize_properties(80-112)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)
MCPForUnity/Editor/Tools/ManageMaterial.cs (4)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
JObject(98-149)MCPForUnity/Editor/Helpers/ObjectResolver.cs (4)
Material(133-140)ObjectResolver(12-199)Texture(145-152)GameObject(109-128)MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs (1)
RenderPipelineUtility(7-194)
MCPForUnity/Editor/Helpers/ComponentOps.cs (3)
MCPForUnity/Editor/Helpers/PropertyConversion.cs (3)
UnityEngine(85-99)PropertyConversion(13-100)ConvertToType(21-53)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (3)
Type(103-108)Type(113-118)Type(123-128)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)NormalizePropertyName(170-199)
Server/src/services/tools/manage_components.py (5)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/services/tools/utils.py (2)
parse_json_payload(27-60)normalize_properties(80-112)Server/src/services/tools/preflight.py (1)
preflight(27-105)Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
🪛 Ruff (0.14.10)
Server/src/services/tools/find_gameobjects.py
84-84: Do not catch blind exception: Exception
(BLE001)
Server/src/services/tools/manage_components.py
128-128: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (54)
TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs (3)
23-26: LGTM! Animation parameter hash fix confirmed.The animation hashes now correctly use "reachX" and "reachY", addressing the critical issue from the previous review. This ensures the animator will find the correct parameters at runtime.
107-112: LGTM! Improved performance with pre-computed hashes.The refactored
ApplyBlendmethod now uses the pre-computed hash fields instead of string literals, which is more efficient for animator parameter access. The null safety check is also a good defensive practice.
760-772: Test scaffolding appears intentional.The local helper functions
TestHelperandIncrementCounterare defined but never invoked. Given this file's purpose as test scaffolding for Claude NL/T editing tests, these unused functions appear to be intentional markers for testing editing capabilities rather than functional code.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (5)
1-52: Well-structured test foundation.The test fixture is well-organized with proper lifecycle management. The
SetUp/TearDownmethods correctly handle object cleanup, including null checks beforeDestroyImmediate. TheCreateTestObjecthelper ensures all created objects are tracked for cleanup.
54-128: LGTM!The
ToJObjecthelper provides robust conversion with appropriate error handling. The bulk creation tests properly track objects via instance ID for cleanup, and the timing thresholds are appropriately generous for CI environments.
133-245: Good use of marker component for test isolation.Using
GameObjectAPIStressTestMarkerto isolate component-based searches is an effective pattern that prevents interference from other scene objects. The pagination tests properly validatetotalCount,hasMore,nextCursor, and include a safety limit (line 240) to prevent infinite loops.
251-354: LGTM!Component operation tests provide good coverage. The float assertions correctly use tolerance values (0.01f), and the test properly accounts for the implicit Transform component in count assertions.
531-573: LGTM!The rapid-fire create-modify-delete cycle test is well-structured with assertions at each step. Good choice to use name modification instead of layer names to avoid potential Unity layer configuration issues.
Server/src/services/tools/manage_script.py (8)
7-7: LGTM: Import added to support tool metadata annotations.The
ToolAnnotationsimport is correctly added and used consistently throughout the file to enrich tool metadata.
67-86: LGTM: Tool annotations correctly indicate destructive operation.The
destructiveHint=Trueflag appropriately reflects that this tool modifies script content.
377-383: LGTM: Annotations properly flag script creation as destructive.
428-434: LGTM: Annotations correctly mark deletion as destructive.
456-462: LGTM: Read-only hint correctly reflects validation behavior.
503-509: LGTM: Destructive hint appropriate for create/delete actions.
577-590: LGTM: Read-only hint matches capability query behavior.
615-621: LGTM: Read-only hint appropriate for SHA retrieval.Server/src/services/tools/execute_menu_item.py (2)
7-7: LGTM!The import is necessary for the decorator annotations below and follows standard Python import conventions.
16-22: No changes needed—ToolAnnotations with destructiveHint is fully supported.The use of
ToolAnnotationswithdestructiveHint=Trueis correct and compatible with mcp>=1.16.0 (your project's minimum version). This annotation pattern is consistently used throughout the codebase across 30+ tools and accurately reflects that this tool can execute arbitrary Unity menu items, including destructive operations.Server/src/services/tools/utils.py (1)
80-112: Well-designed normalization utility.The function handles common LLM/client serialization edge cases robustly. The tuple return pattern
(result, error)provides clear success/failure signaling without exceptions.Note: The
"null"string check (line 103) correctly catches JavaScript serialization bugs wherenullgets stringified rather than omitted, which is the right behavior since actual JSON null would parse to PythonNoneviaparse_json_payload.Server/src/services/tools/manage_components.py (2)
103-108: Value condition correctly handles falsy values.The condition
value is not None(line 104) properly allows falsy values like0,False, and""to be set. This is the correct approach since onlyNoneshould indicate "not provided".
128-129: Broad exception catch is acceptable for top-level tool handler.The static analysis hint (BLE001) flags the broad
Exceptioncatch, but this is appropriate here as a top-level tool handler that must return a structured response. The error message includes the exception details for debugging.Server/src/services/tools/find_gameobjects.py (2)
84-85: Broad exception catch acceptable for tool handler.Same rationale as
manage_components— top-level handlers need to return structured responses.
27-28: No server-side enforcement needed—Unity-side validation handles the maximum.The annotation on
page_sizecorrectly documentsmax: 500. While the Python server doesn't enforce this limit, the Unity handler (FindGameObjects.cs:42) explicitly clamps it:Mathf.Clamp(pagination.PageSize, 1, 500). The current code is working as designed.MCPForUnity/Editor/Helpers/ObjectResolver.cs (1)
156-167: Asset type detection looks comprehensive.The
IsAssetTypehelper covers common Unity asset types. TheUnityEngine.U2Dnamespace string check (line 161) is a reasonable workaround to detect SpriteAtlas without a hard dependency.MCPForUnity/Editor/Helpers/ComponentOps.cs (3)
156-173: Conversion failure detection correctly implemented.The null-check pattern
convertedValue == null && value.Type != JTokenType.Nullproperly distinguishes between:
- Intentional null assignment (JToken is null type) → proceeds
- Conversion failure (result null but input wasn't null) → returns error
This addresses the concern from the previous review about silent fallthrough.
266-292: 2D/3D physics conflict detection is a great UX improvement.This proactively prevents a common Unity pitfall where mixing 2D and 3D physics components leads to unexpected behavior. The check covers both Rigidbody and Collider variants.
228-262: Comprehensive member discovery including serialized fields.The method correctly includes:
- Public writable properties
- Public non-init fields
- Private
[SerializeField]fields (essential for Unity Inspector compatibility)The sorted output ensures consistent results.
MCPForUnity/Editor/Tools/ManageMaterial.cs (3)
19-19: Consistent response type standardization.The migration from anonymous objects to
ErrorResponse/SuccessResponseimproves type safety and API consistency across all tool handlers. This aligns with the PR's goal of consolidating shared patterns.Also applies to: 27-27, 48-48, 53-53
84-91: ObjectResolver integration is clean and consistent.The pattern of creating a
JObjectinstruction and callingObjectResolver.Resolveis applied uniformly. This centralizes asset/object resolution logic and makes the code more maintainable.Also applies to: 139-145
366-449: Unity version compatibility handled correctly.The
#if UNITY_6000_0_OR_NEWERpreprocessor directive properly handles the shader property API changes between Unity versions. Both code paths maintain equivalent functionality..github/workflows/claude-gameobject-suite.yml (4)
219-219: Proper credential redaction in logs.The workflow correctly redacts sensitive information using
sed -E 's/((email|serial|license|password|token)[^[:space:]]*)/[REDACTED]/Ig'before outputting logs. This prevents accidental credential exposure in CI output.Also applies to: 236-237, 241-241, 247-248
211-248: Robust bridge readiness detection.The readiness check uses multiple signals:
- Port connectivity via TCP socket test
- Log pattern matching for startup markers
- Fatal error detection for licensing issues after warm-up period
The 600s deadline with 120s fatal-after threshold provides good balance between allowing startup time and failing fast on known issues.
629-636: License return step properly configured.The conditional
steps.lic.outputs.use_ebl == 'true' && steps.lic.outputs.has_serial == 'true'correctly limits license return to cases where a serial-based license was activated. Thecontinue-on-error: trueensures cleanup proceeds even if return fails.
429-434: Bare<testcase>elements are intentionally designed and work correctly.The backfill logic writes standalone
<testcase>elements without a wrapper, but this is not a problem. These fragments parse successfully as valid XML, and the assembly step explicitly handles this pattern by callingET.parse(frag).getroot()on each fragment before appending to the suite. No parse issues occur.Likely an incorrect or invalid review comment.
MCPForUnity/Editor/Tools/ManageComponents.cs (4)
96-103: Undo registration addressed correctly.The Undo.RecordObject call at line 101 properly records the component before modifying properties, addressing the previous review comment. The implementation now correctly supports undo for both the add operation (handled by ComponentOps.AddComponent) and the property modifications.
200-243: Clean error aggregation pattern.The implementation correctly aggregates property-setting errors and returns a structured response indicating partial failure. The approach of continuing to set other properties even when some fail provides better user experience than failing fast on the first error.
265-289: Well-structured target resolution with appropriate fallbacks.The FindTarget method provides a sensible resolution chain: direct instance ID → parsed string ID → name-based lookup. Including inactive GameObjects by default (line 288) is appropriate for component management operations.
291-308: Error handling is consistent with add action semantics.The SetPropertiesOnComponent helper logs warnings for failed properties but doesn't propagate errors. This is appropriate for the "add" action context where the primary operation (adding the component) succeeded. The warning log provides visibility for debugging while not failing the entire operation.
.github/workflows/claude-nl-suite.yml (3)
754-780: GO pass structure follows established patterns.The new GO pass implementation correctly mirrors the existing NL and T pass patterns with appropriate timeout (20 minutes), retry logic, and coverage checks. The prompt instructs emission of exactly 11 test fragments (GO-0 through GO-10) with clear stop conditions.
868-878: Canonicalization rules are well-defined.The GO test patterns follow the established format and provide specific regex patterns for each test ID. The patterns appropriately match likely test names while remaining flexible enough to handle variations.
946-960: Backfill logic comprehensively updated for GO tests.The DESIRED list now includes all 26 test IDs (NL-0..4, T-A..J, GO-0..10), and the id_from_filename function correctly extracts GO test IDs from filenames. The pattern matching is consistent with the existing NL/T implementation.
MCPForUnity/Editor/Tools/ManageGameObject.cs (7)
25-26: Clean serializer consolidation.The InputSerializer now aliases UnityJsonSerializer.Instance, centralizing serializer configuration and ensuring consistent JSON handling across the codebase.
79-95: Improved prefab handling with clear user guidance.The prefab asset check now provides actionable guidance directing users to the appropriate tools (
manage_assetfor modifications,manage_prefabsfor isolation mode). Prefab instantiation via thecreateaction is preserved.
1227-1243: Unity 2021 compatibility maintained correctly.The code properly uses
FindObjectsOfType(Unity 2021-compatible) instead of newer APIs, with appropriate manual filtering. The filtersearchInactive || (go != null && go.activeInHierarchy)correctly includes all objects when searching inactive, or only active hierarchy objects otherwise.
1530-1596: Property name normalization improves API usability.The implementation tries both original and normalized property names, improving compatibility with human-readable inputs like "Use Gravity" while maintaining backwards compatibility with existing camelCase property names.
1818-1826: Clean delegation to centralized conversion utility.The ConvertJTokenToType method now delegates to PropertyConversion.ConvertToType, centralizing type conversion logic. The inputSerializer parameter is retained for API compatibility but is correctly documented as ignored.
1959-1971: ComponentResolver properly delegates to unified type resolver.The TryResolve method delegates to UnityTypeResolver.TryResolve with a Component constraint, maintaining the same behavior while leveraging the new centralized type resolution infrastructure.
1996-2021: Method rename from GetAIPropertySuggestions to GetFuzzyPropertySuggestions.The rename better reflects the actual implementation (rule-based fuzzy matching) rather than implying AI/ML involvement. The functionality remains unchanged.
Server/src/services/tools/manage_asset.py (3)
24-28: Tool annotations added with appropriate metadata.The ToolAnnotations with
destructiveHint=Truecorrectly signals that this tool can modify assets, helping LLM clients make informed decisions about tool usage.
62-66: Consistent property normalization using shared utility.The normalize_properties helper provides robust handling of various input formats (dict, JSON string, invalid values) with clear error messages. The error prefix "manage_asset:" provides good context for debugging.
71-93: Smart input normalization for search action.The logic correctly handles common LLM mistakes where queries like "t:MonoScript" are placed in
pathinstead ofsearch_pattern. The automatic normalization with info logging improves robustness while maintaining transparency.Server/src/services/tools/manage_material.py (3)
17-47: Robust color normalization with clear error messages.The
_normalize_colorhelper properly handles multiple input formats (list, tuple, JSON string) and validates that colors have 3 or 4 components. Error messages are descriptive and actionable.
92-108: Consistent validation chain with early returns.The normalization sequence follows a clean pattern: validate → return early on error. The check for sentinel values like
"[object Object]"and"undefined"catches common serialization bugs from MCP clients.
50-55: Tool annotations with accurate metadata.The
destructiveHint=Trueannotation is appropriate since most actions (create, set_, assign_) modify state. The description clearly distinguishes read-only actions (ping, get_material_info) from modifying actions.
|
🔥 🔥 🔥 🔥 🔥 🔥 🔥 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
545-595: Consider adding cancellation support for long auto-start attempts.The retry logic can take up to ~75 seconds to exhaust all attempts. While the extended delays are intentional for Windows/dev environments (per commit notes), users have no way to cancel if they realize the server failed to start. Consider:
- Passing a
CancellationTokenthat gets triggered if the user clicks "Stop Server" or closes the window.- Adding periodic UI feedback (e.g., updating status label) so users know the auto-start is in progress.
This is optional given the current UX where the server button remains disabled during the operation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)
🔇 Additional comments (8)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (6)
271-283: LGTM on orphaned session detection.The fire-and-forget pattern is acceptable here since
EndOrphanedSessionAsynchas internal exception handling. The guard condition correctly prevents triggering during toggle operations, and the local state update ensures consistent UI behavior for the remainder of the method.
285-292: Clear separation of server and session lifecycle concerns.The visibility logic correctly differentiates HTTP Local (where session requires a running server) from Stdio/HTTP Remote (where session toggle is always available).
311-318: Good defensive handling for instance name display.The double fallback ensures a sensible default even if path parsing fails. This enhances usability in multi-instance scenarios.
464-472: LGTM!Button state management correctly reflects the lifecycle separation. The enable condition properly prevents interaction during toggle operations while allowing either start or stop actions.
500-530: Proper lifecycle ordering in toggle handler.Correctly ends the session before stopping the server, and auto-starts the session after launching the server. This provides a smooth UX while maintaining proper resource cleanup order.
678-698: LGTM on orphaned session cleanup.The method correctly guards against concurrent operations, handles exceptions gracefully with logging, and ensures UI state is restored in the finally block.
MCPForUnity/Editor/Services/ServerManagementService.cs (2)
948-977: LGTM! Improved netstat parsing avoids false failures.The refactor to run
netstat.exe -anodirectly and filter in C# is a solid improvement. The previous approach withfindstrin a pipe caused the entire command to return exit code 1 when no matches were found. Key improvements:
- Stricter validation ensures the local address column actually ends with
:{port}(Line 970)- Processing stdout regardless of the success flag handles edge cases gracefully
1018-1056: Good two-step validation approach for Windows process identification.The refactor improves reliability by:
- First checking process name via
tasklist(fast filter)- Then validating command line via
wmicwhen available (precise validation)- Gracefully falling back to process name check if wmic access is restricted
Note:
wmicis deprecated on Windows 10/11 in favor of PowerShell's CIM cmdlets (Get-CimInstance). The codebase already uses this modern approach elsewhere (e.g.,TryProcessCommandLineContainsInstanceTokenon line ~170). Consider unifying to PowerShell CIM in a future iteration, thoughwmicremains functional and may be more reliable in restricted environments where PowerShell execution policies are enforced.
- find_gameobjects: Search GameObjects, returns paginated instance IDs only
- manage_components: Component lifecycle (add, remove, set_property)
- unity://scene/gameobject/{id}: Single GameObject data (no component serialization)
- unity://scene/gameobject/{id}/components: All components (paginated)
- unity://scene/gameobject/{id}/component/{name}: Single component by type
- manage_scene get_hierarchy: Now includes componentTypes array
- manage_gameobject: Slimmed to lifecycle only (create, modify, delete)
- Legacy actions (find, get_components, etc.) log deprecation warnings
- ParamCoercion: Centralized int/bool/float/string coercion
- VectorParsing: Vector3/Vector2/Quaternion/Color parsing
- GameObjectLookup: Centralized GameObject search logic
- 76 new Unity EditMode tests for ManageGameObject actions
- 21 new pytest tests for Python tools/resources
- New NL/T CI suite for GameObject API (GO-0 to GO-5)
Addresses LLM confusion with parameter overload by splitting into
focused tools and read-only resources.
Stress Tests (12 new tests): - BulkCreate small/medium batches - FindGameObjects pagination with by_component search - AddComponents to single object - GetComponents with full serialization - SetComponentProperties (complex Rigidbody) - Deep hierarchy creation and path lookup - GetHierarchy with large scenes - Resource read performance tests - RapidFire create-modify-delete cycles NL/T Suite Updates: - Added GO-0..GO-10 tests in nl-gameobject-suite.md - Fixed tool naming: mcp__unity__ → mcp__UnityMCP__ Other: - Fixed LongUnityScriptClaudeTest.cs compilation errors - Added reports/, .claude/local/, scripts/local-test/ to .gitignore All 254 EditMode tests pass (250 run, 4 explicit skips)
- ParamCoercion: Use CultureInfo.InvariantCulture for float parsing - ManageComponents: Move Transform removal check before GetComponent - ManageGameObjectFindTests: Use try-finally for LogAssert.ignoreFailingMessages - VectorParsing: Document that quaternions are not auto-normalized - gameobject.py: Prefix unused ctx parameter with underscore
NL/T Prompt Fixes: - nl-gameobject-suite.md: Remove non-existent list_resources/read_resource from AllowedTools - nl-gameobject-suite.md: Fix parameter names (component_type, properties) - nl-unity-suite-nl.md: Remove unused manage_editor from AllowedTools Test Fixes: - GameObjectAPIStressTests: Add null check to ToJObject helper - GameObjectAPIStressTests: Clarify AudioSource usage comment - ManageGameObjectFindTests: Use built-in 'UI' layer instead of 'Water' - LongUnityScriptClaudeTest: Clean up NL/T test artifacts (Counte42 typo, HasTarget)
- Add missing tools: manage_components, batch_execute, find_gameobjects, refresh_unity - Add missing resources: gameobject_api, editor_state_v2 - Make descriptions more concise across all tools and resources - Ensure documentation matches current MCP server functionality
- Remove Materials folder (40 .mat files from interactive testing) - Remove Shaders folder (5 noise shaders from testing) - Remove test scripts (Bounce*, CylinderBounce* from testing) - Remove Temp.meta and commit.sh
- Remove deprecated switch cases: find, get_components, get_component, add_component, remove_component, set_component_property - Remove deprecated wrapper methods (423 lines deleted from ManageGameObject.cs) - Delete ManageGameObjectFindTests.cs (tests deprecated 'find' action) - Remove deprecated test methods from ManageGameObjectTests.cs - Add GameObject resource URIs to README documentation - Add batch_execute performance tips to README, tool description, and gameobject_api resource - Enhance batch_execute description to emphasize 10-100x performance gains Total: ~1200 lines removed. New API (find_gameobjects, manage_components, resources) is the recommended path forward.
Major architectural improvements: - Create UnityJsonSerializer for shared JSON/Unity type conversion - Create ObjectResolver for unified object resolution (GameObjects, Components, Assets) - Create UnityTypeResolver for consolidated type resolution with caching - Create PropertyConversion for unified JSON→Unity property conversion - Create ComponentOps for low-level component operations - Create Pagination helpers for standardized pagination across tools Tool simplifications: - ManageGameObject: Remove 68-line prefab redirect anti-pattern, delegate to helpers - ManageAsset: Remove ~80 lines duplicate ConvertJTokenToType - ManageScriptableObject: Remove ~40 lines duplicate ResolveType - ManageComponents: Use ComponentOps, UnityTypeResolver (~90 lines saved) - ManageMaterial: Standardize to SuccessResponse/ErrorResponse patterns - FindGameObjects: Use PaginationRequest/PaginationResponse - GameObjectLookup: FindComponentType delegates to UnityTypeResolver Tests: 242/246 passed, 4 skipped (expected)
…bility Python Server: - Extract normalize_properties() to shared utils.py (removes duplication) - Move search_term validation before preflight() for fail-fast - Fix manage_script.py documentation (remove incorrect 'update' reference) - Remove stale comments in execute_menu_item.py, manage_editor.py - Remove misleading destructiveHint from manage_shader.py C# Unity: - Add Vector4Converter (commonly used, was missing) - Fix Unity 2021 compatibility: replace FindObjectsByType with FindObjectsOfType - Add path normalization in ObjectResolver before StartsWith check - Improve ComponentOps.SetProperty conversion error detection - Add Undo.RecordObject in ManageComponents before property modifications - Improve error message clarity in ManageMaterial.cs - Add defensive error handling to stress test ToJObject helper - Increase CI timeout thresholds for test stability GitHub Workflows: - Fix GO test sorting in markdown output (GO-10 now sorts after GO-9) - Add warning logging for fragment parsing errors
BlendXHash/BlendYHash now use 'reachX'/'reachY' to match the actual animator parameter names.
- Fix netstat detection on Windows by running netstat.exe directly instead of piping through findstr (findstr returns exit code 1 when no matches, causing false detection failures) - Increase auto-start retry attempts (20→30) and delays (2s→3s) to handle slow server starts during first install, version upgrades, and dev mode - Only attempt blind connection after 20 failed detection attempts to reduce connection error spam during server startup - Remove verbose debug logs that were spamming the console every frame
dsarno
force-pushed
the
feature/tools-refactoring
branch
from
37261d2 to
3fd6d68
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @MCPForUnity/Editor/Helpers/PropertyConversion.cs:
- Around line 55-69: The TryConvertToType method currently swallows all
exceptions silently; update its catch to log a warning including the caught
exception and context before returning null so it matches the XML doc.
Specifically, in TryConvertToType (which calls ConvertToType), catch the
exception as e and call your logging facility (e.g.,
Debug.LogWarning/Logger.Warn/processLogger) with a message that includes the
targetType, token (or token.Type/token.ToString()), and e.ToString(), then
return null.
🧹 Nitpick comments (13)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
1021-1055: Consider using CIM/PowerShell instead of deprecated wmic.The two-step validation (tasklist for process name, then command line inspection) is a sound approach. However,
wmicis deprecated on modern Windows versions. For consistency with the CIM approach already used at line 170-171, consider using PowerShell'sGet-CimInstancehere as well.🔎 Proposed refactor using CIM
- // Step 2: Try to get command line with wmic for better validation - ExecPath.TryRun("cmd.exe", $"/c wmic process where \"ProcessId={pid}\" get CommandLine /value", Application.dataPath, out var wmicOut, out var wmicErr, 5000); - string wmicCombined = ((wmicOut ?? string.Empty) + "\n" + (wmicErr ?? string.Empty)).ToLowerInvariant(); - string wmicCompact = NormalizeForMatch(wmicOut ?? string.Empty); - - // If we can see the command line, validate it's our server - if (!string.IsNullOrEmpty(wmicCombined) && wmicCombined.Contains("commandline=")) + // Step 2: Try to get command line with CIM for better validation + string ps = $"(Get-CimInstance Win32_Process -Filter 'ProcessId={pid}').CommandLine"; + ExecPath.TryRun("powershell", $"-NoProfile -Command \"{ps}\"", Application.dataPath, out var cimOut, out var cimErr, 5000); + string cimCombined = ((cimOut ?? string.Empty) + "\n" + (cimErr ?? string.Empty)).ToLowerInvariant(); + string cimCompact = NormalizeForMatch(cimOut ?? string.Empty); + + // If we can see the command line, validate it's our server + if (!string.IsNullOrEmpty(cimOut) && !string.IsNullOrWhiteSpace(cimOut)) { - bool mentionsMcp = wmicCompact.Contains("mcp-for-unity") - || wmicCompact.Contains("mcp_for_unity") - || wmicCompact.Contains("mcpforunity") - || wmicCompact.Contains("mcpforunityserver"); - bool mentionsTransport = wmicCompact.Contains("--transporthttp") || (wmicCompact.Contains("--transport") && wmicCompact.Contains("http")); - bool mentionsUvicorn = wmicCombined.Contains("uvicorn"); + bool mentionsMcp = cimCompact.Contains("mcp-for-unity") + || cimCompact.Contains("mcp_for_unity") + || cimCompact.Contains("mcpforunity") + || cimCompact.Contains("mcpforunityserver"); + bool mentionsTransport = cimCompact.Contains("--transporthttp") || (cimCompact.Contains("--transport") && cimCompact.Contains("http")); + bool mentionsUvicorn = cimCombined.Contains("uvicorn");Server/src/services/tools/manage_components.py (2)
84-87: Property normalization logic is correct.The delegation to
normalize_propertiesand error handling work as intended. The shared utility provides robust validation and descriptive error messages.One minor readability consideration: Line 85 reassigns the
propertiesparameter, which shadows the function parameter. If you prefer clarity, you could use a different variable name likenormalized_properties:normalized_properties, props_error = normalize_properties(properties) if props_error: return {"success": False, "message": props_error}Then update line 107-111 to reference
normalized_properties. However, the current implementation works fine.
89-91: Value validation pattern is functional.The validation for
valuefollows a similar pattern to whatnormalize_propertiesdoes for thepropertiesparameter. If you find yourself repeating this validation pattern across multiple tools for single-value parameters, you might consider extracting a lightweightnormalize_valueutility in the future. For now, the current approach works well.Server/src/services/tools/manage_shader.py (1)
40-47: Consider validatingcontentsparameter against action type.The
elsebranch sendscontentswithout encoding for 'read' or 'delete' actions, which contradicts the parameter annotation stating contents is for 'create'/'update' only. While Unity likely ignores unexpected parameters, defensive coding would improve clarity.🔎 Suggested refactor to restrict contents to valid actions
# Base64 encode the contents if they exist to avoid JSON escaping issues if contents is not None: if action in ['create', 'update']: # Encode content for safer transmission params["encodedContents"] = base64.b64encode( contents.encode('utf-8')).decode('utf-8') params["contentsEncoded"] = True - else: - params["contents"] = contents + elif action in ['read', 'delete']: + # contents parameter ignored for read/delete actions + passAlternatively, raise a validation error if contents is provided for incompatible actions:
# Base64 encode the contents if they exist to avoid JSON escaping issues if contents is not None: if action in ['create', 'update']: # Encode content for safer transmission params["encodedContents"] = base64.b64encode( contents.encode('utf-8')).decode('utf-8') params["contentsEncoded"] = True - else: - params["contents"] = contents + elif action in ['read', 'delete']: + return {"success": False, "message": f"'contents' parameter is not applicable for '{action}' action"}MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (2)
143-148: Cache collision risk with simple names.
CacheByNameuses only the type's simple name as key. If two types share the same simple name in different namespaces, the last resolved type will overwrite the previous entry. This could cause incorrect resolution when querying by simple name after resolving a different type with the same name.Consider caching simple names only when unambiguous, or accepting this as a known trade-off for performance.
194-207: Consider caching player assembly names.
PreferPlayerrecalculatesplayerAsmNamesviaCompilationPipeline.GetAssemblieson every call. Since this is also computed inFindCandidates, consider caching the result or sharing the computed set to avoid redundant work during TypeCache fallback.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.cs (1)
39-47: Consider adding camelCasepageNumbertest.The test validates
page_number(snake_case), but based on the relevant code snippet showing support for bothpage_numberandpageNumber, consider adding a test for the camelCase variant for completeness.🔎 Suggested additional test
[Test] public void PaginationRequest_FromParams_ConvertsPageNumberCamelCaseToCursor() { var p = new JObject { ["pageNumber"] = 3, ["pageSize"] = 10 }; var req = PaginationRequest.FromParams(p); Assert.AreEqual(20, req.Cursor); }MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
17-30: Consider addingMatrix4x4Converterto the converters list.The
Matrix4x4Converterexists inUnityTypeConverters.cs(with special handling to avoidValidTRS()crashes on non-TRS matrices), but it's not registered in this shared serializer. If any serialization path encounters aMatrix4x4value, it will fall back to default serialization which may access problematic computed properties.🔎 Proposed addition
public static readonly JsonSerializer Instance = JsonSerializer.Create(new JsonSerializerSettings { Converters = new List<JsonConverter> { new Vector2Converter(), new Vector3Converter(), new Vector4Converter(), new QuaternionConverter(), new ColorConverter(), new RectConverter(), new BoundsConverter(), + new Matrix4x4Converter(), new UnityEngineObjectConverter() } });MCPForUnity/Editor/Helpers/ObjectResolver.cs (1)
156-167: Expand IsAssetType coverage for robustness.The asset type detection is incomplete and may cause resolution failures for common asset types like TextAsset, AnimationController, RuntimeAnimatorController, Avatar, and PhysicMaterial. GameObject prefabs may also be missed.
🔎 Proposed expansion of asset type coverage
private static bool IsAssetType(Type type) { return typeof(Material).IsAssignableFrom(type) || typeof(Texture).IsAssignableFrom(type) || typeof(ScriptableObject).IsAssignableFrom(type) || type.FullName?.StartsWith("UnityEngine.U2D") == true || typeof(AudioClip).IsAssignableFrom(type) || typeof(AnimationClip).IsAssignableFrom(type) || + typeof(RuntimeAnimatorController).IsAssignableFrom(type) || + typeof(Avatar).IsAssignableFrom(type) || + typeof(PhysicMaterial).IsAssignableFrom(type) || + typeof(TextAsset).IsAssignableFrom(type) || typeof(Font).IsAssignableFrom(type) || typeof(Shader).IsAssignableFrom(type) || typeof(ComputeShader).IsAssignableFrom(type); }MCPForUnity/Editor/Tools/ManageGameObject.cs (3)
1818-1826: Consider removing unusedinputSerializerparameter in future refactor.The comment states the
inputSerializerparameter is kept for backwards compatibility but is ignored. Since this is a private method andPropertyConversion.ConvertToTypeusesUnityJsonSerializer.Instanceinternally, the parameter serves no purpose.For clarity, consider:
- Removing the parameter and updating all call sites (since it's private, blast radius is limited)
- Or, if keeping it for some internal reason, update the comment to explicitly state it's a no-op
This is not urgent but would reduce confusion for future maintainers.
1927-1934: Delegation to ObjectResolver is clean.The refactoring to delegate to
ObjectResolver.Resolve()centralizes resolution logic effectively. The backwards compatibility preservation is appropriate.Minor suggestion: The comment could be more specific about who relies on this method (e.g., "Kept for backwards compatibility with UnityEngineObjectConverter and external tooling").
1227-1242: Code correctly handles Unity 2021.3 compatibility constraints.The manual filtering approach is necessary because
FindObjectsOfType(Type)in Unity 2021.3 lacks anincludeInactiveparameter. The non-generic signature was the only available option at that time. This implementation is correct and functional.For optimization potential: If minimum Unity support is ever upgraded to 2022.1+, the generic
FindObjectsOfType<T>(includeInactive)could replace the manual filtering. However, this only affects scene-wide component searches; hierarchy searches already use the efficientGetComponentsInChildrenpath.MCPForUnity/Editor/Tools/ManageMaterial.cs (1)
485-490: Remove the redundant check or update the comment to reflect actual behavior.The
materialPathat line 487 is the result ofNormalizePath()(line 75), which callsSanitizeAssetPath(). TheSanitizeAssetPath()method always guarantees theAssets/prefix—it cannot fail or return a path without it. The check on lines 487-490 is redundant.The comment is misleading: it claims the check "catches edge cases where normalization might fail," but there are no such edge cases. Either remove this unnecessary defensive check or update the comment to clarify that this is a redundant failsafe (and consider whether keeping it adds value).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
.github/workflows/claude-gameobject-suite.yml.github/workflows/claude-nl-suite.ymlMCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/ComponentOps.cs.metaMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Helpers/ObjectResolver.csMCPForUnity/Editor/Helpers/ObjectResolver.cs.metaMCPForUnity/Editor/Helpers/Pagination.csMCPForUnity/Editor/Helpers/Pagination.cs.metaMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Helpers/PropertyConversion.cs.metaMCPForUnity/Editor/Helpers/UnityJsonSerializer.csMCPForUnity/Editor/Helpers/UnityJsonSerializer.cs.metaMCPForUnity/Editor/Helpers/UnityTypeResolver.csMCPForUnity/Editor/Helpers/UnityTypeResolver.cs.metaMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Tools/FindGameObjects.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Runtime/Serialization/UnityTypeConverters.csREADME.mdServer/src/services/tools/batch_execute.pyServer/src/services/tools/execute_menu_item.pyServer/src/services/tools/find_gameobjects.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/manage_shader.pyServer/src/services/tools/utils.pyServer/tests/integration/test_manage_asset_json_parsing.pyTestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.csTestProjects/UnityMCPTests/Assets/Scripts/TestScriptableObjectInstance.asset.metaTestProjects/UnityMCPTests/Assets/Temp.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs
💤 Files with no reviewable changes (1)
- Server/src/services/tools/execute_menu_item.py
✅ Files skipped from review due to trivial changes (1)
- MCPForUnity/Editor/Helpers/PropertyConversion.cs.meta
🚧 Files skipped from review as they are similar to previous changes (22)
- MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs.meta
- TestProjects/UnityMCPTests/Assets/Scripts/TestScriptableObjectInstance.asset.meta
- .github/workflows/claude-gameobject-suite.yml
- Server/src/services/tools/manage_asset.py
- TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.cs.meta
- MCPForUnity/Editor/Helpers/ComponentOps.cs.meta
- TestProjects/UnityMCPTests/Assets/Temp.meta
- Server/src/services/tools/manage_script.py
- Server/src/services/tools/find_gameobjects.py
- README.md
- MCPForUnity/Editor/Helpers/UnityTypeResolver.cs.meta
- MCPForUnity/Editor/Helpers/ParamCoercion.cs
- MCPForUnity/Editor/Tools/ManageScene.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs
- .github/workflows/claude-nl-suite.yml
- MCPForUnity/Editor/Tools/ManageScriptableObject.cs
- Server/src/services/tools/utils.py
- MCPForUnity/Editor/Helpers/Pagination.cs
- Server/src/services/tools/manage_editor.py
- Server/src/services/tools/batch_execute.py
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs
🧰 Additional context used
🧬 Code graph analysis (17)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
Type(1941-1955)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (1)
JObject(54-67)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)
MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (8)
Vector2Converter(36-56)Vector3Converter(11-34)Vector4Converter(163-189)QuaternionConverter(58-84)ColorConverter(86-112)RectConverter(114-140)BoundsConverter(142-161)UnityEngineObjectConverter(253-354)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (2)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/ServerManagementService.cs (1)
IsLocalHttpServerRunning(538-598)
MCPForUnity/Editor/Tools/FindGameObjects.cs (2)
MCPForUnity/Editor/Helpers/Pagination.cs (4)
PaginationRequest(11-66)PaginationRequest(28-65)PaginationResponse(73-147)PaginationResponse(117-146)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)CoerceBool(53-82)
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (2)
MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (4)
Type(103-108)Type(113-118)Type(123-128)UnityTypeResolver(18-215)MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
Type(893-896)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (3)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-212)NormalizeSeparators(21-26)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
Type(288-291)GameObject(57-64)GameObject(69-72)GameObjectLookup(15-312)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (3)
Type(103-108)Type(113-118)Type(123-128)
Server/src/services/tools/manage_material.py (1)
Server/src/services/tools/utils.py (3)
parse_json_payload(27-60)coerce_int(63-77)normalize_properties(80-112)
MCPForUnity/Editor/Tools/ManageAsset.cs (3)
MCPForUnity/Editor/Helpers/MaterialOps.cs (2)
MaterialOps(12-396)ApplyProperties(17-155)MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/PropertyConversion.cs (2)
PropertyConversion(13-100)TryConvertToType(59-69)
Server/src/services/tools/manage_components.py (1)
Server/src/services/tools/utils.py (2)
parse_json_payload(27-60)normalize_properties(80-112)
MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (4)
Material(133-140)ObjectResolver(12-199)Texture(145-152)GameObject(109-128)MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.cs (1)
MCPForUnity/Editor/Helpers/Pagination.cs (4)
PaginationRequest(11-66)PaginationRequest(28-65)PaginationResponse(73-147)PaginationResponse(117-146)
MCPForUnity/Editor/Tools/ManageComponents.cs (4)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)CoerceString(121-128)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
Type(288-291)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (4)
Type(103-108)Type(113-118)Type(123-128)UnityTypeResolver(18-215)MCPForUnity/Editor/Helpers/ComponentOps.cs (4)
Component(23-73)ComponentOps(14-302)RemoveComponent(82-122)SetProperty(132-223)
MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (2)
Type(1941-1955)TryResolve(1968-1971)MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
Type(893-896)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)
MCPForUnity/Editor/Helpers/ComponentOps.cs (5)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
UnityEngine(1931-1934)GameObject(1102-1125)Type(1941-1955)SetProperty(1524-1606)MCPForUnity/Editor/Helpers/PropertyConversion.cs (3)
UnityEngine(85-99)PropertyConversion(13-100)ConvertToType(21-53)MCPForUnity/Editor/Tools/ManageComponents.cs (3)
AddComponent(68-118)GameObject(265-289)SetProperty(161-259)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (3)
Type(103-108)Type(113-118)Type(123-128)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)NormalizePropertyName(170-199)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (3)
MCPForUnity/Editor/Tools/ManageGameObject.cs (5)
ComponentResolver(1962-2098)List(1130-1289)List(1976-1994)List(2000-2021)List(2029-2064)MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
List(228-262)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (1)
List(150-185)
🔇 Additional comments (58)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (2)
54-67: LGTM: Robust error handling for test utility.The try-catch wrapper with fallback to an error object prevents test crashes from serialization failures while preserving diagnostic information. This is appropriate defensive coding for a test helper method.
97-98: Verify the generous timing threshold won't mask regressions.Doubling the threshold to 10 seconds for creating just 10 objects (1 second per object) is extremely permissive, especially compared to the medium batch test at line 126, which allows 15 seconds for 50 objects (300ms per object). While CI variability is a valid concern, this inconsistency suggests the small batch threshold may be overly generous and could hide genuine performance degradation.
Consider monitoring CI run times to establish a data-driven threshold, or investigate why small-batch creation is proportionally slower than medium-batch operations.
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
948-977: LGTM! Solid fix for the netstat parsing issue.Running
netstat.exe -anodirectly and filtering in C# correctly avoids the exit code 1 issue whenfindstrfinds no matches. The stricter port validation usingEndsWith(portSuffix)on the local address column also prevents false positives (e.g., port 80 matching 8080).Server/src/services/tools/manage_material.py (2)
12-12: LGTM! Clean import addition for shared utility.The import of
normalize_propertiesfrom the shared utils module aligns with the PR's goal of consolidating duplicate normalization logic across tools.
98-100: LGTM! Proper usage of shared normalization with consistent error handling.The refactoring correctly:
- Unpacks the tuple return from
normalize_properties- Follows the same error-handling pattern as
_normalize_colorabove (lines 93-95)- Propagates error messages to the caller with appropriate response structure
The shared implementation from utils.py handles edge cases like
"[object Object]","undefined", and JSON parsing, providing robust validation.Server/src/services/tools/manage_components.py (1)
12-12: LGTM! Good consolidation of shared functionality.The import of
normalize_propertiesfrom the shared utils module aligns well with the PR's objective to extract common functionality and eliminate duplicate code across tools.Server/src/services/tools/manage_shader.py (1)
15-18: Past review feedback properly addressed.The removal of
destructiveHint=Truecorrectly resolves the concern that read-only actions shouldn't be globally marked as destructive. The updated description on line 14 clearly distinguishes read-only vs. modifying actions, aligning with the non-destructive pattern used elsewhere in the codebase.Server/tests/integration/test_manage_asset_json_parsing.py (5)
15-37: LGTM!The test correctly verifies that JSON string properties are parsed and passed successfully to the Unity command.
63-86: LGTM!The test correctly verifies that dictionary properties are passed through unchanged without attempting JSON parsing.
89-109: LGTM!The test appropriately verifies that
Noneproperties are handled correctly without triggering JSON parsing.
116-138: LGTM!Both test methods provide good coverage:
test_component_properties_json_string_parsingverifies successful operation with JSON string input.test_component_properties_parsing_verificationstrengthens validation by capturing and verifying that the parameters are actually parsed to a dictionary before being sent to Unity.The verification test is particularly valuable as it confirms the parsing behavior rather than just checking the success status.
Also applies to: 141-164
60-60: Error messages are user-friendly and provide sufficient context.The actual error messages returned by
normalize_properties()include detailed information about the expected format and what was received:
- "properties must be a JSON object (dict), got string that parsed to {type}"
- "properties must be a dict or JSON string, got {type}"
These messages clearly convey both the expected format and the actual type received, providing users with the necessary context to correct the issue.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (1)
147-147: LGTM!The assertion update aligns with the PR-wide standardization from string-based
statusto booleansuccessfields. Usingresult.ToString()as the failure message provides helpful debugging context.MCPForUnity/Editor/Helpers/ObjectResolver.cs.meta (1)
1-11: LGTM!Standard Unity meta file with valid GUID and default MonoImporter settings.
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
17-26: LGTM!Clean extraction of path separator normalization into a reusable utility. The null/empty guard is correct and the method correctly replaces backslashes with forward slashes for cross-platform compatibility.
38-38: Good refactor.Delegating to
NormalizeSeparatorsreduces duplication and ensures consistent normalization behavior across the codebase.MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (2)
43-47: Cache may return type that doesn't satisfy constraint.The cache lookup at lines 44-47 correctly checks
PassesConstraint, but if a type was previously cached from a call without constraints, a subsequent call withrequiredBaseTypewill still return it from cache (and correctly validate). However, if a type is cached and later queried with a different constraint that it doesn't satisfy, the code will fall through to search again, which is correct behavior.No action needed - just confirming the logic handles constraint mismatches properly by continuing the search.
187-192: LGTM on exception handling.
SafeGetTypescorrectly handlesReflectionTypeLoadExceptionby extracting non-null types fromrtle.Types, ensuring partial assembly loading doesn't break the entire resolution process.MCPForUnity/Editor/Helpers/Pagination.cs.meta (1)
1-11: LGTM!Standard Unity meta file with valid GUID and default MonoImporter settings.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs (6)
83-84: LGTM!Assertions correctly updated to use boolean
successfield anderrorfield for failure messages, consistent with the new standardized response format.
94-95: LGTM!Consistent assertion pattern for error validation.
111-112: LGTM!Consistent assertion pattern for slot bounds error validation.
140-140: LGTM!Success assertion with
ToString()fallback provides good debugging context on test failure.
171-171: LGTM!Consistent success assertion pattern.
186-186: LGTM!Consistent success assertion pattern completing the integration test.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.cs (3)
95-105: LGTM!Good test coverage for basic pagination slicing with clear assertions on item count and content.
182-204: LGTM on edge case coverage!Tests for clamped cursor (line 183-192) and negative cursor (line 194-204) ensure robust handling of invalid input ranges.
1-208: Good overall test coverage.The test suite comprehensively covers:
- Parameter parsing with both naming conventions
- Cursor vs page_number precedence
- Default values and custom defaults
- String value coercion
- Page slicing at different positions
- Edge cases (empty list, out-of-range cursor, negative cursor)
- HasMore and NextCursor logic
Well-structured with clear region separation.
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
163-189: LGTM!The
Vector4Converterimplementation is correct and follows the established pattern of the existing converters (Vector3Converter,QuaternionConverter). This addresses the previously identified gap whereVector4was used inMaterialOps.csandManageScriptableObject.cswithout a dedicated converter.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs (1)
57-57: LGTM!The test assertions are correctly updated to align with the new standardized response format that uses a boolean
successfield anderrorfor error messages. The changes are consistent across all test methods.Also applies to: 78-78, 96-96, 115-116, 143-145
MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
40-63: LGTM!Clean integration with the standardized pagination utilities. The page size clamping (1-500) is appropriate, and the response structure includes all necessary pagination metadata (
cursor,nextCursor,totalCount,hasMore).MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
285-291: LGTM!Clean consolidation that delegates to
UnityTypeResolver.ResolveComponent()for unified type resolution. The method signature and null-return contract are preserved, and the<remarks>documentation appropriately documents the delegation.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
550-592: LGTM — improved retry policy for slower startup environments.The increased attempt count (30) and tiered delay strategy (500ms initially, then 3s) appropriately handles Windows/dev mode scenarios where package resolution and antivirus scans can delay server startup significantly.
The "last resort" connection attempt after 20 failed detections is a reasonable fallback for cases where process detection is unreliable but the server is actually running.
MCPForUnity/Editor/Tools/ManageAsset.cs (2)
992-1016: Verify: null conversion result prevents intentional null assignments.
TryConvertToTypereturnsnullboth when conversion fails AND when the source token isnull. The conditionconvertedValue != nullat lines 993 and 1008 prevents setting the property in both cases.If a user intentionally wants to set a property to
null(e.g., clearing a reference), this logic will skip the assignment and returnfalse(no modification).If this is intentional (disallow null assignments via this path), the current logic is correct. Otherwise, consider distinguishing between "conversion failed" and "value is legitimately null."
218-218: LGTM!Consistent migration to use
UnityJsonSerializer.Instancefor material property application, aligning with the centralized serialization approach introduced in this PR.Also applies to: 421-421
MCPForUnity/Editor/Helpers/PropertyConversion.cs (2)
85-99: LGTM: Asset loading is correctly implemented.The string type check, path sanitization, and null handling are all appropriate for the documented behavior of expecting a string path token.
33-52: Excellent exception handling with detailed logging.The progressive exception catching (JsonSerializationException → ArgumentException → Exception) with detailed error messages including token content provides strong observability while preserving error propagation.
MCPForUnity/Editor/Helpers/ObjectResolver.cs (2)
169-198: Well-designed asset loading with appropriate fallbacks.The progressive strategy (typed path → generic path → GUID search) with ambiguity warnings provides robust asset resolution while guiding users toward unambiguous paths.
31-104: Solid resolution logic with appropriate fallbacks and error handling.The asset-first strategy with scene lookup fallback correctly models Unity's object model. Component type resolution with fallback warnings provides good user guidance.
MCPForUnity/Editor/Tools/ManageComponents.cs (2)
68-118: Previous review concern addressed: Undo registration now correct.The implementation now properly records undo state before modifying component properties (line 101), addressing the previous review feedback. The flow correctly separates component creation (handled by ComponentOps.AddComponent with its own undo) from property modification (recorded here).
310-326: Clean delegation to ComponentOps with consistent error handling.The wrapper correctly delegates to ComponentOps.SetProperty and follows a clear null-on-success pattern while logging failures for observability.
MCPForUnity/Editor/Helpers/ComponentOps.cs (4)
23-73: Comprehensive validation with excellent 2D/3D physics conflict prevention.The validation sequence (null checks → type validation → Transform prevention → physics conflict detection) provides robust safety. The physics conflict check at line 47 is particularly valuable for preventing common Unity mistakes.
152-173: Previous review concern resolved: Conversion failures now properly detected.The conversion failure detection at lines 160-164 (and mirrored at lines 181-187, 204-210 for fields) correctly distinguishes between legitimate null assignments and conversion failures, addressing the feedback from the previous review. The error messages are clear and actionable.
266-292: Excellent preventive validation for physics component conflicts.The bidirectional conflict detection (2D ↔ 3D physics) prevents common Unity configuration errors. The descriptive error messages guide users toward the correct resolution.
225-262: Comprehensive member introspection with proper Unity serialization support.The inclusion of private [SerializeField] fields (lines 252-258) alongside public properties and fields correctly models Unity's serialization behavior. Sorting the results improves usability for AI agents and developers.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (2)
125-125: API rename consistently applied across test suite.All references to
GetAIPropertySuggestionshave been updated toGetFuzzyPropertySuggestions, maintaining test coverage while reflecting the improved naming that better describes the fuzzy-matching behavior.Also applies to: 156-156, 166-166, 172-172, 184-184, 189-189
559-633: Strong test coverage for prefab asset handling guidance.The three new tests comprehensively verify the prefab handling behavior:
- Modify/delete operations correctly return guidance errors
- Create (instantiate) operations are properly allowed
- Error messages guide users to appropriate tools (manage_asset, manage_prefabs)
This test coverage directly supports the PR's objective to eliminate the prefab-redirect anti-pattern.
MCPForUnity/Editor/Tools/ManageGameObject.cs (5)
25-26: Good refactoring to shared serializer instance.This eliminates repeated JsonSerializer allocations while maintaining backwards compatibility. The internal property pattern is clean.
79-95: Excellent prefab handling improvement.The explicit guidance directing users to the appropriate tool (
manage_assetormanage_prefabs) prevents confusion and improves the user experience. The exception forcreate(instantiation) makes sense.
1530-1586: Property normalization improves usability.The addition of property name normalization (e.g., "Use Gravity" → "useGravity") significantly improves the user experience by handling common naming patterns. The approach of trying the original name first, then the normalized name, is sound and prevents breaking existing code that uses exact names.
Edge case to be aware of: If a component has both a property named "usegravity" and "useGravity", the normalization could lead to ambiguous behavior. This is unlikely in practice due to C# naming conventions, but worth documenting.
1959-1971: Component resolution consolidation looks good.Delegating to
UnityTypeResolver.TryResolvewith aComponentconstraint centralizes type resolution logic effectively. The backwards compatibility wrapper is appropriate.
1997-2021: API rename toGetFuzzyPropertySuggestionsis more accurate.The rename from
GetAIPropertySuggestionsbetter reflects the actual implementation (rule-based fuzzy matching using Levenshtein distance and heuristics). This improves code clarity and sets appropriate expectations.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (1)
50-166: Test updates are consistent and comprehensive.All test method names and call sites have been consistently updated from
GetAIPropertySuggestionstoGetFuzzyPropertySuggestions. The test coverage remains strong, including:
- Null/empty input handling
- Exact match detection
- Multi-word matching
- Typo tolerance (Levenshtein distance)
- Result limiting
- Caching behavior
- Unity naming conventions (camelCase, prefixes)
- Case-insensitive matching
No issues detected.
MCPForUnity/Editor/Tools/ManageMaterial.cs (5)
19-53: Response type standardization improves API consistency.The conversion from anonymous return objects to explicit
ErrorResponse/SuccessResponsetypes makes the API more predictable and easier to consume. This aligns with best practices for tool development.
86-356: Centralized object resolution viaObjectResolveris effective.The consistent use of
ObjectResolver.Resolve()throughout the file (lines 86, 104, 140, 211, 224, 271, 356) centralizes lookup logic and makes the code more maintainable. The pattern is applied consistently across materials, textures, and GameObjects.
115-573: Shared serializer instance improves performance.The adoption of
UnityJsonSerializer.Instanceacross MaterialOps calls (lines 115, 150, 261, 536, 573) eliminates redundant serializer allocations and ensures consistent Unity type conversion behavior.
57-71:NormalizePathhelper improves path handling consistency.The centralized path normalization ensures consistent handling of material paths throughout the file. The combination of
SanitizeAssetPathand extension validation is appropriate.
458-588:CreateMaterialimplementation is robust.The material creation logic demonstrates good practices:
- Comprehensive validation (path, shader, existing asset)
- Proper error handling with typed responses
- Resource cleanup via
shouldDestroyMaterialflag in finally block- Support for both color parameters and general properties
No issues detected.
| /// <summary> | ||
| /// Tries to convert a JToken to the specified target type. | ||
| /// Returns null and logs warning on failure (does not throw). | ||
| /// </summary> | ||
| public static object TryConvertToType(JToken token, Type targetType) | ||
| { | ||
| try | ||
| { | ||
| return ConvertToType(token, targetType); | ||
| } | ||
| catch | ||
| { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent exception swallowing contradicts documentation.
The documentation states "logs warning on failure," but the method silently catches and ignores all exceptions without logging. This makes debugging conversion failures difficult.
🔎 Proposed fix to add logging
public static object TryConvertToType(JToken token, Type targetType)
{
try
{
return ConvertToType(token, targetType);
}
- catch
+ catch (Exception ex)
{
+ Debug.LogWarning($"[PropertyConversion] Failed to convert token to {targetType?.Name}: {ex.Message}");
return null;
}
}🤖 Prompt for AI Agents
In @MCPForUnity/Editor/Helpers/PropertyConversion.cs around lines 55 - 69, The
TryConvertToType method currently swallows all exceptions silently; update its
catch to log a warning including the caught exception and context before
returning null so it matches the XML doc. Specifically, in TryConvertToType
(which calls ConvertToType), catch the exception as e and call your logging
facility (e.g., Debug.LogWarning/Logger.Warn/processLogger) with a message that
includes the targetType, token (or token.Type/token.ToString()), and
e.ToString(), then return null.
- ManageGameObject.cs: Check tag existence before setting; auto-create undefined tags using InternalEditorUtility.AddTag() instead of relying on exception handling (Unity logs warning, doesn't throw) - manage_gameobject.py: Remove deprecated actions (find, get_components, add_component, remove_component, set_component_property, get_component) from Literal type - these are now handled by find_gameobjects and manage_components tools - Update test suite and unit tests to reflect new auto-create behavior
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.claude/prompts/nl-gameobject-suite.md (1)
14-14: Update mission statement to reflect error-based testing instead of deprecation warnings.Line 14 states "Verify deprecation warnings appear for legacy actions," but the GO-10 test now verifies that removed actions return error responses rather than deprecation warnings. Update this mission item to align with the actual test behavior.
🔎 Suggested fix
- 3) Verify deprecation warnings appear for legacy actions + 3) Verify removed legacy actions return clear error responses directing to new toolsServer/src/services/tools/manage_gameobject.py (2)
175-179: Error message lists invalid actions not in the Literal type.The error message includes actions (
find,add_component,remove_component,set_component_property,get_components,get_component) that are no longer valid per theLiteraltype on line 94. This will confuse users.🔎 Proposed fix
if action is None: return { "success": False, - "message": "Missing required parameter 'action'. Valid actions: create, modify, delete, find, add_component, remove_component, set_component_property, get_components, get_component, duplicate, move_relative" + "message": "Missing required parameter 'action'. Valid actions: create, modify, delete, duplicate, move_relative. For finding GameObjects use find_gameobjects tool. For component operations use manage_components tool." }
209-223: Dead code: validation for removed "find" action.This validation block handles the
findaction, butfindwas removed from the valid actionLiteralon line 94. This code path can never be reached.🔎 Proposed fix - remove dead code
- # Map tag to search_term when search_method is by_tag for backward compatibility - if action == "find" and search_method == "by_tag" and tag is not None and search_term is None: - search_term = tag - - # Validate parameter usage to prevent silent failures - if action == "find": - if name is not None: - return { - "success": False, - "message": "For 'find' action, use 'search_term' parameter, not 'name'. Remove 'name' parameter. Example: search_term='Player', search_method='by_name'" - } - if search_term is None: - return { - "success": False, - "message": "For 'find' action, 'search_term' parameter is required. Use search_term (not 'name') to specify what to find." - }
🧹 Nitpick comments (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
395-410: Consider test isolation for auto-created tags.The test creates a tag
AutoModifyTag12345that will persist in the Unity TagManager after the test completes. While this doesn't affect test correctness, it may accumulate tags over repeated test runs in the Editor.Consider either:
- Cleaning up the tag in TearDown (if Unity API allows removal)
- Using a timestamp-based tag name to avoid collisions across runs
- Documenting this as acceptable test pollution
Server/src/services/tools/manage_gameobject.py (1)
131-151: Consider removing unused parameters for removed actions.Several parameters are defined for actions that were removed from this tool:
search_term,find_all,search_in_children,search_inactive(for removedfindaction)component_name(for removedadd_component/remove_componentactions)page_size,cursor,max_components,include_properties(for removedget_componentsaction)While these don't break functionality, they clutter the API surface and may confuse users.
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
338-363: Extract duplicated tag auto-creation logic.The tag auto-creation logic is duplicated between
CreateGameObject(lines 338-363) andModifyGameObject(lines 594-617). Both blocks:
- Check if tag exists in
InternalEditorUtility.tags- Call
InternalEditorUtility.AddTag(tagToSet)- Handle exceptions with cleanup/error response
- Set the tag on the GameObject
Consider extracting to a helper method like
EnsureTagExistsAndApply(GameObject go, string tag).🔎 Proposed helper method
/// <summary> /// Ensures a tag exists (creating it if necessary) and applies it to the GameObject. /// Returns null on success, or an ErrorResponse on failure. /// </summary> private static object EnsureTagExistsAndApply(GameObject go, string tag, bool cleanupOnFailure = false) { string tagToSet = string.IsNullOrEmpty(tag) ? "Untagged" : tag; if (tagToSet != "Untagged" && !InternalEditorUtility.tags.Contains(tagToSet)) { Debug.Log($"[ManageGameObject] Tag '{tagToSet}' not found. Creating it."); try { InternalEditorUtility.AddTag(tagToSet); } catch (Exception ex) { if (cleanupOnFailure && go != null) UnityEngine.Object.DestroyImmediate(go); return new ErrorResponse($"Failed to create tag '{tagToSet}': {ex.Message}."); } } try { go.tag = tagToSet; return null; // Success } catch (Exception ex) { if (cleanupOnFailure && go != null) UnityEngine.Object.DestroyImmediate(go); return new ErrorResponse($"Failed to set tag to '{tagToSet}': {ex.Message}."); } }Also applies to: 594-617
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.claude/prompts/nl-gameobject-suite.mdMCPForUnity/Editor/Tools/ManageGameObject.csServer/src/services/tools/manage_gameobject.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs
🧰 Additional context used
🧬 Code graph analysis (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
HandleCommand(30-122)
MCPForUnity/Editor/Tools/ManageGameObject.cs (7)
MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (6)
Type(103-108)Type(113-118)Type(123-128)TryResolve(32-98)UnityTypeResolver(18-215)List(150-185)MCPForUnity/Editor/Helpers/ObjectResolver.cs (4)
UnityEngine(31-104)UnityEngine(169-198)GameObject(109-128)ObjectResolver(12-199)MCPForUnity/Editor/Helpers/PropertyConversion.cs (3)
UnityEngine(85-99)PropertyConversion(13-100)ConvertToType(21-53)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)NormalizePropertyName(170-199)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
JObject(98-149)MCPForUnity/Editor/Services/EditorStateCache.cs (2)
JObject(91-217)JObject(219-230)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
HandleCommand(30-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: nl-suite
🔇 Additional comments (10)
.claude/prompts/nl-gameobject-suite.md (1)
118-123: GO-10 test spec correctly documents the error-directed flow.The test clearly specifies that removed actions (e.g.,
find,get_components) should return errors indicating the action is unknown/removed, with guidance to use the new tools. This aligns well with the PR's goal of consolidating tools and removing deprecated actions rather than merely warning about them. The note "(legacy actions were removed, not deprecated)" effectively communicates the breaking change.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
404-422: LGTM - Test correctly validates auto-create tag behavior.The test properly verifies that:
- The create action succeeds with an undefined tag
- A GameObject is created
- The tag is auto-created and assigned correctly
Same note as the modify test regarding tag persistence in the TagManager.
MCPForUnity/Editor/Tools/ManageGameObject.cs (8)
25-26: LGTM - Good consolidation of JSON serializer.Using the shared
UnityJsonSerializer.Instanceeliminates per-call allocation overhead and ensures consistent Unity type conversion across the codebase.
79-95: LGTM - Clear prefab guidance for users.The error message properly guides users to the appropriate tools (
manage_assetormanage_prefabs) when they attempt to modify prefab assets directly.
1196-1211: LGTM - Unity 2021 compatibility for component search.The refactored code correctly handles
FindObjectsOfTypebehavior:
- Properly casts to
Componentbefore accessinggameObject- Manually filters inactive objects when
searchInactiveis false- Adds null check for safety
1499-1501: LGTM - Property name normalization improves usability.The normalization logic tries both original and normalized names (e.g., "Use Gravity" → "useGravity"), improving API ergonomics. This follows the pattern established in
ParamCoercion.NormalizePropertyName.Also applies to: 1515-1517, 1534-1536, 1553-1555
1787-1795: LGTM - Clean delegation to PropertyConversion.The
ConvertJTokenToTypemethod now properly delegates toPropertyConversion.ConvertToType, consolidating type conversion logic. TheinputSerializerparameter is retained for backwards compatibility as documented.
1896-1903: LGTM - Clean delegation to ObjectResolver.
FindObjectByInstructionnow delegates to the newObjectResolver.Resolve()helper, reducing code duplication while maintaining backwards compatibility for existing callers.
1927-1940: LGTM - ComponentResolver delegates to UnityTypeResolver.This properly consolidates type resolution logic while maintaining the existing API surface for backwards compatibility.
1966-1990: LGTM - Renamed to GetFuzzyPropertySuggestions.The rename from
GetAIPropertySuggestionstoGetFuzzyPropertySuggestionsbetter reflects the actual implementation (rule-based fuzzy matching, not AI). The caching and matching logic remain unchanged.
Bug fixes: - Fix searchInactive flag ignored in FindObjectsOfType (use includeInactive overload) - Fix property lookup to try both original and normalized names for backwards compat - Remove dead code for deprecated 'find' action validation - Update error message to list only valid actions Improvements: - Add destructiveHint=True to manage_shader tool - Limit fallback connection attempts (every 3rd attempt) to avoid spamming errors - Consolidate PropertyConversion exception handlers to single catch block - Add tag existence assertion and cleanup in tag auto-creation tests Test fixes: - Update SetComponentProperties_ContinuesAfterException log regex for new error format - Update test_manage_gameobject_param_coercion to test valid actions only
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
550-594: Retry logic improvement is reasonable, butcontinuecauses rapid polling.The extended retry (30 attempts) with progressive delays accommodates Windows/dev mode startup times. However, the
continueat line 579 skips the delay (lines 590-593) for non-matching last-resort attempts (21, 22, 24, 25, 27, 28), causing rapid iteration without pause.🔎 Proposed fix to ensure delay always runs
else if (attempt >= 20) { // After many attempts without detection, try connecting anyway as a last resort. // This handles cases where process detection fails but the server is actually running. // Only try once every 3 attempts to avoid spamming connection errors (at attempts 20, 23, 26, 29). - if ((attempt - 20) % 3 != 0) continue; + if ((attempt - 20) % 3 != 0) + { + // Not a connection attempt, but still wait before next iteration + if (attempt < maxAttempts - 1) + { + await Task.Delay(delay); + } + continue; + } bool started = await bridgeService.StartAsync();
🤖 Fix all issues with AI Agents
In @MCPForUnity/Editor/Helpers/PropertyConversion.cs:
- Around line 64-67: ConvertTo<T> currently casts the result of ConvertToType
directly which can throw if ConvertToType returns null for a non-nullable value
type; change ConvertTo<T> to store the result of ConvertToType(token, typeof(T))
in a local object, then if that result is null return default(T) for
non-nullable value types (check typeof(T).IsValueType and
Nullable.GetUnderlyingType(typeof(T)) == null) or return (T)result when
non-null; this avoids InvalidCastException/NullReferenceException while
preserving nullable/reference behavior.
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs:
- Around line 559-633: The tests fail because ManageGameObject.HandleCommand
expects a camelCase parameter key (prefabPath) but the tests send snake_case
(prefab_path); update HandleCommand to normalize incoming parameter names when
reading the prefab path (use ParamCoercion.NormalizePropertyName("prefab_path")
or apply ParamCoercion.NormalizePropertyName to keys before indexing) so it will
find the value, or alternatively change the tests to use ["prefabPath"]
consistently; reference ManageGameObject.HandleCommand and
ParamCoercion.NormalizePropertyName when making the change.
🧹 Nitpick comments (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
396-420: Approve test logic; consider improving cleanup reliability.The test correctly validates the new auto-create tag behavior. However, the empty catch block on line 419 has two issues:
- If assertions fail before reaching cleanup, the tag persists in the project.
- Silent catch hides cleanup failures that could affect subsequent test runs.
🔎 Suggested improvement using TearDown pattern
Track the created tag and clean it up in
TearDownto ensure cleanup runs even when assertions fail:public class ManageGameObjectModifyTests { private List<GameObject> testObjects = new List<GameObject>(); + private string _createdTestTag; [TearDown] public void TearDown() { foreach (var go in testObjects) { if (go != null) { Object.DestroyImmediate(go); } } testObjects.Clear(); + + // Clean up any test-created tags + if (!string.IsNullOrEmpty(_createdTestTag)) + { + try { InternalEditorUtility.RemoveTag(_createdTestTag); } + catch (System.Exception) { /* Tag may not exist */ } + _createdTestTag = null; + } }Then in the test:
public void Modify_NewTag_AutoCreatesTag() { const string testTag = "AutoModifyTag12345"; + _createdTestTag = testTag; // ... rest of test ... - // Clean up the created tag - try { UnityEditorInternal.InternalEditorUtility.RemoveTag(testTag); } catch { } }Server/src/services/tools/manage_gameobject.py (1)
132-150: Consider removing stale parameters.Several parameters are now obsolete given the narrowed action surface:
search_term,find_all,search_in_children,search_inactive(find action removed)component_name,page_size,cursor,max_components,include_properties(component actions removed)While keeping them may provide backwards compatibility during migration, they add noise to the API. Consider deprecating them with clear documentation or removing in a follow-up.
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
1783-1791: Dead parameter:inputSerializeris now ignored.The
inputSerializerparameter is documented as kept for backwards compatibility but ignored sincePropertyConversion.ConvertToTypeusesUnityJsonSerializer.Instanceinternally. Consider marking it with[Obsolete]or updating call sites to remove it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_shader.pyServer/tests/integration/test_manage_gameobject_param_coercion.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
🧰 Additional context used
🧬 Code graph analysis (5)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (3)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/ServerManagementService.cs (1)
IsLocalHttpServerRunning(538-598)MCPForUnity/Editor/Services/IServerManagementService.cs (1)
IsLocalHttpServerRunning(36-36)
Server/tests/integration/test_manage_gameobject_param_coercion.py (2)
Server/src/services/tools/manage_gameobject.py (1)
manage_gameobject(92-289)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
HandleCommand(30-122)MCPForUnity/Editor/Tools/ManageEditor.cs (1)
RemoveTag(193-219)
MCPForUnity/Editor/Helpers/PropertyConversion.cs (3)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (3)
UnityEngine(31-104)UnityEngine(169-198)T(20-23)MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-212)SanitizeAssetPath(31-45)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
HandleCommand(30-122)MCPForUnity/Editor/Tools/ManageEditor.cs (1)
RemoveTag(193-219)
🔇 Additional comments (20)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
4-4: LGTM!The import is correctly added to support
InternalEditorUtility.tagsandInternalEditorUtility.RemoveTag()used in the updated test.Server/src/services/tools/manage_shader.py (1)
17-17: Documentation improvement clarifies mixed destructive/non-destructive actions.The added comment helpfully documents that the
readaction is non-destructive whilecreate,update, anddeleteare destructive. This provides useful context for developers working with the tool, complementing the description on line 14.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
4-4: LGTM! Appropriate import for tag verification.The
UnityEditorInternalimport is necessary for accessingInternalEditorUtility.tagsandInternalEditorUtility.RemoveTagin the refactored test, and is consistent with tag management patterns elsewhere in the codebase.MCPForUnity/Editor/Helpers/PropertyConversion.cs (1)
75-89: Asset loading implementation looks correct.The method properly validates token type, sanitizes the path using
AssetPathUtility.SanitizeAssetPath, and logs a warning when the asset isn't found. Good defensive approach.Server/tests/integration/test_manage_gameobject_param_coercion.py (2)
7-34: Test correctly validates boolean coercion for modify action.The test properly verifies that string
"true"is coerced to booleanTruefor thesetActiveparameter when using themodifyaction. The monkeypatch approach cleanly captures the parameters sent to Unity.
37-65: Test correctly validates create action with tag parameter.The test verifies that the
createaction properly passesname,tag, andpositionparameters through to Unity. Good coverage of the parameter mapping behavior.Server/src/services/tools/manage_gameobject.py (2)
85-94: Good separation of concerns with narrowed action surface.The updated description clearly guides users to the appropriate tools (
find_gameobjects,manage_components) for operations removed from this tool. TheLiteraltype constraint onactionprovides compile-time documentation of valid actions.
175-179: Helpful error message for missing action.The error message clearly lists valid actions and directs users to the appropriate tools for other operations. Good UX improvement.
MCPForUnity/Editor/Tools/ManageGameObject.cs (6)
25-26: Good consolidation of JSON serializer usage.Using a shared serializer instance via
UnityJsonSerializer.Instanceeliminates per-call allocation and ensures consistent type handling across the codebase.
79-95: Prefab asset check provides clear guidance.The check correctly blocks modify/delete operations on prefab assets and provides actionable guidance to use
manage_assetormanage_prefabsinstead. Thecreateaction is appropriately allowed for prefab instantiation.
337-361: Tag creation logic handles edge cases well.The implementation:
- Checks if tag exists (excluding "Untagged")
- Creates the tag if missing
- Sets the tag with proper exception handling
- Cleans up the created GameObject on failure
Good defensive coding with appropriate error messages.
1194-1207: Fixed:FindObjectsOfTypenow respectsincludeInactive.The updated code correctly passes
searchInactivetoFindObjectsOfType(componentType, searchInactive), fixing the previous issue where inactive objects weren't found whensearchInactivewas true. This aligns with the commit message mentioning "respect includeInactive in FindObjectsOfType."
1892-1899: Good delegation to ObjectResolver.The
FindObjectByInstructionmethod now properly delegates toObjectResolver.Resolve, centralizing the resolution logic. The backwards-compatibility remarks are helpful documentation.
1958-1986: API renamed from GetAIPropertySuggestions to GetFuzzyPropertySuggestions.The rename better reflects the actual implementation (rule-based fuzzy matching using Levenshtein distance) rather than implying AI involvement. Good naming clarity.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (2)
125-127: Test updated for API rename.The test correctly uses
GetFuzzyPropertySuggestionsmatching the renamed API inComponentResolver.
318-321: Updated log expectations match new error flow.The test now expects the
PropertyConversionerror log format, which aligns with the centralized conversion handling in the refactored code.MCPForUnity/Editor/Helpers/ComponentOps.cs (4)
16-73: Well-structured AddComponent with proper validation.The method correctly:
- Validates inputs (null checks, type validation)
- Prevents duplicate Transform
- Checks for 2D/3D physics conflicts before adding
- Uses
Undo.AddComponentfor undo support- Applies sensible defaults for specific types
The physics conflict detection is centralized here as mentioned in the PR objectives.
132-227: Conversion failure detection properly implemented.The
SetPropertymethod now correctly detects conversion failures by checking ifconvertedValue == null && value.Type != JTokenType.Null. This provides clear error messages distinguishing between:
- Intentional null assignment
- Conversion failure
This addresses the previous review feedback about silent conversion failure handling.
229-266: GetAccessibleMembers provides comprehensive member enumeration.The method correctly enumerates:
- Public writable properties
- Public non-init fields
- Private
[SerializeField]fieldsThe sorted output ensures consistent ordering for UI/API consumers.
270-296: Physics conflict check is thorough.The check properly validates mutual exclusivity between 2D and 3D physics components, preventing common Unity errors. The error messages clearly explain the conflict.
| public static T ConvertTo<T>(JToken token) | ||
| { | ||
| return (T)ConvertToType(token, typeof(T)); | ||
| } |
There was a problem hiding this comment.
Potential InvalidCastException for value types.
ConvertTo<T> casts the result to T, but if T is a non-nullable value type and ConvertToType returns null (which happens for null tokens of reference types), this will throw an InvalidCastException or NullReferenceException.
🔎 Proposed fix
public static T ConvertTo<T>(JToken token)
{
- return (T)ConvertToType(token, typeof(T));
+ var result = ConvertToType(token, typeof(T));
+ if (result == null && default(T) != null)
+ {
+ // For non-nullable value types, return default
+ return default;
+ }
+ return (T)result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static T ConvertTo<T>(JToken token) | |
| { | |
| return (T)ConvertToType(token, typeof(T)); | |
| } | |
| public static T ConvertTo<T>(JToken token) | |
| { | |
| var result = ConvertToType(token, typeof(T)); | |
| if (result == null && default(T) != null) | |
| { | |
| // For non-nullable value types, return default | |
| return default; | |
| } | |
| return (T)result; | |
| } |
🤖 Prompt for AI Agents
In @MCPForUnity/Editor/Helpers/PropertyConversion.cs around lines 64 - 67,
ConvertTo<T> currently casts the result of ConvertToType directly which can
throw if ConvertToType returns null for a non-nullable value type; change
ConvertTo<T> to store the result of ConvertToType(token, typeof(T)) in a local
object, then if that result is null return default(T) for non-nullable value
types (check typeof(T).IsValueType and Nullable.GetUnderlyingType(typeof(T)) ==
null) or return (T)result when non-null; this avoids
InvalidCastException/NullReferenceException while preserving nullable/reference
behavior.
| public void Create_WithNewTag_AutoCreatesTag() | ||
| { | ||
| // Expect the error log from Unity about invalid tag | ||
| UnityEngine.TestTools.LogAssert.Expect(LogType.Error, | ||
| new System.Text.RegularExpressions.Regex("Tag:.*NonExistentTag12345.*not defined")); | ||
| const string testTag = "AutoCreatedTag12345"; | ||
|
|
||
| // Tags that don't exist are now auto-created | ||
| var p = new JObject | ||
| { | ||
| ["action"] = "create", | ||
| ["name"] = "TestInvalidTag", | ||
| ["tag"] = "NonExistentTag12345" | ||
| ["name"] = "TestAutoTag", | ||
| ["tag"] = testTag | ||
| }; | ||
|
|
||
| var result = ManageGameObject.HandleCommand(p); | ||
| // Current behavior: logs error but may create object anyway | ||
| Assert.IsNotNull(result, "Should return a result"); | ||
| var resultObj = result as JObject ?? JObject.FromObject(result); | ||
|
|
||
| Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); | ||
|
|
||
| var created = FindAndTrack("TestAutoTag"); | ||
| Assert.IsNotNull(created, "Object should be created"); | ||
| Assert.AreEqual(testTag, created.tag, "Tag should be auto-created and assigned"); | ||
|
|
||
| // Verify tag was actually added to the tag manager | ||
| Assert.That(UnityEditorInternal.InternalEditorUtility.tags, Does.Contain(testTag), | ||
| "Tag should exist in Unity's tag manager"); | ||
|
|
||
| // Clean up if object was created | ||
| FindAndTrack("TestInvalidTag"); | ||
| // Clean up the created tag | ||
| try { UnityEditorInternal.InternalEditorUtility.RemoveTag(testTag); } catch { } | ||
| } |
There was a problem hiding this comment.
Move tag cleanup to TearDown to prevent test pollution.
The test correctly validates tag auto-creation behavior and includes good verification of both the GameObject state and Unity's tag manager. However, the cleanup at line 431 only executes if all assertions pass. If any assertion fails (lines 420, 423, 424, or 427-428), the tag "AutoCreatedTag12345" will persist in Unity's tag manager and affect subsequent test runs.
🔎 Suggested fix: Add tag cleanup to TearDown
Add a list to track created tags and clean them up in TearDown:
public class ManageGameObjectCreateTests
{
private List<GameObject> createdObjects = new List<GameObject>();
+ private List<string> createdTags = new List<string>();
[TearDown]
public void TearDown()
{
foreach (var go in createdObjects)
{
if (go != null)
{
Object.DestroyImmediate(go);
}
}
createdObjects.Clear();
+
+ foreach (var tag in createdTags)
+ {
+ try
+ {
+ if (System.Linq.Enumerable.Contains(InternalEditorUtility.tags, tag))
+ {
+ InternalEditorUtility.RemoveTag(tag);
+ }
+ }
+ catch { }
+ }
+ createdTags.Clear();
}Then in the test method:
public void Create_WithNewTag_AutoCreatesTag()
{
const string testTag = "AutoCreatedTag12345";
+ createdTags.Add(testTag);
// Tags that don't exist are now auto-created
var p = new JObject
{
["action"] = "create",
["name"] = "TestAutoTag",
["tag"] = testTag
};
var result = ManageGameObject.HandleCommand(p);
var resultObj = result as JObject ?? JObject.FromObject(result);
Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString());
var created = FindAndTrack("TestAutoTag");
Assert.IsNotNull(created, "Object should be created");
Assert.AreEqual(testTag, created.tag, "Tag should be auto-created and assigned");
// Verify tag was actually added to the tag manager
Assert.That(UnityEditorInternal.InternalEditorUtility.tags, Does.Contain(testTag),
"Tag should exist in Unity's tag manager");
-
- // Clean up the created tag
- try { UnityEditorInternal.InternalEditorUtility.RemoveTag(testTag); } catch { }
}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| #region Prefab Asset Handling Tests | ||
|
|
||
| [Test] | ||
| public void HandleCommand_WithPrefabPath_ReturnsGuidanceError_ForModifyAction() | ||
| { | ||
| // Arrange - Attempt to modify a prefab asset directly | ||
| var modifyParams = new JObject | ||
| { | ||
| ["action"] = "modify", | ||
| ["target"] = "Assets/Prefabs/MyPrefab.prefab" | ||
| }; | ||
|
|
||
| // Act | ||
| var result = ManageGameObject.HandleCommand(modifyParams); | ||
|
|
||
| // Assert - Should return an error with guidance to use correct tools | ||
| Assert.IsNotNull(result, "Should return a result"); | ||
| var errorResponse = result as MCPForUnity.Editor.Helpers.ErrorResponse; | ||
| Assert.IsNotNull(errorResponse, "Should return an ErrorResponse"); | ||
| Assert.IsFalse(errorResponse.Success, "Should indicate failure"); | ||
| Assert.That(errorResponse.Error, Does.Contain("prefab asset"), "Error should mention prefab asset"); | ||
| Assert.That(errorResponse.Error, Does.Contain("manage_asset"), "Error should guide to manage_asset"); | ||
| Assert.That(errorResponse.Error, Does.Contain("manage_prefabs"), "Error should guide to manage_prefabs"); | ||
| } | ||
|
|
||
| [Test] | ||
| public void HandleCommand_WithPrefabPath_ReturnsGuidanceError_ForDeleteAction() | ||
| { | ||
| // Arrange - Attempt to delete a prefab asset directly | ||
| var deleteParams = new JObject | ||
| { | ||
| ["action"] = "delete", | ||
| ["target"] = "Assets/Prefabs/SomePrefab.prefab" | ||
| }; | ||
|
|
||
| // Act | ||
| var result = ManageGameObject.HandleCommand(deleteParams); | ||
|
|
||
| // Assert - Should return an error with guidance | ||
| Assert.IsNotNull(result, "Should return a result"); | ||
| var errorResponse = result as MCPForUnity.Editor.Helpers.ErrorResponse; | ||
| Assert.IsNotNull(errorResponse, "Should return an ErrorResponse"); | ||
| Assert.IsFalse(errorResponse.Success, "Should indicate failure"); | ||
| Assert.That(errorResponse.Error, Does.Contain("prefab asset"), "Error should mention prefab asset"); | ||
| } | ||
|
|
||
| [Test] | ||
| public void HandleCommand_WithPrefabPath_AllowsCreateAction() | ||
| { | ||
| // Arrange - Create (instantiate) from a prefab should be allowed | ||
| // Note: This will fail because the prefab doesn't exist, but the error should NOT be | ||
| // the prefab redirection error - it should be a "prefab not found" type error | ||
| var createParams = new JObject | ||
| { | ||
| ["action"] = "create", | ||
| ["prefab_path"] = "Assets/Prefabs/NonExistent.prefab", | ||
| ["name"] = "TestInstance" | ||
| }; | ||
|
|
||
| // Act | ||
| var result = ManageGameObject.HandleCommand(createParams); | ||
|
|
||
| // Assert - Should NOT return the prefab redirection error | ||
| // (It may fail for other reasons like prefab not found, but not due to redirection) | ||
| var errorResponse = result as MCPForUnity.Editor.Helpers.ErrorResponse; | ||
| if (errorResponse != null) | ||
| { | ||
| // If there's an error, it should NOT be the prefab asset guidance error | ||
| Assert.That(errorResponse.Error, Does.Not.Contain("Use 'manage_asset'"), | ||
| "Create action should not be blocked by prefab check"); | ||
| } | ||
| // If it's not an error, that's also fine (means create was allowed) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that prefab_path parameter is correctly handled in the create action
# Check if there's mapping between snake_case and camelCase in the codebase
rg -n "prefab_path|prefabPath" --type=cs -C2 | head -100Repository: dsarno/unity-mcp
Length of output: 9428
🏁 Script executed:
# First, let's find the ManageGameObject.cs implementation file
fd -t f "ManageGameObject.cs" --exclude="*Test*"Repository: dsarno/unity-mcp
Length of output: 154
🏁 Script executed:
# Also check the test file to see the full context of how prefab_path is used
fd -t f "ManageGameObjectTests.cs"Repository: dsarno/unity-mcp
Length of output: 224
🏁 Script executed:
# Read the test file around lines 559-633 to see the exact test code
sed -n '590,610p' TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csRepository: dsarno/unity-mcp
Length of output: 1044
🏁 Script executed:
# Search for any parameter mapping or conversion logic in the codebase
rg -n "prefab_path|snake_case|camelCase|ConvertFromSnakeCase|ToSnakeCase" --type=cs -i | head -30Repository: dsarno/unity-mcp
Length of output: 2711
🏁 Script executed:
# Check if there's a JObject parameter handling helper or extension method
rg -n "JObject|JsonConvert|JsonProperty" --type=cs -C2 | grep -A5 -B5 "snake\|camel\|param" | head -50Repository: dsarno/unity-mcp
Length of output: 4490
🏁 Script executed:
# Read the ParamCoercion.cs file to understand parameter name normalization
cat -n MCPForUnity/Editor/Helpers/ParamCoercion.cs | head -200Repository: dsarno/unity-mcp
Length of output: 8673
🏁 Script executed:
# Check if ManageGameObject uses ParamCoercion for parameter handling
rg -n "ParamCoercion|CoerceParams|NormalizeParam" MCPForUnity/Editor/Tools/ManageGameObject.cs -B2 -A2Repository: dsarno/unity-mcp
Length of output: 346
🏁 Script executed:
# Look at the HandleCommand method signature and early parameter extraction
sed -n '80,150p' MCPForUnity/Editor/Tools/ManageGameObject.csRepository: dsarno/unity-mcp
Length of output: 3305
Critical issue: Test will fail due to parameter key mismatch.
The implementation reads @params["prefabPath"] (camelCase) but the test provides ["prefab_path"] (snake_case). JObject indexing is case-sensitive and key-specific, so this will return null and the test won't behave as intended.
While ParamCoercion.NormalizePropertyName exists and handles snake_case to camelCase conversion for other components, it's not applied in ManageGameObject.HandleCommand for parameter extraction. Either:
- Apply
NormalizePropertyNamewhen extracting parameters:string prefabPath = @params[ParamCoercion.NormalizePropertyName("prefab_path")]?.ToString(); - Or update the test to use the camelCase key:
["prefabPath"]
The three tests are otherwise well-structured and verify the expected behaviors correctly.
🤖 Prompt for AI Agents
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
around lines 559 - 633, The tests fail because ManageGameObject.HandleCommand
expects a camelCase parameter key (prefabPath) but the tests send snake_case
(prefab_path); update HandleCommand to normalize incoming parameter names when
reading the prefab path (use ParamCoercion.NormalizePropertyName("prefab_path")
or apply ParamCoercion.NormalizePropertyName to keys before indexing) so it will
find the value, or alternatively change the tests to use ["prefabPath"]
consistently; reference ManageGameObject.HandleCommand and
ParamCoercion.NormalizePropertyName when making the change.
Summary
This PR extracts common functionality from
ManageGameObject,ManageAsset,ManageComponents, and other tools into reusable helper classes. The result is cleaner architecture, reduced code duplication, and a more maintainable codebase.Net change: 36 files, +1,676 / -815 lines (~860 lines of duplicate code eliminated)
New Helper Classes
UnityJsonSerializerJsonSerializerwith Unity type converters (Vector3, Color, etc.)ObjectResolverUnityTypeResolverPropertyConversionComponentOpsPaginationPaginationRequest/PaginationResponse<T>classesKey Changes
1. Removed Prefab Redirect Anti-Pattern
ManageGameObjectpreviously had 68 lines of code that redirected prefab operations toManageAsset. Now it returns helpful guidance instead:2. Standardized Response Patterns
ManageMaterialnow usesSuccessResponse/ErrorResponseconsistently (42 response sites updated).3. Unified Type Resolution
Three duplicate implementations consolidated:
ComponentResolver(ManageGameObject)FindComponentType(GameObjectLookup)ResolveType(ManageScriptableObject)Now all delegate to
UnityTypeResolverwith caching.4. Physics Conflict Detection in ComponentOps
2D/3D physics conflict checking moved to shared
ComponentOps.AddComponent():5. Standardized Pagination
FindGameObjectsnow usesPaginationRequest.FromParams()andPaginationResponse<T>.Create().Testing
Migration Guide
No breaking changes. All existing tool calls work identically.
For contributors working on new tools:
Files Changed
New files (6)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/ObjectResolver.csMCPForUnity/Editor/Helpers/Pagination.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Helpers/UnityJsonSerializer.csMCPForUnity/Editor/Helpers/UnityTypeResolver.csModified tools (6)
ManageGameObject.cs- Delegates to helpers, prefab redirect removedManageAsset.cs- Uses PropertyConversionManageComponents.cs- Uses ComponentOps, UnityTypeResolverManageMaterial.cs- Uses SuccessResponse/ErrorResponse, UnityJsonSerializerManageScriptableObject.cs- Uses UnityTypeResolverFindGameObjects.cs- Uses Pagination helpersModified helpers (2)
GameObjectLookup.cs- FindComponentType delegates to UnityTypeResolverAssetPathUtility.cs- Added NormalizeSeparators()Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.