Skip to content

fix: M1 stop-the-bleeding - 11 honest tools + exec-sim regression harness - #13

Merged
JosephOIbrahim merged 4 commits into
masterfrom
m1-stop-the-bleeding
Jul 2, 2026
Merged

fix: M1 stop-the-bleeding - 11 honest tools + exec-sim regression harness#13
JosephOIbrahim merged 4 commits into
masterfrom
m1-stop-the-bleeding

Conversation

@JosephOIbrahim

@JosephOIbrahim JosephOIbrahim commented Jul 2, 2026

Copy link
Copy Markdown
Owner

What

Commit 1 (fixes): all 11 confirmed bugs from the hand-verified 2026-06-11 review — the actor-resolver asset-API misuse (delete/set_transform), trueTrue, spawn_blueprint label indent, load_level result honesty, focus_actor guard, is_hidden phantom, find_assets escaping, the cloner arg-discard (real fix: _safe_set applied/skipped reporting + mesh child attach), the perception fallback race (trigger→poll→read across separate editor executions with honest capture_status), the metrics uptime-rounding flake, and the wheel-killing shim import.

Commit 2 (harness): tests/exec_sim/ — strict fake unreal module (phantom APIs deliberately absent), a 56-tool registry (47 CODEGEN / 9 DIRECT) with sentinel kwargs, four gates per codegen tool (completeness, compile, exec-under-stub, sentinel survival) plus scripted-failure honesty contracts. Verified red on the unfixed tree (22 failures, all mapping 1:1 to known bugs), green after the fixes. ue_undo/ue_redo become honest not-implemented responses (no editor-transaction route exists in the UE Python API — Epic's 5.8 MCP ships none either); ue_get_material_parameters gets the Material/MIC API-family branch fix.

Commit 3: collision-free sentinels from the adversarial verify wave.

Verification

  • 562 passed, 6 skipped (147 net-new tests); ruff clean
  • ue_mcp.mcp_server imports from outside the repo root (wheel canary)
  • 4-reviewer adversarial verify wave: all pass, no blockers; M2 hardening follow-ups filed (registry perimeter, DIRECT zero-code assertion, expect_error contract, failure-classifier breadth)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added broader test coverage for generated editor scripts and a new exec-simulation harness to better validate tool behavior.
    • Improved material parameter output to handle more asset types and return cleaner, more consistent JSON.
    • Enhanced cloner creation to report applied settings, skipped steps, and mesh attachment status.
  • Bug Fixes

    • Fixed actor lookup for path-based actions, improving delete/transform handling.
    • Improved level loading, undo/redo messaging, actor focus behavior, and screenshot fallback reliability.
    • Corrected asset search escaping and several generated script formatting/boolean issues.

Joseph Ibrahim and others added 3 commits July 2, 2026 10:24
Every fix traces to the hand-verified 2026-06-11 codebase review:
- codegen delete_actor/set_transform: resolve level actors via
  load_object (asset API returned None for subobject paths -> NOT_FOUND)
- blueprints set_component_property: JS `true` -> True (NameError after
  the mutation already ran); spawn_blueprint label indent (8 spaces in
  the nested template -> was IndentationError, never spawned with label)
- level load_level: report the actual load_level() return instead of
  unconditional loaded:true on a destructive op
- editor focus_actor: success only when the focus path actually ran;
  drop duplicate deprecated selection call
- scene get_actor_details: actor.hidden attribute (is_hidden() is not
  a UFUNCTION -> AttributeError)
- assets find_assets: escape_for_fstring on the pattern (only codegen
  path that skipped escaping; broke on quotes/backslashes)
- mograph create_cloner: layout/count/spacing/mesh were validated then
  DISCARDED - now configured post-spawn via _safe_set with per-property
  applied/skipped reporting (lighting.py pattern) + mesh child attach
- perception fallback: take_high_res_screenshot completes on a later
  frame - split into trigger -> poll -> read editor round-trips with
  honest capture_status (was: image:"" with success:true)
- metrics: uptime rounded to 3 decimals (round(x,1) + coarse Py3.12
  monotonic made test_uptime flake)
- mcp_server: import remote_control directly; drop sys.path hack and
  the E402 exemption -> module imports cleanly outside the repo root

415 tests green; ruff clean. Exec-sim regression harness lands separately.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…param fix

tests/exec_sim/ — the layer the mock suite could never be: a strict fake
`unreal` module (phantom APIs deliberately absent), a registry classifying
all 56 tools (49 CODEGEN / 7 DIRECT) with sentinel kwargs, and four gates
per codegen tool: registry completeness, compile, exec-under-stub, and
sentinel survival — plus scripted-failure honesty contracts for every
historical lying-tool bug. Verified red on the unfixed tree (22 failures,
all mapping 1:1 to known bugs), green after the fixes.

Rides along:
- ue_undo/ue_redo: honest not-implemented responses (no phantom-API
  probing, no editor round-trip); no editor-transaction route exists in
  the UE Python API and Epic's 5.8 MCP ships none either
- ue_get_material_parameters: branch MaterialInstanceConstant vs base
  Material API families; str(name) keys (unreal.Name broke json.dumps)
- old TestUndo/RedoAsync updated to the not-implemented contract

562 passed, 6 skipped; ruff clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adversarial review found the sentinel gate's substring matching maskable:
"1.25" hides inside "811.25" (ue_set_transform sx/sy) and "3.25" inside
spacing "433.25" (ue_create_cloner z) - a dropped kwarg could pass. All
flagged sentinels now use unique fractional patterns. Correction to the
prior commit message: registry classification is 47 CODEGEN / 9 DIRECT.
Filed for M2 hardening: registry perimeter excludes mcp_server.py-registered
tools; DIRECT classification lacks a zero-code assertion; expect_error
entries need a contract test; is_failure classifier is whitelist-based.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@JosephOIbrahim, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 46 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f23e5dc2-08ca-4f13-bdba-5643a104ab41

📥 Commits

Reviewing files that changed from the base of the PR and between 6085339 and 2b70658.

📒 Files selected for processing (6)
  • remote_control/codegen.py
  • tests/exec_sim/test_honesty.py
  • ue_mcp/tools/assets.py
  • ue_mcp/tools/editor.py
  • ue_mcp/tools/mograph.py
  • ue_mcp/tools/perception.py
📝 Walkthrough

Walkthrough

This PR fixes several UE MCP tool code-generation issues (undo/redo, actor focus, load level, material parameters, cloner creation, perception fallback capture, actor path resolution, blueprint property syntax) and adds a new exec-simulating test harness with a fake unreal stub, tool registry, fixtures, and gate/honesty tests validating generated Python.

Changes

Codegen Tool Honesty Fixes

Layer / File(s) Summary
Undo/redo and focus actor honesty
ue_mcp/tools/editor.py, tests/test_editor.py
ue_undo/ue_redo return "not implemented" errors instead of calling nonexistent APIs; ue_focus_actor gains subsystem/feature detection; tests updated to match.
Actor path resolution
remote_control/codegen.py
delete_actor_code and set_actor_transform_code resolve actors via unreal.load_object instead of EditorAssetLibrary.load_asset.
Load level and actor visibility
ue_mcp/tools/level.py, ue_mcp/tools/scene.py
ue_load_level reports failure honestly; ue_get_actor_details visibility uses actor.hidden property.
Blueprint property and asset search fixes
ue_mcp/tools/blueprints.py, ue_mcp/tools/assets.py
Fixes invalid true literal to True, corrects label indentation, and escapes search_pattern for f-string safety.
Material parameter collection rewrite
ue_mcp/tools/materials.py
Reworks ue_get_material_parameters to support both materials and material instances with JSON-safe output.
Cloner creation defensive rewrite
ue_mcp/tools/mograph.py
ue_create_cloner adds explicit error handling and tracks applied/skipped property writes and mesh attachment.
Perception fallback polling rework
ue_mcp/tools/perception.py
_fallback_capture splits into trigger and polling stages with capture-status metadata.
Import and metrics cleanup
ue_mcp/mcp_server.py, ue_mcp/metrics.py, pyproject.toml
Switches imports to remote_control, rounds uptime_s to 3 decimals, and removes an unused Ruff ignore.

Estimated code review effort: 4 (Complex) | ~60 minutes

Exec-Simulating Codegen Test Harness

Layer / File(s) Summary
Fake unreal module stub
tests/exec_sim/unreal_stub.py
Implements value types, seeded world state, whitelisted subsystems/libraries, and helpers to install, execute, and parse results.
Tool registry and sentinels
tests/exec_sim/registry.py
Defines ToolEntry, sentinel constants, and per-tool entries building REGISTRY, CODEGEN_TOOLS, DIRECT_TOOLS.
Toolbox fixture and capture wiring
tests/exec_sim/conftest.py
Implements RecordingServer, CaptureUE, and Toolbox to register and invoke tools and capture generated scripts.
Compile/execute/sentinel gate tests
tests/exec_sim/test_codegen_exec.py
Validates registry completeness, compilation, honest execution, and sentinel kwarg preservation.
Scripted-failure honesty tests
tests/exec_sim/test_honesty.py
Forces failure conditions across tools and asserts generated code doesn't falsely report success.
Harness docs and package docstring
tests/exec_sim/README.md, tests/exec_sim/__init__.py
Documents harness purpose, gates, and how to add new tools.

Estimated code review effort: 4 (Complex) | ~75 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test as Codegen Test
  participant Toolbox
  participant Tool as MCP Tool (e.g. ue_create_cloner)
  participant Stub as unreal_stub

  Test->>Toolbox: invoke(tool_name, kwargs)
  Toolbox->>Tool: call registered tool coroutine
  Tool->>Toolbox: return generated Python via CaptureUE.execute_python
  Toolbox-->>Test: captured generated script
  Test->>Stub: exec_generated(script, stub)
  Stub-->>Test: stdout with RESULT line
  Test->>Test: parse_result(stdout), is_failure(result)
  Test-->>Test: assert honest success/failure reporting
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main changes: 11 tool fixes plus the new exec-sim regression harness.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch m1-stop-the-bleeding

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…y, focus route, chokepoint escaping

The adversarial verify wave (4 reviewers, one checking against installed
UE 5.7 plugin/engine source) failed the branch on two real defects:

- perception fallback: a stranded screenshot from a previous capture was
  returned as the NEXT capture with capture_status:"ok" (empirically
  reproduced). The trigger pass now removes any pre-existing file before
  triggering; new honesty contract pins the scenario.
- create_cloner could never run on 5.7: the actor reflects as
  CEClonerActor (/Script/ClonerEffector.CEClonerActor), not the names we
  probed. Rework per plugin-source facts: correct class resolution; real
  5.7 layout catalog (SphereUniform/SphereRandom, no "Sphere");
  read-back-verified writes (SetLayoutName silently ignores unknown
  names, so "applied" now requires a round-trip match); a second
  configure pass after the async layout load; mesh child attached
  KEEP_WORLD at the cloner location (was double-transformed).

Also from the review notes:
- focus_actor: CAMERA ALIGN console fallback (5.7 LevelEditorSubsystem
  has no focus UFUNCTION) — focus claims now carry the executed route
- find_assets escaping moved INTO _CodeGen.find_assets_code so the sync
  client path is covered too; tool-layer escape removed (double-escape)

563 tests green, ruff clean. Live-editor validation of the cloner second
pass lands with the M2 smoke expansion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@JosephOIbrahim
JosephOIbrahim merged commit b8a9dc1 into master Jul 2, 2026
2 of 3 checks passed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
tests/exec_sim/unreal_stub.py (1)

430-440: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Spawned actor naming collides across repeated spawns of the same class.

Every call names the new actor f"{name}_1" and gives it the same path suffix (Line 437). Two spawns of the same class in one test session produce two world_actors entries with an identical get_path_name(), so _find_actor_by_path (Lines 387-391) will always resolve to the first one — a latent trap for any future actor-path-resolution test, which is a major theme of this PR.

♻️ Suggested fix: make spawned names unique
+    _spawn_counter = {"n": 0}
+
     class EditorActorSubsystem:
         `@staticmethod`
         def spawn_actor_from_class(cls, location, rotation, *args, **kwargs):
             if not config.spawn_actor:
                 return None
             name = getattr(cls, "__name__", "Spawned")
-            actor = _Actor(
-                f"{name}_1", class_name=name,
-                path=f"/Game/Maps/TestMap.TestMap:PersistentLevel.{name}_1",
-            )
+            _spawn_counter["n"] += 1
+            label = f"{name}_{_spawn_counter['n']}"
+            actor = _Actor(
+                label, class_name=name,
+                path=f"/Game/Maps/TestMap.TestMap:PersistentLevel.{label}",
+            )
             world_actors.append(actor)
             return actor
🤖 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/exec_sim/unreal_stub.py` around lines 430 - 440, The spawned actor
identity in spawn_actor_from_class is hardcoded to the same suffix, which makes
repeated spawns of the same class collide in world_actors and breaks
_find_actor_by_path lookups. Update spawn_actor_from_class to generate a unique
actor name/path per spawn instead of always using “_1”, and keep the path
construction consistent with that unique name so each _Actor has a distinct
get_path_name().
🤖 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 `@ue_mcp/tools/materials.py`:
- Around line 179-180: Reject non-material assets in the asset handling branch
that uses load_asset(), since it currently treats textures/static meshes as
valid inputs and later returns success with empty parameters. Add an explicit
type guard around the existing is_instance/base material logic in materials.py
so only unreal.Material and unreal.MaterialInstanceConstant are accepted, and
make _collect() or the caller return an error for any other asset type instead
of swallowing the Unreal API failure.

In `@ue_mcp/tools/mograph.py`:
- Around line 111-126: The mesh setup flow in mograph.py leaves behind a spawned
StaticMeshActor if `set_static_mesh()` or `child.attach_to_actor()` fails after
`subsystem.spawn_actor_from_class()` succeeds. Track the spawned `child` outside
the `try` block in this mesh-attachment path, and in the `except` handler for
the same section, destroy that actor before exiting while still appending the
skip reason. Make sure the cleanup is applied around the `mesh_attached` logic
so any partially created actor is removed on failure.

In `@ue_mcp/tools/perception.py`:
- Around line 100-129: The editor Python generated in perception.py is
vulnerable because the user-controlled format value is interpolated directly
into the script and path literal, which can break the generated code. In the
trigger_code block that builds out_path, validate format against an allowed set
before use, and emit the path via a quoted/escaped representation such as
json.dumps() or repr() so the follow-up poll/read snippets stay syntactically
safe. Apply the same hardening in the related capture/read path generation logic
referenced by the perception fallback flow.

---

Nitpick comments:
In `@tests/exec_sim/unreal_stub.py`:
- Around line 430-440: The spawned actor identity in spawn_actor_from_class is
hardcoded to the same suffix, which makes repeated spawns of the same class
collide in world_actors and breaks _find_actor_by_path lookups. Update
spawn_actor_from_class to generate a unique actor name/path per spawn instead of
always using “_1”, and keep the path construction consistent with that unique
name so each _Actor has a distinct get_path_name().
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 490c57b2-0abf-4bcf-82b6-2753b31cc687

📥 Commits

Reviewing files that changed from the base of the PR and between 35268c3 and 6085339.

📒 Files selected for processing (20)
  • pyproject.toml
  • remote_control/codegen.py
  • tests/exec_sim/README.md
  • tests/exec_sim/__init__.py
  • tests/exec_sim/conftest.py
  • tests/exec_sim/registry.py
  • tests/exec_sim/test_codegen_exec.py
  • tests/exec_sim/test_honesty.py
  • tests/exec_sim/unreal_stub.py
  • tests/test_editor.py
  • ue_mcp/mcp_server.py
  • ue_mcp/metrics.py
  • ue_mcp/tools/assets.py
  • ue_mcp/tools/blueprints.py
  • ue_mcp/tools/editor.py
  • ue_mcp/tools/level.py
  • ue_mcp/tools/materials.py
  • ue_mcp/tools/mograph.py
  • ue_mcp/tools/perception.py
  • ue_mcp/tools/scene.py
💤 Files with no reviewable changes (1)
  • pyproject.toml

Comment thread ue_mcp/tools/materials.py
Comment on lines +179 to +180
is_instance = hasattr(unreal, 'MaterialInstanceConstant') and isinstance(asset, unreal.MaterialInstanceConstant)
base = asset.get_base_material() if is_instance else asset

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect existing tests and UE material API assumptions around this tool.
rg -n 'ue_get_material_parameters|get_material_parameters|MaterialInstanceConstant|unreal\.Material' tests ue_mcp

Repository: JosephOIbrahim/UnrealEngine_Bridge

Length of output: 4002


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the relevant tool and test files, then inspect the material parameter path.
ast-grep outline ue_mcp/tools/materials.py --view expanded
printf '\n--- materials.py excerpt ---\n'
sed -n '150,230p' ue_mcp/tools/materials.py

printf '\n--- test_materials.py excerpt ---\n'
sed -n '300,360p' tests/test_materials.py

printf '\n--- unreal stub excerpt ---\n'
sed -n '190,230p' tests/exec_sim/unreal_stub.py

printf '\n--- search for any handling of non-material assets ---\n'
rg -n 'is_material|MaterialInstanceConstant|get_base_material\(\)|sanitize_content_path|load_asset' ue_mcp/tools tests

Repository: JosephOIbrahim/UnrealEngine_Bridge

Length of output: 15003


🌐 Web query:

Unreal Python API Material MaterialInstanceConstant class docs isinstance

💡 Result:

In the Unreal Engine Python API, the MaterialInstanceConstant class is represented by the unreal.MaterialInstanceConstant type [1][2]. It is a standard Unreal Object (UObject) subclass used to manage material instances that are calculated prior to runtime [3]. To check if an object is an instance of this class in your Python script, you use the standard Python isinstance function [1]. Example usage: import unreal # Assuming 'my_asset' is an object loaded from the Content Browser if isinstance(my_asset, unreal.MaterialInstanceConstant): print("This is a Material Instance Constant.") Key technical details: - Module Location: The class is part of the unreal module, which provides access to the engine's reflection system [1]. - Functionality: MaterialInstanceConstant objects are used to modify material appearances without requiring expensive shader recompilation [1]. - Editor Manipulation: While many properties can be accessed directly on the object, common editing operations—such as setting parameters—are often handled via the unreal.MaterialEditingLibrary [4] or specific factory classes when creating new assets [5][6]. For the most accurate and version-specific documentation, you should refer to the Unreal Engine Python API reference corresponding to your specific engine version (e.g., 5.3, 5.4, etc.) via the Epic Developer Community portal [1][7].

Citations:


🌐 Web query:

Unreal Python API Material class unreal.Material docs

💡 Result:

In the Unreal Engine Python API, the unreal.Material class is an asset-level class used to define the visual characteristics of a surface [1]. It is a child class of unreal.MaterialInterface [2]. Key resources and classes related to working with Materials in Python include: 1. unreal.MaterialInterface: This is the base class for materials [2]. Many common operations and properties, such as retrieving the base material, blend modes, or physical materials, are handled through this interface [2]. 2. unreal.MaterialEditingLibrary: This class provides a library of functions for creating and editing material assets within the editor [3]. It includes functionality to get material expressions, manage material usage, and recompile materials [3]. 3. unreal.MaterialLibrary: This class provides Kismet-style library functions for runtime material tasks, such as creating Dynamic Material Instances [4]. 4. unreal.MaterialInstanceDynamic: This class is used for materials that need to be modified at runtime, allowing you to set scalar, vector, and texture parameters dynamically [5]. You can find the official documentation by navigating the Unreal Engine Python API reference on the Epic Developer Community website [3][2][5]. As the Python API is often updated with engine versions, ensure you are viewing the documentation version that matches your current Unreal Engine installation (e.g., 5.4, 5.5, or newer) [3][5].

Citations:


Reject non-material assets here. load_asset() can return textures/static meshes, and this branch treats them as valid materials; _collect() swallows the Unreal API failures and the tool still returns success with empty parameters. Add a guard for unreal.Material / unreal.MaterialInstanceConstant and return an error for anything else.

🤖 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 `@ue_mcp/tools/materials.py` around lines 179 - 180, Reject non-material assets
in the asset handling branch that uses load_asset(), since it currently treats
textures/static meshes as valid inputs and later returns success with empty
parameters. Add an explicit type guard around the existing is_instance/base
material logic in materials.py so only unreal.Material and
unreal.MaterialInstanceConstant are accepted, and make _collect() or the caller
return an error for any other asset type instead of swallowing the Unreal API
failure.

Comment thread ue_mcp/tools/mograph.py
Comment on lines +111 to +126
mesh_attached = False
try:
mesh = unreal.EditorAssetLibrary.load_asset("{safe_mesh}")
if mesh:
child = subsystem.spawn_actor_from_class(
unreal.StaticMeshActor, unreal.Vector({x}, {y}, {z}), unreal.Rotator(0, 0, 0))
if child:
child.static_mesh_component.set_static_mesh(mesh)
child.attach_to_actor(cloner, "", unreal.AttachmentRule.KEEP_RELATIVE,
unreal.AttachmentRule.KEEP_RELATIVE,
unreal.AttachmentRule.KEEP_RELATIVE, False)
mesh_attached = True
else:
skipped.append("mesh: asset not found {safe_mesh}")
except Exception as e:
skipped.append("mesh: " + str(e)[:80])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the file structure first, then inspect the relevant range.
ast-grep outline ue_mcp/tools/mograph.py --view expanded || true

echo
echo "=== Relevant lines in ue_mcp/tools/mograph.py ==="
cat -n ue_mcp/tools/mograph.py | sed -n '1,220p'

echo
echo "=== Search for child cleanup / destroy_actor patterns in ue_mcp/tools ==="
rg -n "destroy_actor|destroy_actor\(|spawn_actor_from_class|attach_to_actor|StaticMeshActor|mesh_attached" ue_mcp/tools -S

Repository: JosephOIbrahim/UnrealEngine_Bridge

Length of output: 12800


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Nearby cleanup pattern in ue_mcp/tools/lighting.py ==="
cat -n ue_mcp/tools/lighting.py | sed -n '160,205p'

echo
echo "=== Surrounding cloner/mesh block in ue_mcp/tools/mograph.py ==="
cat -n ue_mcp/tools/mograph.py | sed -n '118,139p'

echo
echo "=== Search for any cleanup in mograph.py around spawned mesh actor ==="
rg -n "mesh_attached|destroy_actor|spawn_actor_from_class|attach_to_actor|set_static_mesh" ue_mcp/tools/mograph.py -n -S

Repository: JosephOIbrahim/UnrealEngine_Bridge

Length of output: 4398


Clean up the spawned mesh actor on setup failure. If set_static_mesh() or attach_to_actor() throws after spawn_actor_from_class() creates child, the level keeps a stray StaticMeshActor; track it outside the try and destroy it in the except before returning.

🤖 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 `@ue_mcp/tools/mograph.py` around lines 111 - 126, The mesh setup flow in
mograph.py leaves behind a spawned StaticMeshActor if `set_static_mesh()` or
`child.attach_to_actor()` fails after `subsystem.spawn_actor_from_class()`
succeeds. Track the spawned `child` outside the `try` block in this
mesh-attachment path, and in the `except` handler for the same section, destroy
that actor before exiting while still appending the skip reason. Make sure the
cleanup is applied around the `mesh_attached` logic so any partially created
actor is removed on failure.

Comment on lines +100 to 129
"""Fallback: capture via editor screenshot + Python in the editor.

This re-renders the scene (performance cost) but works without the C++ plugin.
take_high_res_screenshot completes on a LATER frame, so the trigger and the
file read must be separate editor executions — one combined exec always saw
a missing file and (before the fix) reported success with an empty image.
"""
code = f"""
import unreal, json, base64, os, tempfile
trigger_code = f"""
import unreal, json, tempfile

# Get viewport info
world = unreal.EditorLevelLibrary.get_editor_world()
subsystem = unreal.get_editor_subsystem(unreal.EditorActorSubsystem)
actors = subsystem.get_all_level_actors()
level_name = world.get_name() if world else "Unknown"

# Get active viewport camera
ecs = unreal.get_editor_subsystem(unreal.UnrealEditorSubsystem)
loc, rot = unreal.Vector(), unreal.Rotator()
try:
vp = unreal.EditorLevelLibrary
loc = ecs.get_level_viewport_camera_info()[0] if hasattr(ecs, 'get_level_viewport_camera_info') else unreal.Vector()
rot = ecs.get_level_viewport_camera_info()[1] if hasattr(ecs, 'get_level_viewport_camera_info') else unreal.Rotator()
if hasattr(ecs, 'get_level_viewport_camera_info'):
loc, rot = ecs.get_level_viewport_camera_info()
except Exception:
pass

# Selected actors
selected = []
sel = unreal.EditorUtilityLibrary.get_selected_assets() if hasattr(unreal, 'EditorUtilityLibrary') else []
try:
sel_actors = unreal.get_editor_subsystem(unreal.EditorActorSubsystem).get_selected_level_actors()
selected = [a.get_actor_label() for a in sel_actors]
selected = [a.get_actor_label() for a in subsystem.get_selected_level_actors()]
except Exception:
pass

# Capture via screenshot
tmp_dir = tempfile.gettempdir().replace("\\\\", "/")
out_path = tmp_dir + "/ue_perception_capture.{format}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Validate format and quote out_path before generating editor Python.

format is user-controlled and embedded into generated Python/paths; a quote or newline can break the script literal. Also emit out_path with json.dumps()/repr() before building the poll/read snippets.

Suggested hardening
 async def _fallback_capture(ue, width: int, height: int, format: str) -> dict:
+    safe_format = format.lower()
+    if safe_format not in {"jpeg", "jpg", "png"}:
+        return {"output": "", "error": f"Invalid format: {format}", "result": None}
+
     """Fallback: capture via editor screenshot + Python in the editor.
@@
-out_path = tmp_dir + "/ue_perception_capture.{format}"
+out_path = tmp_dir + "/ue_perception_capture.{safe_format}"
@@
-    "format": "{format}",
+    "format": "{safe_format}",
@@
     out_path = meta.pop("out_path", "")
+    out_path_literal = json.dumps(out_path)
@@
     poll_code = (
         "import os, json\n"
-        f'print("RESULT:" + json.dumps({{"exists": os.path.exists("{out_path}")}}))'
+        f'print("RESULT:" + json.dumps({{"exists": os.path.exists({out_path_literal})}}))'
     )
@@
-with open("{out_path}", "rb") as f:
+with open({out_path_literal}, "rb") as f:
@@
-os.remove("{out_path}")
+os.remove({out_path_literal})

Also applies to: 185-205

🤖 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 `@ue_mcp/tools/perception.py` around lines 100 - 129, The editor Python
generated in perception.py is vulnerable because the user-controlled format
value is interpolated directly into the script and path literal, which can break
the generated code. In the trigger_code block that builds out_path, validate
format against an allowed set before use, and emit the path via a quoted/escaped
representation such as json.dumps() or repr() so the follow-up poll/read
snippets stay syntactically safe. Apply the same hardening in the related
capture/read path generation logic referenced by the perception fallback flow.

@JosephOIbrahim
JosephOIbrahim deleted the m1-stop-the-bleeding branch July 2, 2026 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant