feat: UE 5.8 plugin fixes (validate_niagara_system, transactional spawn, ListenPorts warning, MCP_NATIVE_PORT env)#479
Conversation
UE 5.8 changed FJsonObject::Values' key type to UE::TSharedString<TCHAR>, so Values.Contains(FString) no longer compiles. Use the public HasField() accessor. Only 5.8 compile break in the plugin. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Previously hard-coded isValid=true. Build a full (non-data-processing) Niagara system view model and harvest UNiagaraStackEntry::GetIssues() Error/Warning across the system + emitter stacks. Full mode is required: UNiagaraStackModuleItem::RefreshIssues() returns no issues in data-processing mode, so per-module errors (incl. unmet dependencies) never surface there. bCanSimulate=false avoids the preview component; SetupSequencer builds only a detached transient Sequencer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pre-spawn: fail MESH_NOT_FOUND if an explicit meshPath can't load. Post-spawn: if a resolved mesh can't be applied, Destroy() the just-spawned actor and return MESH_APPLY_FAILED, so a failed request leaves the world unchanged instead of leaking a half-configured actor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A partial ListenPorts ini override replaces the default wholesale (UE config semantics), silently dropping a default port clients may expect. Keep the user's ports authoritative but log a clear warning when multi-listen is on and a default is missing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Lets a project select a per-instance native MCP port via env without editing committed ini (multi-editor workflow). Mirrors the existing MCP_MAX_* env overrides; additive and non-breaking, falls back to the NativeMCPPort setting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
👋 Thanks for your first Pull Request! We love contributions. Please ensure you have signed off your commits and followed the contribution guidelines. |
|
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 (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughFive independent hardening changes to the MCP Automation Bridge plugin: native transport startup now reads ChangesMCP Automation Bridge Plugin Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
| // which Destroy() the new actor when component setup fails). | ||
| auto RollbackSpawn = [&Spawned]() { | ||
| if (IsValid(Spawned)) { | ||
| Spawned->Destroy(); |
There was a problem hiding this comment.
[nit] Rollback uses Spawned->Destroy(). Other control_actor paths delete editor-world actors via UEditorActorSubsystem::DestroyActor(...) (undo/editor-integrated). Destroy() matches the spline handlers so it's not wrong — just flagging the inconsistency if you'd prefer transaction-aware cleanup here.
| Options.bCanModifyEmittersFromTimeline = false; | ||
| Options.bCanSimulate = false; | ||
| Options.bCompileForEdit = false; | ||
| Options.bIsForDataProcessingOnly = false; |
There was a problem hiding this comment.
[question] Full (bIsForDataProcessingOnly=false) view model is required to get per-module stack issues (data-processing mode early-outs in UNiagaraStackModuleItem::RefreshIssues), but two costs are worth confirming: (1) it's heavyweight per call — Initialize()->RefreshAll() compiles/resets the system and builds a sequencer; (2) if the same UNiagaraSystem is already open in a Niagara editor, this spins a second full VM for it (both register undo / reset), which could disturb the open session. Should validate guard on UAssetEditorSubsystem::FindEditorsForAsset (reuse/skip if open), or is a transient second VM acceptable?
alecray
left a comment
There was a problem hiding this comment.
Self-review as a skeptical maintainer (I authored this). Reviewed all 5 changed files in full.
Checks run
npm run build(tsc) — ✅ PASS (TS unaffected; this is a C++-only PR)npm run test:native-parity— ✅ PASS (23/23 tools, 0 action mismatches; the new error codes + env var did not desync the native↔TS tool catalog)npm run test:unit(vitest) — 17/594 fail, but all pre-existing ondevand in files this PR doesn't touch (14 are parity-parser self-tests; the rest aremanage_effectrouting, environment asset-name, and pipeline-dotnet tests). Not introduced here.- cpplint not available in this environment. C++ instead compile-verified via two clean UE 5.8 builds (isolated bench + a host project) and behavior HTTP-verified (validate detects unmet-dependency errors; spawn returns
MESH_NOT_FOUND/MESH_APPLY_FAILEDand leaves the world unchanged; env override binds the override port; ListenPorts warning fires).
Security — no new input surface: meshPath still routes through SanitizeProjectRelativePath; MCP_NATIVE_PORT is integer-parsed + range-checked (1–65535); the env override changes only the port, not the host/loopback policy. No shell/path is built. Does not touch the TS safety layer.
No blockers. Items to address (left inline):
- [should-fix] Document
MCP_NATIVE_PORTand CHANGELOG the behavior changes (and consider a.upluginbump). - [should-fix] Stricter spawn-on-bad-
meshPathis potentially breaking for existing callers — call it out. - [question] Confirm the full-VM
validate_niagara_systemcost / already-open-asset case is acceptable, or guard viaFindEditorsForAsset. - [nit] Rollback
Destroy()vsUEditorActorSubsystem::DestroyActorconsistency.
Note: this bundles 5 independent fixes — happy to split into separate PRs if you'd prefer to merge piecemeal.
Addresses review feedback on PR ChiR24#479: add a CHANGELOG [Unreleased] section covering all the changes, document the new MCP_NATIVE_PORT env var in the plugin README env-var table + Native MCP Port setting, and explicitly flag the stricter control_actor spawn (bad meshPath now returns MESH_NOT_FOUND) as potentially breaking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…iewed, WPF ported, next-improvement backlog Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hello! I'm working in Unreal 5.8 and I've been using your Unreal MCP plugin. I wanted to contribute some fixes I've done for it to be less error prone. I'm not a UE developer and these were done by Claude Opus on xHigh. I've vetted the work as best I can and the decisions made. I hope this is useful!
Five small, independent fixes for the McpAutomationBridge plugin on UE 5.8 — each is its own commit, so feel free to take any subset. All were built and HTTP-verified in an isolated UE 5.8 C++ project.
1. Build on UE 5.8 (
FJsonObject::Valueskey type)UE 5.8 changed
FJsonObject::Values' key type toUE::TSharedString<TCHAR>, soSettings->Values.Contains(Field)(FString) no longer compiles in the Render console handler. Switched to the publicHasField()accessor — the only 5.8 compile break in the plugin.2.
validate_niagara_systemdetects real errorsPreviously hard-coded
isValid=truewith only soft structural warnings. Now builds a full Niagara system view model and harvestsUNiagaraStackEntry::GetIssues()(Error/Warning) across the system + emitter stacks — the same issues the editor shows, including "The module has unmet dependencies." A data-processing-only VM can't be used (UNiagaraStackModuleItem::RefreshIssues()returns nothing there). Kept lightweight:bCanSimulate=false(no preview component), compile off;SetupSequenceronly builds a detached transient Sequencer.Verified: deleted Emitter State module →
isValid:false+ unmet-dependency error; clean system →isValid:true.3.
control_actorspawn is transactionalA requested mesh that couldn't be applied used to still report success, leaving a misconfigured actor in the level. Now:
MESH_NOT_FOUNDbefore spawning if an explicitmeshPathcan't load; and rollback (Destroy()+MESH_APPLY_FAILED) if a resolved mesh can't be applied. A failed request leaves the world unchanged.4. Warn when
ListenPortsdrops a default bridge portA partial
ListenPortsoverride replaces the8090,8091default wholesale (UE config semantics), silently dropping a port clients expect. Keeps the user's ports authoritative but logs a clear warning (multi-listen) when a default is missing.5.
MCP_NATIVE_PORTenv overridePick the native MCP port via env without editing committed ini — handy for running several editors at once on distinct ports. Mirrors the existing
MCP_MAX_*env overrides; additive and non-breaking, falls back to theNativeMCPPortsetting.🤖 Generated with Claude Code