Conversation
…rEachLoop alias pointing to K2Node_ForEachElementInEnum(enum-only node). Array foreach must use CallArrayFunction nodeType.Bug 2: add_variable ignores variablePinType for Object/Class types.Now reads PinSubCategoryObject and objectClass from the variablePinTypeJSON field so typed component references (e.g. AoStatGeneratorComponent)are created with proper pin types instead of generic UObject.Bug 3: K2Node_DynamicCast fails for Blueprint classes with 'Bad cast node'.ResolveUClass only finds native C++ classes. Added three-stage fallback:(1) native lookup, (2) LoadBlueprintAsset + GeneratedClass from commonpath prefixes, (3) TObjectIterator scan for _C generated class. Alsoadded K2Node_DynamicCast as accepted nodeType alias. ReconstructNodecalled after Finalize to expose the typed 'As ClassName' output pin.Bug 4: K2Node_CallArrayFunction always returns zero pins. Dynamic NewObjectfallback path does not resolve array function wildcard pins. Added explicitCallArrayFunction handler using FGraphNodeCreator<UK2Node_CallFunction>with SetFromFunction + ReconstructNode so TargetArray and ReturnValuepins are correctly exposed for all KismetArrayLibrary functions.Bug 5: connect_pins schema rejection for typed object pins. SingleTryCreateConnection call with no fallback. Added 3-attempt strategy:(1) normal direction, (2) reversed direction, (3) ReconstructNode onboth nodes then retry. Also calls ReconstructNode after success sowildcard/array pins update to the connected type. Error messages nowinclude pin type diagnostic info.get_pin_details: always report pinSubType for any pin with a validPinSubCategoryObject so cast output pins expose their target class name.
📏 Large PR DetectedThis pull request is quite large (1000+ lines changed), which can make reviewing challenging. Suggestions:
This helps reviewers provide better feedback and speeds up the merge process. Thank you! 🙏 |
📝 WalkthroughWalkthroughThe changes refactor and enhance the McpAutomationBridge plugin across multiple systems. Key modifications include removing explicit world/level cleanup steps (EndFrame calls, CleanupWorld, root removals), expanding Blueprint node creation to support dynamic casts and array functions, enhancing pin connection with retry strategies, adding typed variable pin support, simplifying level and sublevel creation workflows, strengthening widget authoring with duplicate name prevention, and removing path sanitization from asset exports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a series of critical bug fixes and usability enhancements across the Blueprint graph, level management, and widget authoring automation handlers. The primary goal is to improve the stability and correctness of automated Blueprint operations, such as node creation and pin connections, especially for complex types like objects and arrays. Additionally, it refines the handling of level creation and cleanup processes and standardizes widget generation for a more consistent and reliable automation experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully addresses several bugs in the blueprint graph handlers, notably improving support for Blueprint classes in casts, correcting array function node creation, and making pin connections more robust. However, my review has identified some critical issues in files that were not mentioned in the PR description, which may have been included accidentally. These include the removal of security-related path sanitization, and the reversion of several critical fixes related to level creation and data layers that could lead to editor crashes. I've also pointed out areas with significant code duplication that could be refactored for better maintainability.
I am having trouble creating individual review comments. Click here to see my feedback.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp (592-777)
The logic for HandleCreateSublevel has been significantly altered. The previous implementation created a new sublevel asset on disk, saved it, and then added a streaming reference. The new implementation only creates a ULevelStreamingDynamic object in memory. This appears to be a regression in functionality, as it no longer creates the sublevel asset itself, which could lead to broken references if the asset doesn't exist. The original code had a comment indicating this was a 'CRITICAL FIX'. This change should be reviewed to ensure it doesn't break the intended functionality of creating new sublevels.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp (1663-1681)
This change removes a critical check for IsUsingExternalObjects() before creating a data layer. The original comment explained that this is necessary to prevent an editor assertion/crash. Removing this check without an alternative safety mechanism is likely to reintroduce crashes when working with data layers on levels that don't have 'One File Per Actor' enabled. This safety check should be restored or replaced with an equivalent validation.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp (1831-1848)
This change removes a critical check for IsUsingExternalObjects() before assigning an actor to a data layer. The original comment explained that this is necessary because non-OFPA actors cannot be assigned to data layers. Removing this check is likely to reintroduce crashes or errors. This safety check should be restored.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_SystemControlHandlers.cpp (341-347)
The call to SanitizeProjectRelativePath for AssetPath has been removed. This function was in place to prevent path traversal attacks (e.g., using .. to access files outside the project directory). Removing this sanitization step introduces a critical security vulnerability. The sanitization should be restored to prevent malicious file access.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintGraphHandlers.cpp (745-754)
The hardcoded list of paths to search for Blueprint assets is a bit brittle. If the project's folder structure changes, this logic will fail. Consider using the Asset Registry to search for the Blueprint asset by name if the initial LoadBlueprintAsset fails. This would be more robust than relying on a fixed set of paths.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintHandlers.cpp (2766-2783)
This block of code for handling variablePinType for 'class' types is identical to the block for 'object' types (lines 2744-2764). This duplication can be avoided by extracting the logic into a helper function. A single function could resolve the PinSubCategoryObject from the payload, which would make the code more maintainable and less error-prone.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp (537-544)
The logic to generate a unique widget name by appending a suffix if the name already exists is duplicated across many of the add_* widget handlers in this file (e.g., add_canvas_panel, add_horizontal_box, etc.). This should be refactored into a helper function to improve maintainability and reduce code duplication. A single helper function could take the UWidgetTree and the base name as arguments and return a unique name.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp (561-578)
The logic for finding and validating a parent widget is duplicated across many of the add_* widget handlers. This logic, which finds the parent by name, checks if it's null, and validates that it's a UPanelWidget, could be extracted into a helper function to reduce code duplication and improve maintainability.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Load the asset | ||
| UObject* Asset = UEditorAssetLibrary::LoadAsset(SafeAssetPath); | ||
| UObject* Asset = UEditorAssetLibrary::LoadAsset(AssetPath); |
There was a problem hiding this comment.
🔴 Security: Removed SanitizeProjectRelativePath for asset path in export_asset handler
The SanitizeProjectRelativePath(AssetPath) call was completely removed from the export_asset handler. The raw, user-provided AssetPath is now passed directly to UEditorAssetLibrary::DoesAssetExist() and UEditorAssetLibrary::LoadAsset() without any sanitization. The old code validated the asset path against traversal attacks (..), absolute Windows paths, and invalid root prefixes before use. This violates the AGENTS.md mandate: SanitizeProjectRelativePath() must be used to "Block traversal attacks" (plugins/McpAutomationBridge/AGENTS.md:29).
(Refers to lines 378-393)
Prompt for agents
In plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_SystemControlHandlers.cpp, around lines 340-393, restore the SanitizeProjectRelativePath call for AssetPath before it is used. After the AssetPath.IsEmpty() check (around line 339), add:
FString SafeAssetPath = SanitizeProjectRelativePath(AssetPath);
if (SafeAssetPath.IsEmpty()) {
SendAutomationError(RequestingSocket, RequestId,
TEXT("Invalid asset path for export"),
TEXT("SECURITY_VIOLATION"));
return true;
}
Then replace all subsequent uses of AssetPath (in DoesAssetExist, LoadAsset, GetBaseFilename, and response fields) with SafeAssetPath.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Add streaming level | ||
| ULevelStreamingDynamic* StreamingLevel = NewObject<ULevelStreamingDynamic>(World, ULevelStreamingDynamic::StaticClass()); | ||
| if (!StreamingLevel) | ||
| { | ||
| Subsystem->SendAutomationResponse(Socket, RequestId, false, | ||
| FString::Printf(TEXT("Failed to create world for sublevel: %s"), *SublevelName), nullptr, TEXT("WORLD_CREATION_FAILED")); | ||
| TEXT("Failed to create streaming level object"), nullptr); | ||
| return true; | ||
| } | ||
|
|
||
| // Initialize the world if not already initialized | ||
| if (!NewSublevelWorld->bIsWorldInitialized) | ||
| { | ||
| NewSublevelWorld->InitWorld(); | ||
| } | ||
| // Configure the streaming level | ||
| StreamingLevel->SetWorldAssetByPackageName(FName(*SublevelPath)); | ||
| StreamingLevel->LevelTransform = FTransform::Identity; | ||
| StreamingLevel->SetShouldBeVisible(true); | ||
| StreamingLevel->SetShouldBeLoaded(true); | ||
|
|
||
| // Mark package dirty | ||
| SublevelPackage->MarkPackageDirty(); | ||
| // Add to world's streaming levels | ||
| World->AddStreamingLevel(StreamingLevel); |
There was a problem hiding this comment.
🔴 HandleCreateSublevel no longer creates sublevel .umap on disk — streaming reference points to nothing
The rewritten HandleCreateSublevel only creates a ULevelStreamingDynamic reference pointing to a sublevel package path, but never creates the actual sublevel world/package or saves it to disk as a .umap file. The old code (removed lines 630-756) created a UPackage, called UWorld::CreateWorld, initialized it, saved it via McpSafeLevelSave, and then created the streaming reference. Without the on-disk asset, the streaming reference points to a non-existent package, which will fail when the engine attempts to load it. The handler reports success despite the sublevel not actually existing.
Prompt for agents
In plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp, the HandleCreateSublevel function (starting around line 457) needs to actually create the sublevel on disk before creating a streaming reference. Before line 533 (where ULevelStreamingDynamic is created), restore the sublevel creation logic:
1. Create a UPackage via CreatePackage(*SublevelPath)
2. Create a UWorld via UWorld::CreateWorld(EWorldType::Inactive, false, FName(*SublevelName), Package)
3. Initialize the world if needed
4. Save it to disk via McpSafeLevelSave
5. Then create the streaming reference pointing to the saved package
6. Clean up the created world from memory (mark transient, collect garbage) to prevent World Memory Leaks
Also restore the idempotent check: before creating, check if FPackageName::DoesPackageExist(SublevelPath) and return early with success if the sublevel already exists.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Mark the world and its package as transient so GC will collect them | ||
| NewWorld->SetFlags(RF_Transient); | ||
| if (Package) | ||
| { | ||
| // Also remove package from root if needed | ||
| if (Package->IsRooted()) | ||
| { | ||
| Package->RemoveFromRoot(); | ||
| } | ||
| Package->SetFlags(RF_Transient); | ||
| } | ||
|
|
||
| // STEP 6: Force garbage collection to remove the world from memory | ||
| // Force garbage collection to remove the world from memory | ||
| // This allows the level to be cleanly loaded later via LoadMap | ||
| CollectGarbage(GARBAGE_COLLECTION_KEEPFLAGS); | ||
| FlushRenderingCommands(); |
There was a problem hiding this comment.
🔴 Removed RemoveFromRoot() makes CollectGarbage ineffective — worlds stay rooted in memory
In HandleCreateLevel's post-save cleanup, RemoveFromRoot() was removed for both the world and its package, while CollectGarbage(GARBAGE_COLLECTION_KEEPFLAGS) is still called. Rooted objects (RF_RootSet) are never collected by GC regardless of other flags like RF_Transient. The comments at McpAutomationBridge_LevelStructureHandlers.cpp:397-401 explicitly state the world "stays in memory as a root object" and causes a "World Memory Leaks" fatal error. Without RemoveFromRoot(), SetFlags(RF_Transient) + CollectGarbage has no effect on rooted objects, defeating the entire cleanup block. The same issue exists in McpAutomationBridgeHelpers.h:1383-1388 (McpSafeLoadMap Step 11) where RemoveFromRoot() was also removed for the pre-existing target world and package.
| // Mark the world and its package as transient so GC will collect them | |
| NewWorld->SetFlags(RF_Transient); | |
| if (Package) | |
| { | |
| // Also remove package from root if needed | |
| if (Package->IsRooted()) | |
| { | |
| Package->RemoveFromRoot(); | |
| } | |
| Package->SetFlags(RF_Transient); | |
| } | |
| // STEP 6: Force garbage collection to remove the world from memory | |
| // Force garbage collection to remove the world from memory | |
| // This allows the level to be cleanly loaded later via LoadMap | |
| CollectGarbage(GARBAGE_COLLECTION_KEEPFLAGS); | |
| FlushRenderingCommands(); | |
| // Mark the world and its package as transient so GC will collect them | |
| NewWorld->SetFlags(RF_Transient); | |
| if (NewWorld->IsRooted()) | |
| { | |
| NewWorld->RemoveFromRoot(); | |
| } | |
| if (Package) | |
| { | |
| if (Package->IsRooted()) | |
| { | |
| Package->RemoveFromRoot(); | |
| } | |
| Package->SetFlags(RF_Transient); | |
| } | |
| // Force garbage collection to remove the world from memory | |
| // This allows the level to be cleanly loaded later via LoadMap | |
| CollectGarbage(GARBAGE_COLLECTION_KEEPFLAGS); | |
| FlushRenderingCommands(); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Mark the world and its package for garbage collection | ||
| ExistingWorld->SetFlags(RF_Transient); | ||
| if (ExistingPackage->IsRooted()) | ||
| { | ||
| ExistingPackage->RemoveFromRoot(); | ||
| } | ||
| ExistingPackage->SetFlags(RF_Transient); | ||
|
|
||
| // Force garbage collection to clean up the existing world |
There was a problem hiding this comment.
🔴 Removed RemoveFromRoot() in McpSafeLoadMap target world cleanup — same GC failure
Same issue as in HandleCreateLevel: the McpSafeLoadMap Step 11 cleanup of a pre-existing target world package removed the RemoveFromRoot() calls for both ExistingWorld and ExistingPackage. The code at McpAutomationBridgeHelpers.h:1383-1388 sets RF_Transient and calls CollectGarbage, but without removing root flags first, the GC will not collect these objects. This means LoadMap will find the stale world in memory and trigger the "World Memory Leaks" fatal error described in the comments.
(Refers to lines 1383-1389)
Was this helpful? React with 👍 or 👎 to provide feedback.
| // because it requires a valid package path | ||
| // Pass false to allow creation of Level Blueprint if it doesn't exist | ||
| ULevelScriptBlueprint* LevelBP = PersistentLevel->GetLevelScriptBlueprint(false); | ||
| ULevelScriptBlueprint* LevelBP = PersistentLevel->GetLevelScriptBlueprint(true); |
There was a problem hiding this comment.
🟡 GetLevelScriptBlueprint(true) prevents creation of missing level blueprints
In HandleOpenLevelBlueprint and HandleAddLevelBlueprintNode, GetLevelScriptBlueprint(false) was changed to GetLevelScriptBlueprint(true). The parameter is bDontCreate — false meant "create if missing" and true means "don't create". The old code explicitly commented: "Pass false to allow creation of Level Blueprint if it doesn't exist". The new code will return nullptr for any level that doesn't yet have a level blueprint, causing both handlers to fail with an error instead of creating one. The new comment on line 1872 ("may fail to create") is also inaccurate — true means it won't attempt creation at all.
| ULevelScriptBlueprint* LevelBP = PersistentLevel->GetLevelScriptBlueprint(true); | |
| ULevelScriptBlueprint* LevelBP = PersistentLevel->GetLevelScriptBlueprint(false); |
Was this helpful? React with 👍 or 👎 to provide feedback.
![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 (3)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_SystemControlHandlers.cpp (1)
328-339:⚠️ Potential issue | 🔴 CriticalCritical: Missing path sanitization for
AssetPathenables directory traversal attacks.The
AssetPathparameter is used directly from user input without sanitization, whileExportPathis properly sanitized. This inconsistency creates a security vulnerability where malicious input like"/Game/../../../SensitiveAsset"could bypass intended access controls.All sibling handlers (LevelHandlers, TextureHandlers, SkeletonHandlers) use
SanitizeProjectRelativePath()before loading assets. This handler should follow the same pattern.🔒 Proposed fix to add path sanitization
if (AssetPath.IsEmpty()) { SendAutomationError(RequestingSocket, RequestId, TEXT("assetPath is required for export"), TEXT("INVALID_ARGUMENT")); return true; } + + FString SafeAssetPath = SanitizeProjectRelativePath(AssetPath); + if (SafeAssetPath.IsEmpty()) { + SendAutomationError(RequestingSocket, RequestId, + FString::Printf(TEXT("Invalid assetPath: contains path traversal (..) or invalid characters: %s"), *AssetPath), + TEXT("SECURITY_VIOLATION")); + return true; + } + AssetPath = SafeAssetPath; if (ExportPath.IsEmpty()) {As per coding guidelines: "Use
SanitizeProjectRelativePath()to validate and sanitize all file paths to prevent directory traversal attacks".Also applies to: 378-378, 393-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_SystemControlHandlers.cpp` around lines 328 - 339, AssetPath is used directly from user input causing possible directory traversal; call SanitizeProjectRelativePath(AssetPath) (same as done for ExportPath and in LevelHandlers/TextureHandlers/SkeletonHandlers) immediately after reading AssetPath, validate the result and if sanitization fails call SendAutomationError(RequestingSocket, RequestId, TEXT("invalid assetPath"), TEXT("INVALID_ARGUMENT")) and return true; apply the same SanitizeProjectRelativePath check for the other AssetPath usages referenced (around the other handlers/lines noted) so all asset loads use the sanitized path.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp (2)
5688-5723:⚠️ Potential issue | 🟠 MajorDon't silently fall back when
parentSlotis invalid.These branches still treat a bad
parentSlotas “attach to root instead” and return success. That reintroduces the silent mis-parenting bug the earlier handlers just fixed, and ifRootWidgetis null or not a panel the request can even succeed without attaching anything. These should mirror the earlierPARENT_NOT_FOUND/INVALID_PARENTbehavior.Also applies to: 5738-5776, 5793-5830
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp` around lines 5688 - 5723, When a non-empty ParentSlot is supplied, do not silently fall back to RootWidget; detect the two error cases and return failures instead: if ParentSlot is provided but FindWidget(FName(*ParentSlot)) returns null, call SendAutomationError(RequestingSocket, RequestId, TEXT("Parent slot not found"), TEXT("PARENT_NOT_FOUND")) and return true; if the found widget or the RootWidget is not a UPanelWidget (i.e., Cast<UPanelWidget> yields null) call SendAutomationError(RequestingSocket, RequestId, TEXT("Invalid parent: not a panel widget"), TEXT("INVALID_PARENT")) and return true; only call Parent->AddChild(SafeZone) when Parent is valid. Apply the same checks and error returns for the other similar blocks referenced (lines ~5738-5776 and ~5793-5830) that use ParentSlot, WidgetBP->WidgetTree->RootWidget, and Parent->AddChild.
4921-4954:⚠️ Potential issue | 🟠 MajorThe later composite adders still skip duplicate-name hardening.
add_minimapnow acceptsname, but it still constructsSlotName,SlotName_Border,SlotName_MapImage, etc. directly. A second request with the same/default name can still collide or be auto-renamed unpredictably, which undoes the uniqueness guarantee added above. The same pattern repeats inadd_compass,add_interaction_prompt,add_objective_tracker,add_damage_indicator, andadd_quest_tracker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp` around lines 4921 - 4954, The code constructs widgets using raw names (SlotName, SlotName + "_Border", "_MapImage", "_PlayerIndicator") which can collide on repeated add_* calls; update the handler to derive a unique base name before constructing child widgets (e.g., call or add an EnsureUniqueSlotName helper that checks WidgetBP->WidgetTree for existing widgets and appends a numeric suffix until unique), then use that returned unique base for MinimapContainer, MinimapBorder, MapImage and PlayerIndicator creation; apply the same fix pattern to the other composite creators (add_compass, add_interaction_prompt, add_objective_tracker, add_damage_indicator, add_quest_tracker) so all constructed names are passed through the same uniqueness function.
🧹 Nitpick comments (4)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp (1)
548-549: Consider adding idempotency check before adding streaming level.Unlike
HandleCreateLevelwhich checks for existing packages,HandleCreateSubleveldoesn't check if a streaming level with the sameWorldAssetPackageNamealready exists. Repeated calls could add duplicate streaming level entries.🔧 Proposed idempotency check
+ // Check if streaming level already exists (idempotency) + for (ULevelStreaming* ExistingLevel : World->GetStreamingLevels()) + { + if (ExistingLevel && ExistingLevel->GetWorldAssetPackageFName().ToString() == SublevelPath) + { + TSharedPtr<FJsonObject> ResponseJson = McpHandlerUtils::CreateResultObject(); + McpHandlerUtils::AddVerification(ResponseJson, ExistingLevel); + ResponseJson->SetStringField(TEXT("sublevelName"), SublevelName); + ResponseJson->SetStringField(TEXT("parentLevel"), World->GetMapName()); + ResponseJson->SetBoolField(TEXT("alreadyExisted"), true); + + Subsystem->SendAutomationResponse(Socket, RequestId, true, + FString::Printf(TEXT("Streaming level already exists: %s"), *SublevelName), ResponseJson); + return true; + } + } + // Add to world's streaming levels World->AddStreamingLevel(StreamingLevel);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp` around lines 548 - 549, HandleCreateSublevel is adding StreamingLevel unconditionally which can create duplicate entries; before calling World->AddStreamingLevel(StreamingLevel) check World’s existing streaming levels (World->GetStreamingLevels() or World->StreamingLevels) for an entry whose WorldAssetPackageName (or equivalent package name getter on the streaming level) matches the new StreamingLevel’s WorldAssetPackageName and skip adding if found. Ensure you compare the same identifier used elsewhere (WorldAssetPackageName) and only call AddStreamingLevel when no existing streaming level with that package name exists.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintGraphHandlers.cpp (2)
750-754: Hardcoded project-specific paths reduce portability.These path prefixes (
/Game/Blueprints/Characters/,/Game/Blueprints/Combat/) appear to be specific to the tactical RPG project mentioned in the PR description. Other projects using this plugin will have different directory structures.Consider making these configurable or removing the project-specific prefixes, keeping only the generic ones:
♻️ Suggested simplification
TArray<FString> PathsToTry; if (TargetClassName.Contains(TEXT("/"))) { PathsToTry.Add(TargetClassName); } else { PathsToTry.Add(FString::Printf(TEXT("/Game/Blueprints/%s"), *TargetClassName)); PathsToTry.Add(FString::Printf(TEXT("/Game/%s"), *TargetClassName)); - PathsToTry.Add(FString::Printf(TEXT("/Game/Blueprints/Characters/%s"), *TargetClassName)); - PathsToTry.Add(FString::Printf(TEXT("/Game/Blueprints/Combat/%s"), *TargetClassName)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintGraphHandlers.cpp` around lines 750 - 754, The hardcoded project-specific path prefixes added to PathsToTry (e.g., "/Game/Blueprints/Characters/%s" and "/Game/Blueprints/Combat/%s") reduce portability; modify the logic in McpAutomationBridge_BlueprintGraphHandlers.cpp where PathsToTry.Add(...) is called so it either only includes generic paths ("/Game/Blueprints/%s" and "/Game/%s") or reads additional prefixes from a configurable source (editor setting, config file, or optional parameter) before formatting with TargetClassName; update the code that builds PathsToTry to use the configurable list (or remove the specific entries) and ensure TargetClassName is still used for formatting.
744-762: Consider sanitizingtargetClassinput before path construction.The
TargetClassNameoriginates from user input (line 734) and is used to construct asset paths forLoadBlueprintAssetwithout sanitization. While the/Game/prefix limits risk, this is inconsistent with the security pattern applied toassetPath/blueprintPathinputs elsewhere in this file (lines 151-168, 209-216).🛡️ Optional: Add sanitization for consistency
+ // Sanitize class name to prevent path manipulation + FString SanitizedClassName = TargetClassName; + SanitizedClassName.ReplaceInline(TEXT(".."), TEXT("")); + SanitizedClassName.ReplaceInline(TEXT("//"), TEXT("/")); + // 2. If that failed, try loading it as a Blueprint asset and using its // GeneratedClass. This is required for Blueprint targets like // "BP_CharacterBase" which resolve to "BP_CharacterBase_C". if (!TargetClass) { // Try common path prefixes if no slash is present TArray<FString> PathsToTry; - if (TargetClassName.Contains(TEXT("/"))) { - PathsToTry.Add(TargetClassName); + if (SanitizedClassName.Contains(TEXT("/"))) { + PathsToTry.Add(SanitizedClassName); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintGraphHandlers.cpp` around lines 744 - 762, Sanitize TargetClassName before constructing PathsToTry: validate and normalize the user-provided TargetClassName (trim whitespace, strip any leading/trailing slashes or dots, remove extensions, and reject or remove any path traversal tokens like ".." or drive/URI characters) prior to using it to build the FStrings and calling LoadBlueprintAsset; update the block that assembles PathsToTry (the code using TargetClassName, TargetClass, and LoadBlueprintAsset in McpAutomationBridge_BlueprintGraphHandlers.cpp) to operate on the sanitized string and bail with a clear error if the name contains disallowed characters.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintHandlers.cpp (1)
2749-2754: UseFJsonObjectConverter+ shared parsing helper forvariablePinTypeThe same manual JSON extraction is duplicated in both branches. Deserialize
variablePinTypeonce into a small struct and reuse forobject/classhandling.As per coding guidelines: "Use
FJsonObjectConverterfor JSON parsing and struct serialization instead of manual JSON parsing".Also applies to: 2768-2773
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintHandlers.cpp` around lines 2749 - 2754, The manual JSON extraction for variablePinType is duplicated; replace it by defining a small struct (e.g., FVariablePinType with fields PinSubCategoryObject and objectClass) and use FJsonObjectConverter::JsonObjectToUStruct to deserialize (*PinTypeObj) once (where PinTypeObj is used in the current branch and the other branch around the similar block at the other occurrence). After deserializing, use the struct's fields to pick the subclass name (prefer PinSubCategoryObject then objectClass) and proceed with the existing object/class handling logic (update usages in the code paths that previously read PinSubCategoryObject/objectClass manually).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintGraphHandlers.cpp`:
- Around line 846-848: The node position is being assigned after
NodeCreator.Finalize() (CallFuncNode->NodePosX and CallFuncNode->NodePosY),
which violates the established FGraphNodeCreator pattern; move the position
assignments to before calling NodeCreator.Finalize() so the node's
NodePosX/NodePosY are set prior to finalization (follow the same approach as the
FinalizeAndReport lambda that sets position before Finalize()).
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintHandlers.cpp`:
- Around line 2755-2763: The code silently falls back to UObject::StaticClass()
when ResolveUClass() fails for an explicitly supplied subtype (variablePinType's
PinSubCategoryObject or objectClass); change this so that if
ResolveUClass(SubClassName) or ResolveUClass(objectClass) returns null AND the
caller explicitly provided that subtype, call SendAutomationError(...) and
return failure instead of assigning UObject::StaticClass(); ensure both the
PinSubCategoryObject branch and the objectClass branch use the same fail-fast
behavior and reference variablePinType/PinType when deciding if the subtype was
explicitly provided.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp`:
- Around line 533-549: The new code creates a ULevelStreamingDynamic
(StreamingLevel) and calls SetWorldAssetByPackageName(FName(*SublevelPath)) then
adds it to the world via World->AddStreamingLevel(StreamingLevel) but does not
ensure the sublevel asset actually exists on disk; restore or add the sublevel
creation step (e.g., call the existing McpSafeLevelSave or equivalent to write
the .umap before creating/adding the streaming reference), or rename this
handler to explicitly be an "add_streaming_level_reference" and require callers
to create the asset, and update SetWorldAssetByPackageName usage and any caller
documentation accordingly so the streaming level doesn't point to a non-existent
package.
- Around line 1938-1943: The error text is misleading because
GetLevelScriptBlueprint(true) attempts to create the blueprint; update the
failure message used in the block that checks ULevelScriptBlueprint* LevelBP =
CurrentLevel->GetLevelScriptBlueprint(true) so it reads "Failed to get or create
Level Blueprint" (matching the behavior and the message used by
HandleOpenLevelBlueprint) by replacing the TEXT("Failed to get Level Blueprint")
passed to Subsystem->SendAutomationResponse with TEXT("Failed to get or create
Level Blueprint").
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp`:
- Around line 676-681: The inline parsing for slot name and visibility drops the
shared visibility mapping (losing SelfHitTestInvisible) and duplicates parsing
logic; replace the manual visibility parse with the centralized
GetVisibility(JsonObject/Payload) call and stop hand-parsing visibility in
McpAutomationBridge_WidgetAuthoringHandlers.cpp (the current block using
GetJsonStringField for "name"/"slotName" and the similar logic at lines
~705-717). Also follow the guideline to use FJsonObjectConverter for
JSON->struct parsing instead of piecemeal GetJsonStringField usage—locate usages
of GetJsonStringField, SlotName, and the ad-hoc visibility handling and switch
them to GetVisibility(Payload) and FJsonObjectConverter-based struct
deserialization to keep behavior consistent and avoid losing
SelfHitTestInvisible.
- Around line 537-545: The code builds a UniqueName when a name collision occurs
but continues to report the original SlotName; change the response payload to
use the resolved UniqueName (the final value after the while loop) wherever the
created widget is referenced so downstream automation targets the real widget
(update the place that assembles the response to use UniqueName instead of
SlotName). Specifically, after the collision loop that uses
WidgetBP->WidgetTree->FindWidget and before/after ConstructWidget (e.g.,
UCanvasPanel* CanvasPanel = ... FName(*UniqueName)), ensure the created widget
name returned/serialized is UniqueName; apply the same change to the other add_*
handlers that perform the same suffixing logic.
---
Outside diff comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_SystemControlHandlers.cpp`:
- Around line 328-339: AssetPath is used directly from user input causing
possible directory traversal; call SanitizeProjectRelativePath(AssetPath) (same
as done for ExportPath and in LevelHandlers/TextureHandlers/SkeletonHandlers)
immediately after reading AssetPath, validate the result and if sanitization
fails call SendAutomationError(RequestingSocket, RequestId, TEXT("invalid
assetPath"), TEXT("INVALID_ARGUMENT")) and return true; apply the same
SanitizeProjectRelativePath check for the other AssetPath usages referenced
(around the other handlers/lines noted) so all asset loads use the sanitized
path.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp`:
- Around line 5688-5723: When a non-empty ParentSlot is supplied, do not
silently fall back to RootWidget; detect the two error cases and return failures
instead: if ParentSlot is provided but FindWidget(FName(*ParentSlot)) returns
null, call SendAutomationError(RequestingSocket, RequestId, TEXT("Parent slot
not found"), TEXT("PARENT_NOT_FOUND")) and return true; if the found widget or
the RootWidget is not a UPanelWidget (i.e., Cast<UPanelWidget> yields null) call
SendAutomationError(RequestingSocket, RequestId, TEXT("Invalid parent: not a
panel widget"), TEXT("INVALID_PARENT")) and return true; only call
Parent->AddChild(SafeZone) when Parent is valid. Apply the same checks and error
returns for the other similar blocks referenced (lines ~5738-5776 and
~5793-5830) that use ParentSlot, WidgetBP->WidgetTree->RootWidget, and
Parent->AddChild.
- Around line 4921-4954: The code constructs widgets using raw names (SlotName,
SlotName + "_Border", "_MapImage", "_PlayerIndicator") which can collide on
repeated add_* calls; update the handler to derive a unique base name before
constructing child widgets (e.g., call or add an EnsureUniqueSlotName helper
that checks WidgetBP->WidgetTree for existing widgets and appends a numeric
suffix until unique), then use that returned unique base for MinimapContainer,
MinimapBorder, MapImage and PlayerIndicator creation; apply the same fix pattern
to the other composite creators (add_compass, add_interaction_prompt,
add_objective_tracker, add_damage_indicator, add_quest_tracker) so all
constructed names are passed through the same uniqueness function.
---
Nitpick comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintGraphHandlers.cpp`:
- Around line 750-754: The hardcoded project-specific path prefixes added to
PathsToTry (e.g., "/Game/Blueprints/Characters/%s" and
"/Game/Blueprints/Combat/%s") reduce portability; modify the logic in
McpAutomationBridge_BlueprintGraphHandlers.cpp where PathsToTry.Add(...) is
called so it either only includes generic paths ("/Game/Blueprints/%s" and
"/Game/%s") or reads additional prefixes from a configurable source (editor
setting, config file, or optional parameter) before formatting with
TargetClassName; update the code that builds PathsToTry to use the configurable
list (or remove the specific entries) and ensure TargetClassName is still used
for formatting.
- Around line 744-762: Sanitize TargetClassName before constructing PathsToTry:
validate and normalize the user-provided TargetClassName (trim whitespace, strip
any leading/trailing slashes or dots, remove extensions, and reject or remove
any path traversal tokens like ".." or drive/URI characters) prior to using it
to build the FStrings and calling LoadBlueprintAsset; update the block that
assembles PathsToTry (the code using TargetClassName, TargetClass, and
LoadBlueprintAsset in McpAutomationBridge_BlueprintGraphHandlers.cpp) to operate
on the sanitized string and bail with a clear error if the name contains
disallowed characters.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintHandlers.cpp`:
- Around line 2749-2754: The manual JSON extraction for variablePinType is
duplicated; replace it by defining a small struct (e.g., FVariablePinType with
fields PinSubCategoryObject and objectClass) and use
FJsonObjectConverter::JsonObjectToUStruct to deserialize (*PinTypeObj) once
(where PinTypeObj is used in the current branch and the other branch around the
similar block at the other occurrence). After deserializing, use the struct's
fields to pick the subclass name (prefer PinSubCategoryObject then objectClass)
and proceed with the existing object/class handling logic (update usages in the
code paths that previously read PinSubCategoryObject/objectClass manually).
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp`:
- Around line 548-549: HandleCreateSublevel is adding StreamingLevel
unconditionally which can create duplicate entries; before calling
World->AddStreamingLevel(StreamingLevel) check World’s existing streaming levels
(World->GetStreamingLevels() or World->StreamingLevels) for an entry whose
WorldAssetPackageName (or equivalent package name getter on the streaming level)
matches the new StreamingLevel’s WorldAssetPackageName and skip adding if found.
Ensure you compare the same identifier used elsewhere (WorldAssetPackageName)
and only call AddStreamingLevel when no existing streaming level with that
package name exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efc73c0f-3d3e-48bc-a890-50616f0b8278
📒 Files selected for processing (8)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeHelpers.hplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintGraphHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_SystemControlHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpHandlerUtils.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpSafeOperations.h
💤 Files with no reviewable changes (2)
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeHelpers.h
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpSafeOperations.h
| NodeCreator.Finalize(); | ||
| CallFuncNode->NodePosX = X; | ||
| CallFuncNode->NodePosY = Y; |
There was a problem hiding this comment.
Position set after Finalize() is inconsistent with the established pattern.
The FinalizeAndReport lambda (lines 335-342) explicitly sets position before Finalize() with the comment "Set position BEFORE finalization per FGraphNodeCreator pattern". However, this handler sets position after finalization.
While this may work functionally for UK2Node_CallFunction, it deviates from the documented pattern and could cause subtle issues with node positioning in the graph editor.
🔧 Proposed fix to match established pattern
FGraphNodeCreator<UK2Node_CallFunction> NodeCreator(*TargetGraph);
UK2Node_CallFunction *CallFuncNode = NodeCreator.CreateNode(false);
CallFuncNode->SetFromFunction(ArrayFunc);
- // Finalize allocates default pins (wildcard at this point)
- NodeCreator.Finalize();
+ // Set position BEFORE finalization per FGraphNodeCreator pattern
CallFuncNode->NodePosX = X;
CallFuncNode->NodePosY = Y;
+ // Finalize allocates default pins (wildcard at this point)
+ NodeCreator.Finalize();
// ReconstructNode forces pin re-evaluation which is required for array
// function nodes so that TargetArray and ReturnValue pins appear correctly.
CallFuncNode->ReconstructNode();📝 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.
| NodeCreator.Finalize(); | |
| CallFuncNode->NodePosX = X; | |
| CallFuncNode->NodePosY = Y; | |
| FGraphNodeCreator<UK2Node_CallFunction> NodeCreator(*TargetGraph); | |
| UK2Node_CallFunction *CallFuncNode = NodeCreator.CreateNode(false); | |
| CallFuncNode->SetFromFunction(ArrayFunc); | |
| // Set position BEFORE finalization per FGraphNodeCreator pattern | |
| CallFuncNode->NodePosX = X; | |
| CallFuncNode->NodePosY = Y; | |
| // Finalize allocates default pins (wildcard at this point) | |
| NodeCreator.Finalize(); | |
| // ReconstructNode forces pin re-evaluation which is required for array | |
| // function nodes so that TargetArray and ReturnValue pins appear correctly. | |
| CallFuncNode->ReconstructNode(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintGraphHandlers.cpp`
around lines 846 - 848, The node position is being assigned after
NodeCreator.Finalize() (CallFuncNode->NodePosX and CallFuncNode->NodePosY),
which violates the established FGraphNodeCreator pattern; move the position
assignments to before calling NodeCreator.Finalize() so the node's
NodePosX/NodePosY are set prior to finalization (follow the same approach as the
FinalizeAndReport lambda that sets position before Finalize()).
| if (!SubClassName.IsEmpty()) { | ||
| if (UClass *SubClass = ResolveUClass(SubClassName)) { | ||
| PinType.PinSubCategoryObject = SubClass; | ||
| } | ||
| } | ||
| } | ||
| if (!PinType.PinSubCategoryObject.IsValid()) { | ||
| PinType.PinSubCategoryObject = UObject::StaticClass(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current subtype resolution + fallback behavior in add_variable branch
rg -n -C6 'variablePinType|PinSubCategoryObject|objectClass|ResolveUClass|UObject::StaticClass' \
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintHandlers.cpp
# Verify ResolveUClass capabilities/limits used by this handler
rg -n -C10 'static inline UClass \*ResolveUClass|FindObject<UClass>|LoadObject<UClass>|TObjectIterator<UClass>' \
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeHelpers.hRepository: ChiR24/Unreal_mcp
Length of output: 8217
Fail fast when an explicit variablePinType subtype cannot be resolved
When caller supplies a variablePinType with a PinSubCategoryObject or objectClass field (lines 2755–2758 and 2774–2777), the code attempts to resolve it via ResolveUClass(). On resolution failure, both the "object" and "class" type handlers silently default to UObject::StaticClass() rather than failing. This is inconsistent with the main type resolution path (lines 2790–2793), which sends a SendAutomationError() when the primary type cannot be resolved. An explicit but unresolvable subtype should not silently succeed with an incorrect, overly broad type; instead, return an error to the caller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_BlueprintHandlers.cpp`
around lines 2755 - 2763, The code silently falls back to UObject::StaticClass()
when ResolveUClass() fails for an explicitly supplied subtype (variablePinType's
PinSubCategoryObject or objectClass); change this so that if
ResolveUClass(SubClassName) or ResolveUClass(objectClass) returns null AND the
caller explicitly provided that subtype, call SendAutomationError(...) and
return failure instead of assigning UObject::StaticClass(); ensure both the
PinSubCategoryObject branch and the objectClass branch use the same fail-fast
behavior and reference variablePinType/PinType when deciding if the subtype was
explicitly provided.
| // Add streaming level | ||
| ULevelStreamingDynamic* StreamingLevel = NewObject<ULevelStreamingDynamic>(World, ULevelStreamingDynamic::StaticClass()); | ||
| if (!StreamingLevel) | ||
| { | ||
| Subsystem->SendAutomationResponse(Socket, RequestId, false, | ||
| FString::Printf(TEXT("Failed to create world for sublevel: %s"), *SublevelName), nullptr, TEXT("WORLD_CREATION_FAILED")); | ||
| TEXT("Failed to create streaming level object"), nullptr); | ||
| return true; | ||
| } | ||
|
|
||
| // Initialize the world if not already initialized | ||
| if (!NewSublevelWorld->bIsWorldInitialized) | ||
| { | ||
| NewSublevelWorld->InitWorld(); | ||
| } | ||
| // Configure the streaming level | ||
| StreamingLevel->SetWorldAssetByPackageName(FName(*SublevelPath)); | ||
| StreamingLevel->LevelTransform = FTransform::Identity; | ||
| StreamingLevel->SetShouldBeVisible(true); | ||
| StreamingLevel->SetShouldBeLoaded(true); | ||
|
|
||
| // Mark package dirty | ||
| SublevelPackage->MarkPackageDirty(); | ||
| // Add to world's streaming levels | ||
| World->AddStreamingLevel(StreamingLevel); |
There was a problem hiding this comment.
Streaming level references a non-existent sublevel asset.
The new implementation creates a ULevelStreamingDynamic reference pointing to SublevelPath, but no actual sublevel .umap file is created on disk. When the streaming level attempts to load, it will fail because the referenced package doesn't exist.
Previous behavior (per AI summary) created the sublevel file via McpSafeLevelSave. The current code only adds a streaming reference to a path that may not exist.
Consider either:
- Restoring sublevel file creation before adding the streaming reference, or
- Renaming this action to
add_streaming_level_referenceand adding a separatecreate_sublevelthat creates the actual sublevel file, or - Documenting that
sublevelPathmust point to an existing level asset
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp`
around lines 533 - 549, The new code creates a ULevelStreamingDynamic
(StreamingLevel) and calls SetWorldAssetByPackageName(FName(*SublevelPath)) then
adds it to the world via World->AddStreamingLevel(StreamingLevel) but does not
ensure the sublevel asset actually exists on disk; restore or add the sublevel
creation step (e.g., call the existing McpSafeLevelSave or equivalent to write
the .umap before creating/adding the streaming reference), or rename this
handler to explicitly be an "add_streaming_level_reference" and require callers
to create the asset, and update SetWorldAssetByPackageName usage and any caller
documentation accordingly so the streaming level doesn't point to a non-existent
package.
| ULevelScriptBlueprint* LevelBP = CurrentLevel->GetLevelScriptBlueprint(true); | ||
| if (!LevelBP) | ||
| { | ||
| Subsystem->SendAutomationResponse(Socket, RequestId, false, | ||
| TEXT("Failed to get or create Level Blueprint"), nullptr); | ||
| TEXT("Failed to get Level Blueprint"), nullptr); | ||
| return true; |
There was a problem hiding this comment.
Error message inconsistent with behavior.
Line 1938 calls GetLevelScriptBlueprint(true) which attempts to create the blueprint if it doesn't exist. However, line 1942 says "Failed to get Level Blueprint" - this should say "Failed to get or create Level Blueprint" to match the behavior (and to be consistent with HandleOpenLevelBlueprint at line 1885).
📝 Proposed fix
if (!LevelBP)
{
Subsystem->SendAutomationResponse(Socket, RequestId, false,
- TEXT("Failed to get Level Blueprint"), nullptr);
+ TEXT("Failed to get or create Level Blueprint"), nullptr);
return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cpp`
around lines 1938 - 1943, The error text is misleading because
GetLevelScriptBlueprint(true) attempts to create the blueprint; update the
failure message used in the block that checks ULevelScriptBlueprint* LevelBP =
CurrentLevel->GetLevelScriptBlueprint(true) so it reads "Failed to get or create
Level Blueprint" (matching the behavior and the message used by
HandleOpenLevelBlueprint) by replacing the TEXT("Failed to get Level Blueprint")
passed to Subsystem->SendAutomationResponse with TEXT("Failed to get or create
Level Blueprint").
| FString UniqueName = SlotName; | ||
| { | ||
| int32 Suffix = 1; | ||
| while (WidgetBP->WidgetTree->FindWidget(FName(*UniqueName)) != nullptr) | ||
| { | ||
| UniqueName = FString::Printf(TEXT("%s_%d"), *SlotName, Suffix++); | ||
| } | ||
| } | ||
| UCanvasPanel* CanvasPanel = WidgetBP->WidgetTree->ConstructWidget<UCanvasPanel>(UCanvasPanel::StaticClass(), FName(*UniqueName)); |
There was a problem hiding this comment.
Return the actual widget name after collision handling.
When a duplicate exists, this branch creates UniqueName but still reports the original slotName. Any follow-up automation that uses the response payload will then target a widget that was never created. The same mismatch is repeated in the other add_* handlers that now suffix duplicates.
💡 Suggested fix
- ResultJson->SetStringField(TEXT("slotName"), SlotName);
+ ResultJson->SetStringField(TEXT("slotName"), UniqueName);Also applies to: 584-585
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp`
around lines 537 - 545, The code builds a UniqueName when a name collision
occurs but continues to report the original SlotName; change the response
payload to use the resolved UniqueName (the final value after the while loop)
wherever the created widget is referenced so downstream automation targets the
real widget (update the place that assembles the response to use UniqueName
instead of SlotName). Specifically, after the collision loop that uses
WidgetBP->WidgetTree->FindWidget and before/after ConstructWidget (e.g.,
UCanvasPanel* CanvasPanel = ... FName(*UniqueName)), ensure the created widget
name returned/serialized is UniqueName; apply the same change to the other add_*
handlers that perform the same suffixing logic.
| // Accept "name" (tool schema field) or "slotName" (legacy), fall back to type default | ||
| FString SlotName = GetJsonStringField(Payload, TEXT("name")); | ||
| if (SlotName.IsEmpty()) | ||
| { | ||
| SlotName = GetJsonStringField(Payload, TEXT("slotName"), TEXT("VerticalBox")); | ||
| } |
There was a problem hiding this comment.
Use the shared visibility mapping here.
This inline parser drops SelfHitTestInvisible, so valid input now becomes Visible. It also adds another hand-maintained payload branch in a file that already has a lot of parsing drift; GetVisibility() keeps the behavior centralized.
💡 Suggested fix
- FString VisibilityStr = GetJsonStringField(Payload, TEXT("visibility"));
- if (!VisibilityStr.IsEmpty())
- {
- if (VisibilityStr.Equals(TEXT("Collapsed"), ESearchCase::IgnoreCase))
- VBox->SetVisibility(ESlateVisibility::Collapsed);
- else if (VisibilityStr.Equals(TEXT("Hidden"), ESearchCase::IgnoreCase))
- VBox->SetVisibility(ESlateVisibility::Hidden);
- else if (VisibilityStr.Equals(TEXT("HitTestInvisible"), ESearchCase::IgnoreCase))
- VBox->SetVisibility(ESlateVisibility::HitTestInvisible);
- else
- VBox->SetVisibility(ESlateVisibility::Visible);
- }
+ const FString VisibilityStr = GetJsonStringField(Payload, TEXT("visibility"));
+ if (!VisibilityStr.IsEmpty())
+ {
+ VBox->SetVisibility(GetVisibility(VisibilityStr));
+ }Also applies to: 705-717
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp`
around lines 676 - 681, The inline parsing for slot name and visibility drops
the shared visibility mapping (losing SelfHitTestInvisible) and duplicates
parsing logic; replace the manual visibility parse with the centralized
GetVisibility(JsonObject/Payload) call and stop hand-parsing visibility in
McpAutomationBridge_WidgetAuthoringHandlers.cpp (the current block using
GetJsonStringField for "name"/"slotName" and the similar logic at lines
~705-717). Also follow the guideline to use FJsonObjectConverter for
JSON->struct parsing instead of piecemeal GetJsonStringField usage—locate usages
of GetJsonStringField, SlotName, and the ad-hoc visibility handling and switch
them to GetVisibility(Payload) and FJsonObjectConverter-based struct
deserialization to keep behavior consistent and avoid losing
SelfHitTestInvisible.
🔴 Request ChangesThanks for tackling these Blueprint bugs! A couple of issues need fixing. Must Fix1. OFPA Check Removal — Will Crash on UE 5.4+
check(GetLevel()->IsUsingExternalObjects());This assertion exists in UE 5.4 through 5.7. Your removed validation was preventing this crash. Creating data layers on non-OFPA levels will now hit this engine check and crash. Please restore the OFPA validation. 2. Wrong Node Class for Array Functions Your PR uses This explains the zero-pins bug you're trying to fix. Use Need Clarification3. World cleanup code removed — The 4. Sublevel creation — The new code only adds a streaming reference but doesn't create the What Looks Good ✅
Summary: Items 1-2 will cause crashes/bugs. Please fix before merging. Happy to discuss! |
Claude claims it was a bug in the original repo. |
|
You're right that The PR diff shows this guard being removed from // CRITICAL: Check if the level uses External Objects (OFPA)
// Data Layer instances require OFPA to be enabled, otherwise AddDataLayerInstance()
// will hit an assertion: "GetLevel()->IsUsingExternalObjects()"
if (!PersistentLevel || !PersistentLevel->IsUsingExternalObjects())
{
// ... returns error before calling CreateDataLayerInstance ...
}A few clarifications:
The guard should stay. |
2 participants
Summary
Fixes 5 bugs discovered during Blueprint automation wiring of a project.
Bugs Fixed
Bug 1 — ForEachLoop alias pointed to wrong node
ForEachLoopwas aliased toK2Node_ForEachElementInEnum(enum-only, no array pins).Removed the incorrect alias. Array iteration should use
nodeType: K2Node_CallArrayFunction.Bug 2 — add_variable ignores variablePinType for Object type
When
variableTypeis"Object"or"Class", the handler always setPinSubCategoryObject = UObject::StaticClass()regardless of thevariablePinTypefield.Now reads
PinSubCategoryObjectorobjectClassfromvariablePinTypeJSON to produceproperly typed component references.
Bug 3 — K2Node_DynamicCast fails for Blueprint classes
ResolveUClassonly finds native C++ classes, so Blueprint targets likeBP_CharacterBasealways failed, producing "Bad cast node". Added three-stage fallback: native lookup →
LoadBlueprintAsset+GeneratedClass→TObjectIteratorscan. AddedK2Node_DynamicCastas accepted
nodeType.ReconstructNodenow called afterFinalizeto expose the typedAs ClassNameoutput pin.Bug 4 — K2Node_CallArrayFunction always returns zero pins
The dynamic
NewObjectfallback doesn't resolve array function wildcard pins. Added anexplicit
CallArrayFunctionhandler usingFGraphNodeCreator<UK2Node_CallFunction>withSetFromFunction+ReconstructNode. AllKismetArrayLibraryfunctions now expose theirpins correctly (
Array_Length,Array_Get, etc.).Bug 5 — connect_pins schema rejection for typed object pins
Single
TryCreateConnectioncall with no fallback. Replaced with a 3-attempt strategy:normal direction → reversed direction →
ReconstructNodeboth nodes and retry.ReconstructNodeis also called on success so wildcard pins propagate their new type.Error messages now include pin type diagnostic info.
Bonus — get_pin_details missing pinSubType
pinSubTypewas only reported inget_nodes, notget_pin_details. Fixed to alwaysreport
pinSubTypefor any pin with a validPinSubCategoryObject.Files Changed
McpAutomationBridge/Private/McpAutomationBridge_BlueprintGraphHandlers.cppMcpAutomationBridge/Private/McpAutomationBridge_BlueprintHandlers.cpp