Skip to content
Merged
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
64 changes: 63 additions & 1 deletion MCPForUnity/Editor/Tools/ManageEditor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using UnityEditor;
using UnityEditor.SceneManagement;
using UnityEditorInternal; // Required for tag management
using UnityEngine;

namespace MCPForUnity.Editor.Tools
{
Expand Down Expand Up @@ -46,6 +47,7 @@ public static object HandleCommand(JObject @params)
// Parameters for specific actions
string tagName = p.Get("tagName");
string layerName = p.Get("layerName");
string prefabPath = p.Get("prefabPath") ?? p.Get("path");
Comment on lines 47 to +50
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 | 🟡 Minor

Reject mismatched prefabPath/path in the Unity handler too.

Line 50 collapses the aliases before validation, so a direct manage_editor caller that sends both values will silently prefer prefabPath instead of failing. It also lets prefabPath="" mask a valid path alias because ?? only falls back on null. The Python surface already rejects conflicting values in Server/src/services/tools/manage_editor.py at Lines 44-48, so the editor layer should enforce the same contract. If you take this route, the new OpenPrefabStage_PrefabPathTakesPrecedenceOverPath test should flip to a rejection case as well.

🛡️ Add the guard before dispatch
-            string prefabPath = p.Get("prefabPath") ?? p.Get("path");
+            string prefabPathParam = p.Get("prefabPath");
+            string pathParam = p.Get("path");
+            if (
+                !string.IsNullOrWhiteSpace(prefabPathParam)
+                && !string.IsNullOrWhiteSpace(pathParam)
+                && !string.Equals(prefabPathParam, pathParam, StringComparison.Ordinal)
+            )
+            {
+                return new ErrorResponse("Provide only one of 'prefabPath' or 'path', or ensure both match.");
+            }
+            string prefabPath = !string.IsNullOrWhiteSpace(prefabPathParam)
+                ? prefabPathParam
+                : pathParam;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@MCPForUnity/Editor/Tools/ManageEditor.cs` around lines 47 - 50, Before
collapsing aliases, validate that callers do not supply conflicting prefab/path
values: read both p.Get("prefabPath") and p.Get("path") into temporaries (treat
empty string as missing via string.IsNullOrEmpty), and if both are present
(non-empty) reject/return an error the same way the Python service does; only
after this guard set prefabPath = non-empty prefabPath if present else path.
Reference the variables prefabPath, p.Get("prefabPath"), p.Get("path") and the
handler method in ManageEditor.cs (e.g., the OpenPrefabStage dispatch) to locate
where to add the guard.


// Route action
switch (action)
Expand Down Expand Up @@ -136,6 +138,8 @@ public static object HandleCommand(JObject @params)
// return SetQualityLevel(@params["qualityLevel"]);

// Prefab Stage
case "open_prefab_stage":
return OpenPrefabStage(prefabPath);
case "close_prefab_stage":
return ClosePrefabStage();

Expand Down Expand Up @@ -176,7 +180,7 @@ public static object HandleCommand(JObject @params)

default:
return new ErrorResponse(
$"Unknown action: '{action}'. Supported actions: play, pause, stop, set_active_tool, add_tag, remove_tag, add_layer, remove_layer, close_prefab_stage, deploy_package, restore_package, undo, redo. Use MCP resources for reading editor state, project info, tags, layers, selection, windows, prefab stage, and active tool."
$"Unknown action: '{action}'. Supported actions: play, pause, stop, set_active_tool, add_tag, remove_tag, add_layer, remove_layer, open_prefab_stage, close_prefab_stage, deploy_package, restore_package, undo, redo. Use MCP resources for reading editor state, project info, tags, layers, selection, windows, prefab stage, and active tool."
);
}
}
Expand Down Expand Up @@ -398,6 +402,64 @@ private static object RemoveLayer(string layerName)

// --- Prefab Stage Methods ---

private static object OpenPrefabStage(string requestedPath)
{
if (string.IsNullOrWhiteSpace(requestedPath))
{
return new ErrorResponse("'prefabPath' parameter is required for open_prefab_stage.");
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

open_prefab_stage now accepts both 'prefabPath' and the compatibility alias 'path' (see HandleCommand extracting prefabPath = p.Get("prefabPath") ?? p.Get("path")), but the validation error still says only "'prefabPath' parameter is required". This is misleading for callers using the alias; update the message to mention both accepted keys (e.g., prefabPath or path).

Suggested change
return new ErrorResponse("'prefabPath' parameter is required for open_prefab_stage.");
return new ErrorResponse("Either 'prefabPath' or 'path' parameter is required for open_prefab_stage.");

Copilot uses AI. Check for mistakes.
}

string sanitizedPath = AssetPathUtility.SanitizeAssetPath(requestedPath);
if (sanitizedPath == null)
{
return new ErrorResponse($"Invalid prefab path (path traversal detected): '{requestedPath}'.");
}

if (!sanitizedPath.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase))
{
return new ErrorResponse($"Prefab path must be within the Assets folder. Got: '{sanitizedPath}'.");
}

if (!sanitizedPath.EndsWith(".prefab", StringComparison.OrdinalIgnoreCase))
{
return new ErrorResponse($"Prefab path must end with '.prefab'. Got: '{sanitizedPath}'.");
}

try
{
GameObject prefabAsset = AssetDatabase.LoadAssetAtPath<GameObject>(sanitizedPath);
if (prefabAsset == null)
{
return new ErrorResponse($"Prefab asset not found at '{sanitizedPath}'.");
}

var prefabStage = PrefabStageUtility.OpenPrefab(sanitizedPath);
bool enteredStage = prefabStage != null
&& string.Equals(prefabStage.assetPath, sanitizedPath, StringComparison.OrdinalIgnoreCase)
&& prefabStage.prefabContentsRoot != null;

if (!enteredStage)
{
return new ErrorResponse($"Failed to open prefab stage for '{sanitizedPath}'. PrefabStageUtility.OpenPrefab did not enter the requested prefab stage.");
}

return new SuccessResponse(
$"Opened prefab stage for '{sanitizedPath}'.",
new
{
prefabPath = sanitizedPath,
openedPrefabPath = prefabStage.assetPath,
rootName = prefabStage.prefabContentsRoot.name,
enteredPrefabStage = enteredStage
}
);
}
catch (Exception e)
{
return new ErrorResponse($"Error opening prefab stage: {e.Message}");
}
}

private static object ClosePrefabStage()
{
try
Expand Down
5 changes: 3 additions & 2 deletions Server/src/services/resources/prefab.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ async def get_prefab_api_docs(_ctx: Context) -> MCPResponse:
"workflow": [
"1. Use manage_asset action=search filterType=Prefab to find prefabs",
"2. Use the asset path to access detailed data via resources below",
"3. Use manage_prefabs tool for prefab stage operations (open, save, close)"
"3. Use manage_editor action=open_prefab_stage / close_prefab_stage for prefab editing UI transitions"
],
"path_encoding": {
"note": "Prefab paths must be URL-encoded when used in resource URIs",
Expand All @@ -80,7 +80,8 @@ async def get_prefab_api_docs(_ctx: Context) -> MCPResponse:
}
},
"related_tools": {
"manage_prefabs": "Open/close prefab stages, save changes, create prefabs from GameObjects",
"manage_editor": "Open/close prefab stages in the Unity Editor UI",
"manage_prefabs": "Headless prefab inspection and modification without opening prefab stages",
"manage_asset": "Search for prefab assets, get asset info",
"manage_gameobject": "Modify GameObjects in open prefab stage",
"manage_components": "Add/remove/modify components on prefab GameObjects"
Expand Down
19 changes: 17 additions & 2 deletions Server/src/services/tools/manage_editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,24 @@
from transport.legacy.unity_connection import async_send_command_with_retry

@mcp_for_unity_tool(
description="Controls and queries the Unity editor's state and settings. Read-only actions: telemetry_status, telemetry_ping. Modifying actions: play, pause, stop, set_active_tool, add_tag, remove_tag, add_layer, remove_layer, close_prefab_stage, deploy_package, restore_package, undo, redo. deploy_package copies the configured MCPForUnity source folder into the project's installed package location (triggers recompile, no confirmation dialog). restore_package reverts to the pre-deployment backup. undo/redo perform Unity editor undo/redo and return the affected group name.",
description="Controls and queries the Unity editor's state and settings. Read-only actions: telemetry_status, telemetry_ping. Modifying actions: play, pause, stop, set_active_tool, add_tag, remove_tag, add_layer, remove_layer, open_prefab_stage, close_prefab_stage, deploy_package, restore_package, undo, redo. open_prefab_stage opens a prefab asset in Unity's prefab editing mode. deploy_package copies the configured MCPForUnity source folder into the project's installed package location (triggers recompile, no confirmation dialog). restore_package reverts to the pre-deployment backup. undo/redo perform Unity editor undo/redo and return the affected group name.",
annotations=ToolAnnotations(
title="Manage Editor",
),
)
async def manage_editor(
ctx: Context,
action: Annotated[Literal["telemetry_status", "telemetry_ping", "play", "pause", "stop", "set_active_tool", "add_tag", "remove_tag", "add_layer", "remove_layer", "close_prefab_stage", "deploy_package", "restore_package", "undo", "redo"], "Get and update the Unity Editor state. close_prefab_stage exits prefab editing mode and returns to the main scene stage. deploy_package copies the configured MCPForUnity source into the project's package location (triggers recompile). restore_package reverts the last deployment from backup. undo/redo perform editor undo/redo."],
action: Annotated[Literal["telemetry_status", "telemetry_ping", "play", "pause", "stop", "set_active_tool", "add_tag", "remove_tag", "add_layer", "remove_layer", "open_prefab_stage", "close_prefab_stage", "deploy_package", "restore_package", "undo", "redo"], "Get and update the Unity Editor state. open_prefab_stage opens a prefab asset in prefab editing mode; close_prefab_stage exits prefab editing mode and returns to the main scene stage. deploy_package copies the configured MCPForUnity source into the project's package location (triggers recompile). restore_package reverts the last deployment from backup. undo/redo perform editor undo/redo."],
tool_name: Annotated[str,
"Tool name when setting active tool"] | None = None,
tag_name: Annotated[str,
"Tag name when adding and removing tags"] | None = None,
layer_name: Annotated[str,
"Layer name when adding and removing layers"] | None = None,
prefab_path: Annotated[str,
"Prefab asset path when opening a prefab stage (e.g. Assets/Prefabs/MyPrefab.prefab)."] | None = None,
path: Annotated[str,
"Compatibility alias for prefab_path when opening a prefab stage."] | None = None,
) -> dict[str, Any]:
# Get active instance from request state (injected by middleware)
unity_instance = await get_unity_instance_from_context(ctx)
Expand All @@ -36,13 +40,24 @@ async def manage_editor(
if action == "telemetry_ping":
record_tool_usage("diagnostic_ping", True, 1.0, None)
return {"success": True, "message": "telemetry ping queued"}

if prefab_path is not None and path is not None and prefab_path != path:
return {
"success": False,
"message": "Provide only one of prefab_path or path, or ensure both values match.",
}

# Prepare parameters, removing None values
params = {
"action": action,
"toolName": tool_name,
"tagName": tag_name,
"layerName": layer_name,
}
if prefab_path is not None:
params["prefabPath"] = prefab_path
elif path is not None:
params["path"] = path
params = {k: v for k, v in params.items() if v is not None}

# Send command using centralized retry helper with instance routing
Expand Down
71 changes: 70 additions & 1 deletion Server/tests/test_manage_editor.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
"""Tests for manage_editor tool."""
import asyncio
import inspect
from types import SimpleNamespace
from unittest.mock import AsyncMock

import pytest

from services.tools.manage_editor import manage_editor
import services.tools.manage_editor as manage_editor_mod
from services.registry import get_registered_tools

# ── Fixture ──────────────────────────────────────────────────────────

Expand Down Expand Up @@ -52,7 +55,7 @@ def test_redo_forwards_to_unity(mock_unity):
UNITY_FORWARDED_ACTIONS = [
"play", "pause", "stop", "set_active_tool",
"add_tag", "remove_tag", "add_layer", "remove_layer",
"close_prefab_stage", "deploy_package", "restore_package",
"open_prefab_stage", "close_prefab_stage", "deploy_package", "restore_package",
"undo", "redo",
]

Expand Down Expand Up @@ -90,3 +93,69 @@ def test_undo_omits_none_params(mock_unity):
assert "toolName" not in params
assert "tagName" not in params
assert "layerName" not in params


# ── open_prefab_stage ────────────────────────────────────────────────


def test_manage_editor_prefab_path_parameters_exist():
"""open_prefab_stage should expose prefab_path plus path alias parameters."""
sig = inspect.signature(manage_editor_mod.manage_editor)
assert "prefab_path" in sig.parameters
assert "path" in sig.parameters
assert sig.parameters["prefab_path"].default is None
assert sig.parameters["path"].default is None


def test_manage_editor_description_mentions_open_prefab_stage():
"""The tool description should advertise the new prefab stage action."""
editor_tool = next(
(t for t in get_registered_tools() if t["name"] == "manage_editor"), None
)
assert editor_tool is not None
desc = editor_tool.get("description") or editor_tool.get("kwargs", {}).get("description", "")
assert "open_prefab_stage" in desc


def test_open_prefab_stage_forwards_prefab_path(mock_unity):
"""prefab_path should map to Unity's prefabPath parameter."""
result = asyncio.run(
manage_editor(
SimpleNamespace(),
action="open_prefab_stage",
prefab_path="Assets/Prefabs/Test.prefab",
)
)
assert result["success"] is True
assert mock_unity["params"]["action"] == "open_prefab_stage"
assert mock_unity["params"]["prefabPath"] == "Assets/Prefabs/Test.prefab"
assert "path" not in mock_unity["params"]


def test_open_prefab_stage_accepts_path_alias(mock_unity):
"""path should remain available as a compatibility alias."""
result = asyncio.run(
manage_editor(
SimpleNamespace(),
action="open_prefab_stage",
path="Assets/Prefabs/Alias.prefab",
)
)
assert result["success"] is True
assert mock_unity["params"]["action"] == "open_prefab_stage"
assert mock_unity["params"]["path"] == "Assets/Prefabs/Alias.prefab"
assert "prefabPath" not in mock_unity["params"]


def test_open_prefab_stage_rejects_conflicting_path_inputs(mock_unity):
"""Conflicting aliases should fail fast before sending a Unity command."""
result = asyncio.run(
manage_editor(
SimpleNamespace(),
action="open_prefab_stage",
prefab_path="Assets/Prefabs/Primary.prefab",
path="Assets/Prefabs/Alias.prefab",
)
)
assert result["success"] is False
assert "Provide only one of prefab_path or path" in result.get("message", "")
Comment on lines +150 to +161
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 | 🟡 Minor

Assert that the conflicting alias case never reaches Unity.

This test verifies the error message, but not the promised "fail before dispatch" behavior. Without checking mock_unity, it would still pass if send_with_unity_instance were called and the failure were synthesized afterward.

🧪 Tighten the expectation
 def test_open_prefab_stage_rejects_conflicting_path_inputs(mock_unity):
     """Conflicting aliases should fail fast before sending a Unity command."""
     result = asyncio.run(
         manage_editor(
             SimpleNamespace(),
             action="open_prefab_stage",
             prefab_path="Assets/Prefabs/Primary.prefab",
             path="Assets/Prefabs/Alias.prefab",
         )
     )
     assert result["success"] is False
     assert "Provide only one of prefab_path or path" in result.get("message", "")
+    assert "params" not in mock_unity
📝 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.

Suggested change
def test_open_prefab_stage_rejects_conflicting_path_inputs(mock_unity):
"""Conflicting aliases should fail fast before sending a Unity command."""
result = asyncio.run(
manage_editor(
SimpleNamespace(),
action="open_prefab_stage",
prefab_path="Assets/Prefabs/Primary.prefab",
path="Assets/Prefabs/Alias.prefab",
)
)
assert result["success"] is False
assert "Provide only one of prefab_path or path" in result.get("message", "")
def test_open_prefab_stage_rejects_conflicting_path_inputs(mock_unity):
"""Conflicting aliases should fail fast before sending a Unity command."""
result = asyncio.run(
manage_editor(
SimpleNamespace(),
action="open_prefab_stage",
prefab_path="Assets/Prefabs/Primary.prefab",
path="Assets/Prefabs/Alias.prefab",
)
)
assert result["success"] is False
assert "Provide only one of prefab_path or path" in result.get("message", "")
assert "params" not in mock_unity
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Server/tests/test_manage_editor.py` around lines 150 - 161, The test
test_open_prefab_stage_rejects_conflicting_path_inputs must also assert that no
Unity command was sent: ensure mock_unity (the fixture) did not have
send_with_unity_instance invoked when manage_editor is called with both
prefab_path and path; after calling manage_editor, add an assertion that
mock_unity.send_with_unity_instance was not called (or use
mock_unity.assert_not_called()/mock_unity.send_with_unity_instance.assert_not_called())
to guarantee the failure occurs before dispatch to Unity.

Loading
Loading