feat: Movie Render Queue (MRQ) handler — 11 actions#324
feat: Movie Render Queue (MRQ) handler — 11 actions#324REMvisual wants to merge 5 commits intoChiR24:mainfrom
Conversation
Adds full MRQ automation support via a new manage_mrq tool with 11 actions: get_queue, get_job_config, get_cvars, set_cvars, get_output_settings, set_output_settings, create_job, delete_job, render, get_render_status, load_preset. Supports reading/writing CVar overrides, output resolution and directory, start/end console commands, preset loading, and render execution. Uses legacy MovieRenderPipeline API (not Movie Render Graph) for compatibility across UE 5.0-5.7. MRQ modules are optional dynamic dependencies — plugin loads cleanly even if MRQ plugin is disabled. Tested end-to-end on UE 5.7 with all read/write/create/delete actions confirmed working. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
|
👋 Thanks for your first Pull Request! We love contributions. Please ensure you have signed off your commits and followed the contribution guidelines. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Movie Render Queue (MRQ) automation: optional MRQ build modules, a new "manage_mrq" automation handler and implementation in the McpAutomationBridge subsystem, plus TypeScript tool definitions and handlers to invoke MRQ actions remotely. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Tools
participant Bridge as MCP_Bridge
participant Subsystem as AutomationSubsystem
participant MRQ as MovieRenderQueue
Client->>Tools: manage_mrq request (action, payload)
Tools->>Bridge: executeAutomationRequest('manage_mrq', payload)
Bridge->>Subsystem: HandleMrqAction(RequestId, Action, Payload, Socket)
Subsystem->>Subsystem: validate & normalize action/payload
alt MRQ modules available
Subsystem->>MRQ: acquire Queue subsystem / job operations
MRQ-->>Subsystem: job/queue data or status
Subsystem-->>Bridge: Send success response (result JSON)
else MRQ modules unavailable
Subsystem-->>Bridge: Send MRQ_NOT_AVAILABLE error
end
Bridge-->>Tools: Relay response
Tools-->>Client: Return final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…lidation Fixes all 8 review findings from Devin AI and CodeRabbit: C++ (McpAutomationBridge_MrqHandlers.cpp): - Add null checks after Cast for CVars and Output pointers in set_cvars and set_output_settings handlers (prevents nullptr dereference) - Sanitize outputDirectory: normalize path, reject .. traversal - Validate create_job: require map and sequence before allocating job - Populate ErrorCode (6th param) on all SendAutomationResponse failure paths so top-level error field is never empty TypeScript (mrq-handlers.ts): - Normalize asset paths (map, sequence, presetPath) to canonical /Game/... form before forwarding to C++ bridge - Validate startCommands/endCommands through CommandValidator safety filter to prevent dangerous console command injection - Use requireAction() for consistent action extraction Schema (consolidated-tool-definitions.ts): - Add missing set_output_settings input fields (zeroPadFrameNumbers, frameNumberOffset, handleFrameCount, useCustomFrameRate, useCustomPlaybackRange, customStartFrame, customEndFrame) - Add outputSchema defining full MRQ response shapes Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
coderabbitai
bot
added
javascript
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Addresses CodeRabbit's second round of review: - set_cvars: early return if Payload is null (prevents nullptr dereference when accessing Payload->TryGetObjectField later) - set_output_settings: same null Payload guard - delete_job: require explicit jobIndex field — destructive action should not silently default to job 0 on malformed requests Read-only handlers (get_cvars, get_output_settings, etc.) keep the default-to-0 behavior, which is intentional and documented in the schema. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp (2)
339-340:⚠️ Potential issue | 🟠 MajorRequire
jobIndexon the remaining mutating actions.
set_cvars,set_output_settings, andload_presetstill fall back to0when the field is omitted, so a malformed request silently rewrites job 0 instead of failing fast.delete_jobalready uses the safer pattern just below.🛠 Suggested pattern
- int32 JobIndex = 0; - Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex); + int32 JobIndex = INDEX_NONE; + if (!Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex)) + { + TSharedPtr<FJsonObject> Err = MakeShared<FJsonObject>(); + Err->SetStringField(TEXT("error"), TEXT("MISSING_JOB_INDEX")); + SendAutomationResponse(RequestingSocket, RequestId, false, + TEXT("set_cvars requires explicit jobIndex"), Err, TEXT("MISSING_JOB_INDEX")); + return true; + }Apply the same check in
set_output_settingsandload_preset.Also applies to: 495-496, 709-715
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp` around lines 339 - 340, The mutating handlers set_cvars, set_output_settings, and load_preset currently call Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex) and silently default JobIndex to 0; update each handler to require the jobIndex field the same way delete_job does: declare int32 JobIndex; check Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex) and if it returns false, respond with an error/early return (matching the existing delete_job error path) instead of proceeding, so malformed requests cannot mutate job 0; apply this change for the occurrences around the JobIndex uses (including the instances mentioned near lines 495-496 and 709-715).
532-547:⚠️ Potential issue | 🔴 Critical
outputDirectorystill needs the project-path sanitizer.The current check only rejects visible
..segments; it still accepts absolute destinations and other out-of-project paths. Please run the input throughSanitizeProjectRelativePath()before persisting it.As per coding guidelines, "Use `SanitizeProjectRelativePath()` to validate and sanitize all file paths to prevent directory traversal attacks" and "Do not use absolute Windows paths in handler code - use relative paths or UE's path resolution utilities".🔒 Suggested fix
if (Payload->TryGetStringField(TEXT("outputDirectory"), StrVal)) { - // Normalize and reject path traversal - FString CleanDir = StrVal; - FPaths::NormalizeFilename(CleanDir); - FPaths::CollapseRelativeDirectories(CleanDir); - if (CleanDir.Contains(TEXT(".."))) + FString SanitizedDir; + if (!SanitizeProjectRelativePath(StrVal, SanitizedDir)) { TSharedPtr<FJsonObject> Err2 = MakeShared<FJsonObject>(); Err2->SetStringField(TEXT("error"), TEXT("INVALID_PATH")); SendAutomationResponse(RequestingSocket, RequestId, false, - TEXT("outputDirectory rejected: path traversal (..) not allowed"), Err2, TEXT("INVALID_PATH")); + TEXT("outputDirectory must stay within the project"), Err2, TEXT("INVALID_PATH")); return true; } - Output->OutputDirectory.Path = CleanDir; + Output->OutputDirectory.Path = SanitizedDir; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp` around lines 532 - 547, The outputDirectory value extracted via Payload->TryGetStringField(TEXT("outputDirectory"), StrVal) must be validated with SanitizeProjectRelativePath() before assigning to Output->OutputDirectory.Path; replace the current manual NormalizeFilename/CollapseRelativeDirectories/.. check with a call to SanitizeProjectRelativePath(StrVal, /*out*/ CleanDir) (or the project's exact sanitizer signature), check its return (or whether CleanDir is empty/invalid) and send the same INVALID_PATH response via SendAutomationResponse(RequestingSocket, RequestId, ...) if sanitization fails, otherwise assign the sanitized CleanDir to Output->OutputDirectory.Path.
🧹 Nitpick comments (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp (1)
103-107: Please move these payload branches onto typedUSTRUCTs.This new handler is hand-parsing every action payload, which makes defaults and required-field rules easy to drift between branches. Defining per-action request/response structs and marshalling them via
FJsonObjectConverterwould make the contract much easier to keep consistent.As per coding guidelines, "Use
FJsonObjectConverterfor JSON parsing and struct serialization instead of manual JSON parsing".Also applies to: 176-177, 274-275, 339-340, 440-441, 495-496, 593-597, 711-715
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp` around lines 103 - 107, Replace the ad-hoc JSON field extraction (e.g., the local FString SubAction and Payload->TryGetStringField usage) with typed USTRUCT request/response types for each action and use FJsonObjectConverter to marshal Payload into those structs; create per-action USTRUCTs (e.g., FMyActionRequest / FMyActionResponse), update the handler functions in McpAutomationBridge_MrqHandlers.cpp to call FJsonObjectConverter::JsonObjectToUStruct on Payload and validate required/defaulted fields on the struct rather than hand-parsing, and then use the typed fields in the existing logic (replace occurrences of Payload->TryGetStringField, Payload.IsValid() branches and similar branches at the other noted locations with struct-driven handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp`:
- Around line 339-340: The mutating handlers set_cvars, set_output_settings, and
load_preset currently call Payload->TryGetNumberField(TEXT("jobIndex"),
JobIndex) and silently default JobIndex to 0; update each handler to require the
jobIndex field the same way delete_job does: declare int32 JobIndex; check
Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex) and if it returns false,
respond with an error/early return (matching the existing delete_job error path)
instead of proceeding, so malformed requests cannot mutate job 0; apply this
change for the occurrences around the JobIndex uses (including the instances
mentioned near lines 495-496 and 709-715).
- Around line 532-547: The outputDirectory value extracted via
Payload->TryGetStringField(TEXT("outputDirectory"), StrVal) must be validated
with SanitizeProjectRelativePath() before assigning to
Output->OutputDirectory.Path; replace the current manual
NormalizeFilename/CollapseRelativeDirectories/.. check with a call to
SanitizeProjectRelativePath(StrVal, /*out*/ CleanDir) (or the project's exact
sanitizer signature), check its return (or whether CleanDir is empty/invalid)
and send the same INVALID_PATH response via
SendAutomationResponse(RequestingSocket, RequestId, ...) if sanitization fails,
otherwise assign the sanitized CleanDir to Output->OutputDirectory.Path.
---
Nitpick comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp`:
- Around line 103-107: Replace the ad-hoc JSON field extraction (e.g., the local
FString SubAction and Payload->TryGetStringField usage) with typed USTRUCT
request/response types for each action and use FJsonObjectConverter to marshal
Payload into those structs; create per-action USTRUCTs (e.g., FMyActionRequest /
FMyActionResponse), update the handler functions in
McpAutomationBridge_MrqHandlers.cpp to call
FJsonObjectConverter::JsonObjectToUStruct on Payload and validate
required/defaulted fields on the struct rather than hand-parsing, and then use
the typed fields in the existing logic (replace occurrences of
Payload->TryGetStringField, Payload.IsValid() branches and similar branches at
the other noted locations with struct-driven handling).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68acc211-aa4b-42af-951f-4c1338eea0fd
📒 Files selected for processing (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp
|
Thanks for the MRQ implementation! I reviewed the full diff and have one critical issue and a few minor ones. Critical: UE 5.0-5.1 Compilation FailureThe file header (lines 30-32) claims: But this is not implemented. There's only one __has_include\ check: \\cpp Verified across all UE versions:
The code also uses \UMoviePipelinePrimaryConfig\ directly in ~12 places (Casts, LoadObject), which will fail to compile on UE 5.0-5.1 even if the include is fixed. Suggested fix: Then replace all \UMoviePipelinePrimaryConfig*\ with \MRQConfigClass*\ throughout. Minor Issues
What Looks Good
Once the UE 5.0-5.1 compatibility is fixed, this is ready to merge. |
UE 5.0-5.1 used MoviePipelineMasterConfig.h (UMoviePipelineMasterConfig), renamed to MoviePipelinePrimaryConfig.h in 5.2+. Add nested __has_include check with a using alias so the handler compiles on all UE 5.x versions. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp (2)
182-183:⚠️ Potential issue | 🟠 MajorRequire explicit
jobIndexinstead of defaulting to0.At Lines 345, 501, and 719 (and current read branches), missing
jobIndexsilently targets job 0. For mutating actions, this can modify the wrong job on malformed requests.✅ Suggested pattern (apply to all affected branches)
- int32 JobIndex = 0; - Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex); + int32 JobIndex = INDEX_NONE; + if (!Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex)) + { + TSharedPtr<FJsonObject> Err = MakeShared<FJsonObject>(); + Err->SetStringField(TEXT("error"), TEXT("MISSING_JOB_INDEX")); + SendAutomationResponse(RequestingSocket, RequestId, false, + TEXT("This action requires explicit jobIndex"), Err, TEXT("MISSING_JOB_INDEX")); + return true; + }Also applies to: 280-281, 345-347, 446-447, 501-503, 715-721
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp` around lines 182 - 183, Currently code defaults JobIndex to 0 when Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex) fails; change to require an explicit jobIndex by first declaring an uninitialized int32 (e.g., int32 JobIndex;) or a sentinel, call Payload->TryGetNumberField and check its boolean result, and if it returns false or the value is out of range return an error/early-fail response for the request instead of proceeding; apply this pattern to all handlers that use JobIndex (the occurrences using JobIndex and Payload->TryGetNumberField in McpAutomationBridge_MrqHandlers.cpp) so mutating actions never silently target job 0.
538-553:⚠️ Potential issue | 🔴 CriticalHarden
outputDirectoryvalidation usingSanitizeProjectRelativePath().Line 552 still persists a user path after only
..checks; absolute (C:\...) and UNC (\\...) paths can still pass.As per coding guidelines, use `SanitizeProjectRelativePath()` to validate and sanitize all file paths to prevent directory traversal attacks.🔒 Proposed fix
if (Payload->TryGetStringField(TEXT("outputDirectory"), StrVal)) { - // Normalize and reject path traversal - FString CleanDir = StrVal; - FPaths::NormalizeFilename(CleanDir); - FPaths::CollapseRelativeDirectories(CleanDir); - if (CleanDir.Contains(TEXT(".."))) + FString SanitizedDir; + if (!SanitizeProjectRelativePath(StrVal, SanitizedDir)) { TSharedPtr<FJsonObject> Err2 = MakeShared<FJsonObject>(); Err2->SetStringField(TEXT("error"), TEXT("INVALID_PATH")); SendAutomationResponse(RequestingSocket, RequestId, false, - TEXT("outputDirectory rejected: path traversal (..) not allowed"), Err2, TEXT("INVALID_PATH")); + TEXT("outputDirectory must be a valid project-relative path"), Err2, TEXT("INVALID_PATH")); return true; } - Output->OutputDirectory.Path = CleanDir; + Output->OutputDirectory.Path = SanitizedDir; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp` around lines 538 - 553, The current validation for outputDirectory (inside the Payload->TryGetStringField block) only checks for ".." and can still accept absolute or UNC paths; replace this with UE's SanitizeProjectRelativePath() call to validate and normalize the path before assigning to Output->OutputDirectory.Path. Specifically, call SanitizeProjectRelativePath(StrVal, /*out*/ CleanDir) (or the correct overload) and if it returns false or produces an empty result, build the Err2 JSON and call SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("outputDirectory rejected: invalid or non-project-relative path"), Err2, TEXT("INVALID_PATH")) and return true; otherwise assign the sanitized CleanDir to Output->OutputDirectory.Path. Ensure you keep the same error JSON shape and early-return behavior used currently in this function.
🤖 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_MrqHandlers.cpp`:
- Around line 20-23: The header block in McpAutomationBridge_MrqHandlers.cpp
incorrectly lists a non-existent action "save_preset"; update the comment to
reflect only implemented actions (e.g., keep "load_preset" and remove
"save_preset") or, if saving should be supported, add the corresponding handler
implementation instead; edit the comment near the top of the file (the Section
3: Preset Management block) to either remove "save_preset" or replace it with
the actual implemented action names so the header accurately documents the file.
- Around line 109-113: The code currently parses JSON payloads field-by-field
(e.g., the Payload variable and SubAction extraction in
McpAutomationBridge_MrqHandlers.cpp) which should be replaced by structured
deserialization using FJsonObjectConverter; define USTRUCTs (or plain UStructs)
matching each action's payload (including jobIndex with the correct type and
optional/default handling), then replace repeated Payload->TryGet*Field calls
across the handlers (the blocks around lines 109–113, 335–440, 490–583, etc.)
with FJsonObjectConverter::JsonObjectToUStruct or JsonObjectStringToUStruct
calls to populate those structs, validate the resulting struct fields, and
update the handler functions to use the strongly typed members instead of manual
TryGet*Field parsing.
---
Duplicate comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp`:
- Around line 182-183: Currently code defaults JobIndex to 0 when
Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex) fails; change to require
an explicit jobIndex by first declaring an uninitialized int32 (e.g., int32
JobIndex;) or a sentinel, call Payload->TryGetNumberField and check its boolean
result, and if it returns false or the value is out of range return an
error/early-fail response for the request instead of proceeding; apply this
pattern to all handlers that use JobIndex (the occurrences using JobIndex and
Payload->TryGetNumberField in McpAutomationBridge_MrqHandlers.cpp) so mutating
actions never silently target job 0.
- Around line 538-553: The current validation for outputDirectory (inside the
Payload->TryGetStringField block) only checks for ".." and can still accept
absolute or UNC paths; replace this with UE's SanitizeProjectRelativePath() call
to validate and normalize the path before assigning to
Output->OutputDirectory.Path. Specifically, call
SanitizeProjectRelativePath(StrVal, /*out*/ CleanDir) (or the correct overload)
and if it returns false or produces an empty result, build the Err2 JSON and
call SendAutomationResponse(RequestingSocket, RequestId, false,
TEXT("outputDirectory rejected: invalid or non-project-relative path"), Err2,
TEXT("INVALID_PATH")) and return true; otherwise assign the sanitized CleanDir
to Output->OutputDirectory.Path. Ensure you keep the same error JSON shape and
early-return behavior used currently in this function.
🪄 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: c6073a54-5a8e-478b-ace5-c3da013cdf8e
📒 Files selected for processing (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp
...s/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp
Show resolved
Hide resolved
| FString SubAction; | ||
| if (Payload.IsValid()) | ||
| { | ||
| Payload->TryGetStringField(TEXT("action"), SubAction); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify manual JSON parsing density and absence of FJsonObjectConverter usage in this handler.
rg -n 'TryGet(String|Number|Bool|Object|Array)Field|Set(String|Number|Bool|Object|Array)Field' plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp
rg -n 'FJsonObjectConverter' plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cppRepository: ChiR24/Unreal_mcp
Length of output: 10061
Refactor to use FJsonObjectConverter for payload deserialization.
This file contains extensive manual JSON field parsing throughout (lines 109–113, 335–440, 490–583, and beyond). Per coding guidelines, structured payloads should be deserialized using FJsonObjectConverter instead of repeated field-by-field TryGet*Field calls. This approach will eliminate parsing drift, enforce consistent validation (e.g., jobIndex handling), and reduce code duplication across action handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp`
around lines 109 - 113, The code currently parses JSON payloads field-by-field
(e.g., the Payload variable and SubAction extraction in
McpAutomationBridge_MrqHandlers.cpp) which should be replaced by structured
deserialization using FJsonObjectConverter; define USTRUCTs (or plain UStructs)
matching each action's payload (including jobIndex with the correct type and
optional/default handling), then replace repeated Payload->TryGet*Field calls
across the handlers (the blocks around lines 109–113, 335–440, 490–583, etc.)
with FJsonObjectConverter::JsonObjectToUStruct or JsonObjectStringToUStruct
calls to populate those structs, validate the resulting struct fields, and
update the handler functions to use the strongly typed members instead of manual
TryGet*Field parsing.
The header listed save_preset as an implemented action but it was never added. Remove the misleading comment. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp (3)
344-346:⚠️ Potential issue | 🟠 MajorRequire explicit
jobIndexin all mutating actions.
set_cvars,set_output_settings, andload_presetstill proceed with implicitjobIndex = 0when the field is missing, which can silently mutate the wrong job.Suggested fix
- int32 JobIndex = 0; - Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex); + int32 JobIndex = 0; + if (!Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex)) + { + TSharedPtr<FJsonObject> Err = MakeShared<FJsonObject>(); + Err->SetStringField(TEXT("error"), TEXT("MISSING_JOB_INDEX")); + SendAutomationResponse(RequestingSocket, RequestId, false, + TEXT("This action requires explicit jobIndex"), Err, TEXT("MISSING_JOB_INDEX")); + return true; + }Also applies to: 500-501, 714-720
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp` around lines 344 - 346, The handlers currently default JobIndex to 0 when calling Payload->TryGetNumberField, which lets mutating actions silently affect job 0; update each mutating handler (e.g., the set_cvars, set_output_settings, load_preset handlers in McpAutomationBridge_MrqHandlers.cpp and the other occurrences around the indicated ranges) to require the "jobIndex" field: remove implicit initialization to 0, call Payload->TryGetNumberField(TEXT("jobIndex"), JobIndex) and check its boolean return (or validate presence) and if missing return an error/early-fail (logging or sending a failure response) instead of proceeding; ensure you reference the JobIndex variable and the exact handler function names when adding the presence check so no handler proceeds without an explicit jobIndex.
537-552:⚠️ Potential issue | 🔴 CriticalHarden
outputDirectoryvalidation withSanitizeProjectRelativePath().Current checks block
..but still permit absolute/UNC paths. This should use the project sanitizer before persisting.As per coding guidelines, use `SanitizeProjectRelativePath()` to validate and sanitize all file paths to prevent directory traversal attacks.Suggested fix
if (Payload->TryGetStringField(TEXT("outputDirectory"), StrVal)) { - // Normalize and reject path traversal - FString CleanDir = StrVal; - FPaths::NormalizeFilename(CleanDir); - FPaths::CollapseRelativeDirectories(CleanDir); - if (CleanDir.Contains(TEXT(".."))) + FString SanitizedDir; + if (!SanitizeProjectRelativePath(StrVal, SanitizedDir)) { TSharedPtr<FJsonObject> Err2 = MakeShared<FJsonObject>(); Err2->SetStringField(TEXT("error"), TEXT("INVALID_PATH")); SendAutomationResponse(RequestingSocket, RequestId, false, - TEXT("outputDirectory rejected: path traversal (..) not allowed"), Err2, TEXT("INVALID_PATH")); + TEXT("outputDirectory must be a valid project-relative path"), Err2, TEXT("INVALID_PATH")); return true; } - Output->OutputDirectory.Path = CleanDir; + Output->OutputDirectory.Path = SanitizedDir; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp` around lines 537 - 552, The outputDirectory handling currently normalizes and checks for ".." but still allows absolute/UNC paths; replace the manual checks by passing the extracted StrVal through SanitizeProjectRelativePath() (use Payload->TryGetStringField to obtain StrVal), verify the returned FString is non-empty and does not start with a path root, and only then assign it to Output->OutputDirectory.Path; on failure call SendAutomationResponse(RequestingSocket, RequestId, false, ...) with an INVALID_PATH error (keep existing error semantics) so paths are properly sanitized and rejected when unsafe.
108-113: 🛠️ Refactor suggestion | 🟠 MajorMove payload parsing to typed structs via
FJsonObjectConverter.Manual
TryGet*Fieldparsing is pervasive and drifts validation behavior across actions; this should be consolidated with structured deserialization.As per coding guidelines, use
FJsonObjectConverterfor JSON parsing and struct serialization instead of manual JSON parsing.Also applies to: 335-440, 489-583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp` around lines 108 - 113, Replace manual field extraction (the SubAction variable and Payload->TryGetStringField usage) with structured deserialization using FJsonObjectConverter: define a lightweight UStruct (e.g., FMrqActionPayload with an FString Action and other expected fields) and call FJsonObjectConverter::JsonObjectToUStruct on the incoming Payload to populate it, then use the typed struct's Action (ToLower) instead of SubAction; repeat the same pattern for the other handler blocks referenced (around lines 335-440 and 489-583) so all JSON parsing uses the same typed structs and FJsonObjectConverter for consistent validation and easier maintenance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp`:
- Around line 344-346: The handlers currently default JobIndex to 0 when calling
Payload->TryGetNumberField, which lets mutating actions silently affect job 0;
update each mutating handler (e.g., the set_cvars, set_output_settings,
load_preset handlers in McpAutomationBridge_MrqHandlers.cpp and the other
occurrences around the indicated ranges) to require the "jobIndex" field: remove
implicit initialization to 0, call Payload->TryGetNumberField(TEXT("jobIndex"),
JobIndex) and check its boolean return (or validate presence) and if missing
return an error/early-fail (logging or sending a failure response) instead of
proceeding; ensure you reference the JobIndex variable and the exact handler
function names when adding the presence check so no handler proceeds without an
explicit jobIndex.
- Around line 537-552: The outputDirectory handling currently normalizes and
checks for ".." but still allows absolute/UNC paths; replace the manual checks
by passing the extracted StrVal through SanitizeProjectRelativePath() (use
Payload->TryGetStringField to obtain StrVal), verify the returned FString is
non-empty and does not start with a path root, and only then assign it to
Output->OutputDirectory.Path; on failure call
SendAutomationResponse(RequestingSocket, RequestId, false, ...) with an
INVALID_PATH error (keep existing error semantics) so paths are properly
sanitized and rejected when unsafe.
- Around line 108-113: Replace manual field extraction (the SubAction variable
and Payload->TryGetStringField usage) with structured deserialization using
FJsonObjectConverter: define a lightweight UStruct (e.g., FMrqActionPayload with
an FString Action and other expected fields) and call
FJsonObjectConverter::JsonObjectToUStruct on the incoming Payload to populate
it, then use the typed struct's Action (ToLower) instead of SubAction; repeat
the same pattern for the other handler blocks referenced (around lines 335-440
and 489-583) so all JSON parsing uses the same typed structs and
FJsonObjectConverter for consistent validation and easier maintenance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0f6e6bb-6c51-4f08-9a78-69e9cc9c5874
📒 Files selected for processing (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MrqHandlers.cpp
|
Thanks for addressing the \MoviePipelinePrimaryConfig\ header compatibility. However, there's a second critical UE 5.0-5.1 incompatibility that wasn't addressed. The \UMoviePipelineConsoleVariableSetting\ API changed significantly between UE 5.1 and 5.2: UE 5.0-5.1: UE 5.2+: The PR uses the UE 5.2+ API in \get_cvars\ and \set_cvars\ handlers: Additionally, \FMoviePipelineConsoleVariableEntry\ doesn't exist in UE 5.0-5.1. Fix needed: Add version-specific handling: \\cpp Other Minor Issues
|
2 participants
Summary
Adds a new
manage_mrqtool for full Movie Render Queue automation:get_queue,get_job_config,get_cvars,set_cvars,get_output_settings,set_output_settings,create_job,delete_job,render,get_render_status,load_presetDesign decisions
UMoviePipelinePrimaryConfig) for compatibility across UE 5.0–5.7. Movie Render Graph (UMovieGraphConfig) is newer but many node subclasses lack API export macros for external modules.AddOptionalDynamicModule()with delay-load DLLs — plugin compiles and loads even if the MRQ plugin is disabled. C++ handler returns a clean error if MRQ classes aren't available._MrqHandlers.cpp, TypeScript handler inhandlers/mrq-handlers.ts, registration in consolidated files.Files changed
McpAutomationBridge.Build.csMcpAutomationBridgeSubsystem.hHandleMrqActiondeclarationMcpAutomationBridgeSubsystem.cppmanage_mrqhandlerMcpAutomationBridge_MrqHandlers.cppconsolidated-tool-definitions.tsmanage_mrqtool definitionconsolidated-tool-handlers.tshandlers/mrq-handlers.tsTest plan
Tested end-to-end on UE 5.7 (compiled clean on both 5.6 and 5.7):
get_queue— returns empty queue, correct job countcreate_job— creates named job at correct indexget_job_config— full config dump with output settings and settings listget_cvars— reads empty CVars, then confirms set CVarsset_cvars(set) — addedr.ScreenPercentage=200,r.Shadow.MaxResolution=4096set_cvars(remove) — removed single CVar, confirmed count decreasedget_output_settings— returns default 1920x1080set_output_settings— changed to 3840x2160, custom output directoryload_preset— correctPRESET_NOT_FOUNDerror on missing assetdelete_job— removed job, confirmed 0 remainingget_render_status— returnsisRendering: falsewhen idlePlugin loads cleanly with MRQ plugin both enabled and disabled (optional dependency).