Skip to content
Merged

Patch #1033

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions MCPForUnity/Editor/Tools/ManageComponents.cs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,26 @@ private static object RemoveComponent(JObject @params, JToken targetToken, strin
return new ErrorResponse($"Component type '{componentTypeName}' not found.");
}

// Use ComponentOps for the actual operation
int? componentIndex = ParamCoercion.CoerceIntNullable(@params["componentIndex"] ?? @params["component_index"]);
if (componentIndex.HasValue)
Comment on lines +149 to +150

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Reject malformed component_index instead of silently falling back.

If componentIndex/component_index is present but non-numeric, CoerceIntNullable returns null, and the code currently falls back to first-match behavior. That can remove or modify the wrong component.

🛠️ Proposed fix
-            int? componentIndex = ParamCoercion.CoerceIntNullable(`@params`["componentIndex"] ?? `@params`["component_index"]);
+            JToken componentIndexToken = `@params`["componentIndex"] ?? `@params`["component_index"];
+            int? componentIndex = ParamCoercion.CoerceIntNullable(componentIndexToken);
+            if (componentIndexToken != null && !componentIndex.HasValue)
+            {
+                return new ErrorResponse("'component_index' must be an integer (zero-based).");
+            }
             if (componentIndex.HasValue)
             {
                 var components = targetGo.GetComponents(type);
-            int? componentIndex = ParamCoercion.CoerceIntNullable(`@params`["componentIndex"] ?? `@params`["component_index"]);
+            JToken componentIndexToken = `@params`["componentIndex"] ?? `@params`["component_index"];
+            int? componentIndex = ParamCoercion.CoerceIntNullable(componentIndexToken);
+            if (componentIndexToken != null && !componentIndex.HasValue)
+            {
+                return new ErrorResponse("'component_index' must be an integer (zero-based).");
+            }
             Component component;

Also applies to: 168-169, 210-212, 220-222

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@MCPForUnity/Editor/Tools/ManageComponents.cs` around lines 149 - 150, The
code currently uses ParamCoercion.CoerceIntNullable(`@params`["componentIndex"] ??
`@params`["component_index"]) and silently treats a non-numeric value as null,
falling back to first-match behavior; change the logic in the blocks referencing
componentIndex (e.g., the componentIndex assignment and the subsequent if
(componentIndex.HasValue) checks at the shown locations and the other
occurrences at lines ~168-169, ~210-212, ~220-222) to first detect whether the
raw parameter key ("componentIndex" or "component_index") exists and, if it
exists but CoerceIntNullable returned null, reject the request (throw an
ArgumentException/return an error) rather than proceeding; only allow fallback
behavior when neither key is present, not when a malformed value was supplied.

{
var components = targetGo.GetComponents(type);
if (componentIndex.Value < 0 || componentIndex.Value >= components.Length)
return new ErrorResponse($"component_index {componentIndex.Value} out of range. Found {components.Length} '{componentTypeName}' component(s).");
if (type == typeof(Transform) || type == typeof(RectTransform))
return new ErrorResponse("Cannot remove Transform or RectTransform components.");
Undo.DestroyObjectImmediate(components[componentIndex.Value]);
Comment on lines +149 to +157

Copilot AI Apr 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When componentIndex is provided, this removal path bypasses ComponentOps.RemoveComponent, so it also bypasses its safety checks (notably the guard that prevents removing Transform). As written, componentType=Transform with componentIndex=0 would attempt Undo.DestroyObjectImmediate on the Transform, which can break the GameObject / editor state.

Add the same validation as ComponentOps.RemoveComponent (at minimum: reject type == typeof(Transform) before destroying), or refactor the index-based removal to reuse a shared safe removal helper that enforces these invariants for all code paths.

Copilot uses AI. Check for mistakes.
EditorUtility.SetDirty(targetGo);
MarkOwningSceneDirty(targetGo);
return new
{
success = true,
message = $"Component '{componentTypeName}' (index {componentIndex.Value}) removed from '{targetGo.name}'.",
data = new { instanceID = targetGo.GetInstanceID(), componentIndex = componentIndex.Value }
};
}

// Use ComponentOps for the actual operation (removes first instance)
bool removed = ComponentOps.RemoveComponent(targetGo, type, out string error);
if (!removed)
{
Expand Down Expand Up @@ -188,7 +207,19 @@ private static object SetProperty(JObject @params, JToken targetToken, string se
return new ErrorResponse($"Component type '{componentType}' not found.");
}

Component component = targetGo.GetComponent(type);
int? componentIndex = ParamCoercion.CoerceIntNullable(@params["componentIndex"] ?? @params["component_index"]);
Component component;
if (componentIndex.HasValue)
{
var components = targetGo.GetComponents(type);
if (componentIndex.Value < 0 || componentIndex.Value >= components.Length)
return new ErrorResponse($"component_index {componentIndex.Value} out of range. Found {components.Length} '{componentType}' component(s).");
component = components[componentIndex.Value];
}
else
{
component = targetGo.GetComponent(type);
}
if (component == null)
{
return new ErrorResponse($"Component '{componentType}' not found on '{targetGo.name}'.");
Expand Down
4 changes: 4 additions & 0 deletions MCPForUnity/Editor/Tools/ManageScript.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1910,6 +1910,10 @@ private static bool TryComputeMethodSpan(
int probe = lineStart - 1;
while (probe > searchStart)
{
// Skip past line-ending chars so LastIndexOf finds the *previous* newline
while (probe > searchStart && (source[probe] == '\n' || source[probe] == '\r'))
probe--;
if (probe <= searchStart) break;
int prevNl = source.LastIndexOf('\n', probe);
if (prevNl < 0 || prevNl < searchStart) break;
string prev = source.Substring(prevNl + 1, attrStart - (prevNl + 1));
Expand Down
39 changes: 36 additions & 3 deletions MCPForUnity/Editor/Tools/Physics/JointOps.cs
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,17 @@ public static object ConfigureJoint(JObject @params)
return new ErrorResponse($"Target GameObject '{targetStr}' not found.");

string jointTypeStr = p.Get("joint_type");
Component joint = ResolveJoint(go, jointTypeStr);
int? componentIndex = ParamCoercion.CoerceIntNullable(@params["componentIndex"] ?? @params["component_index"]);

if (componentIndex.HasValue && string.IsNullOrEmpty(jointTypeStr))
return new ErrorResponse("component_index requires joint_type to be specified.");

Component joint = ResolveJoint(go, jointTypeStr, componentIndex, out int foundCount);
if (joint == null)
{
if (componentIndex.HasValue && foundCount >= 0)
return new ErrorResponse($"component_index {componentIndex.Value} out of range. Found {foundCount} joint(s) on '{go.name}'.");

if (!string.IsNullOrEmpty(jointTypeStr))
return new ErrorResponse($"No joint of type '{jointTypeStr}' found on '{go.name}'.");

Comment thread
sourcery-ai[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -324,6 +332,10 @@ public static object RemoveJoint(JObject @params)
return new ErrorResponse($"Target GameObject '{targetStr}' not found.");

string jointTypeStr = p.Get("joint_type");
int? componentIndex = ParamCoercion.CoerceIntNullable(@params["componentIndex"] ?? @params["component_index"]);

if (componentIndex.HasValue && string.IsNullOrEmpty(jointTypeStr))
return new ErrorResponse("component_index requires joint_type to be specified.");

var jointsToRemove = new List<Component>();

Expand All @@ -342,7 +354,17 @@ public static object RemoveJoint(JObject @params)
}

var components = go.GetComponents(jointComponentType);
jointsToRemove.AddRange(components);

if (componentIndex.HasValue)
{
if (componentIndex.Value < 0 || componentIndex.Value >= components.Length)
return new ErrorResponse($"component_index {componentIndex.Value} out of range. Found {components.Length} '{jointComponentType.Name}' joint(s) on '{go.name}'.");
jointsToRemove.Add(components[componentIndex.Value]);
}
else
{
jointsToRemove.AddRange(components);
}
}
else
{
Expand Down Expand Up @@ -403,16 +425,27 @@ private static GameObject FindTarget(JToken targetToken, string searchMethod)
return GameObjectLookup.FindByTarget(targetToken, searchMethod ?? "by_name", true);
}

private static Component ResolveJoint(GameObject go, string jointTypeStr)
private static Component ResolveJoint(GameObject go, string jointTypeStr, int? index, out int foundCount)
{
foundCount = -1;
if (!string.IsNullOrEmpty(jointTypeStr))
Comment on lines +428 to 431

Copilot AI Apr 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResolveJoint now accepts index, but it’s only applied when joint_type is provided; when joint_type is omitted the index parameter is ignored (and foundCount remains -1). This makes componentIndex effectively a no-op for the auto-detect path and prevents consistent out-of-range reporting.

Consider rejecting componentIndex unless joint_type is set, or applying the index to the auto-detected joint list and setting foundCount accordingly.

Copilot uses AI. Check for mistakes.
{
bool is2D = go.GetComponent<Rigidbody2D>() != null;
var typeMap = is2D ? JointTypes2D : JointTypes3D;
string key = jointTypeStr.ToLowerInvariant();

if (typeMap.TryGetValue(key, out Type jointType))
{
if (index.HasValue)
{
var components = go.GetComponents(jointType);
foundCount = components.Length;
if (index.Value < 0 || index.Value >= components.Length)
return null;
return components[index.Value];
}
return go.GetComponent(jointType);
}

return null;
}
Expand Down
69 changes: 65 additions & 4 deletions MCPForUnity/Editor/Tools/Physics/PhysicsMaterialOps.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public static object Assign(JObject @params)

string searchMethod = p.Get("search_method") ?? "by_name";
string colliderType = p.Get("collider_type");
int? componentIndex = ParamCoercion.CoerceIntNullable(p.GetRaw("componentIndex") ?? p.GetRaw("component_index"));

var go = GameObjectLookup.FindByTarget(targetToken, searchMethod);
if (go == null)
Expand All @@ -98,7 +99,7 @@ public static object Assign(JObject @params)
// Try 3D colliders first
if (mat3D != null)
{
var collider3D = FindCollider3D(go, colliderType);
var collider3D = FindCollider3D(go, colliderType, componentIndex);
if (collider3D != null)
{
Undo.RecordObject(collider3D, "Assign Physics Material");
Expand All @@ -116,12 +117,25 @@ public static object Assign(JObject @params)
}
};
}
if (componentIndex.HasValue)
{
var type3D = !string.IsNullOrEmpty(colliderType) ? UnityTypeResolver.ResolveComponent(colliderType) : typeof(Collider);
if (type3D != null && typeof(Collider).IsAssignableFrom(type3D))
{
int count3D = go.GetComponents(type3D).Length;
return new ErrorResponse($"component_index {componentIndex.Value} out of range. Found {count3D} '{type3D.Name}' collider(s) on '{go.name}'.");
}
else if (!string.IsNullOrEmpty(colliderType))
{
return new ErrorResponse($"Unknown or invalid 3D collider type: '{colliderType}'.");
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

// Try 2D colliders
if (mat2D != null)
{
var collider2D = FindCollider2D(go, colliderType);
var collider2D = FindCollider2D(go, colliderType, componentIndex);
if (collider2D != null)
{
Undo.RecordObject(collider2D, "Assign Physics Material 2D");
Expand All @@ -139,6 +153,19 @@ public static object Assign(JObject @params)
}
};
}
if (componentIndex.HasValue)
{
var type2D = !string.IsNullOrEmpty(colliderType) ? UnityTypeResolver.ResolveComponent(colliderType) : typeof(Collider2D);
if (type2D != null && typeof(Collider2D).IsAssignableFrom(type2D))
{
int count2D = go.GetComponents(type2D).Length;
return new ErrorResponse($"component_index {componentIndex.Value} out of range. Found {count2D} '{type2D.Name}' collider(s) on '{go.name}'.");
}
else if (!string.IsNullOrEmpty(colliderType))
{
return new ErrorResponse($"Unknown or invalid 2D collider type: '{colliderType}'.");
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

return new ErrorResponse($"No suitable collider found on '{go.name}'.");
Expand Down Expand Up @@ -398,29 +425,63 @@ private static object Configure2D(string path, JObject properties)
// Assign helpers
// =====================================================================

private static Collider FindCollider3D(GameObject go, string colliderType)
private static Collider FindCollider3D(GameObject go, string colliderType, int? index = null)
{
if (!string.IsNullOrEmpty(colliderType))
{
var type = UnityTypeResolver.ResolveComponent(colliderType);
if (type != null && typeof(Collider).IsAssignableFrom(type))
{
if (index.HasValue)
{
var components = go.GetComponents(type);
if (index.Value < 0 || index.Value >= components.Length)
return null;
return components[index.Value] as Collider;
}
return go.GetComponent(type) as Collider;
}
return null;
}

if (index.HasValue)
{
var colliders = go.GetComponents<Collider>();
if (index.Value < 0 || index.Value >= colliders.Length)
return null;
return colliders[index.Value];
}

return go.GetComponent<Collider>();
}

private static Collider2D FindCollider2D(GameObject go, string colliderType)
private static Collider2D FindCollider2D(GameObject go, string colliderType, int? index = null)
{
if (!string.IsNullOrEmpty(colliderType))
{
var type = UnityTypeResolver.ResolveComponent(colliderType);
if (type != null && typeof(Collider2D).IsAssignableFrom(type))
{
if (index.HasValue)
{
var components = go.GetComponents(type);
if (index.Value < 0 || index.Value >= components.Length)
return null;
return components[index.Value] as Collider2D;
}
return go.GetComponent(type) as Collider2D;
}
return null;
}

if (index.HasValue)
{
var colliders = go.GetComponents<Collider2D>();
if (index.Value < 0 || index.Value >= colliders.Length)
return null;
return colliders[index.Value];
}

return go.GetComponent<Collider2D>();
}

Expand Down
8 changes: 4 additions & 4 deletions MCPForUnity/Editor/Tools/Vfx/LineCreate.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ internal static class LineCreate
public static object CreateLine(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

Vector3 start = ManageVfxCommon.ParseVector3(@params["start"]);
Vector3 end = ManageVfxCommon.ParseVector3(@params["end"]);
Expand Down Expand Up @@ -50,7 +50,7 @@ public static object CreateLine(JObject @params)
public static object CreateCircle(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

Vector3 center = ManageVfxCommon.ParseVector3(@params["center"]);
float radius = @params["radius"]?.ToObject<float>() ?? 1f;
Expand Down Expand Up @@ -102,7 +102,7 @@ public static object CreateCircle(JObject @params)
public static object CreateArc(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

Vector3 center = ManageVfxCommon.ParseVector3(@params["center"]);
float radius = @params["radius"]?.ToObject<float>() ?? 1f;
Expand Down Expand Up @@ -157,7 +157,7 @@ public static object CreateArc(JObject @params)
public static object CreateBezier(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

Vector3 start = ManageVfxCommon.ParseVector3(@params["start"]);
Vector3 end = ManageVfxCommon.ParseVector3(@params["end"]);
Expand Down
10 changes: 5 additions & 5 deletions MCPForUnity/Editor/Tools/Vfx/LineRead.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ namespace MCPForUnity.Editor.Tools.Vfx
internal static class LineRead
{
public static LineRenderer FindLineRenderer(JObject @params)
{
GameObject go = ManageVfxCommon.FindTargetGameObject(@params);
return go?.GetComponent<LineRenderer>();
}
=> ManageVfxCommon.FindComponent<LineRenderer>(@params);

public static string FindLineRendererError(JObject @params)
=> ManageVfxCommon.FindComponentError<LineRenderer>(@params);

public static object GetInfo(JObject @params)
{
LineRenderer lr = FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = FindLineRendererError(@params) };

var positions = new Vector3[lr.positionCount];
lr.GetPositions(positions);
Expand Down
14 changes: 7 additions & 7 deletions MCPForUnity/Editor/Tools/Vfx/LineWrite.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ internal static class LineWrite
public static object SetPositions(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

RendererHelpers.EnsureMaterial(lr);

Expand All @@ -35,7 +35,7 @@ public static object SetPositions(JObject @params)
public static object AddPosition(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

RendererHelpers.EnsureMaterial(lr);

Expand All @@ -53,7 +53,7 @@ public static object AddPosition(JObject @params)
public static object SetPosition(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

RendererHelpers.EnsureMaterial(lr);

Expand All @@ -72,7 +72,7 @@ public static object SetPosition(JObject @params)
public static object SetWidth(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

RendererHelpers.EnsureMaterial(lr);

Expand All @@ -91,7 +91,7 @@ public static object SetWidth(JObject @params)
public static object SetColor(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

RendererHelpers.EnsureMaterial(lr);

Expand All @@ -116,7 +116,7 @@ public static object SetMaterial(JObject @params)
public static object SetProperties(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

RendererHelpers.EnsureMaterial(lr);

Expand Down Expand Up @@ -176,7 +176,7 @@ public static object SetProperties(JObject @params)
public static object Clear(JObject @params)
{
LineRenderer lr = LineRead.FindLineRenderer(@params);
if (lr == null) return new { success = false, message = "LineRenderer not found" };
if (lr == null) return new { success = false, message = LineRead.FindLineRendererError(@params) };

int count = lr.positionCount;
Undo.RecordObject(lr, "Clear Line");
Expand Down
Loading