# Hayba MCP — Crash Audit + Architecture Review (2026-06-22)

Method: fanned out audit agents across ~60 C++ files under the **verified dispatch model** (every handler runs on the game thread via HaybaMCPTcpServer.cpp:133), then adversarially refuted every candidate. 59 candidates → 21 confirmed → 14 distinct after dedup. 38 refuted as false positives (mostly the incorrect "off-game-thread API" class).

> Note: one workflow-confirmed item (HaybaMCPMaterialHandler.cpp:491 "null class") was re-checked by hand and is a FALSE POSITIVE — line 492 already returns an error on null. Excluded below.

## Confirmed crashes (deduped, ranked)

| # | Severity | Class | Site | Crash |
|---|---|---|---|---|
| 1 | Critical | A-deadlock-uaf | `HaybaMCPCommandHandler.cpp:226` | CaptureBeforeState: AsyncTask(GameThread)+Wait(2s) deadlocks the game thread, then UAF on Done |
| 2 | High | B-gamethread-block | `HaybaMCPBuildHandler.cpp:248` | build_project / build_cook / build_generate_project_files block the game thread for up to 300 seconds |
| 3 | High | A-deadlock-uaf | `HaybaMCPIdleHandler.cpp:248` | GameThread deadlock: AsyncTask(GameThread) queued then game thread blocks on FEvent wait |
| 4 | High | C-null-deref | `HaybaMCPEditorHandler.cpp:105` | StaticCast<FEditorViewportClient*> on VP->GetClient() without type check — crash during PIE or non-editor viewports |
| 5 | High | B-gamethread-block | `HaybaMCPPIEHandler.cpp:325` | PIEWaitFor / PIEAssert: TimeoutMs parameter is int32 but assigned from TryGetNumberField (double) without overflow/negative check — negative value produces instant return or underflow in deadline arithmetic |
| 6 | High | F-assert | `HaybaMCPMaterialHandler.cpp:1259` | MatCompile: RecompileMaterial called on a material that may have a pathological graph — can hit engine check(NormalCodeChunk != INDEX_NONE) and crash the editor |
| 7 | High | B-gamethread-block | `HaybaMCPRenderHandler.cpp:309` | RunOnGameThread: FPlatformProcess::Sleep(0.25s) called in a subsystem-wait loop on the game thread |
| 8 | High | B-gamethread-block | `HaybaMCPTestHandler.cpp:221` | test_run: FPlatformProcess::Sleep(0.01s) busy-wait loop on the game thread for each test |
| 9 | High | F-assert | `HaybaMCPSplineHandler.cpp:159` | spline_set_point: unchecked Index passed to SetLocationAtSplinePoint — engine assert on out-of-range |
| 10 | High | A-deadlock-uaf | `HaybaMCPRenderHandler.cpp:470` | render_camera dispatches AsyncTask to the game thread then blocks the game thread — same confirmed deadlock pattern |
| 11 | Medium | D-oob | `HaybaMCPBehaviorTreeHandler.cpp:43` | FindNodeByGuidRecursive infinite recursion / stack overflow on cyclic BT graph |
| 12 | Low | B-gamethread-block | `HaybaMCPIdleHandler.cpp:269` | uint32 overflow turns user-supplied large timeout_s into near-zero FEvent wait, silently returning wrong result |
| 13 | Low | B-gamethread-block | `HaybaMCPEditorHandler.cpp:319` | StreamLog reads entire log file synchronously on the game thread — can freeze editor for seconds on large logs |

## Detail

### Critical — HaybaMCPCommandHandler.cpp:226 (A-deadlock-uaf)
**CaptureBeforeState: AsyncTask(GameThread)+Wait(2s) deadlocks the game thread, then UAF on Done**

Why: The finding is confirmed real. CaptureBeforeState (called from ProcessCommand at line 664, which runs ON the game thread per the verified dispatch model) calls AsyncTask(ENamedThreads::GameThread, ...) at line 226, then blocks with Done->Wait(2000) at line 269. Because the caller is already occupying the game thread, the enqueued task cannot be dequeued and run until ProcessCommand returns — but Wait holds ProcessCommand hostage. After the 2-second timeout expires, Wait returns, Done is returned to the pool at line 270 via ReturnSynchEventToPool(Done), and the local TMap Before goes out of scope as the function returns. The pending task is still in the game-thread queue with two dangling captures: (1) a raw FEvent* Done that now belongs to the synch-event pool (or a new owner), so Done->Trigger() at line 267 is a use-after-free/corruption of whatever event now owns that memory; (2) a reference &Before to the already-destroyed local TMap, so every Before.Add() call inside the lambda is a dangling-reference write. There is no IsInGameThread() guard anywhere in the file (grep confirmed zero matches). This deadlock fires on every actor_delete, actor_set_transform, actor_set_tags, and actor_set_property command, with the UAF/dangling-ref executing ~2 seconds later when the game thread drains its queue on the next frame after ProcessCommand returns.

Fix: In CaptureBeforeState, check IsInGameThread() at the top: if already on the game thread, execute the lambda body inline immediately (no AsyncTask, no event needed) and return. This is the only correct fix because there is no safe way to cross-schedule back to the game thread while already holding it. The Wait timeout comment ("to avoid deadlock if game thread is stalled") is wrong — the game thread is not stalled, it IS this thread. Remove the AsyncTask+Wait+ReturnSynchEventToPool pattern entirely for the game-thread caller path. Example guard: if (IsInGameThread()) { /* inline the actor-read body here, writing directly into Before */ } else { /* existing AsyncTask+Wait path for any hypothetical non-game-thread callers */ }.

### High — HaybaMCPBuildHandler.cpp:248 (B-gamethread-block)
**build_project / build_cook / build_generate_project_files block the game thread for up to 300 seconds**

Why: The finding is real. The verified dispatch model places every Handle() call on the game thread via AsyncTask(ENamedThreads::GameThread) in HaybaMCPTcpServer.cpp:133. Inside RunOrBackground() (line 215), the code fires a background AsyncTask to run the subprocess and then immediately calls Future.WaitFor(FTimespan::FromSeconds(300.0)) at line 248 — on the calling thread, which IS the game thread. This is a straight class-B game-thread block: the UE editor is frozen for up to 300 seconds on any build_project, build_cook, or build_generate_project_files command. The comment at line 228 ("The handler is already invoked off the game thread by the TCP server") is factually wrong per the verified dispatch model and is the root of the incorrect safety assumption. There is no IsInGameThread() guard, no run-inline path, and no early escape before the 300s wait.

Fix: In Handle() (or wherever Cmd_BuildProject/Cmd_BuildCook/Cmd_GenerateProjectFiles are called), wrap the entire call in AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask, ...) and return a {job_id, status:"queued"} envelope immediately to the game thread, without any blocking wait. The RunOrBackground helper can then call Future.WaitFor safely on that background thread. Alternatively, add an IsInGameThread() guard inside RunOrBackground: if running on the game thread, skip the WaitFor entirely and always return the "running" envelope, letting the background AsyncTask complete asynchronously.

### High — HaybaMCPIdleHandler.cpp:248 (A-deadlock-uaf)
**GameThread deadlock: AsyncTask(GameThread) queued then game thread blocks on FEvent wait**

Why: The verified dispatch model is confirmed by HaybaMCPTcpServer.cpp:133: Handle() executes on the game thread inside an AsyncTask(ENamedThreads::GameThread,...). The file header comment (lines 3-7) claims Handle() runs on "the TCP server's per-request handler thread (NOT the game thread)" — that assumption is wrong. At line 248, Handle() queues another AsyncTask(ENamedThreads::GameThread,...) to register the ticker and capture BusyOnEntry. At line 270, it then blocks the game thread with DoneEvent->Wait(WaitTimeoutMs). Because the game thread is already occupied running Handle(), the queued AsyncTask at line 248 cannot be dequeued and executed — UE's game thread task queue is processed between frames, not re-entrantly. The ticker is therefore never registered, PollOnce never runs, and DoneEvent->Trigger() is never called. The Wait() unblocks only after the full WaitTimeoutMs timeout (default: (uint32)(60.0 * 1000.0) + 1000 = 61,000 ms). Every single invocation of wait_for_idle or wait_for_shaders freezes the editor for 61 seconds — no ticks, no rendering, no input. This is a concrete, deterministic hang triggered by any call to either command.

Fix: Since Handle() already runs on the game thread, the AsyncTask at line 248 is unnecessary and harmful. Move its body (GC nudge, BusyOnEntry capture, AddTicker) to execute inline in Handle() before the Wait. Then replace the blocking Wait with a non-blocking approach: either (a) spin FTSTicker::GetCoreTicker().Tick(DeltaTime) in a tight loop with brief FPlatformProcess::Sleep(0.0f) calls to yield to other game-thread tasks, allowing PollOnce to run in the same frame pump without blocking; or (b) restructure as a deferred/async response that returns immediately with a pending token and sends the real response when the ticker fires Done. The simplest safe fix for the existing structure: remove AsyncTask entirely, run the setup inline (already on game thread), then loop calling FTSTicker::GetCoreTicker().Tick(POLL_INTERVAL_SECONDS) + FPlatformProcess::Sleep(POLL_INTERVAL_SECONDS) until DoneEvent is signaled, which allows the ticker delegate to fire within the same loop iteration. Note: spinning the ticker on the game thread still blocks editor rendering during the wait; a fully non-blocking deferred-response architecture is the correct long-term fix.

### High — HaybaMCPEditorHandler.cpp:105 (C-null-deref)
**StaticCast<FEditorViewportClient*> on VP->GetClient() without type check — crash during PIE or non-editor viewports**

Why: The null-check on line 106 (`if (!Client)`) does NOT protect against the PIE mistype. UE's `StaticCast<T*>` is `static_cast<T*>` — a compile-time bit-pattern reinterpret with no runtime type information. During PIE, when the user clicks into the game viewport, `GEditor->GetActiveViewport()` returns the PIE viewport (`FSceneViewport`) whose `GetClient()` returns a non-null `UGameViewportClient*`. `StaticCast<FEditorViewportClient*>` reinterprets that pointer without nulling it. The resulting `Client` pointer is non-null, so the guard on line 106/107 passes, and `Client->SetViewLocation(Location)` is dispatched through `FEditorViewportClient`'s vtable layout applied to a `UGameViewportClient` object — which is a completely unrelated class hierarchy. This is a vtable mismatch causing undefined behavior (wrong virtual dispatch, likely a crash). The exact trigger: user sends `editor_set_camera` while PIE is running and the game window has focus. The same defect exists at line 161 (CaptureViewport, calls `GetViewLocation`/`GetViewRotation` on mistyped pointer) and line 453 (SetViewportMode, calls `SetViewMode` on mistyped pointer).

Fix: Before the StaticCast, verify the client is actually an FEditorViewportClient. The safest pattern in editor code is: `FViewportClient* RawClient = VP->GetClient(); FEditorViewportClient* Client = (RawClient && RawClient->IsA(FEditorViewportClient::StaticClass())) ? static_cast&lt;FEditorViewportClient*&gt;(RawClient) : nullptr;` — then keep the existing null-return guard. Alternatively, use the Level Editor module accessor `FLevelEditorModule&amp; LEM = FModuleManager::GetModuleChecked&lt;FLevelEditorModule&gt;("LevelEditor"); TSharedPtr&lt;ILevelEditor&gt; LE = LEM.GetFirstLevelEditor(); if (!LE) return Err(...); FEditorViewportClient* Client = LE-&gt;GetActiveViewportClient().Get();` which is always typed correctly and returns null during PIE. Apply the same fix at lines 161 and 453.

### High — HaybaMCPPIEHandler.cpp:325 (B-gamethread-block)
**PIEWaitFor / PIEAssert: TimeoutMs parameter is int32 but assigned from TryGetNumberField (double) without overflow/negative check — negative value produces instant return or underflow in deadline arithmetic**

Why: Confirmed class B game-thread hang. UE 5.7's `TryConvertNumber` template (JsonValue.cpp:99) validates that the JSON double is within `[TNumericLimits<int32>::Min(), TNumericLimits<int32>::Max()]` before writing to `TimeoutMs` — so any value that fits in int32 (including large positives like 2,000,000,000) is accepted verbatim. With `timeout_ms: 2000000000` the deadline becomes `StartTime + 2,000,000.0 seconds` (~23 days). The while loop at line 337 then blocks the game thread in 5ms sleep slices (`FPlatformProcess::Sleep(0.005f)`) for that entire duration, freezing the entire UE editor. The negative-value path (`timeout_ms: -1`) is a separate semantic bug: `TryConvertNumber` also accepts -1 as valid int32, the deadline is `StartTime - 0.001`, the while condition is false immediately, and the function returns `matched:false, reason:"timeout"` without waiting at all — silently wrong for `PIEAssert` callers. Neither path is a false positive. The large-positive path is the concrete hang trigger.

Fix: After line 326, clamp the value: `TimeoutMs = FMath::Clamp(TimeoutMs, 0, 30000);`. This caps the maximum wait at 30 seconds and treats negative (or zero) inputs as an immediate poll-once-and-return, matching user intent. For PIEAssert specifically, a minimum of 1 ms (or rejecting 0) could also be enforced to avoid the silent-pass-by-instant-timeout semantic bug.

### High — HaybaMCPMaterialHandler.cpp:1259 (F-assert)
**MatCompile: RecompileMaterial called on a material that may have a pathological graph — can hit engine check(NormalCodeChunk != INDEX_NONE) and crash the editor**

Why: The crash is real and the code's own comment at lines 1247-1250 explicitly concedes it. Concrete trigger: call `material_compile` on a UMaterial whose shading model uses Substrate (MP_FrontMaterial) before connecting the FrontMaterial or Normal pin. `UMaterialEditingLibrary::RecompileMaterial` drives the full HLSL translator synchronously; inside `FHLSLMaterialTranslator` the Substrate slab compiler calls `check(NormalCodeChunk != INDEX_NONE)` when the Normal chunk was never emitted. A UE `check()` calls `FDebug::AssertFailed()` → `abort()` in non-shipping builds — it is not a C++ exception and cannot be caught. There is no pre-flight guard in `MatCompile` (lines 1251-1276): no test for `MP_FrontMaterial` shading model, no connectivity check on the Normal/FrontMaterial input, no dry-run path. An AI agent building a material incrementally (add nodes, set properties, then compile) will hit this on the very first `material_compile` call before wiring the Normal pin, crashing the entire editor process. Severity is High rather than Critical because (a) the per-edit handlers correctly defer compilation and save progress to disk, so work is not lost, and (b) the crash only fires on an explicit `material_compile` call, not silently mid-build. But the editor process does terminate unconditionally when triggered.

Fix: Before calling `RecompileMaterial`, add a Substrate pre-flight guard in `MatCompile`:

```cpp
// Guard: Substrate materials require FrontMaterial (Normal) to be connected
// before the HLSL translator runs, otherwise check(NormalCodeChunk!=INDEX_NONE) crashes.
if (Mat->IsUsingNewHLSLGenerator() || Mat->GetShadingModels().HasAnyShadingModel({MSM_Strata}))
{
    // Check that the FrontMaterial material property input is connected.
    FMaterialInputInfo FMInfo = Mat->GetExpressionInputForProperty(MP_FrontMaterial);
    if (!FMInfo.IsConnected())
        return FHaybaHandlerResult::Err(
            TEXT("material_compile: Substrate material has no FrontMaterial connection — "
                 "connect the FrontMaterial (Normal) pin before compiling to avoid a fatal engine check()"));
}
```

Alternatively, expose a `dry_run` parameter: when true, call `Mat->GetMaterialResource(GMaxRHIShaderPlatform)->GetCompileErrors()` only (skipping `RecompileMaterial`) so the agent can query errors without triggering the translator. The pre-flight connectivity check is the simpler and more robust fix.

### High — HaybaMCPRenderHandler.cpp:309 (B-gamethread-block)
**RunOnGameThread: FPlatformProcess::Sleep(0.25s) called in a subsystem-wait loop on the game thread**

Why: The verified dispatch model places Handle() on the game thread. Handle() (line 470) posts RunOnGameThread via AsyncTask(ENamedThreads::GameThread, ...) then immediately blocks (line 474) on S->DoneEvent->Wait(BlockTimeoutMs) where BlockTimeoutMs = (TimeoutSeconds + 60.0) * 1000 = 90,000 ms by default. Because Handle() is already occupying the game thread, the queued inner AsyncTask cannot dequeue and execute until Handle() returns — but Handle() is waiting for an event that only RunOnGameThread (the queued task) can Trigger(). This is a confirmed 90-second game-thread hang on every render_camera call: the inner task is permanently stuck behind the blocking outer call, so DoneEvent is never signaled until the bounded wait expires. The Sleep loop at line 309 is never reached; the actual freeze is the 90s Wait at line 474. The file's own header comment (lines 3-6) documents the correct design (Handle on TCP thread, AsyncTask for game-thread work, FEvent for sync) but the TcpServer wraps the whole command dispatch in AsyncTask(GameThread) making that design assumption invalid.

Fix: Add an IsInGameThread() guard before the AsyncTask dispatch in Handle(). If already on the game thread, call RunOnGameThread(S) directly (inline, no AsyncTask) and then read S's results without waiting on the event. Only use the AsyncTask + FEvent::Wait path when Handle() is confirmed to be running off the game thread. Concretely at line 470: if (IsInGameThread()) { RunOnGameThread(S); } else { AsyncTask(ENamedThreads::GameThread, [S]() { RunOnGameThread(S); }); S->DoneEvent->Wait(BlockTimeoutMs); } Then branch the response-building logic accordingly. Alternatively, fix at the TcpServer layer by NOT wrapping render_camera dispatch in AsyncTask(GameThread), running it on a worker thread so the FEvent+AsyncTask pattern in Handle() works as originally designed.

### High — HaybaMCPTestHandler.cpp:221 (B-gamethread-block)
**test_run: FPlatformProcess::Sleep(0.01s) busy-wait loop on the game thread for each test**

Why: Concrete trigger: send `test_run` with any test that uses `ADD_LATENT_AUTOMATION_COMMAND`. The handler runs on the game thread. Lines 221-228 call `FPlatformProcess::Sleep(0.01f)` inside the latent-command pump loop. Sleep on the game thread does NOT yield to the engine's main-loop tick machinery — engine `Tick()` calls, world timers, rendering, and input processing are all suspended while sleeping. `FAutomationTestFramework::ExecuteLatentCommands()` advances latent commands whose completion depends on engine ticks (e.g., `FWaitLatentCommand`, any command that polls actor/world state). Because no ticks occur inside Sleep, those commands never return true, so the loop runs to the full 120-second per-test wall-clock timeout. Every latent test in the suite triggers a full 120-second freeze. With `test_names=["all"]`, hundreds of latent tests each produce a 120-second editor blackout (no render, no input, no TCP keepalive). This is a concrete functional hang, not a theoretical one: the Sleep/no-tick mismatch is a known UE pitfall documented in Epic's own automation test runner, which pumps latent commands only from within the engine's main loop Tick.

Fix: Do not Sleep on the game thread. The correct pattern in UE is to drive latent automation commands from a FTickerDelegate or FTimerHandle registered on the game thread, which lets the engine tick normally between iterations. Replace the while/Sleep loop with: (1) synchronous-only mode — call ExecuteLatentCommands() once without looping (returns immediately for non-latent tests, skips latent ones); or (2) register an FTicker delegate that calls ExecuteLatentCommands() each engine tick and calls StopTest()/continues to the next test on completion or timeout, then returns the aggregated result via a completion callback (requires making the command async from the handler's perspective, e.g. returning a pending result and filling it from the ticker). Option (1) is the minimal safe fix: remove the while loop and the Sleep entirely, call ExecuteLatentCommands() once, and document that latent tests are not supported via this handler. Option (2) is correct but requires restructuring the handler to be asynchronous. Do not use Sleep, spinwait, or FEvent::Wait on the game thread for any duration longer than a single frame.

### High — HaybaMCPSplineHandler.cpp:159 (F-assert)
**spline_set_point: unchecked Index passed to SetLocationAtSplinePoint — engine assert on out-of-range**

Why: In `SplineSetPoint` (line 137-164 of HaybaMCPSplineHandler.cpp), `Index` is read from JSON at line 147 via `TryGetNumberField` and immediately passed to `Spline->SetLocationAtSplinePoint(Index, ...)` at line 159 with no range check against `Spline->GetNumberOfSplinePoints()`. The engine's `USplineComponent::SetLocationAtSplinePoint` uses `check(Index >= 0 && Index < SplineInfo.Points.Num())` — a hard assert that terminates the process in Debug/Development. Any caller sending `{"command":"spline_set_point","actor_id":"...","index":999,"location":[0,0,0]}` against a spline with fewer than 1000 points hits this unconditionally. A negative index (e.g., `index:-1`) also trips the check. The `SplineRemovePoint` path at line 184 has the same defect with `RemoveSplinePoint(Index)` and no guard.

Fix: Before line 159, add: `if (Index < 0 || Index >= Spline->GetNumberOfSplinePoints()) return FHaybaHandlerResult::Err(FString::Printf(TEXT("spline_set_point: index %d out of range (spline has %d points)"), Index, Spline->GetNumberOfSplinePoints()));`. Apply the same guard in `SplineRemovePoint` before line 184 (note: valid range there is `[0, GetNumberOfSplinePoints()-1]`, and the spline must retain at least 2 points, so also guard `Spline->GetNumberOfSplinePoints() < 2`).

### High — HaybaMCPRenderHandler.cpp:470 (A-deadlock-uaf)
**render_camera dispatches AsyncTask to the game thread then blocks the game thread — same confirmed deadlock pattern**

Why: The deadlock is real and concrete. Handle() runs on the game thread (verified dispatch model, TcpServer.cpp:133). Line 470 queues AsyncTask(ENamedThreads::GameThread, [S]{ RunOnGameThread(S); }), then line 474 immediately calls S->DoneEvent->Wait(BlockTimeoutMs) with BlockTimeoutMs = (TimeoutSeconds + 60.0) * 1000 (default ~90,000 ms). Since the game thread is already blocked inside Wait(), the queued AsyncTask cannot be scheduled until the game thread becomes free — which only happens after the bounded wait expires. Result: every render_camera call deadlocks the entire editor/game thread for the full timeout duration (90 seconds by default, or up to TimeoutSeconds+60 for any caller-supplied wait_timeout_s). The UAF portion of the claim is NOT confirmed: lines 493+ are gated behind bSignaled==true, and the timeout branch (lines 479-491) explicitly avoids reading task-written fields (correct comment at line 481). So the crash class is a guaranteed game-thread hang (class B, also meeting class A's definition of AsyncTask+Wait without IsInGameThread guard), not a UAF. The trigger is simply any call to render_camera — no special input required.

Fix: At the top of Handle() (or at line 470, before queuing), add an IsInGameThread() branch: if (IsInGameThread()) { RunOnGameThread(S); } else { AsyncTask(ENamedThreads::GameThread, [S]() { RunOnGameThread(S); }); S->DoneEvent->Wait(BlockTimeoutMs); }. Given the current dispatch model runs Handle() on the game thread, the AsyncTask+Wait path is dead code in practice; the direct inline call is always the correct path. The DoneEvent allocation and Wait can be skipped entirely when running inline, since RunOnGameThread(S) will have completed synchronously before the response-building code at line 476 is reached.

### Medium — HaybaMCPBehaviorTreeHandler.cpp:43 (D-oob)
**FindNodeByGuidRecursive infinite recursion / stack overflow on cyclic BT graph**

Why: BTConnect (line 264) appends Child into ParentComp->Children without checking whether Child is an ancestor of Parent (or equals Parent). A concrete three-command sequence creates a real cycle: (1) bt_add_node with no parent creates composite root A; (2) bt_add_node with parent_guid=A creates composite child B; (3) bt_connect with parent_guid=B, child_guid=A — FindNodeByGuidRecursive finds both (A is root, B is its child), then A is inserted into B->Children, yielding A->B->A->... Any subsequent bt_get_info, bt_add_node, or bt_connect on that BT then calls FindNodeByGuidRecursive or WalkNode on the cyclic structure, recursing without bound until a stack overflow (null-check at line 45 never fires because no pointer is null, only the graph is cyclic). The crash is deterministic and MCP-reachable with valid GUID inputs.

Fix: Add a visited-set guard to FindNodeByGuidRecursive and WalkNode. For FindNodeByGuidRecursive, thread a TSet&lt;UBTNode*&gt;& Visited parameter and add `if (!Visited.Add(Node).IsAlreadyInSet)` / `return nullptr` at entry. For WalkNode, same pattern. Alternatively (and more robustly), reject the cycle at BTConnect time: before appending, walk from Child down and verify that Parent is not reachable from Child; return an error if it is. This is the cheaper fix because it prevents the corrupt state rather than tolerating it.

### Low — HaybaMCPIdleHandler.cpp:269 (B-gamethread-block)
**uint32 overflow turns user-supplied large timeout_s into near-zero FEvent wait, silently returning wrong result**

Why: The uint32 overflow is real but not a crash. At line 269, `(uint32)(SharedState->TimeoutSeconds * 1000.0) + 1000` has two problems for large inputs: (1) casting a double >= ~4294967.296 to uint32 is undefined behavior per C++ §7.8 (value outside representable range); on MSVC x64 it typically yields 0, so `WaitTimeoutMs` becomes 0+1000 = 1000 ms regardless of the user-supplied timeout; (2) for values just under UINT32_MAX, the `+1000` addition can itself overflow to a small number. In both cases `DoneEvent->Wait` returns in ~1ms with bSignaled=false, and the handler returns a false-timeout response — zero-duration wait, wrong `durationMs`, wrong `bAllSettled`. No null dereference or memory corruption follows: `TickHandle.IsValid()` at line 286 is false (the AsyncTask that registers the ticker is blocked on the game thread and has not run), so `RemoveTicker` is skipped safely, and `SharedState` shared ownership prevents use-after-free. The overflow is a genuine silent correctness failure for any `timeout_s` >= ~4294966 s, but it does not crash.

Fix: Clamp TimeoutSeconds before computing WaitTimeoutMs. At the parse site (lines 197-211), after reading the value add: `State.TimeoutSeconds = FMath::Clamp(State.TimeoutSeconds, 0.0, 3600.0);`. Then at line 269 compute safely: `const double ClampedMs = FMath::Clamp(SharedState->TimeoutSeconds * 1000.0 + 1000.0, 1.0, (double)MAX_uint32); const uint32 WaitTimeoutMs = (uint32)ClampedMs;`. The clamp on parse is the primary fix; the safe cast is a belt-and-suspenders guard.

### Low — HaybaMCPEditorHandler.cpp:319 (B-gamethread-block)
**StreamLog reads entire log file synchronously on the game thread — can freeze editor for seconds on large logs**

Why: The synchronous `Reader->Serialize(Buf.GetData(), Size)` at line 322 blocks the game thread for the entire duration of the disk read. For a normal UE editor session the log file is typically a few MB and this is imperceptible. The `TotalSize()==INT64_MAX` OOM/overflow scenario is not realistic on a normal Windows file handle — `FArchiveFileReaderGeneric::TotalSize()` returns the actual OS file size, not a sentinel. The `Size+1` int64 overflow path therefore cannot be triggered in practice. A secondary concrete path does exist: if a log file exceeds ~2 GB (unusual but not impossible with verbose logging), `Size` (int64) is silently narrowed to int32 when passed to `TArray::SetNumUninitialized`, the buffer is under-allocated, `Serialize` writes `Size` bytes past the end of the buffer (heap corruption), and `Buf[Size] = 0` writes out of bounds — a crash. For files under 2 GB the result is a class-B game-thread hang (editor freezes, input/tick stalled) whose duration scales with file size, not a crash. This is a real but low-severity issue: it requires either a >2 GB log file to crash, or a large-enough file to cause a noticeable freeze.

Fix: Add a size guard before allocating: `const int64 kMaxRead = 64 * 1024 * 1024LL; // 64 MB` then seek to `FMath::Max(0LL, Size - kMaxRead)` before reading, and clamp the read length to `kMaxRead`. This eliminates both the game-thread stall (reads only the tail of the file) and the int64→int32 narrowing overflow (64 MB is safely within int32 range). Example: `const int64 ReadSize = FMath::Min(Size, kMaxRead); Reader->Seek(Size - ReadSize); TArray<uint8> Buf; Buf.SetNumUninitialized((int32)ReadSize + 1); Reader->Serialize(Buf.GetData(), ReadSize); Buf[ReadSize] = 0;`


## Architecture deepening candidates

### 1. A single FHaybaGameThread::Run seam every handler uses to marshal-and-wait
- **Files:** unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPLegacyHandler.cpp:51-98 (the correct copy, private/static); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPRenderHandler.cpp:191-565 (hand-rolled copy #2, missing IsInGameThread guard); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp:234-301 (hand-rolled copy #3, also unguarded); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp:133-141 (the dispatch that already puts ProcessCommand on the game thread); new file unreal/.../Public/HaybaMCPGameThread.h
- **Problem:** There are three independent implementations of 'run this work on the game thread and wait for a bounded result with a pooled FEvent.' Only one of them — FHaybaMCPLegacyHandler::RunOnGameThread — is correct, because it is the only one that asks IsInGameThread() and runs inline (legacy .cpp:53-55). The render and idle handlers omit that guard entirely (grep for IsInGameThread in both returns nothing) and unconditionally AsyncTask(ENamedThreads::GameThread) then block on DoneEvent->Wait(). But ProcessCommand already runs on the game thread at both its call sites (TcpServer.cpp:133 wraps it in AsyncTask(GameThread); HaybaMCPModule.cpp:444 calls it inline from Slate/UI = also game thread). So the idle handler schedules a core ticker that is only serviced by the game thread, then blocks the game thread waiting for it — the ticker can never fire, guaranteeing a 60s+ timeout every call; the render handler blocks the game thread on a render task that can only run on the game thread — a self-deadlock until the 90s wait expires. The marshalling correctness is decided in three places and is wrong in two of them. The FEvent-pool-return-on-timeout policy also differs across the three copies (legacy leaks on timeout by design; render/idle tie return to a shared-ptr destructor), so the one genuinely subtle reasoning is re-derived inconsistently each time.
- **Solution:** Extract the legacy handler's RunOnGameThread into one public module-level primitive — FHaybaHandlerResult FHaybaGameThread::Run(TFunction<FHaybaHandlerResult()> Work, double TimeoutSeconds). It does exactly what the legacy version does: if IsInGameThread() run Work() inline and return; otherwise marshal via AsyncTask + pooled FEvent, block with the bounded timeout, and apply the single agreed on-timeout policy (leak the event, return a framed timeout error). Every handler that touches UWorld/GEditor/LoadObject calls it instead of hand-rolling. The render and idle handlers collapse their bespoke FRenderState/FWaitState FEvent plumbing onto it: their game-thread bodies become the Work lambda, and because Run short-circuits when already on the game thread, the deadlock disappears — the render/wait code just runs inline. Delete the now-stale 'NOT the game thread' threading comments (RenderHandler.cpp:3-6, IdleHandler.cpp:3-7, RenderHandler.h:11-13) and replace them with one statement on the seam.
- **Benefits:** Locality: the marshal-and-wait decision (inline-if-on-GT, pooled event, timeout, leak-on-timeout) lives in one ~40-line function instead of being re-derived in three handlers; the subtle 2026-05-23-postmortem reasoning is stated once. Leverage: it fixes two live deadlocks for free (idle always-times-out, render self-deadlock) because the single correct guard now covers them; every future game-thread-touching handler inherits correctness by calling Run rather than by remembering to add IsInGameThread. Tests improve dramatically: today none of these paths are unit-testable because the FEvent plumbing is welded into each handler; a free function taking a TFunction can be tested directly — assert it runs inline on the game thread, assert it marshals and blocks off-thread, assert the timeout path returns the framed error — with no UWorld. The render/idle handlers then need only their pure render/poll logic tested.
- **Deletion test:** Deleting the seam concentrates complexity: there is no other place that knows the correct inline-if-already-on-game-thread + bounded-FEvent + leak-on-timeout recipe, so removing it forces every handler to re-implement it (which is exactly today's state, and two of three got it wrong). It is genuinely deep — a one-line call site hides thread marshalling, lifetime-safe event pooling, and a deadlock-avoiding short-circuit. This is a real module, not a pass-through.

### 2. Make ProcessCommand state its own threading contract instead of trusting two callers
- **Files:** unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp:476-749 (ProcessCommand); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp:133-141 (caller #1, wraps in AsyncTask(GameThread)); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPModule.cpp:426-456 (caller #2, SendTcpCommand, calls inline on the UI/game thread)
- **Problem:** ProcessCommand is the dispatch core, but which thread it runs on is decided entirely by its callers, and the two callers disagree about the contract. The TCP path marshals to the game thread before calling it (TcpServer.cpp:133); SendTcpCommand calls it directly inline (Module.cpp:444) from Slate button handlers. Nothing in ProcessCommand asserts or documents which thread it expects. The handlers it dispatches to silently depend on this: most do GEditor/UWorld work assuming the game thread, the legacy handler defensively re-marshals (and notes at LegacyHandler.cpp:113-117 that 'the TCP server already marshals before dispatch today'), and render/idle assume the opposite and self-deadlock. The actual invariant — 'a handler always runs on the game thread' — is an unwritten convention spread across the TCP server, the module, and 34 handlers, so any new caller or any handler author can violate it without the compiler or a test noticing. The CaptureBeforeState helper (CommandHandler.cpp:225-271) even does its own marshal-and-wait inside ProcessCommand, which is only correct under one of the two contracts.
- **Solution:** Make ProcessCommand own its threading contract instead of inheriting it. Pick the invariant the whole plugin already assumes — handlers run on the game thread — and enforce it in one place: at the top of ProcessCommand, if not IsInGameThread() route through the same FHaybaGameThread::Run seam (candidate 1) so the body always executes on the game thread regardless of caller. Then both call sites simplify: TcpServer drops its AsyncTask(GameThread) wrapper (ProcessCommand now self-marshals), and SendTcpCommand keeps calling inline. Document the contract in HaybaMCPCommandHandler.h next to the ProcessCommand declaration, and drop the now-redundant per-handler re-marshalling (legacy can call its Cmd_* inline once the core guarantees the thread).
- **Benefits:** Locality: the 'handlers run on the game thread' invariant moves from an unwritten convention spread across the TCP server, the module, and every handler into one guard at the dispatch boundary. Leverage: every handler — including the 34 that quietly assume the game thread — becomes correct by construction; new callers of ProcessCommand (sliver runtime, python_run re-entry, a future second transport) can't violate the contract because the core enforces it. The TCP server's Run() loop and SendTcpCommand both shed threading responsibility, shrinking the seam each occupies. Tests improve: ProcessCommand becomes callable from a test thread and you can assert it always ran the handler on the game thread, rather than each handler re-testing marshalling; CaptureBeforeState's inline FEvent wait collapses onto the same seam.
- **Deletion test:** Deleting the explicit contract (reverting to caller-decides) just moves the complexity back out to the callers and handlers — exactly today's diffuse, deadlock-prone state — which is the signature of a missing module rather than an unnecessary one. Adding the guard concentrates a currently-scattered invariant, so it passes the deletion test as a real boundary.

### 3. One error-envelope convention through FHaybaHandlerResult, not two
- **Files:** unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/IHaybaMCPHandler.h:5-26 (FHaybaHandlerResult, the Ok/Err factory); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp:733-748 (router turns Result into MakeOk/MakeErrorResponse); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPRenderHandler.cpp:479-558 (returns Ok() with ok=false nested in data); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPBuildHandler.cpp (same nested-ok pattern); mcp-tools/hayba-mcp/src/tools/tool-executor.ts (the TS side that must branch on the envelope)
- **Problem:** The handler result type FHaybaHandlerResult has exactly one way to express failure — Err(msg) — and 528 call sites across the handlers use it, so the router cleanly maps bOk to MakeOkResponse vs MakeErrorResponse (CommandHandler.cpp:733-748). But render and build opt out: render_camera builds its own {ok:false, error:{kind,engineHint}} object and returns it via FHaybaHandlerResult::Ok (RenderHandler.cpp:485-490, 503-511), so a render FAILURE travels as a transport-level success envelope (ok:true at the wire) with a nested ok:false. That means the TS ToolExecutor — whose whole job per CONTEXT is to normalise UE errors into one UeToolError with a code discriminator — has to special-case these handlers: a render failure looks ok at the envelope but is actually an error two levels down. The single most important wire invariant (the {ok,error,code} envelope) has a quiet exception, and the exception is invisible at the type level because Ok() happily carries a failure payload. Render also invented a richer error shape ({kind, engineHint, attempted}) that the flat ErrorMessage string can't carry, which is the real reason it bypassed the convention.
- **Solution:** Give FHaybaHandlerResult a structured error so the rich shape render wants is expressible without escaping the convention: keep Err(FString) but add Err(TSharedPtr<FJsonObject>) (or fields like ErrorKind/EngineHint alongside ErrorMessage). The router then serialises structured errors into the standard error envelope (MakeErrorResponse gains an optional details object, additive on the wire per the wire-compat principle). Render and build return Err(structuredObj) for their failure cases instead of Ok(dataWithNestedOkFalse). The wire then has exactly one failure shape; the TS executor branches only on the top-level ok/code, never on a nested ok.
- **Benefits:** Locality: failure is expressed one way (Result.Err) for all 34 handlers, and the wire envelope has one failure shape instead of one-plus-two-exceptions; the rich {kind,engineHint} data render needs lives in the sanctioned channel. Leverage: the TS ToolExecutor and any other client stop special-casing render/build — every UE error maps to UeToolError uniformly, which is the documented contract. Tests improve: a test can assert 'a failing handler produces an error envelope' as a single property over all handlers rather than excepting render/build; round-trip tests of the structured error become possible because the shape is typed, not ad-hoc JSON assembled inside two handlers.
- **Deletion test:** Deleting the structured-error capability would force render/build back to either flattening their diagnostics into a string (losing kind/engineHint, a real regression) or re-inventing the nested-ok escape hatch — i.e. the complexity reappears at the handlers. So the addition concentrates the failure-shape decision at the result type. Note this is the weaker of the candidates: FHaybaHandlerResult is already a shallow value type, and if render/build were simply made to use Err(string) the case for adding structure weakens — flag it as lower-priority than candidates 1 and 2.

### 4. Turn ProcessCommand's hand-written special-case chain into the same handler registry it already owns
- **Files:** unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp:529-651 (six hand-coded if (Cmd==...) special cases before the registry lookup at 653); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/IHaybaMCPHandler.h:28-47 (the IHaybaMCPHandler interface these bypass); unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp:445-467 (RegisterHandler / CommandToHandler map)
- **Problem:** FHaybaMCPCommandHandler already has a real dispatch seam: handlers register their commands into CommandToHandler and ProcessCommand looks them up (line 653). But ProcessCommand also carries a parallel, hand-maintained if-chain of six commands that are dispatched *before* the registry — hayba_propose_plan, ui_memory_set, ui_tool_stream_new_turn, ui_tool_stream, the plan-mode gate, and get_setting (lines 529-651). These are UI/host-mirror commands implemented as free functions and inline blocks inside the router. The effect is two dispatch mechanisms in one function: a data-driven registry and a literal switch. The router file is ~290 lines of which a large fraction is these special cases plus push-to-panel helpers (PushSceneGraphToPanel, PushPhysicsResultsToPanel, etc., lines 69-186), so the dispatch core mixes routing with handler bodies. Adding a UI command means editing the router's if-chain (and remembering it runs before auth/plan-mode/journal in ways the registry path doesn't), which is exactly the kind of place where the python_exec/python_run destructive-gate bug (noted at line 30) crept in. The IsDestructiveCommand list (lines 26-35) is a third hand-maintained command-name table that must stay in sync with the registry.
- **Solution:** Move the six special-case commands behind the existing IHaybaMCPHandler seam: introduce a small UiBridgeHandler (and fold get_setting into a SettingsHandler) that registers hayba_propose_plan/ui_memory_set/ui_tool_stream*/get_setting like every other domain, so ProcessCommand has exactly one dispatch path (the registry lookup). The cross-cutting concerns the special cases currently sit in front of — auth, plan-mode gate, journal, diff/tool-stream push — become explicit ordered stages around the single dispatch call rather than code that some commands skip by being early-returns. The push-to-panel helpers move into the handlers that produce that data (scene graph push into the scene-graph handler, etc.) or behind a thin result-observer hook, shrinking the router to parse -> auth -> gate -> dispatch -> journal -> respond.
- **Benefits:** Locality: there is one way to add a command (register a handler) instead of two (register, or hand-edit the router's if-chain); the destructive-command and special-case name tables stop being separate things a maintainer must keep in sync — the source of the python_run gate bug. Leverage: every command, including the UI bridge ones, automatically flows through auth, plan-mode, and journaling in the same order, closing the class of bugs where a command silently bypasses a stage by being an early special case. Tests improve: the router becomes testable as a fixed pipeline (assert auth runs before dispatch, assert every registered command is journalled) instead of needing a case per special command; UI-bridge handlers become unit-testable in isolation like the other 34 handlers rather than only reachable through ProcessCommand.
- **Deletion test:** Deleting the special-case chain (by absorbing it into the registry) removes a whole second dispatch mechanism — complexity is concentrated into the one registry the class already maintains, not merely moved. The registry+pipeline is the deep module; the if-chain is the shallow duplicate. Passing the deletion test here means the router stops being two routers.

### 5. A typed param-extraction module behind FHaybaHandlerResult (HaybaParams)
- **Files:** New: unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPParams.h (+ .cpp). Callers: every handler under Private/handlers/ — most acutely HaybaMCPStaticMeshHandler.cpp (lines 32-35, 100-109), HaybaMCPDataAssetHandler.cpp (124-129, 166-167, 187-194), HaybaMCPActorHandler.cpp (108-130), HaybaMCPTextureHandler.cpp (49-51). FHaybaHandlerResult lives in Public/IHaybaMCPHandler.h.
- **Problem:** The same three lines — TryGetStringField into a local, !P.IsValid() / IsEmpty() check, FHaybaHandlerResult::Err with a hand-typed message — recur 47 times for `path` alone across 14 files (`actor_id` 22x, `name` 11x, `material_path` 11x). Because every call hand-writes its own error string, the wording has drifted into three incompatible dialects: `missing path` (36x), `missing arg path` (5x), `missing param path`. The agent on the other end of the protocol seam sees an inconsistent error vocabulary it cannot pattern-match, and the `IsEmpty()` guard is silently omitted in some handlers (Actor's Spawn checks it; several `mesh_*`/`tex_*` paths do not, so an empty string flows into TryLoad and fails deeper with a worse message).
- **Solution:** Add a tiny param-accessor wrapping the JSON object plus the command name: RequireString("path") returns either the value or a populated FHaybaHandlerResult::Err with one canonical message (`<cmd>: missing required param 'path'`), OptString/RequireNumber/RequireVec3 likewise. Handlers change from 'three lines + a literal' to one line that early-returns the error. The accessor centralizes the empty-string check and the cmd-prefixed message format.
- **Benefits:** Locality: the rule 'what a missing/empty param looks like to the agent' lives in one module instead of being re-decided per call site; fixing the empty-string gap fixes it everywhere. Leverage: a new handler author writes RequireString and inherits the validated, consistently-worded behaviour for free. Tests: param validation becomes a pure function over an FJsonObject with no UObject/editor dependency, so the parse-half of every handler gets its first unit tests (today there are none for handlers).
- **Deletion test:** Passes. Deleting HaybaParams forces all 47+ sites back to hand-rolled TryGet/IsEmpty/Err triples and re-fragments the error vocabulary — complexity reconcentrates at the call sites, proving the module was absorbing real behaviour, not just renaming it.

### 6. Promote HaybaPersistAsset + asset-create into a shared AssetOps module
- **Files:** Source of the good implementation: HaybaMCPMaterialHandler.cpp (HaybaPersistAsset at line 136; used 14x at lines 434,461,518,589,656,740,980,1017,1070,1126,1176,1268). Inferior re-implementations to delete: HaybaMCPDataAssetHandler.cpp (data_create persist at 148-153 via MarkPackageDirty+SaveLoadedAsset; data_set persist at 220-228). Other asset-create/persist sites: HaybaMCPBlueprintHandler.cpp, HaybaMCPUIHandler.cpp, HaybaMCPNetworkHandler.cpp (all carry CreateAsset/SavePackage). New: Public/HaybaMCPAssetOps.h/.cpp.
- **Problem:** 'CreateAsset, then immediately persist so a crash before first save does not lose it' is a real, hard-won invariant — the Material handler documents it explicitly (lines 430-432) and uses a robust SavePackage-by-filename with RF_Public|RF_Standalone|SAVE_NoError. But HaybaPersistAsset is a file-private static, so the other 8 asset-touching handlers cannot use it; DataAsset instead uses the weaker UEditorAssetLibrary::SaveLoadedAsset path. There are now two different answers to 'how do we durably persist a created/edited asset', and only one of them carries the crash-resilience reasoning. New asset commands copy whichever neighbour they happened to read.
- **Solution:** Lift HaybaPersistAsset and a CreateAsset(name, packagePath, class, factory)->{object,saved,error} helper into an AssetOps module that returns the already-conventional {path,name,saved,save_error} fragment the Material handler emits. Handlers call AssetOps::CreateAndPersist and merge the returned fields; DataAsset's two bespoke persist blocks are deleted in favour of the one with the documented crash invariant.
- **Benefits:** Locality: the crash-resilient save invariant lives in exactly one place with its rationale, instead of one strong copy plus several weaker divergent ones. Leverage: every future create-asset command inherits the correct persist + the standard saved/save_error response shape. Tests: AssetOps is the natural seam to fake — a test double records 'persist called with these flags' without writing to disk or needing a live editor, so create/persist semantics become assertable.
- **Deletion test:** Passes. Deleting AssetOps does not relocate the work — it scatters the SavePackage flags and the crash-resilience comment back across 9 handlers and re-admits the weaker SaveLoadedAsset variant. The complexity it concentrates (one correct durable-save policy) reappears as divergent duplicates, the signature of genuine depth.

### 7. Unify reflection get/set into one Reflection module (lift SetPropByReflection)
- **Files:** Strong implementation to promote: HaybaMCPMaterialHandler.cpp — SetPropByReflection (line 218, ~100 lines: enums-by-string, bool bitfields, struct-from-array LinearColor/Vector/Vector4/Color, object-ref-by-path) and SetStructFieldByReflection (line 189). Weaker/duplicate sites to converge: HaybaMCPDataAssetHandler.cpp — ReflectObjectProperties (83) and data_set's FJsonObjectConverter::JsonValueToUProperty one-liner (212-218); HaybaMCPActorHandler.cpp (SetProps via FindPropertyByName + ExportText); HaybaMCPDocsHandler.cpp / HaybaMCPBehaviorTreeHandler.cpp (their own FProperty walks). New: Public/HaybaMCPReflection.h/.cpp.
- **Problem:** There are at least three different engines for 'turn a JSON value into a UPROPERTY write' and 'turn a UObject into a JSON property bag'. Material's SetPropByReflection is the deep one (string→enum coercion, component-mask bitfields, struct-from-array, object refs by asset path) but is trapped as a translation-unit static. DataAsset's data_set takes the shallow FJsonObjectConverter path, which silently cannot do several of those coercions — so the same `data_set value="FunctionInput_Scalar"` that works on a material node fails on a data asset. This is capability drift masquerading as duplication: the weaker copies are quietly less correct, and the agent cannot predict which property writes will succeed.
- **Solution:** Extract the Material reflection coercer (object + struct variants) and DataAsset's ReflectObjectProperties reader into a Reflection module: SetByReflection(obj, name, jsonValue)->bool and ReflectToJson(obj)->FJsonObject. data_set, Material's MatSet/SetMatProps, Actor SetProps and the Docs/BehaviorTree walks all route through it. The single setter becomes the one place enum/bitfield/struct/objref coercion is defined.
- **Benefits:** Locality: 'what JSON shapes can write what property types' is decided once; adding (say) FText or soft-object support upgrades every set-property command at once. Leverage: handlers stop carrying reflection knowledge — they just name a target and a value. Tests: reflection coercion is pure over a UObject CDO and can be exercised against a small test UCLASS with one property of each kind, finally giving the trickiest, most bug-prone code real coverage instead of being validated only through full editor automation.
- **Deletion test:** Passes strongly. Deleting the module forces the rich coercion logic back into Material and re-leaves DataAsset/Actor on their weaker variants — i.e. it both duplicates code AND re-opens the capability gap. A module whose deletion reintroduces a correctness divergence (not just line count) is concentrating real behaviour.

### 8. Editor-loading seam so handlers are testable away from UObject/GEditor
- **Files:** Inline load/world access scattered through handlers: HaybaMCPStaticMeshHandler.cpp (FSoftObjectPath(Path).TryLoad at 37,111), HaybaMCPTextureHandler.cpp (53), HaybaMCPDataAssetHandler.cpp (UEditorAssetLibrary::LoadAsset 169,196), HaybaMCPActorHandler.cpp (LoadClass 112; FindActorByName/TActorIterator 20-29; GEditor->GetEditorSubsystem 116), HaybaMCPMaterialHandler.cpp (LoadObject 361,381). Plus FindActorByLabel_GameThread duplicated in HaybaMCPCommandHandler.cpp (191). Interface: IHaybaMCPHandler in Public/IHaybaMCPHandler.h. New: an IHaybaEditorContext (or HaybaResolve) facade with a live + fake implementation.
- **Problem:** Every handler reaches straight for global editor singletons — FSoftObjectPath::TryLoad, UEditorAssetLibrary::LoadAsset, LoadClass, GEditor->GetEditorSubsystem, TActorIterator over GEditor's world. The 'resolve a path to a typed object, null-check, build an Err' triple is repeated dozens of times (the Cast<UStaticMesh>(...TryLoad()) + `failed to load %s` pattern alone recurs across mesh/texture/material/data handlers), and FindActorByLabel is even duplicated between the Actor handler and the router. Because resolution is welded to global state, no handler can be exercised without a running editor — which is exactly why the handler layer has zero unit tests today (only TestHandler, which just proxies UE's automation runner). The CONTEXT principle 'two adapters = a real seam' holds for the TS ToolExecutor but has no UE-side analogue: there is one (live) way to reach the editor.
- **Solution:** Introduce a thin resolution facade passed to handlers: ResolveAsset<T>(path)->T*-or-Err, ResolveClass(name), FindActorByLabel(label), GetEditorWorld(). A LiveEditorContext wraps today's globals; an InMemoryEditorContext returns canned objects for tests. Handler dispatch takes the context (constructor-injected or as a Handle() argument). The repeated load+null-check+Err triple collapses into one ResolveAsset call that returns the typed object or the canonical error.
- **Benefits:** Locality: object/class/actor resolution and its error wording live behind one interface instead of being re-derived at every TryLoad site; the duplicated FindActorByLabel collapses to one. Leverage: the load-error message format and any future path-normalisation apply uniformly. Tests: this is the seam that makes the whole handler family unit-testable — with a fake context, a handler's command logic (param→resolve→mutate→response) runs with no editor, mirroring the TS InMemoryToolExecutor doctrine and turning the 31 handlers from integration-only into testable modules.
- **Deletion test:** Passes. Deleting the context facade does not move complexity elsewhere — it re-welds every handler to GEditor/LoadObject globals, restores the duplicated FindActorByLabel, and removes the only place a fake could be injected, dropping handler testability back to zero. That a deletion would forfeit testability across 31 files (not merely re-inline a helper) marks it as a depth-bearing seam rather than a shallow wrapper.

### 9. Single tool descriptor: collapse the 3-place registration + dead wrapper schema into one captured module
- **Files:** mcp-tools/hayba-mcp/src/tools/index.ts (registerToolsCore eager block lines ~258-1711; recordEagerSchemas lines 1713-~2035); every per-tool wrapper, e.g. mcp-tools/hayba-mcp/src/tools/actor/actor-spawn.ts, mcp-tools/hayba-mcp/src/tools/material/material-create.ts (and the other 42 files that `export const schema`); mcp-tools/hayba-mcp/src/tools/hayba-tool-meta.ts; mcp-tools/hayba-mcp/src/tools/schema-registry.ts; mcp-tools/hayba-mcp/src/tools/tool-meta-registry.ts
- **Problem:** A single tool (say actor_spawn) is currently specified in FOUR disconnected places: (1) `server.tool('actor_spawn', appendMeta(desc, meta), {inline zod shape}, closure)` in index.ts; (2) `remember('actor_spawn', meta)` right below it; (3) `reg('actor_spawn', {inline zod shape again}, cost, returns)` inside recordEagerSchemas ~1300 lines away; and (4) `export const schema` + a re-validating handler in actor-spawn.ts. The Zod shape is hand-copied in places 1 and 3 and re-declared in place 4. I verified index.ts imports only 2 of the 44 wrapper `schema` exports (wait-for-idle, render-camera) — the other 42 are dead, so the inline shape, the reg shape, and the wrapper's own schema can silently diverge. The `returns` string and `cost` live only in reg(); the meta lives in the wrapper AND is re-passed in index. Adding or editing one tool means touching three regions of a 2035-line file plus the wrapper, with no compiler link between them.
- **Solution:** Make each tool module export ONE descriptor object — `{ name, description, schema (ZodObject), meta (cost/effects/when/not_when), returns, handler }` — and have it own its handler that calls executeCommand directly (the wrapper already does this). Replace the eager block, the `remember` calls, and recordEagerSchemas with a single loop over an array of imported descriptors that, per tool, calls server.tool(name, appendMeta(description, meta), schema.shape, handler), registerToolMeta(name, meta), and recordSchema(name, {shape: schema.shape, cost: meta.cost, returns}). The capturing shim in deferred mode is unchanged — it still intercepts server.tool. Drop the now-redundant `export const schema` re-validation inside handlers since the registry shape is the one source.
- **Benefits:** Locality: a tool is defined in exactly one file; the Zod shape, returns, cost, meta, and handler sit together and cannot drift because there is only one copy. Leverage: index.ts shrinks from 2035 lines to a thin descriptor loop; recordEagerSchemas (the ~320-line third copy of every schema) disappears entirely; adding a tool is one new file plus one array entry instead of edits in three regions. Tests: the descriptor array is directly assertable (one test can verify every descriptor has matching meta/returns/cost and a parseable schema), and the InMemoryToolExecutor seam already lets each handler be unit-tested against its real schema rather than a copy — today a wrapper test validates a schema the live server never uses.
- **Deletion test:** Deleting recordEagerSchemas today would break get_tool_signature for ~40 tools (it is the only feeder of `returns`/`cost` into schema-registry for eagerly-registered tools), proving it carries real behaviour but in the wrong place — it is depth smeared across a third copy. After the change, deleting any single descriptor cleanly removes exactly one tool from all three consumers (MCP surface, meta registry, signature registry) — the mark of a real module boundary rather than parallel lists that must be kept in sync by hand.

### 10. Generate sidecar.json param docs from C++ instead of hand-authoring them (close the field-level drift the lint can't see)
- **Files:** mcp-tools/hayba-mcp/src/legacy-commands/sidecar.json (54KB, hand-authored); mcp-tools/hayba-mcp/scripts/check-legacy-wrappers.mjs; unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPLegacyHandler.cpp (GetCommands() + the per-command param parsing); mcp-tools/hayba-mcp/src/tools/code-mode/get-tool-signature.ts (legacyToSignature)
- **Problem:** sidecar.json is the hand-authored source of truth for legacy-command param docs AND the ue_legacy allowlist. The lint (check-legacy-wrappers.mjs) regex-scrapes only the TEXT("...") name list out of GetCommands() and checks name-existence in three directions. The get-tool-signature.ts comment block explicitly admits the gap: 'the CI lint catches the existence drift, but field-level drift would still slip through.' So a C++ handler that reads a new `params->GetStringField(TEXT("resolution"))` while sidecar.json still lists the old param set passes CI green, and the agent gets get_tool_signature docs that lie about the real params — exactly the failure mode (agent reaches for python_run on bad signature) the 2026-05-23 postmortem was about.
- **Solution:** Add a generator step that parses each Cmd_* handler body in HaybaMCPLegacyHandler.cpp for its `GetField`/`Get*Field`/`TryGet*` param reads and the JSON keys it sets on the response, and emits the params[] and returns.fields[] of sidecar.json (keeping only human prose — descriptions, notes, agent_callable — as a hand-authored overlay merged on top). Run it in the same npm script as the lint; CI fails if the committed sidecar params diverge from the regenerated ones. The allowlist and descriptions stay hand-curated; only the machine-checkable param/returns names become derived.
- **Benefits:** Locality: the param contract lives once, in the C++ that actually parses it; sidecar.json keeps only the editorial layer. Leverage: every legacy command's signature becomes correct-by-construction for the agent, and the same scrape that already finds GetCommands() names is extended to find the field reads — one tool, more depth. Tests: the generator is pure (cpp text in, JSON out) so it is unit-testable on fixtures, and the existing runLegacyWrapperCheck gains a fourth, field-level invariant instead of a documented blind spot.
- **Deletion test:** Deleting the hand-authored params arrays today loses real, agent-facing behaviour (get_tool_signature docs) with no other source — confirming the data is load-bearing but stored in the wrong place (a hand-copy of the .cpp). After the change, deleting the generated section just triggers regeneration from the authoritative .cpp; deleting the editorial overlay degrades gracefully to name+type docs. The behaviour concentrates at its true source rather than living as a parallel transcription.

### 11. Unify the eager-block closure boilerplate behind one adapter so per-tool wrappers stop existing as pure pass-throughs
- **Files:** mcp-tools/hayba-mcp/src/tools/index.ts (the ~30 closures of form `async (params) => { const r = await xHandler(params, session); return { content: r.content, isError: r.isError }; }` and the 18 `withNicheBriefing(...)` variants); mcp-tools/hayba-mcp/src/tools/niche-briefing.ts; mcp-tools/hayba-mcp/src/tools/types.ts (ToolHandler); representative wrappers material-create.ts, actor/actor-spawn.ts; mcp-tools/hayba-mcp/src/tools/tool-executor.ts (executeCommand)
- **Problem:** Every eager registration wraps its imported handler in a near-identical closure: 14 sites are literally `return { content: r.content, isError: r.isError }` and 18 are `return withNicheBriefing(domain, session, r)`. The handler it wraps (e.g. materialCreateHandler) is itself a thin, uniform shape: safeParse the args, call executeCommand(cmd, data), JSON.stringify the result. So for the simple tools there are two layers of boilerplate (the closure in index.ts and the wrapper file) that together add nothing but a name and a domain string. This is the shallow duplication that makes index.ts 2035 lines and makes 'add a tool' feel like ceremony.
- **Solution:** Introduce one adapter, e.g. `defineUeTool({ name, schema, meta, returns, domain? })`, that returns the standard handler (parse against the registry shape → executeCommand(name) → stringify) and, when `domain` is set, wraps the result in appendNicheBriefing. Tools that are genuinely special (python_run, hayba_propose_plan, the capture/PIE tools, anything with custom result transforms) keep a bespoke handler — the adapter is the default, not a mandate. The per-tool wrapper file collapses into a single defineUeTool call; the index.ts closure disappears because the descriptor already carries the handler.
- **Benefits:** Locality: the parse→dispatch→stringify→optional-briefing pipeline exists in exactly one place (the adapter) instead of being re-spelled in 30+ closures and 40+ wrapper bodies. Leverage: niche-briefing wiring becomes a `domain` field rather than a hand-applied wrapper call that is easy to forget on a new material tool (today it is applied to 18 of the material/asset tools by hand). Tests: the adapter is tested once against InMemoryToolExecutor for the four behaviours (valid parse, invalid parse → isError, UeToolError surfacing, briefing-on-first-touch); per-tool tests shrink to 'this tool maps to this cmd with this schema', which is the only thing actually distinct.
- **Deletion test:** Deleting a single per-tool wrapper today only moves complexity — the inline closure in index.ts must then inline the parse/dispatch logic, so nothing is saved. That is the signature of a shallow module (it just forwards). After introducing the adapter, deleting it would force every tool to re-inline the same four-step pipeline, concentrating real complexity back into one place — confirming the adapter is deep (much uniform behaviour behind a one-line call) where the current wrappers are not.


## Refuted (false positives) — 38

- `HaybaMCPActorHandler.cpp:534` actor_call_function: ProcessEvent(Func, nullptr) crashes when function has return value or out-params — The claim rests on a factually incorrect assertion about UE5 internals. In UE5, `UFunction::ParmsSize` is computed as the total byte size of ALL properties bearing the `CPF_Parm` flag in the function's property list. Return values carry `CPF_ReturnParm | CPF_OutParm | CPF_Parm` — they ARE counted in `ParmsSize`. Therefore any BlueprintCallable function that has a return value will have `ParmsSize == sizeof(ReturnType) > 0` and will be correctly blocked by the `if (Func->ParmsSize > 0)` guard at line 534. The premise that "a function with only a return value has ParmsSize == 0 and slips through" is simply wrong about how UE computes that field. Equally, any function with `FUNC_HasOutParms` set must have at least one property with `CPF_OutParm`, which is also `CPF_Parm`, so it too contributes to `ParmsSize` — it cannot be the case that `FUNC_HasOutParms` is set while `ParmsSize == 0`. The guard `ParmsSize > 0` is a superset of `(NumParms > 0 || HasAnyFunctionFlags(FUNC_HasOutParms))` for the purpose of protecting the `ProcessEvent(Func, nullptr)` call: any function that requires a non-null parms buffer has at least one `CPF_Parm` property and therefore `ParmsSize > 0`. No concrete input exists that would pass the guard and then crash in `ProcessEvent`.
- `HaybaMCPAssetHandler.cpp:445` asset_search with include_thumbnails blocks the game thread for shader compilation per asset — The finding conflates "slow" with "crash/hang." EThumbnailTextureFlushMode::NeverFlush is specifically chosen to skip the blocking GPU readback path — RenderThumbnail enqueues render commands and returns without calling FlushRenderingCommands() or waiting for GPU completion. The returned uncompressed data may simply be empty/zeroed when no cached render is available, and the code already handles that at line 455. GetAsset() at line 441 does synchronous disk I/O for unloaded assets, which is slow but bounded and non-crashing — identical to what the editor's own Content Browser does when generating thumbnails. There is no AsyncTask(GameThread)+blocking-wait deadlock (class A), no unbounded FPlatformProcess::Sleep loop or timeout overflow (class B), no unchecked NULL (null checks exist at lines 435, 442, 455, 461, 468), no out-of-bounds access, no iterator invalidation, no engine check() reachable from bad input, and no TCP socket state shared with the read thread. The 50-thumbnail cap makes this equivalent in cost to scrolling through 50 items in the Content Browser — a performance concern, not a crash vector.
- `HaybaMCPBlueprintHandler.cpp:440` blueprint_document: LinkedTo[0] pin's owning node used without null check after GetOwningNode() — The null guard at line 442 (`if (!NextNode) break`) fully protects every subsequent use of NextNode in the loop body. The while-condition at line 438 (`Then->LinkedTo.Num() > 0`) guarantees LinkedTo[0] is a valid access. The ternary at line 441 guards the GetOwningNode() call against a null Next pin. All three pointer dereferences (LinkedTo[0], GetOwningNode(), NextNode->GetNodeTitle, NextNode->Pins) are provably guarded. There is no concrete input or timing that reaches an unguarded null dereference — the finding is a false positive as confirmed by the candidate's own description.
- `HaybaMCPIdleHandler.cpp:286` Data race on FWaitState fields between timeout path (game thread) and in-flight PollOnce ticker (game thread post-RemoveTicker) — The alleged race between the timeout path (lines 291-292) and an in-flight PollOnce tick is impossible. Both the timeout path in Handle() and PollOnce run exclusively on the game thread (PollOnce via FTSTicker, which is game-thread-ticked). The game thread is cooperative and single-threaded: when Handle() is executing at line 288-292, the game thread is occupied by Handle() and the ticker cannot dispatch PollOnce. When PollOnce is executing, Handle() is not. RemoveTicker at line 288 unregisters the delegate while the game thread is in Handle(); no subsequent tick of PollOnce can be dispatched before Handle() writes lines 291-292, because both events require the same game thread. There is zero concurrency between them and therefore no data race on bAllSettled/FinalDurationMs. The TSharedRef also prevents any use-after-free. The finding misapplies a multi-threaded race model to two game-thread-serialized execution paths.
- `HaybaMCPEditorHandler.cpp:92` SetCamera dereferences P without IsValid() check — The dispatcher in HaybaMCPCommandHandler.cpp (lines 512-518) unconditionally guarantees that Params is non-null before any handler is called. It extracts the "params" JSON object and then applies: `if (!Params.IsValid()) Params = MakeShared<FJsonObject>();`. This means SetCamera always receives a valid (non-null) TSharedPtr<FJsonObject>, even when the caller sends `{}` or omits "params" entirely. When "location" is absent, TryGetArrayField on line 92 returns false and the function returns the error string on line 93 — no dereference of a null pointer occurs. The finding is a false positive.
- `HaybaMCPLevelHandler.cpp:80` LevelLoad, LevelCreate, LevelGotoBookmark dereference P without IsValid() check — The candidate claim that P can be a null TSharedPtr is refuted by the dispatcher. In ProcessCommand (HaybaMCPCommandHandler.cpp lines 512-518), after attempting to extract "params" from the JSON envelope, there is an unconditional fallback: `if (!Params.IsValid()) Params = MakeShared<FJsonObject>();`. This means every handler always receives a valid (non-null) TSharedPtr<FJsonObject> — either the actual params object from the wire frame, or a freshly-allocated empty one. When LevelLoad/LevelCreate/LevelGotoBookmark call P->TryGetStringField on an empty object, TryGetStringField returns false (field not found), and the existing `|| Path.IsEmpty()` guard immediately returns the error response. No null dereference is possible on any input, including a request with no "params" field at all.
- `HaybaMCPFoliageHandler.cpp:69` FoliageAddInstance, FoliageRemoveInstances, FoliagePaintAt dereference P without IsValid() check — The candidate finding is a false positive. `ProcessCommand` in HaybaMCPCommandHandler.cpp lines 512-518 unconditionally guarantees that the `Params` TSharedPtr passed to every handler is non-null. The relevant code is:

```cpp
TSharedPtr<FJsonObject> Params;
const TSharedPtr<FJsonObject>* ParamsObj = nullptr;
if (Parsed->TryGetObjectField(TEXT("params"), ParamsObj) && ParamsObj && ParamsObj->IsValid())
    Params = *ParamsObj;
if (!Params.IsValid()) Params = MakeShared<FJsonObject>();
```

The fallback `Params = MakeShared<FJsonObject>()` on line 518 runs whenever `params` is absent, null, or not an object in the incoming JSON. Consequently the `P` parameter received by `FoliageAddInstance`, `FoliageRemoveInstances`, and `FoliagePaintAt` is always a valid, non-null `TSharedPtr<FJsonObject>` pointing to at least an empty object. There is no code path where `P` can be null at the point of the `P->TryGetStringField(...)` calls. The foliage handlers' responses to a missing `foliage_type_path` field will correctly return an error via the `TryGetStringField` return value check (lines 69-70, 123-124, 206-207), which is exactly the right pattern here.
- `HaybaMCPISMHandler.cpp:81` IsmCreateActor, IsmAddInstance, IsmAddInstances, IsmClearInstances dereference P without IsValid() check — The claim assumes P can be a null/invalid TSharedPtr when it reaches the ISM handlers. In practice, ProcessCommand in HaybaMCPCommandHandler.cpp (lines 512-518) guarantees this cannot happen. After attempting to extract the "params" JSON object field, line 518 unconditionally sets Params = MakeShared&lt;FJsonObject&gt;() if the extracted value is invalid. This means every handler always receives a non-null, valid (though possibly empty) TSharedPtr&lt;FJsonObject&gt;. Calling TryGetStringField or TryGetArrayField on an empty FJsonObject is safe — those methods simply return false and leave the output unchanged, which is exactly what the existing field-missing guards in the ISM handlers then catch to return a proper error. There is no code path that delivers a null P to IsmCreateActor, IsmAddInstance, IsmAddInstances, or IsmClearInstances.
- `HaybaMCPLegacyHandler.cpp:75` AsyncTask(GameThread)+blocking-Wait called while already on the game thread — deadlock when Handle() is invoked from the game thread — The finding is a false positive. The `IsInGameThread()` guard at line 53 of `RunOnGameThread()` fires correctly for every current call path. The only site that calls `Handle()` is `HaybaMCPCommandHandler.cpp:677`, which is always executed inside the TCP server's `AsyncTask(ENamedThreads::GameThread, ...)` wrapper — so `IsInGameThread()` returns true, `Work()` runs inline at line 55, and the `AsyncTask` + `Wait` block at lines 75-84 is never reached. No deadlock, no UAF. The candidate explicitly concedes this ("the guard fires correctly") and then constructs a hypothetical around future callers that do not exist in the codebase today. There is no current concrete input or timing that reaches the dangerous path.
- `HaybaMCPLegacyHandler.cpp:643` Cmd_CreateGraph: GetArrayField("nodes") / GetArrayField("edges") called without null-check — asserts/crashes on missing or non-array JSON field — The UE 5.7 source (verified in C:/Program Files/Epic Games/UE_5.7/Engine/Source/Runtime/Json) shows that GetObjectField, GetArrayField, and the underlying AsObject()/AsArray() do NOT call check() or any fatal assert on missing/wrong-type fields. GetObjectField calls GetField which returns a MakeShared FJsonValueNull() for missing fields; AsObject() on that value falls through TryGetObject returning false, sets Object to a static empty FJsonObject, calls ErrorMessage which only logs UE_LOG(LogJson, Error/Warning), and returns the static empty object. No crash occurs. The GetArrayField calls at lines 643 and 718 are additionally protected by ValidateGraphJson already having verified "nodes" and "edges" exist as arrays before execution reaches those lines. The GetObjectField("properties") at line 689 on a node that omits "properties" will return a valid-but-empty FJsonObject; the PropsObj.IsValid() guard at line 690 will be true but iterating an empty Values map is a no-op. There is no crash path in any of the flagged sites.
- `HaybaMCPLegacyHandler.cpp:649` Cmd_CreateGraph: NodeValue->AsObject() used without null-check — returns null TSharedPtr if JSON value is not an object — The unsafe `NodeValue->AsObject()` at line 648 (and `EdgeValue->AsObject()` at line 721) is unreachable with a non-object array element. `ValidateGraphJson` (line 825) calls `TryGetObject` on every node element and calls `AddError` for any that is not a valid JSON object. The result is checked at lines 609–615: if `ValidationErrors.Num() > 0`, `Cmd_CreateGraph` returns immediately with an error response, never entering the node-iteration loop. Reaching line 648 therefore requires that `ValidateGraphJson` returned zero errors, which is only possible when every array element passed the `TryGetObject` check — exactly the condition that makes `AsObject()` safe to dereference. The same gate protects the edges loop at line 721. The claim correctly describes the absence of a local null-check, but misses that the hard pre-validation gate makes the unsafe path unreachable for the malformed input it describes.
- `HaybaMCPMaterialHandler.cpp:564` MatConnectNodes: To->GetInput(ToInputIndex) result used without null-check — returns null for out-of-range index, crashes in ConnectByIndex lambda — The candidate finding is self-refuting. At line 565, `FExpressionInput* In = To->GetInput(ToInputIndex)` is immediately followed at line 566 by `if (!In) return false;` — a correct null guard. The caller at line 584 then checks `if (!ConnectByIndex(From, To)) return FHaybaHandlerResult::Err(...)`. There is no null dereference path. Additionally, the ToInputIndex-based branch is only entered when `ToInputIndex >= 0` (line 582), so a missing/absent parameter defaults to -1 and bypasses ConnectByIndex entirely. No crash class applies here.
- `HaybaMCPMaterialHandler.cpp:750` MatApply: GEditor null-deref — FindActorInWorld called with potentially-null World pointer, then Actor->FindComponentByClass result used without slot-count check — The null-deref portion of the claim is refuted by the code itself: `Actor` is null-checked at line 754 and `SMC` is null-checked at line 760 — both return an error before reaching line 761. The out-of-bounds portion is also a false positive: `UMeshComponent::SetMaterial` in UE 5.x does NOT assert or `check()` that `ElementIndex < GetNumMaterials()`. The implementation guards only on `ElementIndex >= 0` and then calls `OverrideMaterials.SetNum(ElementIndex + 1, false)` to grow the override array silently. An out-of-range `slot_index` from JSON (e.g., 99 on a 1-slot mesh) would produce a harmless no-op material override on a non-existent slot, not a crash. There is no engine `check()`/`checkf()` on the slot count in the UE 5 `SetMaterial` path.
- `HaybaMCPPerfHandler.cpp:51` GetPerfStats: GNumDrawCallsRHI[0] / GNumPrimitivesDrawnRHI[0] accessed without checking array bounds — undefined behavior if the RHI has not initialized the per-GPU arrays — The claim that GNumDrawCallsRHI[0] and GNumPrimitivesDrawnRHI[0] read "uninitialized memory" when GDynamicRHI is null is incorrect. Both are engine-level globals declared as fixed-size arrays (e.g., int32 GNumDrawCallsRHI[MAX_NUM_GPUS]) with static storage duration. Per the C++ standard, all objects with static storage duration are zero-initialized before program startup, unconditionally. Index 0 is always within bounds (MAX_NUM_GPUS is typically 4, so valid indices are 0-3). In a headless/commandlet context where the RHI has not been initialized, these arrays will simply contain 0 — accurate and safe to read. There is no UB, no uninitialized memory, and no out-of-bounds access. The code author already correctly guarded the actual RHI API call (RHIGetTextureMemoryStats via GDynamicRHI at line 65) which would crash if called without a live RHI; the global counter reads need no such guard.
- `HaybaMCPLegacyHandler.cpp:1210` Cmd_WizardChat: Steps[0]->AsObject() called without null-check — AsObject() returns null if the FJsonValue is not an object type — The `Steps` array is a stack-local variable created inside the `if (Message.StartsWith(TEXT("[INIT]")))` branch (line 1163). Every element is inserted exclusively through the local `AddStep` lambda (lines 1166-1171), which always constructs a `FJsonValueObject` wrapping a non-null `FJsonObject`. `FJsonValueObject::AsObject()` unconditionally returns the wrapped shared pointer, so it cannot return null for these elements. The array is never exposed to any external code path between construction and consumption at lines 1207-1214; there is no way for a non-Object value to enter `Steps`. The loop at line 1207 iterates `Steps.Num()` elements, and the check at line 1214 guards `Steps[0]` behind `Steps.Num() > 0`. No concrete input can produce a null from `AsObject()` here.
- `HaybaMCPRenderHandler.cpp:473` BlockTimeoutMs uint32 overflow when TimeoutSeconds is large — nearly-instant spurious timeout — The uint32 overflow is real only at astronomically large inputs (~49.7 days), but it cannot crash. Three concrete refutations: (1) Outcome at overflow is a spurious render_timeout response, not a crash — the timeout path (lines 479-490) is explicitly designed for exactly this scenario and correctly avoids reading S's task-written fields. (2) No use-after-free is possible: RunOnGameThread captures S as a TSharedPtr (ThreadSafe), so the game-thread task holds its own shared ref and S stays alive regardless of whether BlockTimeoutMs wraps to 1ms. The comment at line 467-469 explicitly documents this invariant. (3) The "less extreme" arithmetic in the claim is wrong: wait_timeout_s=4295 → (4295+60)*1000 = 4,355,000ms which fits comfortably in uint32 (max ~4,294,967,295); there is no overflow at 71 minutes. The only true overflow threshold is ~49.7 days of timeout. Additionally, the handler runs on the TCP per-request thread (explicitly documented at lines 3-6: "Handle() is invoked from the TCP server's per-request thread, NOT the game thread"), so the AsyncTask(GameThread)+Wait pattern is the correct cross-thread design with no deadlock. At worst, an adversarial caller passing wait_timeout_s=5e9 gets a spurious timeout error response instead of waiting — a minor UX issue, not a crash or memory safety issue.
- `HaybaMCPSplineHandler.cpp:125` spline_add_point: unchecked Index passed to AddSplinePointAtIndex — engine assert on out-of-range — The claim assumes `AddSplinePointAtIndex` passes the caller's index directly to `TArray::Insert` without its own bounds check. The UE 5.7 engine source at `C:/Program Files/Epic Games/UE_5.7/Engine/Source/Runtime/Engine/Private/Components/SplineComponent.cpp` line 1412 disproves this: the function opens with `if (Index >= 0 && Index <= NumPoints)` and only executes the insert body when that condition is true. An out-of-range index (e.g., Index=5 on a 2-point spline) causes the entire insert block to be skipped silently — no `TArray::Insert` is ever called with the bad index, no engine `check()` fires, no crash occurs. The handler's own guard (`Index >= 0`) is therefore sufficient to prevent the only inputs the engine doesn't itself handle (negative indices). This is a false positive.
- `HaybaMCPSceneGraphHandler.cpp:101` ParseWindow: WinObj inner pointer dereferenced without validity check after TryGetObjectField — The crash path does not exist. Tracing UE 5.7 source (C:/Program Files/Epic Games/UE_5.7/Engine/Source/Runtime/Json/Private/Dom/JsonObject.cpp line 239 and JsonValue.cpp line 565): `TryGetObjectField` returns `true` only when `Field->TryGetObject(OutObject)` succeeds, which for `FJsonValueObject` sets `OutObject = &Value` (a pointer to the `TSharedPtr<FJsonObject>` member of a heap-allocated object) and returns `true`. That pointer is never null. If the JSON field is `null`, the stored entry is `FJsonValueNull`, whose `TryGetObject` returns `false` without touching `OutObject`, so `TryGetObjectField` returns `false` and the early-return at line 102 fires. If the JSON field is an actual object, the `TSharedPtr<FJsonObject>` is always non-null because `SetObjectField` stores a `FJsonValueNull` (not `FJsonValueObject`) when given a null pointer, and the JSON parser never constructs a `FJsonValueObject` with a null inner object. Therefore: when `TryGetObjectField` returns `true`, `WinObj` points to a valid non-null `TSharedPtr<FJsonObject>`, making `WinObj->IsValid()` safe and `(*WinObj)->TryGetArrayField(...)` at lines 106/108 safe. The missing explicit `!WinObj` check is a style inconsistency compared to other call sites (e.g., HaybaMCPCommandHandler.cpp:87, HaybaMCPCommandHandler.cpp:331) but does not produce a reachable null dereference given the engine API contract.
- `HaybaMCPUIHandler.cpp:219` WidgetToJson: recursive call passes ChildObj.ToSharedRef() without null check on ChildObj — WidgetToJson (line 179-182) unconditionally initializes Out = MakeShared<FJsonObject>() before any branching, so it returns a valid non-null TSharedPtr on every code path — including the early-exit `if (!W) return Out;`. The null-Child guard at line 217 (`if (!Child) continue;`) ensures the recursive call is only made with a non-null UWidget*, which means WidgetToJson's `!W` branch is never taken anyway. ChildObj is therefore always a valid TSharedPtr when ToSharedRef() is called at line 219. The audit finding itself admits this is purely hypothetical ("if a future code change makes WidgetToJson return a null TSharedPtr") — no concrete crash path exists in the current code.
- `HaybaMCPTcpServer.cpp:154` ReadMessage: infinite spin loop when Recv returns true with 0 bytes — The socket is blocking. FTcpSocketBuilder at line 49 does not call .AsNonBlocking(), and no SetNonBlocking() call appears anywhere in the plugin. On a blocking socket, the underlying recv() either blocks until bytes arrive (BytesRead > 0, Recv returns true) or returns 0 on graceful peer close / -1 on error. UE's FSocketBSD::Recv maps both of those non-positive outcomes to returning false, which the existing `if (!Socket->Recv(...)) return false;` guards at lines 157-159 and 181-183 already catch. The only way to get Recv()==true with BytesRead==0 on a blocking UE socket in practice is a spurious wakeup or platform EINTR, which UE's BSD socket layer retries internally. The spin-loop scenario requires a non-blocking socket (EAGAIN/EWOULDBLOCK path), which is not the case here. The loops cannot busy-spin on this code path.
- `HaybaMCPTcpServer.cpp:220` SendMessage: partial body send not detected — client receives truncated framed response and hangs — The accepted client socket is a blocking TCP stream socket on loopback (127.0.0.1 only). On a blocking socket, FSocket::Send() (wrapping the platform send() syscall) either transfers all requested bytes into the kernel send buffer and returns the full count, or returns an error (negative/zero BytesSent). A partial-count return on a blocking loopback socket is not possible in normal operation — the kernel buffer on Windows loopback is far larger than any MCP response. If the header send fails (BytesSent != 4), the if-guard on line 221 correctly skips the body send entirely; the socket then has an error state that the read loop's next Recv() will detect, setting bAlive = false. There is no scenario on this transport where the client receives a correct 4-byte length prefix but only a partial body: either the full body is sent, or the send fails and the connection tears down on the next read. The missing body-BytesSent check is a code quality gap, not a triggerable crash or protocol hang.
- `HaybaMCPTcpServer.cpp:74` Shutdown: WaitForCompletion blocks indefinitely while HandleClientConnection is blocked in ReadMessage — The claim conflates two different threads. Thread->WaitForCompletion() at line 76 waits on the FRunnableThread that runs FHaybaMCPTcpServer::Run() (line 91-113). That loop calls ListenSocket->WaitForPendingConnection with a 100ms timeout (line 96), so it checks bIsRunning every 100ms and exits promptly after bIsRunning is set false. HandleClientConnection (line 115) is NOT the runnable body; it is dispatched onto a UE thread-pool worker via AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask,...) at line 105. That task runs on a pool thread, not the named thread whose completion Shutdown waits on. Therefore the blocking Recv inside ReadMessage (line 120) does not block WaitForCompletion at all — those are independent threads. The named thread exits within ~100ms. The described "hang" cannot occur via the path the candidate describes. There is a distinct latent issue (HandleClientConnection workers hold a raw this pointer and continue running after Shutdown() destroys the FHaybaMCPTcpServer, which could be a use-after-free), but that is a different bug class and is not what the candidate claims.
- `HaybaMCPModule.cpp:81` StartupModule: FindPlugin result not null-checked before ->GetBaseDir() — FindPlugin("HaybaMCPToolkit") called from within that plugin's own StartupModule cannot return null. The Unreal plugin system loads and registers the .uplugin descriptor into IPluginManager before it loads any module DLLs. By the time StartupModule() executes, the plugin is guaranteed to be present in IPluginManager's registry. All three scenarios in the claim (name mismatch, loaded before registration, packaged without the plugin) would prevent the DLL from being loaded or StartupModule from being called in the first place — they cannot produce a state where the code at line 81 is reached but FindPlugin returns null. This is the standard UE idiom used in Epic's own sample plugins.
- `HaybaMCPModule.cpp:452` SendTcpCommand: GetObjectField("data") returns null for missing/non-object field and result is passed unchecked to callback — Both call sites guard against the null Data pointer before dereferencing it. The create_graph callback (HaybaMCPChatPanel.cpp:794) checks `if (!bOk || !Response.IsValid())` before touching Response — a null TSharedPtr is not IsValid(), so the error path exits before any field access. The execute_graph callback (line 823) checks `if (!bOk) { return; }` first, and then wraps the Response access in `if (Response.IsValid() && ...)` at line 825. In neither call site does a null Data (from GetObjectField returning empty TSharedPtr on a missing/non-object field) reach an unchecked dereference. The code is stylistically fragile — GetObjectField silently returns null rather than TryGetObjectField making the absent-field case explicit — but no concrete crash path exists with the current callers.
- `HaybaMCPCommandHandler.cpp:48` MaybeShowPlanModePrompt: AsyncTask(GameThread) dispatched from the game thread without inline guard — No crash path exists under the verified dispatch model. MaybeShowPlanModePrompt() (lines 37-55) posts AsyncTask(GameThread, [](){...}) with an empty capture list and no blocking wait afterwards. Because there is no FEvent->Wait, poll loop, or any synchronization primitive, this is not a class-A deadlock. No raw pointer is captured into the task so there is no use-after-free. The lambda constructs FNotificationInfo on its own stack and calls FSlateNotificationManager::Get().AddNotification(), which is safe on the game thread — exactly where the task will run. Posting a game-thread task from the game thread is redundant indirection in UE but is not harmful: the task graph queues it and executes it after the current handler yields; the game thread is never blocked. The candidate's speculative concern ("if dispatch model changes in the future") is explicitly out of scope for a concrete crash finding.
- `HaybaMCPLandscapeImporter.cpp:75` ChooseBestComponentSizeForImport result unverifiable — zero QuadsPerComponent feeds ALandscape::Import with mismatched size/data — The claim assumes ChooseBestComponentSizeForImport can leave its outputs at zero for a non-standard resolution such as 500x500. It cannot. UE's FLandscapeImportHelper::ChooseBestComponentSizeForImport iterates over every valid landscape component configuration and always selects the nearest enclosing valid size; it has no return-failure path and always writes non-zero values to OutQuadsPerSection, OutSectionsPerComponent, and OutComponentCount for any positive input dimensions. The code also has an explicit pre-check at lines 61-69 that rejects DescW <= 0 or DescH <= 0, so the only inputs that reach line 75 are positive dimensions in the range 1..8193 — exactly the domain the helper was designed for. For a 500x500 input the helper would produce a valid enclosing grid (e.g., QuadsPerSection=63, SectionsPerComponent=2, ComponentCount such that the total is >= 500), and TransformHeightmapImportData would correctly rescale the import data to that grid. No zero-outputs path exists in practice, and the subsequent ALandscape::Import call would receive a consistent (non-zero) SizeX-1/SizeY-1 and a FinalHeightData array that matches. There is no missing null-check after the call that would be load-bearing, because the function cannot silently fail. The finding is a theoretical concern about missing post-call validation on an output variable that in practice is never left at zero by that API.
- `HaybaMCPLandscapeImporter.cpp:96` TransformHeightmapImportData return value not checked — empty FinalHeightData passed to ALandscape::Import — The claimed crash path is blocked by an explicit dimension guard at lines 63-69 that rejects DescW or DescH outside [1, 8193] before any component sizing or data loading occurs. With both dimensions guaranteed >= 1, ChooseBestComponentSizeForImport (a standard UE5 engine function) produces valid non-zero OutQuadsPerSection and OutSectionsPerComponent, making QuadsPerComponent >= 1 and SizeX/SizeY >= 2. TransformHeightmapImportData with ELandscapeImportTransformType::ExpandCentered and a valid non-zero target resolution (SizeX, SizeY) is contractually required by the engine to fill FinalHeightData to exactly SizeX*SizeY elements — it is a resize/pad operation on the already-loaded ImportData whose source resolution matches the descriptor entry used to produce that data. No concrete input that passes the early-exit guards can cause TransformHeightmapImportData to leave FinalHeightData empty or wrongly sized. The QuadsPerComponent=0 subcase cited in the finding cannot occur because DescW/DescH >= 1 is enforced before the component sizing call.
- `HaybaMCPLandscapeImporter.cpp:107` Zero WorldSizeKm or MaxHeightM from JSON produces degenerate scale vector passed to SetActorRelativeScale3D and ALandscape::Import — Zero WorldSizeKm or MaxHeightM from JSON is possible (TryGetNumberField overwrites the default when the key is present with value 0), and it produces ScaleXY=0.0f or ScaleZ=0.0f passed to SetActorRelativeScale3D. However, SetActorRelativeScale3D is an unchecked setter in UE — it accepts any FVector including zero without asserting. ALandscape::Import (lines 139-146) operates on the heightmap resolution integers (SizeX-1, SizeY-1), which are already validated as non-zero by the DescW/DescH guard at line 63 and the ChooseBestComponentSizeForImport output. The scale vector is purely a transform property and is not read by Import. UE's landscape streaming proxy setup and component registration paths do not contain a check()/ensure() on the actor's scale magnitude. The result of passing zero scale is a degenerate (invisible or flat) landscape actor, which is a silent correctness bug, not a check()/crash. The claim's assertion that "UE checks for non-degenerate actor scales in several places (e.g. the physics and rendering subsystems) and can fire ensure()/check() when a zero scale is used during Landscape component registration or streaming proxy setup" is speculative and does not apply to the landscape actor path — landscape components do not go through the rigid-body physics stack that asserts on zero scale.
- `HaybaMCPSequencerHandler.cpp:113` FindOrCreateBinding: unchecked GetMovieScene() null before MS->GetPossessableCount() — Both callers guard against null MS before invoking FindOrCreateBinding. SeqAddTrack (line 185-186) and SeqAddCameraCut (line 296-297) each call `Seq->GetMovieScene()` and return an error if the result is null — they only proceed to call FindOrCreateBinding when MS is confirmed non-null. Since all handlers run serialized on the game thread and the ULevelSequence object is stack-referenced (preventing GC of its sub-objects), `Seq->GetMovieScene()` cannot return a different (null) value between the caller's check and the re-fetch inside FindOrCreateBinding. The property is a simple getter with no side effects and cannot be nulled out by any concurrent path during the handler's execution. There is no concrete input or timing that reaches line 113 with MS==null.
- `HaybaMCPSequencerHandler.cpp:121` FindOrCreateBinding: BindPossessableObject called with potentially null World — The null-World path cannot produce a crash here for two reasons. First, in the dominant call paths (lines 206-209 and 299-303), a null world causes FindActorByPathOrName to return null (line 61 guards it), which triggers an early error return before FindOrCreateBinding is ever reached — so the null world cannot reach BindPossessableObject via that route. Second, ULevelSequence::BindPossessableObject accepts a null world context: it uses the world pointer solely as a path-resolution context passed to UObject::GetPathName(World), which gracefully handles null by returning the full absolute path instead of a relative one. There is no check()/ensure() on the world pointer inside BindPossessableObject in UE5 engine code. The only edge case where World could be null and the actor non-null is the StaticFindObject/LoadObject path in FindActorByPathOrName (lines 63-69), which bypasses the null-world guard — but even then BindPossessableObject does not crash, it just stores an absolute object path instead of a level-relative one. This is a semantic/correctness concern (bindings may not resolve at runtime without a relative path), not a crash.
- `HaybaMCPGASHandler.cpp:119` GasGrantAbility: BP->GeneratedClass used without null check — The claim decomposes into two sub-risks. Sub-risk 1 (null GeneratedClass): line 121 immediately checks `if (!AbilityClass)` and falls through to the `LoadClass` fallback, then line 125 early-returns on null — fully guarded, not a crash path. Sub-risk 2 (null CDO at line 128): `UClass::GetDefaultObject<T>()` in UE4/5 never returns null for a non-null, registered class — if the CDO does not yet exist it is created inline by `CreateDefaultObject()`. Furthermore, the `IsChildOf(UGameplayAbility::StaticClass())` guard at line 125 is only true for a properly registered class whose CDO is already a `UGameplayAbility` subclass instance; the `Cast<UGameplayAbility>` inside the templated `GetDefaultObject<UGameplayAbility>()` will always succeed for such a class. There is no concrete input or timing under which a non-null `AbilityClass` that passes `IsChildOf` produces a null CDO from `GetDefaultObject`. The proposed scenario ("AbilityClass passes IsChildOf but GetDefaultObject returns null") cannot be triggered in normal or malformed-input conditions.
- `HaybaMCPGASHandler.cpp:270` GasApplyEffect: EffectCDO null deref — GetDefaultObject unchecked before ApplyGameplayEffectToSelf — By the time execution reaches line 270, `EffectClass` is guaranteed non-null by the check at line 267. In Unreal Engine, `UClass::GetDefaultObject<T>()` on a non-null `UClass` never returns null — the CDO is created during class registration/load and is unconditionally present. The abstract-class scenario cited by the audit is incorrect: `CLASS_Abstract` prevents `SpawnActor` from constructing instances, but the CDO is still created and returned by `GetDefaultObject`. There is no code path through lines 258-267 that leaves `EffectClass` non-null yet without a valid CDO. The null-deref cannot occur.
- `HaybaMCPNiagaraHandler.cpp:75` NiagaraList: P dereferenced without IsValid() check — `ProcessCommand` in `HaybaMCPCommandHandler.cpp` lines 512-518 unconditionally guarantees that `Params` is non-null before dispatching to any handler. After attempting `TryGetObjectField("params", ...)`, it falls back with `if (!Params.IsValid()) Params = MakeShared<FJsonObject>();`. This means the `TSharedPtr<FJsonObject>` passed as `P` to `Handle()` and onward to `NiagaraList()` is always a valid, heap-allocated (possibly empty) object. A caller sending `niagara_list` with a missing or null `params` field receives a synthesized empty `FJsonObject`; `P->TryGetStringField(...)` at line 75 simply returns false and leaves `PathPrefix` as an empty `FString`, which is handled correctly. There is no code path that delivers an invalid `TSharedPtr` to `NiagaraList`.
- `HaybaMCPGASHandler.cpp:259` GasApplyEffect: BP->GeneratedClass assigned without null check, same pattern as GasGrantAbility — The guard at line 267 (`if (!EffectClass || !EffectClass->IsChildOf(...))`) covers both null cases before line 270 is reached. The finding's null-`GeneratedClass` scenario is handled: if `BP->GeneratedClass` is null, `EffectClass` stays null, the fallback at line 263 runs, and if that also fails line 267 returns an error — no path reaches line 270 with a null pointer. The secondary claim that `GetDefaultObject` can return null for a non-null `UClass*` that passes `IsChildOf` is incorrect: Unreal's `GetDefaultObject<T>()` always returns the CDO (creating it on demand) for a valid, non-null class pointer. There is no concrete input or timing that produces a null deref here.
- `HaybaMCPTcpServer.cpp:181` ReadMessage loops forever when Recv returns true but BytesRead==0 — stalled peer hangs the background reader thread — The socket is created with FTcpSocketBuilder without .AsNonBlocking(), so it is a blocking socket. On a blocking socket, Socket->Recv() does not return true with BytesRead==0 while waiting for data — it simply blocks until data arrives, the peer closes, or an error occurs. The "Recv returns true but BytesRead==0 causes infinite spin" behavior is specific to non-blocking sockets (EAGAIN/EWOULDBLOCK). On a blocking socket, Recv either: (a) blocks until data is available and returns true with BytesRead>0, (b) returns false when the peer closes or an error occurs (which exits the loop via the `if (!Socket->Recv(...)) return false` guard at lines 157 and 181). There is no path that produces a true/BytesRead==0 spin on a blocking socket. The finding conflates non-blocking socket semantics with blocking socket behavior.
- `HaybaMCPMaterialHandler.cpp:419` Multiple MatXxx handlers dereference P without a validity check — null-deref if called with a null params pointer — The finding is a false positive. In `HaybaMCPCommandHandler.cpp` at lines 512-518, `ProcessCommand` explicitly null-guards the params before dispatching: `if (!Params.IsValid()) Params = MakeShared<FJsonObject>();`. This runs unconditionally before the `(*Found)->Handle(Cmd, Params)` call at line 677. So `P` is always a valid (non-null), heap-allocated `FJsonObject` — at worst an empty one — when any `MatXxx` handler is reached. There is no code path through the production dispatch that can deliver a null `TSharedPtr<FJsonObject>` to `Handle()`. The "test harness / future refactor" scenario named in the finding is hypothetical and not a currently reachable crash.
- `HaybaMCPStaticMeshHandler.cpp:105` mesh_set_lod reads lod_index without validating against GetNumSourceModels upper bound — potential OOB on large JSON integer — The bounds check (line 115) and the only call that dereferences LodIndex into the source models array (GetSourceModel at line 119) are both inside the same #if WITH_EDITOR block. The #else branch at line 136 returns an error string immediately — it never touches LodIndex against any array. So there is no code path in a non-editor build that could OOB-access using the unchecked LodIndex. In the editor build, the guard at line 115 fires before GetSourceModel, returning an error for any out-of-range value including INT32_MAX. The double-to-int32 truncation for extreme JSON integers is a style concern but not a crash: even if truncation produces INT32_MAX, the >= Mesh->GetNumSourceModels() comparison catches it immediately since no real mesh has 2^31-1 LODs. There is no reachable out-of-bounds dereference.
- `HaybaMCPBlueprintHandler.cpp:47` BrokenBlueprintsRef static TSet is mutated inside AsyncTask lambda captures — potential iterator invalidation if a compile is triggered off the game thread — The candidate explicitly concedes that both mutation paths (RecompileAndTrack at lines 106-107, Compile at lines 371-372) and the read path (MaybeRejectIfBroken at line 56) run on the game thread under the verified dispatch model, so there is no current data race. The entire claim rests on a speculative future risk ("any future caller path that invokes RecompileAndTrack from a ticker, asset post-import callback, or async compile notification"). Reading the full file confirms there are no FTicker, FTimerHandle, OnAssetPostImport, or any background/async callers of these methods anywhere in the file — the only entry point is Handle(), which the dispatch model guarantees runs serialized on the game thread. There is no concrete input or timing that produces a crash today. The function-local static TSet Add/Remove/Contains operations are safe under single-threaded (game-thread-only) access. This is a code-smell observation about future maintainability, not an actual crash.
