-
Notifications
You must be signed in to change notification settings - Fork 890
feat: add open_prefab_stage action to manage_editor #968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |||||
| using UnityEditor; | ||||||
| using UnityEditor.SceneManagement; | ||||||
| using UnityEditorInternal; // Required for tag management | ||||||
| using UnityEngine; | ||||||
|
|
||||||
| namespace MCPForUnity.Editor.Tools | ||||||
| { | ||||||
|
|
@@ -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"); | ||||||
|
|
||||||
| // Route action | ||||||
| switch (action) | ||||||
|
|
@@ -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(); | ||||||
|
|
||||||
|
|
@@ -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." | ||||||
| ); | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -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."); | ||||||
|
||||||
| 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."); |
| 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 ────────────────────────────────────────────────────────── | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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", | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 🧪 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reject mismatched
prefabPath/pathin the Unity handler too.Line 50 collapses the aliases before validation, so a direct
manage_editorcaller that sends both values will silently preferprefabPathinstead of failing. It also letsprefabPath=""mask a validpathalias because??only falls back onnull. The Python surface already rejects conflicting values inServer/src/services/tools/manage_editor.pyat Lines 44-48, so the editor layer should enforce the same contract. If you take this route, the newOpenPrefabStage_PrefabPathTakesPrecedenceOverPathtest should flip to a rejection case as well.🛡️ Add the guard before dispatch
🤖 Prompt for AI Agents