feat: implement Phase 34 Editor Utilities#468
Open
Jbrandan wants to merge 5 commits into
Open
Conversation
github-actions
Bot
added
area/tools
area/server
area/docs
area/testing
area/plugin
size/xl
labels
Contributor
📏 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! 🙏 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces the Data & Persistence phase (Phase 31) and Phase 34 control extensions. It adds canonical ChangesData Persistence & Project Settings Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Jbrandan
changed the title
Jbrandan
changed the title
![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: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
patch_cpp_feedback.py-4-6 (1)
4-6: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse portable path construction across
patch_cpp_feedback.py,patch_handlers.py, andpatch_ts_handlers.py. All three scripts hardcode Windows-style relative paths, so they fail on POSIX before any patching runs. The shared root cause is path construction from raw backslash-separated strings instead of normalized path components.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch_cpp_feedback.py` around lines 4 - 6, The scripts currently build repo_root and file paths using raw backslash strings (see repo_root and handlers_file in patch_cpp_feedback.py) which breaks on POSIX; update all three scripts (patch_cpp_feedback.py, patch_handlers.py, patch_ts_handlers.py) to construct paths portably by composing path components (e.g., using os.path.join or pathlib.Path with path parts) and normalizing them (os.path.normpath or Path.resolve()) so repo_root and any derived filenames (like handlers_file) work on both Windows and POSIX systems; ensure no hardcoded backslashes remain and replace raw r"plugins\McpAutomationBridge\Source\McpAutomationBridge\Private" with a joined/normalized form.patch_cpp_feedback.py-7-82 (1)
7-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winVerify required substitutions in
patch_cpp_feedback.py,patch_handlers.py, andpatch_ts_handlers.py. Each script uses unchecked text substitutions and unconditionally reports success, so any upstream formatting drift leaves the C++ or TypeScript target unchanged with no failure. The shared root cause is missing match-count/assertion logic around required edits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch_cpp_feedback.py` around lines 7 - 82, The scripts (patch_cpp_feedback.py, patch_handlers.py, patch_ts_handlers.py) perform blind text.replace operations (see occurrences like content.replace(...), and the UUmgFileTransformation::ApplyJsonStringToUmgAsset replacement using old_apply_func/new_apply_func) and always print success; add verification after each substitution to assert the expected match was found and replaced (e.g., count occurrences before/after or use regex with count), raise an exception or exit non‑zero if any required replacement (includes changes and the ApplyJsonStringToUmgAsset function swap identified by old_apply_func/new_apply_func) did not occur, and update success logging to only run when all assertions pass so upstream formatting drift fails the script instead of silently reporting success.patch_handlers.py-28-43 (1)
28-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake reruns no-ops in
patch_handlers.pyandpatch_ts_handlers.py. Both scripts replace text with output that still matches the same search pattern, so rerunning them keeps adding duplicateGEditorguards orsanitizePathassignments. The shared root cause is non-idempotent replacement logic in the widget-authoring patch pipeline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch_handlers.py` around lines 28 - 43, The replacements are not idempotent: patch_handlers.py and patch_ts_handlers.py replace lines like "UUmgGetSubsystem* GetSubsystem = GEditor->GetEditorSubsystem<UUmgGetSubsystem>();" and "UUmgSetSubsystem* SetSubsystem = GEditor->GetEditorSubsystem<UUmgSetSubsystem>();" with blocks that include "if (!GEditor)" so rerunning the script keeps inserting duplicates; fix by making the replacement conditional — first check whether the target is already wrapped (e.g., search for an existing "if (!GEditor)" guard or the specific wrapped snippet) and only perform the replace when the guard is absent, or use a regex that matches the unguarded assignment and rejects matches already preceded by the guard; apply the same idempotent check for the sanitizePath assignment in patch_ts_handlers.py so reruns become no-ops.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp-110-114 (1)
110-114: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReplace mocked data asset handlers with real implementations or remove from schema.
The placeholder returns success for
create_data_*andcreate_curve_*actions without implementing them. These actions are exposed in the TypeScriptdataActionSet(consolidated-routing.ts lines 76-87) and will mislead users. Either:
- Implement the handlers before merging, or
- Remove these actions from the TypeScript schema until ready.
Shipping advertised-but-nonfunctional actions degrades user trust and creates hidden technical debt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp` around lines 110 - 114, The current branch contains mocked handlers that always return success for SubAction values starting with "create_data_" or "create_curve_" (see SubAction.StartsWith(...) in McpAutomationBridge_DataHandlers.cpp and the SendAutomationResponse call), but these actions are advertised in the TypeScript dataActionSet (consolidated-routing.ts); replace the mock with real implementations that perform the actual asset/curve creation and return appropriate success/error payloads via Subsystem->SendAutomationResponse (including error details on failure), or remove the corresponding actions from the TypeScript dataActionSet until implementations exist so clients aren’t misled.Source: Coding guidelines
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp-103-103 (1)
103-103: 📐 Maintainability & Code Quality | 🟠 MajorGameplay tags created via
AddNativeGameplayTagwon’t persist across sessions.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp(line 103) registers the tag only in-memory usingUGameplayTagsManager::Get().AddNativeGameplayTag(...). Repo searches show no code that writes/updatesGameplayTags.ini/UGameplayTagsSettings(noSaveGameplayTags,PersistGameplayTag,ExportGameplayTags, or similar persistence), so tags created through this runtime handler won’t survive editor/game restarts unless the operation is run again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp` at line 103, Current code only calls UGameplayTagsManager::Get().AddNativeGameplayTag(FName(*TagName), TagComment) which registers the tag in-memory and won’t persist; update the handler to also add the tag to the persistent settings and save them: obtain the UGameplayTagsSettings (e.g., GetMutableDefault<UGameplayTagsSettings>() or via IGameplayTagsModule::Get().GetGameplayTagsSettings()), push a new FGameplayTagTableRow or add the tag string to the GameplayTags array on that settings instance, call SaveConfig() on the settings object (and optionally call UGameplayTagsManager::Get().RefreshGameplayTagTables() or IGameplayTagsModule::Get().RequestGameplayTag to refresh runtime state), ensuring the tag is both registered in-memory and written to GameplayTags.ini so it survives restarts.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlActor/McpAutomationBridge_ControlActorSelection.cpp-7-98 (1)
7-98: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftStub handlers claim success without performing work.
Eight of the nine handlers are non-functional placeholders that ignore all input parameters and unconditionally return
success: truewith hardcoded messages. Production code must not claim successful completion of operations that were never attempted.
HandleControlActorDeselectAll(lines 47-58) is the only handler with partial logic, but it still lacks validation (it silently skips theGEditor->SelectNonecall whenGEditoris null without returning an error).All other handlers (select, select_by_class, select_by_tag, select_in_volume, get_selected, group, ungroup, run_utility) must:
- Read and validate required parameters from
Payload- Perform the actual editor operation
- Return structured results (e.g., selected actor names/paths)
- Report errors when operations fail
Example: Proper implementation pattern for HandleControlActorGetSelected
bool UMcpAutomationBridgeSubsystem::HandleControlActorGetSelected( const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { + if (!GEditor) { + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("EDITOR_NOT_AVAILABLE"), + TEXT("Editor not available"), nullptr); + return true; + } + TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Selected actors retrieved.")); + + TArray<TSharedPtr<FJsonValue>> SelectedActors; + for (FSelectionIterator It(GEditor->GetSelectedActorIterator()); It; ++It) { + if (AActor* Actor = Cast<AActor>(*It)) { + TSharedPtr<FJsonObject> ActorObj = MakeShared<FJsonObject>(); + ActorObj->SetStringField(TEXT("name"), Actor->GetName()); + ActorObj->SetStringField(TEXT("label"), Actor->GetActorLabel()); + ActorObj->SetStringField(TEXT("class"), Actor->GetClass()->GetName()); + SelectedActors.Add(MakeShared<FJsonValueObject>(ActorObj)); + } + } + + Result->SetArrayField(TEXT("actors"), SelectedActors); + Result->SetBoolField(TEXT("success"), true); + Result->SetStringField(TEXT("message"), + FString::Printf(TEXT("Retrieved %d selected actors."), SelectedActors.Num())); + SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Got Selected"), Result); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlActor/McpAutomationBridge_ControlActorSelection.cpp` around lines 7 - 98, The handlers currently return hardcoded success without validating input or performing editor work; update each handler (HandleControlActorSelect, HandleControlActorSelectByClass, HandleControlActorSelectByTag, HandleControlActorSelectInVolume, HandleControlActorDeselectAll, HandleControlActorGetSelected, HandleControlActorGroup, HandleControlActorUngroup, HandleControlActorRunUtility) to: validate required fields in Payload (e.g., actor name/path, class name, tag, volume bounds, utility name), check GEditor and relevant world/context early and return a failure JSON via SendAutomationResponse if missing, perform the actual editor operation using the appropriate editor APIs (SelectActor(s), SelectNone, iterate selection to build a result array of actor names/paths, grouping/ungrouping APIs, run editor utility), and populate Result with structured data (success bool, message, and a results array/object of actor identifiers or error details) before calling SendAutomationResponse; ensure HandleControlActorDeselectAll returns an error when GEditor is null instead of silently succeeding and include clear error messages when operations fail or no matching actors are found.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp-6-64 (1)
6-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAll Phase 34 placeholder handlers return success without implementation in
McpAutomationBridge_ControlEditorUtilities.cpp(6 handlers) andMcpAutomationBridge_AssetWorkflowBrowser.cpp(7 handlers).All 13 Phase 34 handlers share one root cause: they ignore the
Payloadparameter and returnsuccess: truewithout implementing functionality. This misleads clients into believing operations succeeded when nothing actually happened. The handlers should either returnNOT_IMPLEMENTEDerror responses or read and validate expected parameters fromPayloadbefore returning success (if deferring full implementation to a later phase).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp` around lines 6 - 64, The Phase 34 control-editor handlers (HandleControlEditorSetGridSettings, HandleControlEditorSetSnapSettings, HandleControlEditorManageLayouts, HandleControlEditorCreateCustomMode, HandleControlEditorSpawnUtilityWidget, HandleControlEditorRunUtilityTask) currently ignore Payload and always send success; change each to either (1) validate expected fields from Payload and only call SendAutomationResponse with success=true when required parameters are present and the minimal action is performed, or (2) return a clear NOT_IMPLEMENTED error response by calling SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), Result) with Result->SetStringField("message","Not implemented") so clients aren’t misled. Update the Result json and response message strings in these functions and ensure Payload is checked (e.g., required keys) before reporting success.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp-16-24 (1)
16-24: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleControlEditorSetSnapSettingsreturnssuccess: truewithout reading parameters or implementing functionality. Callers will believe snap settings were applied when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleControlEditorSetSnapSettings( const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Snap settings applied.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Snap settings updated"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Snap settings handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp` around lines 16 - 24, The handler UMcpAutomationBridgeSubsystem::HandleControlEditorSetSnapSettings currently returns success without applying any settings; update this function to parse the incoming Payload (read expected fields like grid size, snap enabled booleans or relevant keys), apply those values to the Control Editor subsystem or call the appropriate setter methods, and only send a success response if the application succeeds; if the feature isn't implemented yet, change the response to send a NOT_IMPLEMENTED result instead of success (use SendAutomationResponse with an appropriate failure flag and message) so callers don't believe snap settings were applied.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp-35-43 (1)
35-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleAddToCollectionreturnssuccess: truewithout reading thecollectionNameorassetPathparameters or adding to a collection. Callers will believe the asset was added when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleAddToCollection( const FString &RequestId, const FString &Action, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Added to collection.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Added to collection"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Add to collection handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp` around lines 35 - 43, HandleAddToCollection currently always returns success without using Payload; update UMcpAutomationBridgeSubsystem::HandleAddToCollection to read the expected parameters (e.g., "collectionName" and "assetPath") from the Payload FJsonObject, attempt to perform the add-to-collection operation via the appropriate domain/asset APIs (or call into the existing collection management methods), and only send a success SendAutomationResponse if the add actually succeeded; if the functionality isn't implemented yet, return a NOT_IMPLEMENTED style response instead of success (use SendAutomationResponse with success=false and a descriptive message) so callers don't assume the asset was added.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp-5-13 (1)
5-13: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleNavigateToPathreturnssuccess: truewithout reading thepathparameter or implementing navigation. Callers will believe navigation occurred when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleNavigateToPath( const FString &RequestId, const FString &Action, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Navigated to path in Content Browser.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Navigated"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Navigate to path handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp` around lines 5 - 13, HandleNavigateToPath currently always reports success without doing anything; update it to read the "path" from the Payload (Payload->GetStringField("path")), attempt the actual Content Browser navigation using the appropriate editor API, and only send a success response via SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Navigated"), Result) when navigation actually succeeds; if you cannot implement navigation here (no editor APIs available), return a NOT_IMPLEMENTED style response by setting Result->SetBoolField("success", false) and Result->SetStringField("message", TEXT("Not implemented: navigation not performed")) and call SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), Result) instead.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp-65-73 (1)
65-73: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleRunAssetActionUtilityreturnssuccess: truewithout reading theutilityPathorassetPathsparameters or running a utility. Callers will believe the utility ran when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleRunAssetActionUtility( const FString &RequestId, const FString &Action, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Asset Action Utility ran.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Utility Executed"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Run asset action utility handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp` around lines 65 - 73, HandleRunAssetActionUtility currently always reports success without using payload params; update UMcpAutomationBridgeSubsystem::HandleRunAssetActionUtility to parse "utilityPath" and "assetPaths" from the Payload TSharedPtr<FJsonObject> and invoke the appropriate utility execution code (or, if execution is not yet implemented, return a NOT_IMPLEMENTED response). Specifically, inspect Payload->GetStringField("utilityPath") and Payload->GetArrayField("assetPaths"), validate inputs, attempt to run the utility (or skip execution if unimplemented), and call SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), Result) when not implemented or include real success/failure details in Result when executed.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp-36-44 (1)
36-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleControlEditorCreateCustomModereturnssuccess: truewithout reading parameters or implementing functionality. Callers will believe the custom mode was created when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleControlEditorCreateCustomMode( const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Custom editor mode created.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Mode created"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Custom mode creation handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp` around lines 36 - 44, The current UMcpAutomationBridgeSubsystem::HandleControlEditorCreateCustomMode always returns success without doing work; change it to return a NOT_IMPLEMENTED response instead: build a Result JSON (reuse the existing Result variable) with success=false and a clear message like "Not implemented", and call SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("Not implemented"), Result); then return false so callers don't assume the mode was created; if you prefer, validate incoming Payload first and only implement actual creation later, but for now ensure the function signals NOT_IMPLEMENTED through Result and the SendAutomationResponse call rather than claiming success.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp-15-23 (1)
15-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleSyncToAssetreturnssuccess: truewithout reading theassetPathparameter or implementing sync. Callers will believe the Content Browser synced to the asset when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleSyncToAsset( const FString &RequestId, const FString &Action, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Synced to asset in Content Browser.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Synced"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Sync to asset handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp` around lines 15 - 23, HandleSyncToAsset currently always returns success; update UMcpAutomationBridgeSubsystem::HandleSyncToAsset to read the "assetPath" string from the provided Payload and only perform the Content Browser sync implementation (or call the existing editor/content browser API) when assetPath is present; if assetPath is missing or you cannot perform the sync here, respond using SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), Result) (or return a NOT_IMPLEMENTED-style error) and set Result fields to include an explanatory message instead of unconditionally returning success. Ensure you reference Payload->GetStringField("assetPath"), RequestId, RequestingSocket and SendAutomationResponse when making the change.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp-26-34 (1)
26-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleControlEditorManageLayoutsreturnssuccess: truewithout reading parameters or implementing functionality. Callers will believe the layout was managed when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleControlEditorManageLayouts( const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Editor layout updated.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Layout managed"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Layout management handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp` around lines 26 - 34, The handler UMcpAutomationBridgeSubsystem::HandleControlEditorManageLayouts currently always returns success without processing Payload; replace the placeholder behavior with a NOT_IMPLEMENTED response: inspect Payload and implement layout management or, if not ready, set Result->SetBoolField("success", false) and use SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), Result) (or the project's standard NOT_IMPLEMENTED error code/message) so callers get an accurate failure; ensure you reference and update the function HandleControlEditorManageLayouts and the SendAutomationResponse call site accordingly.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp-6-14 (1)
6-14: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleControlEditorSetGridSettingsreturnssuccess: truewithout reading parameters or implementing functionality. Callers will believe the grid settings were applied when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED or read expected parameters
bool UMcpAutomationBridgeSubsystem::HandleControlEditorSetGridSettings( const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Grid settings applied.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Grid settings updated"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Grid settings handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp` around lines 6 - 14, HandleControlEditorSetGridSettings currently always reports success without parsing Payload or applying settings; update UMcpAutomationBridgeSubsystem::HandleControlEditorSetGridSettings to parse the expected parameters from Payload (e.g., grid enabled, grid size, snap settings — whatever the editor API expects), validate them, call the appropriate control-editor API to apply the grid settings, and only set Result->SetBoolField("success", true) and SendAutomationResponse success when the apply call succeeds; if required parameters are missing or the apply fails, return a NOT_IMPLEMENTED/false response by setting success=false, a descriptive message, and send that via SendAutomationResponse(RequestingSocket, RequestId, false,...). Ensure you reference and use the existing Result JSON object and RequestId/RequestingSocket in the updated flow.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp-45-53 (1)
45-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleSetAssetColorreturnssuccess: truewithout reading theassetPathorcolorparameters or setting asset color. Callers will believe the color was set when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleSetAssetColor( const FString &RequestId, const FString &Action, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Set asset/folder color.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Color set"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Set asset color handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp` around lines 45 - 53, HandleSetAssetColor currently returns success without doing anything; change it to return a NOT_IMPLEMENTED-style response instead of claiming "Color set." Specifically, in UMcpAutomationBridgeSubsystem::HandleSetAssetColor replace the Result->SetBoolField/SetStringField success message with a clear not-implemented response (use SendAutomationResponse to send RequestId with success=false or a NOT_IMPLEMENTED flag and an explanatory message), and do not parse or apply assetPath/color until a real implementation is added; keep references to HandleSetAssetColor and SendAutomationResponse so reviewers can find and later implement actual parameter parsing and color application.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp-46-54 (1)
46-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleControlEditorSpawnUtilityWidgetreturnssuccess: truewithout reading parameters or implementing functionality. Callers will believe the widget was spawned when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleControlEditorSpawnUtilityWidget( const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Editor utility widget spawned.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Widget spawned"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Utility widget spawn handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp` around lines 46 - 54, The handler UMcpAutomationBridgeSubsystem::HandleControlEditorSpawnUtilityWidget currently returns success without doing any work; change it to return a NOT_IMPLEMENTED response instead: stop claiming the widget was spawned, build a Result JSON indicating not implemented (e.g., success=false and a "Not implemented" message) and call SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("Not implemented"), Result) or use the project's canonical NOT_IMPLEMENTED response helper/enum if one exists; leave actual spawning logic for a future implementation.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp-55-63 (1)
55-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleShowInExplorerreturnssuccess: truewithout reading theassetPathparameter or opening the explorer. Callers will believe the explorer was opened when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleShowInExplorer( const FString &RequestId, const FString &Action, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Shown in explorer.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Opened Explorer"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Show in explorer handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp` around lines 55 - 63, HandleShowInExplorer currently returns success without doing anything; update it to read the "assetPath" string from the Payload, and either implement the explorer action (e.g., convert the path to an absolute path and call FPlatformProcess::ExploreFolder or FPlatformProcess::LaunchFileInDefaultExternalApplication on that path) and send a true response only on success, or if you don't implement it now, change the response to indicate NOT_IMPLEMENTED (use SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("Not implemented"), Result) and set success=false and message accordingly). Reference the UMcpAutomationBridgeSubsystem::HandleShowInExplorer function and the SendAutomationResponse call to locate where to change the behavior.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp-56-64 (1)
56-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleControlEditorRunUtilityTaskreturnssuccess: truewithout reading parameters or implementing functionality. Callers will believe the task was executed when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleControlEditorRunUtilityTask( const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Editor utility task executed.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Task executed"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Utility task execution handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp` around lines 56 - 64, The function HandleControlEditorRunUtilityTask currently always reports success without doing any work; change it to return a NOT_IMPLEMENTED response instead. Update the Result JSON (Result variable) to set success to false and message to "NOT_IMPLEMENTED" (or similar), and call SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), Result) so callers receive the correct failure status; leave a TODO comment in HandleControlEditorRunUtilityTask for the real implementation to be added later.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp-25-33 (1)
25-33: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaceholder returns success without implementation.
HandleCreateCollectionreturnssuccess: truewithout reading thecollectionNameparameter or creating a collection. Callers will believe the collection was created when nothing actually happened.🔧 Recommended fix: Return NOT_IMPLEMENTED
bool UMcpAutomationBridgeSubsystem::HandleCreateCollection( const FString &RequestId, const FString &Action, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { - TSharedPtr<FJsonObject> Result = MakeShared<FJsonObject>(); - Result->SetBoolField(TEXT("success"), true); - Result->SetStringField(TEXT("message"), TEXT("Created collection.")); - SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Collection created"), Result); + SendStandardErrorResponse(this, RequestingSocket, RequestId, TEXT("NOT_IMPLEMENTED"), + TEXT("Create collection handler not yet implemented."), nullptr); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp` around lines 25 - 33, UMcpAutomationBridgeSubsystem::HandleCreateCollection currently returns success:true without implementing creation; change it to report not-implemented instead of claiming success by building a Result JSON with success=false (or an error code like "NOT_IMPLEMENTED") and an explanatory message, then call SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("Not implemented"), Result); do not attempt to read or act on Payload/collectionName until a real implementation is added so callers are not misled.
🟡 Minor comments (6)
tests/mcp-tools/gameplay/manage-data.test.mjs-44-49 (1)
44-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace broad expectation mask with narrow alternative.
Line 48 uses
expected: 'success|error', which violates the coding guideline: "Do not use broad expectation masks such assuccess|error."For this "delete non-existent slot" scenario, choose one of:
- Error-primary (if the implementation should fail when slot not found):
expected: 'error|not found'- Success-primary with narrow alternative (if the implementation succeeds idempotently):
expected: 'success|not found'The guideline allows narrow state alternatives like
not found, but forbids the genericsuccess|errormask.📝 Proposed fix
{ scenario: 'SAVE: delete_save_slot (not found)', toolName: 'manage_data', arguments: { action: 'delete_save_slot', slotName: SLOT_NAME, userIndex: 0 }, - expected: 'success|error', // Can return error if not found depending on implementation + expected: 'success|not found', },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mcp-tools/gameplay/manage-data.test.mjs` around lines 44 - 49, Update the test case object for the scenario 'SAVE: delete_save_slot (not found)' in tests/mcp-tools/gameplay/manage-data.test.mjs by replacing the broad expectation string 'success|error' with a narrow alternative; decide whether the implementation should be error-primary or idempotent and set the expected field accordingly to either 'error|not found' (if delete should fail when the slot is missing) or 'success|not found' (if delete should be idempotent), keeping the rest of the scenario (toolName 'manage_data', arguments including SLOT_NAME and userIndex) unchanged.Source: Coding guidelines
scripts/smoke-test.ts-18-18 (1)
18-18: 📐 Maintainability & Code Quality | 🟡 MinorAlign tool-count documentation/guidance with canonical tool definitions (25)
scripts/smoke-test.tsexpectstotalTools: 25src/tools/definitions/shared/all-tool-definitions.tsenumerates 25 parent tool definitionsUpdate
src/tools/AGENTS.mdand any “maintain exactly 23”/canonical-surface documentation (and ensuremanage_datais included) so the documented canonical tool count matches the actual list (25).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/smoke-test.ts` at line 18, The smoke-test asserts totalTools: z.literal(25) but documentation still says "maintain exactly 23"; update the canonical documentation and guidance to reflect the actual tool list of 25 as defined in src/tools/definitions/shared/all-tool-definitions.ts, ensuring the manage_data tool is listed and any wording referencing "23" is changed to "25" (or to a dynamic phrase like "canonical tool count (25)" where appropriate) in src/tools/AGENTS.md and any other docs that mention the canonical surface so they match the totalTools symbol used in scripts/smoke-test.ts.src/tools/orchestration/consolidated-handler-registration.ts-220-224 (1)
220-224: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove redundant conditional or reject unknown actions.
Both branches of the conditional call
handleDataToolswith identical arguments, making thedataActionSet.has(action)check useless. Either:
- Remove the conditional entirely (simpler), or
- Reject actions NOT in
dataActionSetwith an error (aligns with guideline to "reject unknown actions with domain/action context").Other registrations like
manage_asset(lines 91-108) route to different handlers based on action sets, suggesting option 2 may be intended.🔧 Proposed fixes
Option 1 (simpler): Remove the redundant conditional
toolRegistry.register('manage_data', async (args, tools) => { - const action = getToolAction(args); - if (dataActionSet.has(action)) return await handleDataTools(action, args, tools); - return await handleDataTools(action, args, tools); + return await handleDataTools(getToolAction(args), args, tools); });Option 2 (stricter): Reject unknown actions
toolRegistry.register('manage_data', async (args, tools) => { const action = getToolAction(args); if (dataActionSet.has(action)) return await handleDataTools(action, args, tools); - return await handleDataTools(action, args, tools); + return { success: false, error: 'UNKNOWN_ACTION', message: `Unknown data action: ${action}` }; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/orchestration/consolidated-handler-registration.ts` around lines 220 - 224, The tool registration for 'manage_data' contains a redundant conditional: toolRegistry.register('manage_data', ...) calls getToolAction and then always calls handleDataTools regardless of dataActionSet.has(action); either remove the conditional and directly return await handleDataTools(action, args, tools), or (preferred to match manage_asset behavior) validate the action against dataActionSet and throw/reject an error for unknown actions (include action and domain/context in the error) before calling handleDataTools; update the toolRegistry.register('manage_data', ...) block and use getToolAction, dataActionSet, and handleDataTools identifiers accordingly.Source: Coding guidelines
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp-116-118 (1)
116-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unknown subActions instead of silently succeeding.
The default handler returns success for any unrecognized
subAction, violating the guideline to "reject unknown actions with domain/action context." This masks bugs and future typos.🔧 Proposed fix
- // Default handler - Subsystem->SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Data action executed."), ResultJson); - return true; + // Unknown action + Subsystem->SendAutomationError(RequestingSocket, RequestId, + FString::Printf(TEXT("Unknown data subAction: %s"), *SubAction), + TEXT("UNKNOWN_ACTION")); + return true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp` around lines 116 - 118, The default handler currently returns success for unknown subAction values; change it to reject unknown subActions by calling Subsystem->SendAutomationResponse with success=false and a clear message that includes the domain ("Data"), the action name, and the unknown subAction (use the existing RequestingSocket, RequestId, and ResultJson parameters), and return false; also add a warning log mentioning the unrecognized subAction to aid debugging.Source: Coding guidelines
src/tools/handlers/data/data-handlers.ts-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace
Promise<any>with a specific return type.The return type
Promise<any>violates the strict TypeScript guideline. UsePromise<Record<string, unknown>>or a more specific interface.🔧 Proposed fix
-export async function handleDataTools(action: string, args: HandlerArgs, tools: ITools): Promise<any> { +export async function handleDataTools(action: string, args: HandlerArgs, tools: ITools): Promise<Record<string, unknown>> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/handlers/data/data-handlers.ts` at line 6, The function handleDataTools currently returns Promise<any>; update its signature to return a concrete type such as Promise<Record<string, unknown>> (or a more specific interface if available) to satisfy strict TypeScript rules — change the exported function declaration (handleDataTools) to use the new return type and adjust any related type aliases or imports (HandlerArgs, ITools) and call sites to align with the chosen return type so type-checking passes.Source: Coding guidelines
src/tools/handlers/core/project-settings-handlers.ts-2-2 (1)
2-2: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace
anywithIToolstype.The
toolsparameter usesany, violating the strict TypeScript guideline. Import and useIToolsfromtool-interfaces.js.🔧 Proposed fix
+import type { ITools } from '../../../types/tools/tool-interfaces.js'; import { executeAutomationRequest } from '../foundation/dispatch/common-handlers.js'; -export async function handleProjectSettingsTools(action: string, args: Record<string, unknown>, tools: any): Promise<Record<string, unknown>> { +export async function handleProjectSettingsTools(action: string, args: Record<string, unknown>, tools: ITools): Promise<Record<string, unknown>> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/handlers/core/project-settings-handlers.ts` at line 2, Update the handleProjectSettingsTools function signature to replace the loose any on the tools parameter with the proper ITools interface: import ITools from 'tool-interfaces.js' at the top of the file and change the parameter type in export async function handleProjectSettingsTools(action: string, args: Record<string, unknown>, tools: ITools): Promise<Record<string, unknown>>; ensure any usages of tools inside the function conform to ITools and adjust imports/exports if needed so the file compiles.Source: Coding guidelines
🧹 Nitpick comments (3)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Project/McpAutomationBridge_ProjectSettingsHandlers.cpp (1)
4-18: 🎯 Functional Correctness | ⚡ Quick winPlaceholder implementation lacks action validation.
The handler is marked as a placeholder (lines 8-9) and currently returns success for any
SubActionwithout validation. Consider adding a check to ensureSubActionmatches one of the expected actions from the tool schema (create_collision_channel, create_collision_profile, etc.) to catch invalid actions early during development.🛡️ Suggested validation guard
bool UMcpAutomationBridgeSubsystem::HandleProjectSettingsAction(const FString& RequestId, const FString& Action, const TSharedPtr<FJsonObject>& Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket) { FString SubAction = McpConsolidatedActions::GetPayloadSubAction(Payload); + + // Validate SubAction against known actions + static const TSet<FString> ValidActions = { + TEXT("create_collision_channel"), TEXT("create_collision_profile"), + TEXT("configure_channel_responses"), TEXT("configure_object_type"), + TEXT("configure_trace_channel"), TEXT("set_actor_collision_profile"), + TEXT("create_physical_material"), TEXT("set_physical_material_properties") + }; + + if (!ValidActions.Contains(SubAction)) + { + SendAutomationError(RequestingSocket, RequestId, + FString::Printf(TEXT("Unknown project settings action: %s"), *SubAction), + TEXT("UNKNOWN_ACTION")); + return false; + } // Placeholder implementations until Phase 34 properties are finalized.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Project/McpAutomationBridge_ProjectSettingsHandlers.cpp` around lines 4 - 18, HandleProjectSettingsAction currently accepts any SubAction and always returns success; add a validation guard in UMcpAutomationBridgeSubsystem::HandleProjectSettingsAction that calls McpConsolidatedActions::GetPayloadSubAction, checks the returned SubAction against an explicit allowed set (e.g., "create_collision_channel", "create_collision_profile", and other schema-defined actions), and if the SubAction is not in that set call SendAutomationResponse(RequestingSocket, RequestId, false, <meaningful error message>, Result) and return false; only proceed to build the success Result and send the success response when the SubAction is validated.src/tools/handlers/core/project-settings-handlers.ts (1)
2-18: 🎯 Functional Correctness | ⚡ Quick winConsider validating required fields before dispatch.
The handler forwards all arguments without validating action-specific required fields. As per coding guidelines, handlers should validate required fields before dispatch. Consider adding validation for common project settings parameters (e.g., collision profile names, physical material properties).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/handlers/core/project-settings-handlers.ts` around lines 2 - 18, The handler handleProjectSettingsTools currently forwards all args to executeAutomationRequest without validating action-specific required fields; add pre-dispatch validation inside handleProjectSettingsTools that checks the incoming action and validates required parameters (e.g., for collision profile actions ensure collisionProfileName is present, for physical material actions ensure required material properties like density/friction are provided), return a structured error { success: false, message: "...", error: "missing field X" } immediately if validation fails, and only call executeAutomationRequest when validation passes; keep the existing catch behavior for runtime errors.Source: Coding guidelines
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp (1)
36-36: 🔒 Security & Privacy | 💤 Low valueConsider validating config file paths to prevent unauthorized access.
read_config_valueandwrite_config_valueaccept user-providedConfigFilenamewithout validation. If sensitive configs (e.g., encryption keys, credentials) are accessible, this could be a security risk. Consider restricting to safe config files (e.g., Game.ini, Engine.ini) or documenting the security model.Also applies to: 55-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp` at line 36, read_config_value and write_config_value currently pass the user-supplied ConfigFilename directly into GConfig->GetString / GConfig->SetString, allowing arbitrary file paths; add validation to canonicalize and restrict ConfigFilename before calling GConfig: reject path traversal or absolute paths outside the game's Config directory, enforce a whitelist (e.g., "Game.ini","Engine.ini") or a configured set of allowed filenames, and return/log an error if the filename is invalid; implement the check in the entry points (read_config_value/write_config_value) so GConfig->GetString and GConfig->SetString are only called with validated filenames.
Comment on lines
+41
to
+54
| bool HandleControlActorCallFunction(const FString &RequestId, | ||
| const TSharedPtr<FJsonObject> &Payload, | ||
| TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); | ||
|
|
||
| // Selection & Grouping (Phase 34) | ||
| bool HandleControlActorSelect(const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); | ||
| bool HandleControlActorSelectByClass(const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); | ||
| bool HandleControlActorSelectByTag(const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); | ||
| bool HandleControlActorSelectInVolume(const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); | ||
| bool HandleControlActorDeselectAll(const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); | ||
| bool HandleControlActorGetSelected(const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); | ||
| bool HandleControlActorGroup(const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); | ||
| bool HandleControlActorUngroup(const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); | ||
| bool HandleControlActorRunUtility(const FString &RequestId, const TSharedPtr<FJsonObject> &Payload, TSharedPtr<FMcpBridgeWebSocket> RequestingSocket); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Function signature mismatch causes compile error.
These functions are declared as free functions in the header, but implemented as UMcpAutomationBridgeSubsystem member functions in McpAutomationBridge_ControlActorSelection.cpp. The dispatcher in McpAutomationBridge_ControlActorDispatch.cpp (lines 128-145) calls them as member functions.
The declarations must be moved inside the UMcpAutomationBridgeSubsystem class definition or into a subsystem extension header that declares them as member functions.
🔧 Proposed fix
These declarations belong in the subsystem class, not as free functions. If this header extends the subsystem, wrap them in the class scope:
+// Forward declare or ensure UMcpAutomationBridgeSubsystem is visible
+class UMcpAutomationBridgeSubsystem;
+
+// Inside UMcpAutomationBridgeSubsystem class declaration (or in a friend/extension pattern):
bool HandleControlActorCallFunction(const FString &RequestId,
const TSharedPtr<FJsonObject> &Payload,
TSharedPtr<FMcpBridgeWebSocket> RequestingSocket);Alternatively, if these are truly free functions, prefix implementations with the correct scope and ensure the dispatcher calls them correctly (e.g., ::HandleControlActorSelect(...)).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlActor/McpAutomationBridge_ControlActorSupport.h`
around lines 41 - 54, The declarations for the control-actor handlers (e.g.,
HandleControlActorCallFunction, HandleControlActorSelect,
HandleControlActorSelectByClass, HandleControlActorSelectByTag,
HandleControlActorSelectInVolume, HandleControlActorDeselectAll,
HandleControlActorGetSelected, HandleControlActorGroup,
HandleControlActorUngroup, HandleControlActorRunUtility) are currently free
functions but their implementations and callers expect them as
UMcpAutomationBridgeSubsystem member methods; fix by moving these declarations
into the UMcpAutomationBridgeSubsystem class definition (or the subsystem
extension header) so the signatures match the implementations and dispatcher
calls, or alternatively keep them as free functions by changing the
implementations and dispatcher calls to the global scope
(::HandleControlActorSelect, etc.) so the declaration/definition and call sites
all use the same scope.
- Updated python patch scripts to use portable os.path.join paths and added safe os.path.exists checks. - Updated patch_handlers.py and patch_ts_handlers.py to be idempotent and verify replacements. - Implemented true functionality for HandleControlActorGetSelected using GEditor iterator. - Updated HandleControlActorDeselectAll to properly check GEditor pointer. - Changed all other 14 placeholder Editor Utilities and Asset Workflow handlers to correctly return NOT_IMPLEMENTED instead of a false success message.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
patch_ts_handlers.py (1)
3-3: ⚡ Quick winRemove unused
sysimport.The
sysmodule is imported but never used in this script.import re import os -import sys🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch_ts_handlers.py` at line 3, The `sys` module is imported at the top of patch_ts_handlers.py but is never used anywhere in the script. Remove the unused `import sys` statement to clean up the imports and reduce unnecessary dependencies.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp (1)
1-1: 💤 Low valueConsider explicit include for SendStandardErrorResponse.
The file uses
SendStandardErrorResponsebut relies on transitive inclusion throughMcpAutomationBridgeSubsystem.h. Adding an explicit include improves compile-time resilience if the subsystem header changes.📦 Optional include addition
Based on context snippet 1, the helper is defined at:
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Foundation/BridgeHelpers/Responses/McpAutomationBridgeHelpersResponses.h`#include` "McpAutomationBridgeSubsystem.h" +#include "Foundation/BridgeHelpers/Responses/McpAutomationBridgeHelpersResponses.h" // Placeholder implementations for the Phase 34 Content Browser / Asset Utilities.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp` at line 1, The file McpAutomationBridge_AssetWorkflowBrowser.cpp currently relies on a transitive include through McpAutomationBridgeSubsystem.h to access the SendStandardErrorResponse helper function, which reduces compile-time resilience. Add an explicit include statement for McpAutomationBridgeHelpersResponses.h (the header that directly defines SendStandardErrorResponse) near the top of the file alongside the existing includes, ensuring the dependency is direct rather than transitive.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@patch_ts_handlers.py`:
- Around line 62-65: The repl function captures the action variable by reference
rather than by value, creating a late-binding closure bug that could cause
incorrect behavior if refactored. Modify the repl function definition to bind
action via a default argument parameter, ensuring the variable is captured by
value at the time of function definition rather than by reference.
- Around line 72-78: The print statement for "TS Patching complete." is
currently at module level and executes regardless of whether the target file
exists. Move the print statement inside the if os.path.exists(file_path) block
so that the completion message only prints when the file actually exists and
patching has been performed. This ensures the message is only shown when
patching actually occurs, not when the file is missing.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp`:
- Around line 5-52: All seven handler functions (HandleNavigateToPath,
HandleSyncToAsset, HandleCreateCollection, HandleAddToCollection,
HandleSetAssetColor, HandleShowInExplorer, and HandleRunAssetActionUtility) have
unused Action and Payload parameters that will trigger compiler warnings. Add
(void)Action; (void)Payload; statements at the beginning of each function body
to explicitly mark these parameters as intentionally unused and suppress the
warnings.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp`:
- Around line 6-11: Add GEditor availability checks to all six Phase 34 editor
utility handlers in McpAutomationBridge_ControlEditorUtilities.cpp to ensure
they return EDITOR_NOT_AVAILABLE error code when the editor is unavailable,
rather than NOT_IMPLEMENTED. For HandleControlEditorSetGridSettings (lines 6-11)
and the five other handlers at lines 13-18, 20-25, 27-32, 34-39, and 41-46, add
a GEditor null check at the start of each function that calls
SendStandardErrorResponse with EDITOR_NOT_AVAILABLE error code when GEditor is
nullptr, following the same pattern used in HandleControlActorDeselectAll and
other ControlActor handlers.
---
Nitpick comments:
In `@patch_ts_handlers.py`:
- Line 3: The `sys` module is imported at the top of patch_ts_handlers.py but is
never used anywhere in the script. Remove the unused `import sys` statement to
clean up the imports and reduce unnecessary dependencies.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cpp`:
- Line 1: The file McpAutomationBridge_AssetWorkflowBrowser.cpp currently relies
on a transitive include through McpAutomationBridgeSubsystem.h to access the
SendStandardErrorResponse helper function, which reduces compile-time
resilience. Add an explicit include statement for
McpAutomationBridgeHelpersResponses.h (the header that directly defines
SendStandardErrorResponse) near the top of the file alongside the existing
includes, ensuring the dependency is direct rather than transitive.
🪄 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: f13170a2-c7a3-41a4-b2d7-10ba55f26bfa
📒 Files selected for processing (6)
patch_cpp_feedback.pypatch_handlers.pypatch_ts_handlers.pyplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/AssetWorkflow/McpAutomationBridge_AssetWorkflowBrowser.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlActor/McpAutomationBridge_ControlActorSelection.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/ControlEditor/McpAutomationBridge_ControlEditorUtilities.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- patch_handlers.py
- patch_cpp_feedback.py
Jbrandan
force-pushed
the
feat/phase-34-editor-utilities
branch
from
cc85a9a to
92f2662
Compare
- patch_ts_handlers.py: removed unused sys import, fixed late-binding closure bug, moved print statement inside block. - McpAutomationBridge_AssetWorkflowBrowser.cpp: added explicit include for SendStandardErrorResponse, and suppressed unused variable warnings. - McpAutomationBridge_ControlEditorUtilities.cpp: added GEditor availability checks to return EDITOR_NOT_AVAILABLE error.
Owner
|
Thanks for the Phase 34 work! The TS wiring and registration look clean. A few things need to be addressed before this can merge safely. BlockerBuild will fail. Please either restore it, likely as Concerns
Smaller notes
Suggested path forwardEither finish the native implementations or narrow the TS tool schemas to only expose actions that are actually implemented. After that, this should be much safer to merge. The overall TS structure looks good — the main thing is making sure the exposed actions match the native behavior. |
LuuOW
reviewed
Jun 17, 2026
LuuOW
left a comment
LuuOW
left a comment
There was a problem hiding this comment.
Technical audit: code patterns and architectural layout verified for system reliability.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
This PR implements the requested features for Phase 34: Editor Utilities. It expands the functionality of the existing tools (
control_editor,manage_asset, andcontrol_actor) and introduces a new standalone tool (manage_project_settings) to handle project configurations cleanly. The implementation natively maps these commands into the Unreal Engine editor via the McpAutomationBridge.Changes
manage_project_settingstool for collision and physical material configurations (manage_collisions,manage_physical_materials).control_editor.manage_asset.control_actor.smoke-test.tsto accommodate 25 functional tools.McpTool_ManageProjectSettingsand connected the dispatcher subsystems (McpAutomationBridge_ControlEditorUtilities,McpAutomationBridge_AssetWorkflowBrowser,McpAutomationBridge_ControlActorSelection).Related Issues
Related to #467
Type of Change
Testing
Pre-Merge Checklist