feat: replace prefab stage actions with headless modify_contents #635
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideReplaces stage-based prefab editing with a new headless modify_contents workflow and updates the Python tool and tests accordingly, while removing the now-unused PrefabStage resource. Sequence diagram for headless modify_contents prefab editing workflowsequenceDiagram
actor Agent
participant ManagePrefabsTool
participant UnityBridge
participant ManagePrefabs
participant PrefabUtility
participant AssetDatabase
Agent->>ManagePrefabsTool: manage_prefabs(action=modify_contents, prefab_path, target, ...)
ManagePrefabsTool->>ManagePrefabsTool: Build params dict (position, rotation, scale, name, tag, layer, setActive, parent, componentsToAdd, componentsToRemove)
ManagePrefabsTool->>UnityBridge: send_with_unity_instance(tool=manage_prefabs, action=modify_contents, params)
UnityBridge->>ManagePrefabs: HandleCommand(params)
ManagePrefabs->>ManagePrefabs: Validate prefabPath via AssetPathUtility.SanitizeAssetPath
ManagePrefabs->>PrefabUtility: LoadPrefabContents(sanitizedPath)
PrefabUtility-->>ManagePrefabs: prefabContents GameObject
ManagePrefabs->>ManagePrefabs: FindInPrefabContents(prefabContents, target)
ManagePrefabs->>ManagePrefabs: ApplyModificationsToPrefabObject(targetGo, params, prefabContents)
ManagePrefabs-->>ManagePrefabs: (modified, error)
alt error is not null
ManagePrefabs-->>UnityBridge: ErrorResponse
UnityBridge-->>ManagePrefabsTool: error payload
ManagePrefabsTool-->>Agent: error result
else no error
ManagePrefabs->>PrefabUtility: SaveAsPrefabAsset(prefabContents, sanitizedPath, out success)
PrefabUtility-->>ManagePrefabs: success flag
alt success is false
ManagePrefabs-->>UnityBridge: ErrorResponse
UnityBridge-->>ManagePrefabsTool: error payload
ManagePrefabsTool-->>Agent: error result
else success is true
ManagePrefabs->>AssetDatabase: Refresh
ManagePrefabs-->>UnityBridge: SuccessResponse(prefabPath, targetName, modified, transform, componentTypes)
UnityBridge-->>ManagePrefabsTool: success payload
ManagePrefabsTool-->>Agent: success result
end
end
ManagePrefabs->>PrefabUtility: UnloadPrefabContents(prefabContents)
Class diagram for updated ManagePrefabs headless editing APIsclassDiagram
class ManagePrefabs {
<<static>>
-ACTION_CREATE_FROM_GAMEOBJECT string
-ACTION_GET_INFO string
-ACTION_GET_HIERARCHY string
-ACTION_MODIFY_CONTENTS string
-SupportedActions string
+HandleCommand(@params JObject) object
-CreatePrefabFromGameObject(@params JObject) object
-GetInfo(@params JObject) object
-GetHierarchy(@params JObject) object
-ModifyContents(@params JObject) object
-FindInPrefabContents(prefabContents GameObject, target string) GameObject
-ApplyModificationsToPrefabObject(targetGo GameObject, @params JObject, prefabRoot GameObject) (bool, ErrorResponse)
-BuildHierarchyItemsRecursive(transform Transform, parentPath string, items List~HierarchyItem~) void
}
class ErrorResponse {
+ErrorResponse(message string)
+message string
+success bool
}
class SuccessResponse {
+SuccessResponse(message string, data object)
+message string
+data object
+success bool
}
class VectorParsing {
+ParseVector3(token JToken) Vector3?
}
class ComponentResolver {
+TryResolve(typeName string, componentType out Type, error out string) bool
}
class PrefabUtilityHelper {
+GetComponentTypeNames(targetGo GameObject) List~string~
}
class AssetPathUtility {
+SanitizeAssetPath(path string) string
}
class McpLog {
+Info(message string) void
+Warn(message string) void
+Error(message string) void
}
class PrefabUtility {
+LoadPrefabContents(assetPath string) GameObject
+SaveAsPrefabAsset(prefabContents GameObject, assetPath string, success out bool) void
+UnloadPrefabContents(prefabContents GameObject) void
}
class GameObject {
+name string
+tag string
+layer int
+activeSelf bool
+transform Transform
+SetActive(value bool) void
+AddComponent(componentType Type) Component
+GetComponent(componentType Type) Component
}
class Transform {
+localPosition Vector3
+localEulerAngles Vector3
+localScale Vector3
+parent Transform
+SetParent(parent Transform, worldPositionStays bool) void
+IsChildOf(parent Transform) bool
+Find(path string) Transform
+GetComponentsInChildren(includeInactive bool) List~Transform~
}
class LayerMask {
+NameToLayer(layerName string) int
}
class UnityEngineObject {
}
class Component {
}
class Vector3 {
+x float
+y float
+z float
}
ManagePrefabs --> ErrorResponse : uses
ManagePrefabs --> SuccessResponse : uses
ManagePrefabs --> VectorParsing : uses
ManagePrefabs --> ComponentResolver : uses
ManagePrefabs --> PrefabUtilityHelper : uses
ManagePrefabs --> AssetPathUtility : uses
ManagePrefabs --> McpLog : uses
ManagePrefabs --> PrefabUtility : uses
ManagePrefabs --> GameObject : manipulates
GameObject --> Transform : has
Component --> UnityEngineObject : inherits
GameObject --> UnityEngineObject : inherits
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRemoved the PrefabStage editor resource and its "get_prefab_stage" command; replaced stage-based prefab workflows with a headless Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManagePrefabs as ManagePrefabs (Editor)
participant PrefabUtility
participant AssetDatabase
Client->>ManagePrefabs: modify_contents(prefab_path, target, modifications)
ManagePrefabs->>PrefabUtility: LoadPrefabContents(prefab_path)
PrefabUtility-->>ManagePrefabs: prefab_contents
ManagePrefabs->>ManagePrefabs: Find target object in prefab_contents
ManagePrefabs->>ManagePrefabs: Apply modifications (transform, components, properties)
ManagePrefabs->>PrefabUtility: SaveAsPrefabAsset(prefab_contents, prefab_path)
PrefabUtility->>AssetDatabase: Register/Update assets
AssetDatabase-->>PrefabUtility: saved
ManagePrefabs->>PrefabUtility: UnloadPrefabContents(prefab_contents)
ManagePrefabs-->>Client: success_response(modified_state, object_info)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs:476-478` </location>
<code_context>
+ return modifyResult.error;
+ }
+
+ // Save the prefab
+ bool success;
+ PrefabUtility.SaveAsPrefabAsset(prefabContents, sanitizedPath, out success);
+
+ if (!success)
</code_context>
<issue_to_address>
**suggestion (performance):** Skip saving when no modifications were made to avoid unnecessary asset writes.
You already compute `modifyResult.modified`, but still call `SaveAsPrefabAsset` unconditionally. In automated/headless use this creates unnecessary writes and version-control noise. Consider returning success early when `modified == false` (and reflecting that in the message), and only calling `SaveAsPrefabAsset` when changes were actually made.
Suggested implementation:
```csharp
// Apply modifications
var modifyResult = ApplyModificationsToPrefabObject(targetGo, @params, prefabContents);
if (modifyResult.error != null)
{
return modifyResult.error;
}
// Skip saving when no modifications were made to avoid unnecessary asset writes
if (!modifyResult.modified)
{
return new SuccessResponse($"Prefab '{sanitizedPath}' is already up to date; no changes were applied.");
}
// Save the prefab
bool success;
PrefabUtility.SaveAsPrefabAsset(prefabContents, sanitizedPath, out success);
if (!success)
{
return new ErrorResponse($"Failed to save prefab '{sanitizedPath}'.");
}
return new SuccessResponse($"Prefab '{sanitizedPath}' updated successfully.");
```
The exact success/response types may differ in your codebase. You will need to:
1. Replace `SuccessResponse` with whatever success response type you actually use (e.g. `OkResponse`, `Success`, etc.), and adjust constructor arguments as appropriate.
2. If you already have standardized success/error message patterns, align the new messages with those conventions.
3. Ensure this method's return type matches the `SuccessResponse`/`ErrorResponse` usage above; if it is a common response base type, both subclasses should be compatible.
4. If save failure handling is implemented elsewhere in this method, integrate the `!modified` early-return in a way that doesn't duplicate or conflict with existing save/error logic.
</issue_to_address>
### Comment 2
<location> `Server/src/services/tools/manage_prefabs.py:23-30` </location>
<code_context>
}
@mcp_for_unity_tool(
description=(
"Manages Unity Prefab assets and stages. "
</code_context>
<issue_to_address>
**suggestion:** Update the tool description to reflect that stage-based actions are no longer supported.
The decorator description still mentions "stages" even though `open_stage`, `save_open_stage`, and `close_stage` have been removed and only headless operations remain. Please rephrase it to emphasize prefab asset CRUD/headless editing and remove references to stages to avoid confusing tool consumers.
```suggestion
@mcp_for_unity_tool(
description=(
"Manages Unity Prefab assets via headless operations (no UI, no prefab stages). "
"Actions: get_info, get_hierarchy, create_from_gameobject, modify_contents. "
"Use modify_contents for headless prefab editing (no UI, no dialogs) - ideal for automated workflows. "
"Use manage_asset action=search filterType=Prefab to list prefabs."
),
annotations=ToolAnnotations(
```
</issue_to_address>
### Comment 3
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.cs:45-51` </location>
<code_context>
#region CREATE Tests
[Test]
- public void CreateFromGameObject_CreatesNewPrefab_WithValidParameters()
+ public void CreateFromGameObject_CreatesNewPrefab()
</code_context>
<issue_to_address>
**suggestion (testing):** Reintroduce a test that verifies `create_from_gameobject` can find inactive scene objects when `searchInactive` is true.
The previous `CreateFromGameObject_FindsInactiveObject_WhenSearchInactiveIsTrue` test covered both failure without `searchInactive` and success with it enabled, which the new combined test no longer exercises. Please add a dedicated `searchInactive` test (even a simplified one) so regressions in inactive-object lookup are still caught.
Suggested implementation:
```csharp
#region CREATE Tests
[Test]
public void CreateFromGameObject_CreatesNewPrefab()
{
string prefabPath = Path.Combine(TempDirectory, "NewPrefab.prefab").Replace('\\', '/');
GameObject sceneObject = new GameObject("TestObject");
["prefabPath"] = prefabPath
}));
}
[Test]
public void CreateFromGameObject_FindsInactiveObject_WhenSearchInactiveIsTrue()
{
// Arrange
string prefabPath = Path.Combine(TempDirectory, "InactivePrefab.prefab").Replace('\\', '/');
GameObject sceneObject = new GameObject("InactiveTestObject");
sceneObject.SetActive(false);
// Act + Assert:
// 1) Without searchInactive we expect the call to fail / not find the object.
// 2) With searchInactive = true we expect the call to succeed and create the prefab.
// 1) Call without searchInactive flag and verify it fails appropriately.
// (Use the same invocation pattern as CreateFromGameObject_CreatesNewPrefab but
// omit the searchInactive parameter and assert the expected failure.)
// Example (adjust to your helper/assert style):
//
// Assert.Throws<SomeExpectedException>(() =>
// CallTool("create_from_gameobject", new Dictionary<string, object>
// {
// ["gameObjectName"] = sceneObject.name,
// ["prefabPath"] = prefabPath
// }));
//
// or if you use result objects:
//
// var resultWithoutSearchInactive = CallTool(...);
// Assert.IsFalse(resultWithoutSearchInactive.success);
// 2) Call with searchInactive = true and verify it now succeeds and creates the prefab.
//
// var resultWithSearchInactive = CallTool("create_from_gameobject", new Dictionary<string, object>
// {
// ["gameObjectName"] = sceneObject.name,
// ["prefabPath"] = prefabPath,
// ["searchInactive"] = true
// });
//
// Assert.IsTrue(resultWithSearchInactive.success);
// Assert.That(File.Exists(prefabPath), Is.True);
}
```
To fully wire this test to your existing infrastructure you should:
1. Replace the commented `CallTool`/assert snippets with the actual helper you use in `CreateFromGameObject_CreatesNewPrefab` (e.g. your MCP/tool invocation method and its result type).
2. Use the same parameter keys that `create_from_gameobject` currently expects:
- For the scene object reference, either pass the object itself (if that’s how the existing test does it) or pass `sceneObject.name` and adjust the key accordingly.
- Add the `"searchInactive"` (or your actual flag name) boolean parameter only in the success case.
3. Use the same assertion style as the other tests:
- For the failure case: either assert a specific exception is thrown or assert the returned result indicates failure.
- For the success case: assert success and that the prefab was created at `prefabPath` (e.g. via `File.Exists(prefabPath)` or your existing helper).
4. If your existing test uses async patterns (e.g. `await CallToolAsync(...)`), update this new test to be `async Task` and `await` the calls consistently.
</issue_to_address>
### Comment 4
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.cs:260` </location>
<code_context>
-
[Test]
- public void SaveOpenStage_WithForce_SavesEvenWhenNotDirty()
+ public void ModifyContents_TargetsChildrenByNameAndPath()
{
- string prefabPath = CreateTestPrefab("ForceSaveTest");
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `modify_contents` parent re-parenting behavior, including the hierarchy loop guard.
`modify_contents` also supports a `parent` parameter and has an `IsChildOf` guard to prevent hierarchy loops, but those paths aren’t covered by tests. Please add one test that verifies a valid re-parent within the prefab succeeds, and another that verifies attempting to parent a node under one of its descendants fails with the expected error, so we cover both the normal and loop-prevention behavior.
Suggested implementation:
```csharp
[Test]
public void ModifyContents_ReparentsChildWithinPrefab()
{
string prefabPath = CreateNestedTestPrefab("ReparentTest");
try
{
// Reparent a child under another sibling within the same prefab
var result = ToJObject(ManagePrefabs.HandleCommand(new JObject
{
["command"] = "modify_contents",
["asset_path"] = prefabPath,
["targets"] = new JArray
{
new JObject
{
// These names/paths assume the nested prefab created by CreateNestedTestPrefab
// exposes a root with at least two children; adjust as needed to match
["path"] = "Root/ChildA",
["parent"] = "Root/ChildB"
}
}
}));
// Verify the command succeeded (no hierarchy loop triggered)
Assert.AreEqual("ok", (string)result["status"], "Expected modify_contents to succeed for a valid reparent within the prefab.");
}
finally
{
SafeDeleteAsset(prefabPath);
}
}
[Test]
public void ModifyContents_PreventsHierarchyLoopsWhenReparenting()
{
string prefabPath = CreateNestedTestPrefab("HierarchyLoopTest");
try
{
// Attempt to parent an ancestor under one of its descendants, which should be rejected
var result = ToJObject(ManagePrefabs.HandleCommand(new JObject
{
["command"] = "modify_contents",
["asset_path"] = prefabPath,
["targets"] = new JArray
{
new JObject
{
// Attempt to parent Root under one of its descendants, e.g. Root/ChildA
// so that the underlying IsChildOf guard is exercised.
["path"] = "Root",
["parent"] = "Root/ChildA"
}
}
}));
// Verify the command failed with an appropriate error
Assert.AreEqual("error", (string)result["status"], "Expected modify_contents to fail when creating a hierarchy loop.");
StringAssert.Contains(
"hierarchy",
(string)result["message"],
"Expected error message to mention invalid hierarchy / loop prevention.");
}
finally
{
SafeDeleteAsset(prefabPath);
}
}
[Test]
public void ModifyContents_TargetsChildrenByNameAndPath()
{
string prefabPath = CreateNestedTestPrefab("TargetTest");
try
{
// Target by name
var nameResult = ToJObject(ManagePrefabs.HandleCommand(new JObject
{
```
1. The exact object paths (`"Root/ChildA"`, `"Root/ChildB"`, `"Root"`) must be updated to match the actual hierarchy produced by `CreateNestedTestPrefab`. Replace these strings with the real node paths (for example, `"Parent/Child"` patterns the existing tests already assert on).
2. The response contract from `ManagePrefabs.HandleCommand` may differ (for example, using `"success"` instead of `"status"`, or naming the error field differently). Align the assertions:
- Change `result["status"]` to the actual field used to indicate success/failure.
- Change `result["message"]` and the `StringAssert.Contains("hierarchy", ...)` check to match the real error field name and message substring used when the `IsChildOf` guard triggers.
3. If `CreateNestedTestPrefab` does not create a root node named `"Root"` with at least two children, adjust the test to reparent one real child under another within the prefab, and for the loop test, choose an ancestor/descendant pair that will trigger the `IsChildOf` check.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
45
to
51
| #region CREATE Tests | ||
|
|
||
| [Test] | ||
| public void CreateFromGameObject_CreatesNewPrefab_WithValidParameters() | ||
| public void CreateFromGameObject_CreatesNewPrefab() | ||
| { | ||
| string prefabPath = Path.Combine(TempDirectory, "NewPrefab.prefab").Replace('\\', '/'); | ||
| GameObject sceneObject = new GameObject("TestObject"); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Reintroduce a test that verifies create_from_gameobject can find inactive scene objects when searchInactive is true.
The previous CreateFromGameObject_FindsInactiveObject_WhenSearchInactiveIsTrue test covered both failure without searchInactive and success with it enabled, which the new combined test no longer exercises. Please add a dedicated searchInactive test (even a simplified one) so regressions in inactive-object lookup are still caught.
Suggested implementation:
#region CREATE Tests
[Test]
public void CreateFromGameObject_CreatesNewPrefab()
{
string prefabPath = Path.Combine(TempDirectory, "NewPrefab.prefab").Replace('\\', '/');
GameObject sceneObject = new GameObject("TestObject");
["prefabPath"] = prefabPath
}));
}
[Test]
public void CreateFromGameObject_FindsInactiveObject_WhenSearchInactiveIsTrue()
{
// Arrange
string prefabPath = Path.Combine(TempDirectory, "InactivePrefab.prefab").Replace('\\', '/');
GameObject sceneObject = new GameObject("InactiveTestObject");
sceneObject.SetActive(false);
// Act + Assert:
// 1) Without searchInactive we expect the call to fail / not find the object.
// 2) With searchInactive = true we expect the call to succeed and create the prefab.
// 1) Call without searchInactive flag and verify it fails appropriately.
// (Use the same invocation pattern as CreateFromGameObject_CreatesNewPrefab but
// omit the searchInactive parameter and assert the expected failure.)
// Example (adjust to your helper/assert style):
//
// Assert.Throws<SomeExpectedException>(() =>
// CallTool("create_from_gameobject", new Dictionary<string, object>
// {
// ["gameObjectName"] = sceneObject.name,
// ["prefabPath"] = prefabPath
// }));
//
// or if you use result objects:
//
// var resultWithoutSearchInactive = CallTool(...);
// Assert.IsFalse(resultWithoutSearchInactive.success);
// 2) Call with searchInactive = true and verify it now succeeds and creates the prefab.
//
// var resultWithSearchInactive = CallTool("create_from_gameobject", new Dictionary<string, object>
// {
// ["gameObjectName"] = sceneObject.name,
// ["prefabPath"] = prefabPath,
// ["searchInactive"] = true
// });
//
// Assert.IsTrue(resultWithSearchInactive.success);
// Assert.That(File.Exists(prefabPath), Is.True);
}To fully wire this test to your existing infrastructure you should:
- Replace the commented
CallTool/assert snippets with the actual helper you use inCreateFromGameObject_CreatesNewPrefab(e.g. your MCP/tool invocation method and its result type). - Use the same parameter keys that
create_from_gameobjectcurrently expects:- For the scene object reference, either pass the object itself (if that’s how the existing test does it) or pass
sceneObject.nameand adjust the key accordingly. - Add the
"searchInactive"(or your actual flag name) boolean parameter only in the success case.
- For the scene object reference, either pass the object itself (if that’s how the existing test does it) or pass
- Use the same assertion style as the other tests:
- For the failure case: either assert a specific exception is thrown or assert the returned result indicates failure.
- For the success case: assert success and that the prefab was created at
prefabPath(e.g. viaFile.Exists(prefabPath)or your existing helper).
- If your existing test uses async patterns (e.g.
await CallToolAsync(...)), update this new test to beasync Taskandawaitthe calls consistently.
|
|
||
| [Test] | ||
| public void SaveOpenStage_WithForce_SavesEvenWhenNotDirty() | ||
| public void ModifyContents_TargetsChildrenByNameAndPath() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for modify_contents parent re-parenting behavior, including the hierarchy loop guard.
modify_contents also supports a parent parameter and has an IsChildOf guard to prevent hierarchy loops, but those paths aren’t covered by tests. Please add one test that verifies a valid re-parent within the prefab succeeds, and another that verifies attempting to parent a node under one of its descendants fails with the expected error, so we cover both the normal and loop-prevention behavior.
Suggested implementation:
[Test]
public void ModifyContents_ReparentsChildWithinPrefab()
{
string prefabPath = CreateNestedTestPrefab("ReparentTest");
try
{
// Reparent a child under another sibling within the same prefab
var result = ToJObject(ManagePrefabs.HandleCommand(new JObject
{
["command"] = "modify_contents",
["asset_path"] = prefabPath,
["targets"] = new JArray
{
new JObject
{
// These names/paths assume the nested prefab created by CreateNestedTestPrefab
// exposes a root with at least two children; adjust as needed to match
["path"] = "Root/ChildA",
["parent"] = "Root/ChildB"
}
}
}));
// Verify the command succeeded (no hierarchy loop triggered)
Assert.AreEqual("ok", (string)result["status"], "Expected modify_contents to succeed for a valid reparent within the prefab.");
}
finally
{
SafeDeleteAsset(prefabPath);
}
}
[Test]
public void ModifyContents_PreventsHierarchyLoopsWhenReparenting()
{
string prefabPath = CreateNestedTestPrefab("HierarchyLoopTest");
try
{
// Attempt to parent an ancestor under one of its descendants, which should be rejected
var result = ToJObject(ManagePrefabs.HandleCommand(new JObject
{
["command"] = "modify_contents",
["asset_path"] = prefabPath,
["targets"] = new JArray
{
new JObject
{
// Attempt to parent Root under one of its descendants, e.g. Root/ChildA
// so that the underlying IsChildOf guard is exercised.
["path"] = "Root",
["parent"] = "Root/ChildA"
}
}
}));
// Verify the command failed with an appropriate error
Assert.AreEqual("error", (string)result["status"], "Expected modify_contents to fail when creating a hierarchy loop.");
StringAssert.Contains(
"hierarchy",
(string)result["message"],
"Expected error message to mention invalid hierarchy / loop prevention.");
}
finally
{
SafeDeleteAsset(prefabPath);
}
}
[Test]
public void ModifyContents_TargetsChildrenByNameAndPath()
{
string prefabPath = CreateNestedTestPrefab("TargetTest");
try
{
// Target by name
var nameResult = ToJObject(ManagePrefabs.HandleCommand(new JObject
{- The exact object paths (
"Root/ChildA","Root/ChildB","Root") must be updated to match the actual hierarchy produced byCreateNestedTestPrefab. Replace these strings with the real node paths (for example,"Parent/Child"patterns the existing tests already assert on). - The response contract from
ManagePrefabs.HandleCommandmay differ (for example, using"success"instead of"status", or naming the error field differently). Align the assertions:- Change
result["status"]to the actual field used to indicate success/failure. - Change
result["message"]and theStringAssert.Contains("hierarchy", ...)check to match the real error field name and message substring used when theIsChildOfguard triggers.
- Change
- If
CreateNestedTestPrefabdoes not create a root node named"Root"with at least two children, adjust the test to reparent one real child under another within the prefab, and for the loop test, choose an ancestor/descendant pair that will trigger theIsChildOfcheck.
dsarno
force-pushed
the
feature/prefab-headless-editing
branch
from
f400086 to
b0702af
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
Server/src/services/tools/manage_prefabs.py (1)
47-69: Back‑compat gap:create_from_gameobjectnow rejectsnameas a target alias.The server-side required-param check only accepts
target, while the Unity side still acceptsnameas a fallback. Any existing callers sendingnameonly will now fail before reaching Unity. Consider mappingname → targetfor this action before validation.🛠️ Suggested fix
) -> dict[str, Any]: + # Back-compat: accept "name" as alias for "target" in create_from_gameobject + if action == "create_from_gameobject": + if (target is None or (isinstance(target, str) and not target.strip())) and isinstance(name, str) and name.strip(): + target = name + # Validate required parameters required = REQUIRED_PARAMS.get(action, [])
Removes stage-based prefab editing (open_stage, close_stage, save_open_stage) in favor of headless modify_contents action that uses PrefabUtility.LoadPrefabContents for reliable automated workflows without UI dialogs. Changes: - Remove open_stage, close_stage, save_open_stage actions - Add modify_contents action for headless prefab editing - Support targeting objects by name or path (e.g., "Turret/Barrel") - Support transform, tag, layer, setActive, name, parent, components operations - Skip saving when no modifications made (avoids unnecessary asset writes) - Delete PrefabStage.cs resource (no longer needed) - Update Python tool description to remove "stages" reference - Consolidate tests from 29 to 14 (covers complex prefabs, reparenting, hierarchy loop guard) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
dsarno
force-pushed
the
feature/prefab-headless-editing
branch
from
b0702af to
4d10cfe
Compare
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Human speaking: The manage_prefabs tool has no reason to open the stage -- it can operate on prefabs headlessly without the stage (a very human affordance). So getting rid of the stage also helps with issues like the save dialog popping up, and worrying about marking stage files as dirty, which were interfering with clean prefab ops. So let's try this and see how it works.
Summary
open_stage,close_stage,save_open_stage) which had issues with dirty flag tracking and potential save dialogsmodify_contentsaction that usesPrefabUtility.LoadPrefabContents()for headless prefab editing - ideal for automated/agentic workflows"Turret/Barrel")PrefabStage.csresource (no longer needed)Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Replace stage-based prefab editing with a new headless modify_contents flow and update bindings/tests accordingly.
New Features:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.