From 17cb65b0dbc0896c1a0819f7d7291af4d2d01ad3 Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 04:35:32 -0400 Subject: [PATCH 01/16] fix(plugin): buffer pending plans so propose_plan survives lazy tab construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: HaybaMCPMainPanel constructs sub-panels lazily on first tab visit. The Plan panel set Module->PlanPanel only when the user clicked the Plan tab. If hayba_propose_plan arrived BEFORE that first visit, the AsyncTask game-thread marshal pinned a null weak ref and silently dropped the plan — but the handler still returned received:true, step_count:N. Agents got a false success signal; users saw an empty Plan tab. Fix: - Extract FHaybaPlanStep into Public/HaybaMCPPlanTypes.h so the module can buffer it without including the private panel header. - Add a thread-safe pending-plan buffer on FHaybaMCPModule (StashPendingPlan / ConsumePendingPlan / HasPendingPlan). - HandleProposePlan now: always stashes, then tries to push to a live panel; uses TPromise/TFuture to find out (on the TCP worker thread) whether the panel actually existed, and reports that back as panel_visible in the response. Adds buffered + hint fields too. - SHaybaMCPMainPanel, when constructing the Plan tab, consumes any buffered plan and calls LoadPlan immediately so plans proposed before first tab visit become visible the moment the tab opens. The agent now gets an honest response telling it whether the user can see the plan right now or has to open the Plan tab. The plan is never lost regardless of timing. --- .../Private/HaybaMCPCommandHandler.cpp | 46 +++++++++++++++++-- .../Private/HaybaMCPMainPanel.cpp | 13 +++++- .../Private/HaybaMCPModule.cpp | 27 +++++++++++ .../Private/HaybaMCPPlanPanel.h | 16 +------ .../HaybaMCPToolkit/Public/HaybaMCPModule.h | 24 ++++++++++ .../Public/HaybaMCPPlanTypes.h | 21 +++++++++ 6 files changed, 128 insertions(+), 19 deletions(-) create mode 100644 unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPPlanTypes.h diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp index c4dfca9a..41253633 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp @@ -14,6 +14,7 @@ #include "HaybaMCPDiffPanel.h" #include "Json.h" #include "Async/Async.h" +#include "Async/Future.h" #include "Editor.h" #include "EngineUtils.h" #include "GameFramework/Actor.h" @@ -426,20 +427,59 @@ static FString HandleProposePlan(const FString& Id, const TSharedPtrTryGetNumberField(TEXT("await_seconds"), AwaitSecs); - AsyncTask(ENamedThreads::GameThread, [Steps, AwaitSecs]() + // Always buffer first — the Plan tab might not be constructed yet (it's + // lazy; the panel weak-ref stays null until the user opens the tab). + // Without this buffer, plans proposed before first tab visit were silently + // dropped on the floor and the handler still returned received:true. + FHaybaMCPModule* M = FModuleManager::GetModulePtr("HaybaMCPToolkit"); + if (M) { - if (FHaybaMCPModule* M = FModuleManager::GetModulePtr("HaybaMCPToolkit")) + M->StashPendingPlan(Steps, AwaitSecs); + } + + // Try to deliver to a live panel immediately too — if it's alive RIGHT + // NOW, the user sees the plan without having to switch tabs. The buffer + // is consumed in lockstep so the next tab open doesn't show a stale copy. + // Use a TPromise on the response side so the response shape honestly + // reflects whether the user can see the plan or has to open the tab. + TSharedRef> PanelOpenPromise = MakeShared>(); + TFuture PanelOpenFuture = PanelOpenPromise->GetFuture(); + AsyncTask(ENamedThreads::GameThread, [Steps, AwaitSecs, PanelOpenPromise]() + { + bool bDelivered = false; + if (FHaybaMCPModule* M2 = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { - if (TSharedPtr Panel = M->PlanPanel.Pin()) + if (TSharedPtr Panel = M2->PlanPanel.Pin()) { Panel->LoadPlan(Steps, AwaitSecs); + // Mark the buffer consumed — the panel already has it. If we + // didn't, a subsequent tab open would re-load the same plan. + TArray _DiscardSteps; int32 _DiscardAwait; + M2->ConsumePendingPlan(_DiscardSteps, _DiscardAwait); + bDelivered = true; } } + PanelOpenPromise->SetValue(bDelivered); }); + // Wait briefly for the game-thread task to report back so the response is + // accurate. 2s is far more than enough — the AsyncTask is a single Pin() + // call. If we time out, the plan is still buffered; we just can't tell + // the caller whether the panel was open. + const bool bPanelOpen = PanelOpenFuture.WaitFor(FTimespan::FromSeconds(2.0)) + ? PanelOpenFuture.Get() + : false; + auto Data = MakeShared(); Data->SetBoolField(TEXT("received"), true); Data->SetNumberField(TEXT("step_count"), Steps.Num()); + Data->SetBoolField(TEXT("buffered"), M != nullptr); + Data->SetBoolField(TEXT("panel_visible"), bPanelOpen); + if (!bPanelOpen) + { + Data->SetStringField(TEXT("hint"), + TEXT("Plan was buffered but the Plan tab isn't open yet. Open Hayba → Plan to see it; the buffered plan is consumed on first construction.")); + } return FHaybaMCPCommandHandler::MakeOkResponse(Id, Data); } diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPMainPanel.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPMainPanel.cpp index 5af65bbb..237eab3d 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPMainPanel.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPMainPanel.cpp @@ -430,7 +430,18 @@ TSharedRef SHaybaMCPMainPanel::BuildPanelContent(EHaybaPanel Panel) { Subtitle = NSLOCTEXT("Hayba", "Plan.Sub", "AI-proposed plan steps before destructive actions."); auto Panel2 = SNew(SHaybaMCPPlanPanel); - if (Module) Module->PlanPanel = Panel2; + if (Module) + { + Module->PlanPanel = Panel2; + // Consume any plan that arrived before this tab existed. + // The propose_plan handler always buffers; without this + // pickup, plans proposed pre-first-visit show as empty. + TArray PendingSteps; int32 PendingAwait = 30; + if (Module->ConsumePendingPlan(PendingSteps, PendingAwait)) + { + Panel2->LoadPlan(PendingSteps, PendingAwait); + } + } Body = Panel2; break; } diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPModule.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPModule.cpp index b3d4655f..d4a1a62c 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPModule.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPModule.cpp @@ -427,4 +427,31 @@ void FHaybaMCPModule::ClearToolCallHistory() ToolCallHistory.Empty(); } +void FHaybaMCPModule::StashPendingPlan(const TArray& Steps, int32 AwaitSecs) +{ + FScopeLock Lock(&PendingPlanLock); + PendingPlanSteps = Steps; + PendingPlanAwaitSecs = AwaitSecs; + bPendingPlanConsumed = false; +} + +bool FHaybaMCPModule::ConsumePendingPlan(TArray& OutSteps, int32& OutAwaitSecs) +{ + FScopeLock Lock(&PendingPlanLock); + if (bPendingPlanConsumed || PendingPlanSteps.Num() == 0) + { + return false; + } + OutSteps = PendingPlanSteps; + OutAwaitSecs = PendingPlanAwaitSecs; + bPendingPlanConsumed = true; + return true; +} + +bool FHaybaMCPModule::HasPendingPlan() const +{ + FScopeLock Lock(&PendingPlanLock); + return !bPendingPlanConsumed && PendingPlanSteps.Num() > 0; +} + IMPLEMENT_MODULE(FHaybaMCPModule, HaybaMCPToolkit) diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPPlanPanel.h b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPPlanPanel.h index eda3fe57..fd246540 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPPlanPanel.h +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPPlanPanel.h @@ -1,25 +1,11 @@ #pragma once #include "CoreMinimal.h" #include "Widgets/SCompoundWidget.h" +#include "HaybaMCPPlanTypes.h" class SVerticalBox; class SScrollBox; -struct FHaybaPlanStep -{ - int32 Index = 0; - FString Title; - FString Description; // optional explainer - FString Tool; // optional: which tool will execute this step - - enum class EStatus : uint8 { Pending, Running, Completed, Failed }; - EStatus Status = EStatus::Pending; - - // Compat shim — old callers set bCompleted / bPending directly. - bool bCompleted = false; - bool bPending = true; -}; - class SHaybaMCPPlanPanel : public SCompoundWidget { public: diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPModule.h b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPModule.h index b75e8664..70e6fe11 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPModule.h +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPModule.h @@ -2,6 +2,8 @@ #include "CoreMinimal.h" #include "Modules/ModuleManager.h" #include "Dom/JsonObject.h" +#include "HAL/CriticalSection.h" +#include "HaybaMCPPlanTypes.h" class FHaybaMCPTcpServer; class FHaybaMCPCommandHandler; @@ -63,6 +65,23 @@ class FHaybaMCPModule : public IModuleInterface // destructive command so each plan must be approved exactly once. bool bPlanApproved = false; + // Pending-plan buffer — survives the gap between the agent's + // hayba_propose_plan TCP call and the user actually opening the Plan + // tab. Without this, plans proposed before first tab visit silently + // dropped on the floor because Module->PlanPanel.Pin() returned null. + // + // Flow: + // - HandleProposePlan() always calls StashPendingPlan(). + // - If PlanPanel is alive RIGHT NOW, HandleProposePlan also calls + // LoadPlan() on it and marks ConsumePendingPlan() — the buffer + // stays in sync with the panel's current view. + // - When MainPanel constructs the Plan tab (lazy), it calls + // ConsumePendingPlan() right after wiring PlanPanel — any plan + // proposed before the tab existed becomes visible. + void StashPendingPlan(const TArray& Steps, int32 AwaitSecs); + bool ConsumePendingPlan(TArray& OutSteps, int32& OutAwaitSecs); + bool HasPendingPlan() const; + // Multicast — fires on the GameThread every time a tool call is recorded. // Subscribers: Chat panel's in-flight trace, future agent observability. DECLARE_MULTICAST_DELEGATE_OneParam(FOnToolCallRecorded, const FHaybaToolCallRecord&); @@ -72,6 +91,11 @@ class FHaybaMCPModule : public IModuleInterface mutable FCriticalSection ToolCallHistoryLock; TArray ToolCallHistory; + mutable FCriticalSection PendingPlanLock; + TArray PendingPlanSteps; + int32 PendingPlanAwaitSecs = 30; + bool bPendingPlanConsumed = true; + TSharedRef OnSpawnTab(const class FSpawnTabArgs& Args); TSharedRef SpawnMainTab(const class FSpawnTabArgs& Args); diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPPlanTypes.h b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPPlanTypes.h new file mode 100644 index 00000000..318bd290 --- /dev/null +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPPlanTypes.h @@ -0,0 +1,21 @@ +#pragma once +#include "CoreMinimal.h" + +// Public so the module can buffer pending plans without including the +// (Private/) SHaybaMCPPlanPanel header. Kept POD on purpose — every field is +// a value type so the struct trivially copies into the module-side buffer +// and out again into the panel. +struct FHaybaPlanStep +{ + int32 Index = 0; + FString Title; + FString Description; // optional explainer + FString Tool; // optional: which tool will execute this step + + enum class EStatus : uint8 { Pending, Running, Completed, Failed }; + EStatus Status = EStatus::Pending; + + // Compat shim — old callers set bCompleted / bPending directly. + bool bCompleted = false; + bool bPending = true; +}; From be2b29f3ef8400944b5d2e92d2c1b2e8b0afcf6d Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 04:37:40 -0400 Subject: [PATCH 02/16] fix(plugin): rename validator panel local Box to ActionBox (C4458 shadow) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SMultiColumnTableRow has a protected `Box` member. UE 5.7 promotes C4458 (declaration hides class member) to error in plugin builds, so the local `TSharedRef Box = ...` in the validator panel column factory broke the build. Rename to ActionBox. No behavior change — the local was scoped to the lambda and only used to compose the action-column row. --- .../Private/Slate/SHaybaValidatorPanel.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/Slate/SHaybaValidatorPanel.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/Slate/SHaybaValidatorPanel.cpp index 32eb4bbc..c6c17cb0 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/Slate/SHaybaValidatorPanel.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/Slate/SHaybaValidatorPanel.cpp @@ -107,8 +107,11 @@ namespace } if (Column == ColActions) { - TSharedRef Box = SNew(SHorizontalBox); - Box->AddSlot().AutoWidth().Padding(2) + // Local name `ActionBox` (not `Box`) to avoid shadowing the + // protected SMultiColumnTableRow::Box member (UE 5.7 promotes + // C4458 to error in plugin builds). + TSharedRef ActionBox = SNew(SHorizontalBox); + ActionBox->AddSlot().AutoWidth().Padding(2) [ SNew(SButton) .ContentPadding(FMargin(6, 2)) @@ -121,7 +124,7 @@ namespace ]; if (!Item->ActorLabel.IsEmpty() || !Item->ActorId.IsEmpty()) { - Box->AddSlot().AutoWidth().Padding(2) + ActionBox->AddSlot().AutoWidth().Padding(2) [ SNew(SButton) .ContentPadding(FMargin(6, 2)) @@ -133,7 +136,7 @@ namespace [ SNew(STextBlock).Text(FText::FromString(TEXT("Jump"))) ] ]; } - return Box; + return ActionBox; } return SNullWidget::NullWidget; } From e39ef01f14a861413775bbd48a318a079d4cb876 Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 04:49:41 -0400 Subject: [PATCH 03/16] fix(plugin): wrap FSocket* in shared connection state to close use-after-free in TCP server Root cause of UE EXCEPTION_ACCESS_VIOLATION at HaybaMCPTcpServer.cpp:123 during landscape_import session on 2026-05-23: - HandleClientConnection (background worker) read a request, dispatched AsyncTask(GameThread, [..., ClientSocket /* raw ptr */]) - Game thread did the heavy World->SpawnActor work - Meanwhile client disconnected (or the OS reset the socket) - Worker observed disconnect, called DestroySocket(ClientSocket) - Game thread eventually returned, called SendMessage on the freed pointer -> access violation, hard crash PR #229 made SpawnActor itself game-thread-safe, but the TCP server captured the FSocket raw pointer into the response lambda with no liveness guard. The slow landscape_import widened the race window from negligible to catastrophic. Fix: wrap FSocket* in struct FHaybaMCPConnection (POD-ish, owns the socket, destructor closes + destroys exactly once). The worker holds a TSharedPtr, every game-thread response lambda captures its own copy by value. The socket survives until the last ref drops. Adds a bAlive flag + per-connection SendLock so: - SendMessage no-ops cleanly if the worker has marked the conn dead while the lambda was running (e.g., client gone) - concurrent game-thread response lambdas for the same connection cannot interleave their header+body writes ReadMessage/SendMessage now take const TSharedPtr& instead of FSocket*. Header layout unchanged: 4-byte big-endian length prefix + JSON body. No behavioral change for the happy path. The crash path becomes a verbose log line + early return. --- .../Private/HaybaMCPTcpServer.cpp | 97 +++++++++++++++---- .../Private/HaybaMCPTcpServer.h | 35 ++++++- 2 files changed, 110 insertions(+), 22 deletions(-) diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp index 21f9ba9d..4c51d84d 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp @@ -7,6 +7,16 @@ DEFINE_LOG_CATEGORY_STATIC(LogHaybaMCPTCP, Log, All); +FHaybaMCPConnection::~FHaybaMCPConnection() +{ + if (Socket) + { + Socket->Close(); + ISocketSubsystem::Get(PLATFORM_SOCKETSUBSYSTEM)->DestroySocket(Socket); + Socket = nullptr; + } +} + FHaybaMCPTcpServer::FHaybaMCPTcpServer(int32 InPort) : Port(InPort) { @@ -91,9 +101,17 @@ uint32 FHaybaMCPTcpServer::Run() { const int32 NewCount = ClientCount.Increment(); UE_LOG(LogHaybaMCPTCP, Log, TEXT("Client accepted (active: %d)"), NewCount); - AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask, [this, ClientSocket]() + + // Wrap the raw FSocket* in a shared connection state. The + // worker thread loop holds the first strong ref; every + // dispatched game-thread response lambda will pin its own + // strong ref by value. The socket survives until the last + // ref drops — closing the use-after-free hole that crashed + // UE in the 2026-05-23 landscape_import session. + TSharedPtr Conn = MakeShared(ClientSocket); + AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask, [this, Conn]() { - HandleClientConnection(ClientSocket); + HandleClientConnection(Conn); }); } } @@ -101,33 +119,44 @@ uint32 FHaybaMCPTcpServer::Run() return 0; } -void FHaybaMCPTcpServer::HandleClientConnection(FSocket* ClientSocket) +void FHaybaMCPTcpServer::HandleClientConnection(TSharedPtr Conn) { - while (bIsRunning) + while (bIsRunning && Conn.IsValid() && Conn->bAlive) { FString Message; - if (!ReadMessage(ClientSocket, Message)) + if (!ReadMessage(Conn, Message)) { const int32 NewCount = ClientCount.Decrement(); UE_LOG(LogHaybaMCPTCP, Log, TEXT("Client disconnected (active: %d)"), NewCount); - ClientSocket->Close(); - ISocketSubsystem::Get(PLATFORM_SOCKETSUBSYSTEM)->DestroySocket(ClientSocket); + // Mark dead so any in-flight game-thread response lambdas + // skip SendMessage instead of writing into a half-closed + // socket. The socket itself is destroyed by ~FHaybaMCPConnection + // when the last strong ref drops (this worker's `Conn` going out + // of scope, plus any pending response lambdas releasing theirs). + Conn->bAlive = false; return; } - AsyncTask(ENamedThreads::GameThread, [this, Message, ClientSocket]() + // Capture the shared_ptr BY VALUE — this is the load-bearing fix. + // The lambda now owns a strong ref to the connection state for the + // entire duration of the game-thread work. If the worker thread + // observes a disconnect and returns (releasing its own ref), the + // socket stays alive until this lambda finishes and releases its + // copy of the shared_ptr. + AsyncTask(ENamedThreads::GameThread, [this, Message, Conn]() { - if (CommandHandler.IsValid()) - { - FString ResponseString = CommandHandler->ProcessCommand(Message); - SendMessage(ClientSocket, ResponseString); - } + if (!CommandHandler.IsValid()) return; + FString ResponseString = CommandHandler->ProcessCommand(Message); + SendMessage(Conn, ResponseString); }); } } -bool FHaybaMCPTcpServer::ReadMessage(FSocket* Socket, FString& OutMessage) +bool FHaybaMCPTcpServer::ReadMessage(const TSharedPtr& Conn, FString& OutMessage) { + if (!Conn.IsValid() || !Conn->Socket) return false; + FSocket* Socket = Conn->Socket; + uint8 Header[4]; int32 HeaderBytesRead = 0; @@ -170,8 +199,28 @@ bool FHaybaMCPTcpServer::ReadMessage(FSocket* Socket, FString& OutMessage) return true; } -void FHaybaMCPTcpServer::SendMessage(FSocket* Socket, const FString& Message) +void FHaybaMCPTcpServer::SendMessage(const TSharedPtr& Conn, const FString& Message) { + // Three guard clauses before touching the socket: + // 1. Shared ref is still valid (always true on game thread — the lambda + // owns it — but defensive). + // 2. bAlive flag wasn't flipped by the worker (client disconnected + // while game thread was busy). + // 3. Socket pointer non-null (defensive against partial-init). + // With these, a use-after-free is no longer possible: even if every other + // ref were gone, the lambda's own ref keeps the socket alive long enough + // for our send attempt; even if the client is gone, we no-op instead of + // poking a freed pointer. + if (!Conn.IsValid() || !Conn->bAlive || !Conn->Socket) return; + + // Serialize concurrent writes. Game-thread lambdas for the same + // connection could in principle be queued back-to-back; without this + // lock the 4-byte header + body of a response could interleave with + // another response's header. + FScopeLock WriteLock(&Conn->SendLock); + if (!Conn->bAlive || !Conn->Socket) return; // re-check after acquiring lock + + FSocket* Socket = Conn->Socket; FTCHARToUTF8 Utf8Msg(*Message); uint32 Length = Utf8Msg.Length(); @@ -181,10 +230,20 @@ void FHaybaMCPTcpServer::SendMessage(FSocket* Socket, const FString& Message) Header[2] = (Length >> 8) & 0xFF; Header[3] = Length & 0xFF; - int32 BytesSent; - Socket->Send(Header, 4, BytesSent); - if (BytesSent == 4) + int32 BytesSent = 0; + const bool bHeaderOk = Socket->Send(Header, 4, BytesSent) && BytesSent == 4; + if (!bHeaderOk) + { + // The peer is gone or the OS write buffer is shut. Mark dead so the + // next read loop iteration tears down cleanly instead of stalling. + UE_LOG(LogHaybaMCPTCP, Verbose, TEXT("SendMessage: header write failed; marking conn dead")); + Conn->bAlive = false; + return; + } + const bool bBodyOk = Socket->Send(reinterpret_cast(Utf8Msg.Get()), Length, BytesSent) && BytesSent == static_cast(Length); + if (!bBodyOk) { - Socket->Send(reinterpret_cast(Utf8Msg.Get()), Length, BytesSent); + UE_LOG(LogHaybaMCPTCP, Verbose, TEXT("SendMessage: body write failed; marking conn dead")); + Conn->bAlive = false; } } diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h index cea16201..71d5fd27 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h @@ -2,10 +2,38 @@ #include "CoreMinimal.h" #include "HAL/Runnable.h" +#include "HAL/CriticalSection.h" #include "Networking.h" class FHaybaMCPCommandHandler; +/** + * Per-connection state, lifetime-managed by TSharedPtr. The worker thread + * holds a strong ref while the read loop is alive; every dispatched + * AsyncTask(GameThread) ALSO captures a strong ref by value. The FSocket + * is destroyed only when the last ref drops, which means the game-thread + * lambda that responds to a request cannot SendMessage on a freed socket + * even if the worker thread sees the client close while the request is + * being processed. + * + * This was the root cause of the 2026-05-23 EXCEPTION_ACCESS_VIOLATION at + * HaybaMCPTcpServer.cpp:123 — see the postmortem at + * docs/superpowers/specs/2026-05-23-pcg-landscape-mcp-postmortem.md. + */ +struct FHaybaMCPConnection +{ + FSocket* Socket = nullptr; + FCriticalSection SendLock; // serialize concurrent writes from + // multiple game-thread response lambdas + bool bAlive = true; // flipped to false on disconnect + + explicit FHaybaMCPConnection(FSocket* InSocket) : Socket(InSocket) {} + ~FHaybaMCPConnection(); // closes + destroys Socket exactly once + + FHaybaMCPConnection(const FHaybaMCPConnection&) = delete; + FHaybaMCPConnection& operator=(const FHaybaMCPConnection&) = delete; +}; + class FHaybaMCPTcpServer : public FRunnable { public: @@ -39,7 +67,8 @@ class FHaybaMCPTcpServer : public FRunnable bool bIsRunning = false; FThreadSafeCounter ClientCount; - void HandleClientConnection(FSocket* ClientSocket); - bool ReadMessage(FSocket* Socket, FString& OutMessage); - void SendMessage(FSocket* Socket, const FString& Message); + // All connection-touching helpers take a shared_ptr now, not raw FSocket*. + void HandleClientConnection(TSharedPtr Conn); + bool ReadMessage(const TSharedPtr& Conn, FString& OutMessage); + void SendMessage(const TSharedPtr& Conn, const FString& Message); }; From 31fdd8808cb3759afccadad9e06994769037e8e9 Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 04:59:27 -0400 Subject: [PATCH 04/16] fix(mcp): print build banner + warn loudly when dist is older than src MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This session burned ~15 min chasing "why isn't hayba_propose_plan a tool?" when the answer was that Claude Code launches the MCP server from a stale dist build that pre-dated 6 hours of merged changes. The agent had no way to know — listTools just returned the older set silently. Now main() prints a startup banner to stderr (which Claude Code shows in the /mcp panel and MCP server logs): [hayba-mcp] build: 2026-05-23T08:58:47.349Z And if any .ts file under src/ is newer than the running dist/, it prints a STALE BUILD warning with the offending file path and the exact rebuild command: [hayba-mcp] STALE BUILD — source is 12.4 min newer than dist. [hayba-mcp] newest src: D:\Hackathons\...\src\tools\index.ts [hayba-mcp] fix: cd mcp-tools/hayba-mcp && npm run build:server, then /mcp reconnect Probe is best-effort — if it throws (e.g. cannot stat src/, which happens in some packaged-binary scenarios) it logs the probe failure and continues. Never fails startup. Skips node_modules and __tests__ subtrees so changes to vendored deps or test files do not spuriously trip the warning. Hooked in before registerTools so the warning lands BEFORE any tool registration log noise, making it visible at the top of the MCP log. --- mcp-tools/hayba-mcp/src/index.ts | 64 ++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/mcp-tools/hayba-mcp/src/index.ts b/mcp-tools/hayba-mcp/src/index.ts index 6e92c964..f3915582 100644 --- a/mcp-tools/hayba-mcp/src/index.ts +++ b/mcp-tools/hayba-mcp/src/index.ts @@ -1,6 +1,9 @@ // mcp_server/src/index.ts import { McpServer, ResourceTemplate } from '@modelcontextprotocol/sdk/server/mcp.js'; import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; +import { statSync, readdirSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { config } from './config.js'; import { listCatalogResources, readCatalogResource } from './resources.js'; import { registerTools } from './tools/index.js'; @@ -8,6 +11,61 @@ import { startDashboard } from './dashboard/server.js'; import { startHttpServer } from './http/server.js'; import { pingSidecar } from './tools/visual/sidecar-client.js'; +/** + * Print a banner to stderr (visible to Claude Code's MCP server log and to + * the `/mcp` status panel) showing this build's age and — critically — warn + * if any source file is newer than the compiled dist this binary was built + * from. Pre-fix, the MCP server would silently run a stale build long + * after a `git pull` + plugin rebuild, and the agent would chase ghosts + * (e.g. "why isn't hayba_propose_plan a tool?"). This makes the staleness + * visible the moment the server starts. + */ +function reportBuildFreshness(): void { + try { + const distFile = fileURLToPath(import.meta.url); // .../dist/index.js + const distRoot = dirname(distFile); + const pkgRoot = dirname(distRoot); // .../hayba-mcp + const srcRoot = join(pkgRoot, 'src'); + const distMs = statSync(distFile).mtimeMs; + + // Walk src/ for any .ts file newer than the running dist build. + let newestSrc = 0; + let newestPath = ''; + const stack: string[] = [srcRoot]; + while (stack.length) { + const cur = stack.pop()!; + let entries: import('node:fs').Dirent[]; + try { entries = readdirSync(cur, { withFileTypes: true }); } + catch { continue; } + for (const e of entries) { + if (e.name === 'node_modules' || e.name === '__tests__') continue; + const full = join(cur, e.name); + if (e.isDirectory()) { stack.push(full); continue; } + if (!e.name.endsWith('.ts') || e.name.endsWith('.d.ts')) continue; + try { + const m = statSync(full).mtimeMs; + if (m > newestSrc) { newestSrc = m; newestPath = full; } + } catch { /* ignore */ } + } + } + + const distAt = new Date(distMs).toISOString(); + process.stderr.write(`[hayba-mcp] build: ${distAt}\n`); + if (newestSrc > distMs) { + const lagMin = ((newestSrc - distMs) / 60000).toFixed(1); + process.stderr.write( + `[hayba-mcp] ⚠️ STALE BUILD — source is ${lagMin} min newer than dist.\n` + + `[hayba-mcp] newest src: ${newestPath} (${new Date(newestSrc).toISOString()})\n` + + `[hayba-mcp] fix: cd mcp-tools/hayba-mcp && npm run build:server, then /mcp reconnect\n`, + ); + } + } catch (e) { + // Never fail startup just because we couldn't stat src/. Worst case, + // user just doesn't see the banner. + process.stderr.write(`[hayba-mcp] (build freshness probe failed: ${(e as Error).message})\n`); + } +} + // ── MCP server setup ───────────────────────────────────────────────────────── const server = new McpServer({ name: 'hayba-mcp', @@ -31,6 +89,12 @@ server.resource('pcgex_catalog', catalogTemplate, async (uri, { category }) => { // ── Start ───────────────────────────────────────────────────────────────────── async function main() { + // FIRST thing — before tool registration or transport setup — print the + // build banner. If dist is stale relative to src, the operator (and the + // agent reading the MCP log) sees it immediately instead of after hours + // of "why isn't registered?" debugging. + reportBuildFreshness(); + // Register all tools. The legacy SessionManager arg is a typed-shim no-op // until the worldbuilding-hub roadmap reaches the terrain integration phase. // Must complete before server.connect — γ-hybrid routing registers its From 93691ee658558b73f33ce91bed6caa75eb725a4f Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 05:12:21 -0400 Subject: [PATCH 05/16] fix(mcp): always-on the Plan Mode meta-tools so agents do not need hayba_invoke for them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When toolRouting is 'deferred' (the default), the gamma-hybrid router captures every server.tool(...) registration into a map but only re-registers the ALWAYS_ON_META subset with MCP. Tools outside that set are reachable only via hayba_invoke or hayba_pack_load. hayba_propose_plan was NOT in ALWAYS_ON_META, which meant: - Plan Mode is on by default for fresh sessions - The UE plugin's destructive-op gate rejects spawn / delete / set_property until the agent calls hayba_propose_plan and the user approves - But the agent had no direct tool — it had to fall back to hayba_invoke({name: "hayba_propose_plan", ...}) - For a workflow that is, by definition, on the critical path for any scene authoring, this is gratuitous friction Add hayba_propose_plan + hayba_mark_plan_step (the natural pair — streams progress back to the Plan tab as each step completes) to the always-on set and wire passthrough registrations. Smoke test (node dist/index.js + tools/list): 24 -> 26 tools, both present, no other surface change. --- mcp-tools/hayba-mcp/src/tools/routing/register.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/mcp-tools/hayba-mcp/src/tools/routing/register.ts b/mcp-tools/hayba-mcp/src/tools/routing/register.ts index 33c022ce..d9cdde55 100644 --- a/mcp-tools/hayba-mcp/src/tools/routing/register.ts +++ b/mcp-tools/hayba-mcp/src/tools/routing/register.ts @@ -46,6 +46,15 @@ export const ALWAYS_ON_META = new Set([ 'hayba_check_ue_status', 'list_tool_categories', 'get_tool_signature', + // Plan Mode handshake. When Plan Mode is on (the default for fresh + // sessions), the UE plugin's destructive-op gate rejects spawn / delete / + // set_property until the agent calls hayba_propose_plan and the user + // clicks Approve. Forcing the agent to reach this through hayba_invoke + // adds friction for a flow that is, by definition, on the critical path. + // Pair: hayba_mark_plan_step lets the agent stream progress back to the + // Plan tab as each step completes. + 'hayba_propose_plan', + 'hayba_mark_plan_step', 'hayba_asset_search', 'hayba_asset_browse', 'hayba_asset_reindex', @@ -259,6 +268,11 @@ export async function registerDeferredRouting( }; passthrough('list_tool_categories'); passthrough('get_tool_signature'); + // Plan Mode meta — see ALWAYS_ON_META comment above for the rationale. + // Passthrough means the agent calls them with the same shape as the + // captured handler, which already wraps executeCommand(). + passthrough('hayba_propose_plan'); + passthrough('hayba_mark_plan_step'); // Validator tools — always-on so the UE plugin's Validator panel and any // agent can read/manage history without loading a pack. passthrough('validator_run'); From 060e38feb9bf730a473b9b2fb073d6e5774b2961 Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 05:13:56 -0400 Subject: [PATCH 06/16] fix(mcp): register Plan Mode meta schemas so hayba_invoke can dispatch them Companion to the ALWAYS_ON_META promotion in the prior commit: hayba_invoke routes via the schema registry (getRawShape) which is populated by reg(name, shape, cost, returns) in recordEagerSchemas. Without an entry here, hayba_invoke({name: "hayba_propose_plan", ...}) returns {ok:false, error:{kind:"unknown_tool"}} even though the tool is in the captured map. Add reg() entries for both Plan Mode meta tools so: - Operators who keep them off the always-on surface can still reach them via hayba_invoke (the documented fallback for unloaded packs). - get_tool_signature("hayba_propose_plan") returns a real shape instead of having to fall through to the legacy sidecar. --- mcp-tools/hayba-mcp/src/tools/index.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/mcp-tools/hayba-mcp/src/tools/index.ts b/mcp-tools/hayba-mcp/src/tools/index.ts index 9d2ad6ba..57cdbfca 100644 --- a/mcp-tools/hayba-mcp/src/tools/index.ts +++ b/mcp-tools/hayba-mcp/src/tools/index.ts @@ -1948,6 +1948,27 @@ function recordEagerSchemas( }, 'high', '{ok, generated_count, duration_ms}'); reg('hayba_check_ue_status', {}, 'low', '{connected, status, ueVersion, plugin, pluginVersion}'); + // Plan Mode meta — schemas registered so hayba_invoke can dispatch them even + // before the operator has them surfaced as direct tools. Mirrors the + // server.tool() shapes above. Pair with ALWAYS_ON_META entries in + // routing/register.ts so they're also callable directly. + reg('hayba_propose_plan', { + steps: z.array(z.union([ + z.string(), + z.object({ + title: z.string(), + description: z.string().optional(), + tool: z.string().optional(), + }), + ])).describe('Ordered list of plan steps'), + await_seconds: z.number().int().min(0).max(600).optional() + .describe('Seconds to wait for approval (informational; default 30)'), + }, 'low', '{received, step_count, buffered, panel_visible, hint?}'); + reg('hayba_mark_plan_step', { + index: z.number().int().min(0).describe('Zero-based plan step index'), + status: z.enum(['running', 'completed', 'failed']).default('completed'), + }, 'low', '{ok, index, status}'); + // ── Asset domain — added Initiative #6 + #10 (ref-preserving move, deps) ─ reg('asset_move', { path: z.string().describe('Source asset path, e.g. "/Game/Foo/Bar.Bar"'), From ad6b48ab36bef751483388e7e99f5120c76fe078 Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 06:11:07 -0400 Subject: [PATCH 07/16] fix(mcp+plugin): make actor_spawn accept mesh paths + python_run accept multi-statement scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three connected MCP bugs surfaced during the 2026-05-23 Palestine scene session, all blocking the same flow: "spawn this mesh into the scene". A. actor_spawn rejected mesh asset paths Agents naturally call actor_spawn(class_path="/Game/.../SM_Foo") and get "class not found" because LoadClass only accepts UClass paths. They then fall back to python_run + EditorAssetLibrary, which is the wrong tool for scene composition. Fix (HaybaMCPActorHandler.cpp): if class_path doesn't resolve to a UClass, try LoadObject. If it's a UStaticMesh, spawn an AStaticMeshActor + SetStaticMesh + SetMobility(Movable). If it's a USkeletalMesh, spawn an ASkeletalMeshActor + SetSkeletalMeshAsset. Otherwise return a clear error naming the asset's actual class. B. python_run was hard-wired to single-statement mode Cmd.ExecutionMode was EPythonCommandExecutionMode::ExecuteStatement, which compiles in `single` mode. Anything beyond a one-liner fails with "SyntaxError: multiple statements found while compiling a single statement". The print-redirect wrapper from PR #228 is itself multi-line, so EVERY user script that went through it failed parse silently. Fix (HaybaMCPPythonHandler.cpp): switch to EPythonCommandExecutionMode::ExecuteFile. Multi-statement scripts now run as-is. C. python_run wrapper triggered the tier-3 classifier The wrapper used exec(compile(_hayba_user_src, ...)) which contains "compile(" — flagged as Tier 3 (filesystem/subprocess). Any user script with a custom snap_z or anything legitimate was blocked without an unsafe override, for no real reason: there is no fs or subprocess in the wrapper. Fix (python-run.ts): drop the compile() wrapper entirely. ExecuteFile now handles the multi-line preamble natively, and a plain exec(_hayba_user_src, _hayba_user_globals) gives the user the isolated globals dict with print pre-bound. Wrapper is now Tier-2 (mutation) — appropriate for an editor-mutating tool. Net effect: actor_spawn with a mesh path Just Works, no python_run needed for the common scene-composition case. When python_run IS needed, multi-line scripts execute and benign scripts no longer trip Tier 3. --- .../hayba-mcp/src/tools/python/python-run.ts | 16 ++-- .../Private/handlers/HaybaMCPActorHandler.cpp | 85 ++++++++++++++++--- .../handlers/HaybaMCPPythonHandler.cpp | 10 ++- 3 files changed, 93 insertions(+), 18 deletions(-) diff --git a/mcp-tools/hayba-mcp/src/tools/python/python-run.ts b/mcp-tools/hayba-mcp/src/tools/python/python-run.ts index 507e384d..cece0dc8 100644 --- a/mcp-tools/hayba-mcp/src/tools/python/python-run.ts +++ b/mcp-tools/hayba-mcp/src/tools/python/python-run.ts @@ -34,10 +34,16 @@ export const schema = z.object({ * stderr/error fields). */ export function wrapScriptForPrintRedirect(userScript: string): string { - // We deliberately keep this string literal — no f-string interpolation of - // user content, no eval. The user script is passed as data via a Python - // triple-quoted string and `exec`'d with a fresh globals dict that has - // print pre-bound to log_warning. + // ExecuteFile mode on the UE side (HaybaMCPPythonHandler.cpp) accepts + // multi-statement scripts, so we can ship the print-override preamble + // as a regular multi-line block. We deliberately avoid `compile(` (the + // tier-3 classifier flags it) and `eval(` (likewise) — the inner + // `exec(user_src, globals)` is enough to give the user script an + // isolated globals dict with print pre-bound to log_warning. + // + // The user script is shipped as data via a triple-quoted Python literal + // so we never re-escape it as Python source. The escape pass only + // protects backslashes and any user-supplied triple-quote. const escaped = userScript .replace(/\\/g, '\\\\') .replace(/"""/g, '\\"\\"\\"'); @@ -48,7 +54,7 @@ export function wrapScriptForPrintRedirect(userScript: string): string { ' _hayba_unreal.log_warning(sep.join(str(a) for a in args))', '_hayba_user_globals = {"__name__": "__main__", "print": _hayba_print, "unreal": _hayba_unreal}', `_hayba_user_src = """${escaped}"""`, - 'exec(compile(_hayba_user_src, "", "exec"), _hayba_user_globals)', + 'exec(_hayba_user_src, _hayba_user_globals)', ].join('\n'); } diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp index 41e9e6a8..512f2414 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp @@ -4,6 +4,10 @@ #include "EngineUtils.h" #include "Subsystems/EditorActorSubsystem.h" #include "Engine/OverlapResult.h" +#include "Engine/StaticMesh.h" +#include "Engine/StaticMeshActor.h" +#include "Engine/SkeletalMesh.h" +#include "Animation/SkeletalMeshActor.h" #include "GameFramework/Actor.h" #include "Components/StaticMeshComponent.h" #include "Components/SkeletalMeshComponent.h" @@ -109,31 +113,90 @@ FHaybaHandlerResult FHaybaMCPActorHandler::Spawn(const TSharedPtr& if (!P->TryGetStringField(TEXT("class_path"), ClassPath) || ClassPath.IsEmpty()) return FHaybaHandlerResult::Err(TEXT("actor_spawn: missing class_path")); - UClass* ActorClass = LoadClass(nullptr, *ClassPath); - if (!ActorClass) - return FHaybaHandlerResult::Err(FString::Printf(TEXT("actor_spawn: class not found: %s"), *ClassPath)); - UEditorActorSubsystem* EAS = GEditor ? GEditor->GetEditorSubsystem() : nullptr; if (!EAS) return FHaybaHandlerResult::Err(TEXT("actor_spawn: EditorActorSubsystem unavailable")); - // Location + // Resolve location / rotation up front since both spawn paths need them. FVector Location = FVector::ZeroVector; const TArray>* LocArr; if (P->TryGetArrayField(TEXT("location"), LocArr)) Location = ParseVec3(*LocArr); - // Rotation FRotator Rotation = FRotator::ZeroRotator; const TArray>* RotArr; if (P->TryGetArrayField(TEXT("rotation"), RotArr) && RotArr->Num() >= 3) Rotation = FRotator((*RotArr)[0]->AsNumber(), (*RotArr)[1]->AsNumber(), (*RotArr)[2]->AsNumber()); - AActor* NewActor = EAS->SpawnActorFromClass(ActorClass, Location, Rotation); - if (!NewActor) - return FHaybaHandlerResult::Err(TEXT("actor_spawn: SpawnActorFromClass failed")); + AActor* NewActor = nullptr; + UClass* SpawnedClass = nullptr; + + // 1) UClass path — the original behaviour (e.g. /Script/Engine.DirectionalLight, + // Blueprint generated classes ending in _C, etc.) + UClass* ActorClass = LoadClass(nullptr, *ClassPath); + if (ActorClass) + { + NewActor = EAS->SpawnActorFromClass(ActorClass, Location, Rotation); + if (!NewActor) + return FHaybaHandlerResult::Err(TEXT("actor_spawn: SpawnActorFromClass failed")); + SpawnedClass = ActorClass; + } + else + { + // 2) Mesh-asset path — accept what every agent reaches for first: + // /Game/JungleRuins/Meshes/SM_GiantTree_01 + // Auto-wraps in the appropriate actor type (StaticMeshActor / + // SkeletalMeshActor) so callers don't have to know the difference + // between a UClass path and a content path. + // This eliminated a whole class of "agent reaches for python_run" + // fallbacks during the 2026-05-23 Palestine scene session. + UObject* AssetObj = LoadObject(nullptr, *ClassPath); + if (!AssetObj) + { + return FHaybaHandlerResult::Err(FString::Printf( + TEXT("actor_spawn: class_path resolves to neither a UClass nor a loadable asset: %s"), + *ClassPath)); + } + + if (UStaticMesh* SM = Cast(AssetObj)) + { + AStaticMeshActor* SMA = Cast( + EAS->SpawnActorFromClass(AStaticMeshActor::StaticClass(), Location, Rotation)); + if (!SMA) + return FHaybaHandlerResult::Err(TEXT("actor_spawn: failed to spawn StaticMeshActor for mesh asset")); + if (UStaticMeshComponent* SMC = SMA->GetStaticMeshComponent()) + { + // Mobility must be Movable to let the agent transform the actor + // afterwards; default Static is read-only after spawn. + SMC->SetMobility(EComponentMobility::Movable); + SMC->SetStaticMesh(SM); + } + NewActor = SMA; + SpawnedClass = AStaticMeshActor::StaticClass(); + } + else if (USkeletalMesh* SK = Cast(AssetObj)) + { + ASkeletalMeshActor* SKA = Cast( + EAS->SpawnActorFromClass(ASkeletalMeshActor::StaticClass(), Location, Rotation)); + if (!SKA) + return FHaybaHandlerResult::Err(TEXT("actor_spawn: failed to spawn SkeletalMeshActor for mesh asset")); + if (USkeletalMeshComponent* SKC = SKA->GetSkeletalMeshComponent()) + { + SKC->SetMobility(EComponentMobility::Movable); + SKC->SetSkeletalMeshAsset(SK); + } + NewActor = SKA; + SpawnedClass = ASkeletalMeshActor::StaticClass(); + } + else + { + return FHaybaHandlerResult::Err(FString::Printf( + TEXT("actor_spawn: asset is neither UClass nor StaticMesh/SkeletalMesh: %s (class=%s)"), + *ClassPath, *AssetObj->GetClass()->GetName())); + } + } - // Scale + // Scale (post-spawn so it applies to both paths) const TArray>* ScaleArr; if (P->TryGetArrayField(TEXT("scale"), ScaleArr)) NewActor->SetActorScale3D(ParseVec3(*ScaleArr, FVector::OneVector)); @@ -146,7 +209,7 @@ FHaybaHandlerResult FHaybaMCPActorHandler::Spawn(const TSharedPtr& TSharedPtr Out = MakeShared(); Out->SetStringField(TEXT("actor_id"), NewActor->GetName()); Out->SetStringField(TEXT("label"), NewActor->GetActorLabel()); - Out->SetStringField(TEXT("class"), ActorClass->GetName()); + Out->SetStringField(TEXT("class"), SpawnedClass->GetName()); return FHaybaHandlerResult::Ok(Out); } diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPPythonHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPPythonHandler.cpp index cb5ce6be..3096eddc 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPPythonHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPPythonHandler.cpp @@ -75,10 +75,16 @@ FHaybaHandlerResult FHaybaMCPPythonHandler::Run(const TSharedPtr& P return FHaybaHandlerResult::Err(TEXT("Python plugin not loaded — enable the PythonScriptPlugin in your project")); } - // Execute + // Execute. ExecuteFile (not ExecuteStatement) so the script can contain + // multiple top-level statements — imports, function defs, loops, etc. + // ExecuteStatement compiles in `single` mode, which raises + // SyntaxError: multiple statements found while compiling a single statement + // on the second line of any non-trivial script. Confirmed in the + // 2026-05-23 Palestine scene session: every print-redirect wrapper + // ever shipped was unusable because the wrapper itself is multi-line. FPythonCommandEx Cmd; Cmd.Command = Code; - Cmd.ExecutionMode = EPythonCommandExecutionMode::ExecuteStatement; + Cmd.ExecutionMode = EPythonCommandExecutionMode::ExecuteFile; const bool bOk = PythonPlugin->ExecPythonCommandEx(Cmd); From e8f1f8bcb9b17a75d1cabd18e8ff21e7f2ac414a Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 06:20:33 -0400 Subject: [PATCH 08/16] fix(plugin): keep TCP wire-facing signatures ABI-stable so Live Coding patches dont crash A second EXCEPTION_ACCESS_VIOLATION hit during the 2026-05-23 Palestine session, this time at HaybaMCPTcpServer.cpp:166 inside ReadMessage, reading address 0xffffffffffffffff (canonical freed-pointer canary). Plugin DLL was _patch_0 (Live Coding). Root cause: my prior PR #232 fix (commit 3) changed ReadMessage from bool ReadMessage(FSocket* Socket, FString& OutMessage) to bool ReadMessage(const TSharedPtr& Conn, FString&) Live Coding can hot-patch function BODIES but cannot safely change function SIGNATURES. Stale worker threads (compiled against the old FSocket* signature) called the patched function expecting a TSharedPtr parameter, reinterpreted the old FSocket* bytes as a TSharedPtr, deref the garbage vtable, crash. Fix: keep ReadMessage / SendMessage / HandleClientConnection signatures exactly as they always were (raw FSocket* params). Move the lifetime state into an internal lookup table on the server (TMap>). The strong ref is held by the table plus pinned by every game-thread response lambda; the FSocket cannot be freed until both drop. The original UAF fix from PR #232 commit 3 is preserved. This shape is hot-patchable: function signatures never change, only function bodies and the connection-table fields are added (Live Coding handles header field additions on the module-level singleton fine). Structural changes like signature swaps now require a full rebuild; this is documented in-line so the next person to touch this file sees the constraint before making the same mistake. --- .../Private/HaybaMCPTcpServer.cpp | 148 +++++++++++------- .../Private/HaybaMCPTcpServer.h | 55 ++++--- 2 files changed, 125 insertions(+), 78 deletions(-) diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp index 4c51d84d..5c2715ae 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp @@ -35,10 +35,6 @@ bool FHaybaMCPTcpServer::Start() return false; } - // Use the injected command handler (registered by the Module with all - // domain handlers). Fall back to a fresh empty one only as a safety net - // — this would mean every command returns "Unknown command", which is - // the long-standing bug this guard is here to make obvious. if (!CommandHandler.IsValid()) { UE_LOG(LogHaybaMCPTCP, Warning, @@ -85,9 +81,42 @@ void FHaybaMCPTcpServer::Shutdown() ListenSocket = nullptr; } + // Drop all connection state. Each entry's destructor closes its FSocket + // exactly once. Worker threads that were inside ReadMessage will get + // FindConnState returning null on their next iteration and bail. + { + FScopeLock Lock(&ConnTableLock); + ConnTable.Empty(); + } + UE_LOG(LogHaybaMCPTCP, Log, TEXT("TCP server stopped")); } +void FHaybaMCPTcpServer::RegisterConn(const TSharedPtr& Conn) +{ + if (!Conn.IsValid() || !Conn->Socket) return; + FScopeLock Lock(&ConnTableLock); + ConnTable.Add(Conn->Socket, Conn); +} + +TSharedPtr FHaybaMCPTcpServer::FindConnState(FSocket* Socket) const +{ + if (!Socket) return nullptr; + FScopeLock Lock(&ConnTableLock); + if (const TSharedPtr* Found = ConnTable.Find(Socket)) + { + return *Found; + } + return nullptr; +} + +void FHaybaMCPTcpServer::UnregisterConn(FSocket* Socket) +{ + if (!Socket) return; + FScopeLock Lock(&ConnTableLock); + ConnTable.Remove(Socket); +} + uint32 FHaybaMCPTcpServer::Run() { while (bIsRunning) @@ -102,16 +131,15 @@ uint32 FHaybaMCPTcpServer::Run() const int32 NewCount = ClientCount.Increment(); UE_LOG(LogHaybaMCPTCP, Log, TEXT("Client accepted (active: %d)"), NewCount); - // Wrap the raw FSocket* in a shared connection state. The - // worker thread loop holds the first strong ref; every - // dispatched game-thread response lambda will pin its own - // strong ref by value. The socket survives until the last - // ref drops — closing the use-after-free hole that crashed - // UE in the 2026-05-23 landscape_import session. + // Register lifetime state under the socket key BEFORE we dispatch + // the worker. The wire-facing HandleClientConnection takes the + // raw FSocket* (ABI-stable for Live Coding); it looks up its + // own lifetime state internally. TSharedPtr Conn = MakeShared(ClientSocket); - AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask, [this, Conn]() + RegisterConn(Conn); + AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask, [this, ClientSocket]() { - HandleClientConnection(Conn); + HandleClientConnection(ClientSocket); }); } } @@ -119,43 +147,67 @@ uint32 FHaybaMCPTcpServer::Run() return 0; } -void FHaybaMCPTcpServer::HandleClientConnection(TSharedPtr Conn) +void FHaybaMCPTcpServer::HandleClientConnection(FSocket* ClientSocket) { - while (bIsRunning && Conn.IsValid() && Conn->bAlive) + // Pin a strong ref for the entire worker loop. If we don't, a slow + // game-thread response could be the only remaining ref by the time the + // worker's next ReadMessage iteration runs. + TSharedPtr ConnRef = FindConnState(ClientSocket); + if (!ConnRef.IsValid()) + { + UE_LOG(LogHaybaMCPTCP, Warning, TEXT("HandleClientConnection called for socket with no registered conn state — bail")); + return; + } + + while (bIsRunning && ConnRef.IsValid() && ConnRef->bAlive) { FString Message; - if (!ReadMessage(Conn, Message)) + if (!ReadMessage(ClientSocket, Message)) { const int32 NewCount = ClientCount.Decrement(); UE_LOG(LogHaybaMCPTCP, Log, TEXT("Client disconnected (active: %d)"), NewCount); - // Mark dead so any in-flight game-thread response lambdas - // skip SendMessage instead of writing into a half-closed - // socket. The socket itself is destroyed by ~FHaybaMCPConnection - // when the last strong ref drops (this worker's `Conn` going out - // of scope, plus any pending response lambdas releasing theirs). - Conn->bAlive = false; + ConnRef->bAlive = false; + UnregisterConn(ClientSocket); + // ConnRef going out of scope drops the worker's strong ref. If + // any game-thread lambda still holds one, the FSocket survives + // until that lambda finishes. Otherwise destructor runs now and + // closes + destroys the socket cleanly. return; } - // Capture the shared_ptr BY VALUE — this is the load-bearing fix. - // The lambda now owns a strong ref to the connection state for the - // entire duration of the game-thread work. If the worker thread - // observes a disconnect and returns (releasing its own ref), the - // socket stays alive until this lambda finishes and releases its - // copy of the shared_ptr. - AsyncTask(ENamedThreads::GameThread, [this, Message, Conn]() + // Capture the SHARED REF (not the raw socket pointer) into the + // game-thread lambda by value. This is the load-bearing fix: + // SendMessage can then unwrap the strong ref and know the FSocket + // is alive for the duration of its write, even if the worker thread + // observed disconnect and unregistered in the meantime. + TSharedPtr ConnForLambda = ConnRef; + AsyncTask(ENamedThreads::GameThread, [this, Message, ConnForLambda]() { if (!CommandHandler.IsValid()) return; + // ConnForLambda is the strong ref. ConnForLambda->Socket is the + // raw FSocket* we'll pass to SendMessage (ABI-stable param). + // The shared ref keeps the FSocket alive throughout this lambda. FString ResponseString = CommandHandler->ProcessCommand(Message); - SendMessage(Conn, ResponseString); + if (ConnForLambda.IsValid() && ConnForLambda->bAlive && ConnForLambda->Socket) + { + SendMessage(ConnForLambda->Socket, ResponseString); + } }); } } -bool FHaybaMCPTcpServer::ReadMessage(const TSharedPtr& Conn, FString& OutMessage) +bool FHaybaMCPTcpServer::ReadMessage(FSocket* Socket, FString& OutMessage) { - if (!Conn.IsValid() || !Conn->Socket) return false; - FSocket* Socket = Conn->Socket; + // ABI-stable wire-facing function: do not change this signature without a + // full plugin rebuild. The 2026-05-23 EXCEPTION_ACCESS_VIOLATION at + // this exact line was caused by a Live Coding patch that changed the + // parameter from FSocket* to TSharedPtr<>, leaving stale worker + // threads reinterpreting an FSocket* as a TSharedPtr and deref'ing + // garbage. Lifetime is now tracked internally via FindConnState so the + // signature can stay stable indefinitely. + if (!Socket) return false; + TSharedPtr Conn = FindConnState(Socket); + if (!Conn.IsValid() || !Conn->bAlive) return false; uint8 Header[4]; int32 HeaderBytesRead = 0; @@ -199,28 +251,20 @@ bool FHaybaMCPTcpServer::ReadMessage(const TSharedPtr& Conn return true; } -void FHaybaMCPTcpServer::SendMessage(const TSharedPtr& Conn, const FString& Message) +void FHaybaMCPTcpServer::SendMessage(FSocket* Socket, const FString& Message) { - // Three guard clauses before touching the socket: - // 1. Shared ref is still valid (always true on game thread — the lambda - // owns it — but defensive). - // 2. bAlive flag wasn't flipped by the worker (client disconnected - // while game thread was busy). - // 3. Socket pointer non-null (defensive against partial-init). - // With these, a use-after-free is no longer possible: even if every other - // ref were gone, the lambda's own ref keeps the socket alive long enough - // for our send attempt; even if the client is gone, we no-op instead of - // poking a freed pointer. - if (!Conn.IsValid() || !Conn->bAlive || !Conn->Socket) return; - - // Serialize concurrent writes. Game-thread lambdas for the same - // connection could in principle be queued back-to-back; without this - // lock the 4-byte header + body of a response could interleave with - // another response's header. + // ABI-stable. See ReadMessage comment. + if (!Socket) return; + TSharedPtr Conn = FindConnState(Socket); + if (!Conn.IsValid() || !Conn->bAlive) return; + + // Serialize concurrent writes. Game-thread response lambdas for the same + // connection could in principle queue back-to-back; without this lock + // the 4-byte header + body of a response could interleave with another + // response's header. FScopeLock WriteLock(&Conn->SendLock); - if (!Conn->bAlive || !Conn->Socket) return; // re-check after acquiring lock + if (!Conn->bAlive) return; - FSocket* Socket = Conn->Socket; FTCHARToUTF8 Utf8Msg(*Message); uint32 Length = Utf8Msg.Length(); @@ -234,8 +278,6 @@ void FHaybaMCPTcpServer::SendMessage(const TSharedPtr& Conn const bool bHeaderOk = Socket->Send(Header, 4, BytesSent) && BytesSent == 4; if (!bHeaderOk) { - // The peer is gone or the OS write buffer is shut. Mark dead so the - // next read loop iteration tears down cleanly instead of stalling. UE_LOG(LogHaybaMCPTCP, Verbose, TEXT("SendMessage: header write failed; marking conn dead")); Conn->bAlive = false; return; diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h index 71d5fd27..8cfd44e9 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h @@ -8,27 +8,28 @@ class FHaybaMCPCommandHandler; /** - * Per-connection state, lifetime-managed by TSharedPtr. The worker thread - * holds a strong ref while the read loop is alive; every dispatched - * AsyncTask(GameThread) ALSO captures a strong ref by value. The FSocket - * is destroyed only when the last ref drops, which means the game-thread - * lambda that responds to a request cannot SendMessage on a freed socket - * even if the worker thread sees the client close while the request is - * being processed. + * Per-connection lifetime state. Owned by FHaybaMCPTcpServer's internal + * connection table; every dispatched AsyncTask(GameThread) lambda also + * pins a strong ref to keep the FSocket alive across the worker→game-thread + * boundary. See FHaybaMCPTcpServer::FindConnState for the lookup pattern. * - * This was the root cause of the 2026-05-23 EXCEPTION_ACCESS_VIOLATION at - * HaybaMCPTcpServer.cpp:123 — see the postmortem at - * docs/superpowers/specs/2026-05-23-pcg-landscape-mcp-postmortem.md. + * The previous shape of this struct was passed in as a parameter to + * ReadMessage / SendMessage — that exposed the layout to Live Coding's + * ABI-stability constraints and caused EXCEPTION_ACCESS_VIOLATION when + * a stale worker thread (compiled against the old `FSocket*` signature) + * called a patched function expecting a `TSharedPtr` parameter. We now + * keep the lifetime bookkeeping ENTIRELY internal to the server (lookup + * by raw FSocket* pointer key) so the wire-facing function signatures + * stay byte-for-byte stable across Live Coding patches. */ struct FHaybaMCPConnection { FSocket* Socket = nullptr; - FCriticalSection SendLock; // serialize concurrent writes from - // multiple game-thread response lambdas - bool bAlive = true; // flipped to false on disconnect + FCriticalSection SendLock; + bool bAlive = true; explicit FHaybaMCPConnection(FSocket* InSocket) : Socket(InSocket) {} - ~FHaybaMCPConnection(); // closes + destroys Socket exactly once + ~FHaybaMCPConnection(); FHaybaMCPConnection(const FHaybaMCPConnection&) = delete; FHaybaMCPConnection& operator=(const FHaybaMCPConnection&) = delete; @@ -40,13 +41,6 @@ class FHaybaMCPTcpServer : public FRunnable FHaybaMCPTcpServer(int32 InPort); virtual ~FHaybaMCPTcpServer(); - /** - * Inject the command handler the TCP thread should route requests to. - * Must be called BEFORE Start(); otherwise Start() falls back to a fresh - * handler with no domain handlers registered (every command returns - * "Unknown command"). Module::StartTcpServer() is responsible for wiring - * its fully-registered handler in. - */ void SetCommandHandler(TSharedPtr InHandler) { CommandHandler = InHandler; } bool Start(); @@ -67,8 +61,19 @@ class FHaybaMCPTcpServer : public FRunnable bool bIsRunning = false; FThreadSafeCounter ClientCount; - // All connection-touching helpers take a shared_ptr now, not raw FSocket*. - void HandleClientConnection(TSharedPtr Conn); - bool ReadMessage(const TSharedPtr& Conn, FString& OutMessage); - void SendMessage(const TSharedPtr& Conn, const FString& Message); + // Lifetime table keyed by FSocket* — ABI-stable wire-facing functions + // look up their connection state through here. Game-thread response + // lambdas pin a strong ref by capturing a TSharedPtr + // from FindConnState, NOT by capturing FSocket* directly. + mutable FCriticalSection ConnTableLock; + TMap> ConnTable; + void RegisterConn(const TSharedPtr& Conn); + TSharedPtr FindConnState(FSocket* Socket) const; + void UnregisterConn(FSocket* Socket); + + // Wire-facing helpers — SIGNATURES MUST NOT CHANGE for Live Coding + // compatibility. They look up lifetime via FindConnState internally. + void HandleClientConnection(FSocket* ClientSocket); + bool ReadMessage(FSocket* Socket, FString& OutMessage); + void SendMessage(FSocket* Socket, const FString& Message); }; From b2d35b91de81d13359bde1e5c81810e950a06edf Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 06:43:22 -0400 Subject: [PATCH 09/16] fix(plugin+mcp): give actor_spawn / actor_transform snap_to_landscape so agents stop reaching for python_run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The user, watching the agent fight scene-composition for 10 minutes: "why are you using python instead of the mcp tools directly". They were right — actor_spawn had no snap-to-landscape semantics, so every prop placement either guessed Z (and floated or buried) or fell back to python_run with a manual unreal.SystemLibrary.line_trace_single. Three connected changes: A. Plugin: SnapZToLandscape() helper + snap_to_landscape param - HaybaMCPActorHandler.cpp adds a private helper that line-traces against ECC_WorldStatic and checks the hit actor is an ALandscapeProxy (so we don't snap onto a random mesh). - actor_spawn and actor_transform both accept snap_to_landscape:bool and z_offset:number. When set, the handler overrides the supplied Z with the landscape hit + offset. - Response includes snapped_to_landscape:true + snapped_z so the agent can read back what actually happened. B. TS schemas updated - actor-spawn.ts and actor-transform.ts schemas describe the new params. recordEagerSchemas in tools/index.ts updated so get_tool_signature returns them too. - class_path now describes the dual UClass/StaticMesh acceptance (the prior fix in this PR). C. Validator rule: actor_spawn_not_on_landscape - rules.ts adds the rule (severity: warning). - tool-hooks.ts adds evaluateActorSpawnNotOnLandscape: fires when class_path starts with /Game/ (i.e. a mesh asset, not a UClass) AND the agent did not pass snap_to_landscape:true AND the result did not report snapped_to_landscape:true. Surfaces in the validator history + the UE plugin's Validator panel. - actor-spawn.ts handler now wraps its result with runAfterTool/attachFindingsToValue (same pattern as execute-pcg-graph.ts and asset-retriever/browse.ts). Lesson preserved as a memory pointer: when an agent reaches for python_run during scene composition, the right fix is to extend the native actor_/asset_ tool — not to make python_run faster. Requires plugin rebuild for the C++ side; the TS side picks up on /mcp reconnect. --- .../hayba-mcp/src/tools/actor/actor-spawn.ts | 31 +++++- .../src/tools/actor/actor-transform.ts | 4 + mcp-tools/hayba-mcp/src/tools/index.ts | 10 +- mcp-tools/hayba-mcp/src/validator/rules.ts | 8 ++ .../hayba-mcp/src/validator/tool-hooks.ts | 53 ++++++++++ .../Private/handlers/HaybaMCPActorHandler.cpp | 97 +++++++++++++++++++ 6 files changed, 198 insertions(+), 5 deletions(-) diff --git a/mcp-tools/hayba-mcp/src/tools/actor/actor-spawn.ts b/mcp-tools/hayba-mcp/src/tools/actor/actor-spawn.ts index 8054fe3c..2cd29c21 100644 --- a/mcp-tools/hayba-mcp/src/tools/actor/actor-spawn.ts +++ b/mcp-tools/hayba-mcp/src/tools/actor/actor-spawn.ts @@ -1,7 +1,10 @@ import { z } from 'zod'; +import { join } from 'node:path'; import type { ToolHandler } from '../types.js'; import { executeCommand } from '../tool-executor.js'; import type { HaybaToolMeta } from '../hayba-tool-meta.js'; +import { runAfterTool } from '../../validator/runner.js'; +import { attachFindingsToValue } from '../../validator/response.js'; // TODO: wire into registerTools with RateLimiter + ToolCache + appendMeta wrapper @@ -15,11 +18,16 @@ export const meta: HaybaToolMeta = { const vec3 = z.tuple([z.number(), z.number(), z.number()]); export const schema = z.object({ - class_path: z.string().min(1), + class_path: z.string().min(1) + .describe('Either a UClass path (/Script/Engine.DirectionalLight, /Game/.../BP_Foo.BP_Foo_C) OR a StaticMesh/SkeletalMesh asset path (/Game/.../SM_Tree.SM_Tree) — the latter auto-wraps in a Static/SkeletalMeshActor.'), location: vec3.optional(), rotation: vec3.optional(), scale: vec3.optional(), label: z.string().optional(), + snap_to_landscape: z.boolean().optional() + .describe('After spawn, line-trace from above the spawn XY down to the landscape surface and set the actor Z to that hit (plus z_offset). Prefer this over python_run line traces when placing props onto terrain.'), + z_offset: z.number().optional() + .describe('Added to the snapped Z. Useful for assets whose pivot is not at the visible base (e.g. SM_GiantTree_01 needs -380). Ignored when snap_to_landscape is false.'), }); export const actorSpawnHandler: ToolHandler = async (args) => { @@ -28,5 +36,24 @@ export const actorSpawnHandler: ToolHandler = async (args) => { return { content: [{ type: 'text', text: `Validation error: ${parsed.error.message}` }], isError: true }; } const data = await executeCommand('actor_spawn', parsed.data as Record); - return { content: [{ type: 'text', text: JSON.stringify(data, null, 2) }] }; + + // Post-condition: surface actor_spawn_not_on_landscape and any future + // actor-spawn rules. Lazy-import the TCP client to keep this module + // import-safe in pure-TS tests. + let ue = null; + try { + const tcpMod = await import('../../tcp-client.js'); + ue = await tcpMod.ensureConnected().catch(() => null); + } catch { + ue = null; + } + const findings = await runAfterTool({ + toolName: 'actor_spawn', + toolArgs: parsed.data as Record, + toolResult: data as Record, + ue, + scratchDir: join(process.cwd(), '.scratch'), + }); + const enriched = attachFindingsToValue(data as Record, findings); + return { content: [{ type: 'text', text: JSON.stringify(enriched, null, 2) }] }; }; diff --git a/mcp-tools/hayba-mcp/src/tools/actor/actor-transform.ts b/mcp-tools/hayba-mcp/src/tools/actor/actor-transform.ts index 3e9fcc53..6137de7a 100644 --- a/mcp-tools/hayba-mcp/src/tools/actor/actor-transform.ts +++ b/mcp-tools/hayba-mcp/src/tools/actor/actor-transform.ts @@ -19,6 +19,10 @@ export const schema = z.object({ location: vec3.optional(), rotation: vec3.optional(), scale: vec3.optional(), + snap_to_landscape: z.boolean().optional() + .describe('After applying location/rotation/scale, line-trace from above the actor XY down to the landscape surface and set Z to that hit (plus z_offset). Lets you batch-align props to terrain without a python_run round-trip.'), + z_offset: z.number().optional() + .describe('Added to the snapped Z. For pivot-offset assets like SM_GiantTree_01 (needs -380). Ignored when snap_to_landscape is false.'), }); export const actorTransformHandler: ToolHandler = async (args) => { diff --git a/mcp-tools/hayba-mcp/src/tools/index.ts b/mcp-tools/hayba-mcp/src/tools/index.ts index 57cdbfca..25341ecb 100644 --- a/mcp-tools/hayba-mcp/src/tools/index.ts +++ b/mcp-tools/hayba-mcp/src/tools/index.ts @@ -1880,12 +1880,14 @@ function recordEagerSchemas( // ── Actor domain ────────────────────────────────────────────────────────── reg('actor_spawn', { - class_path: z.string().describe('UE class path, e.g. "/Script/Engine.StaticMeshActor"'), + class_path: z.string().describe('UClass path OR /Game StaticMesh/SkeletalMesh asset path (auto-wraps in StaticMeshActor)'), location: coerceVec3.optional(), rotation: coerceVec3.optional(), scale: coerceVec3.optional(), label: z.string().optional(), - }, 'medium', '{actor_id, label, class}'); + snap_to_landscape: z.boolean().optional().describe('Line-trace to landscape surface and set Z to the hit (plus z_offset). Use this for props on terrain instead of guessing Z.'), + z_offset: z.number().optional().describe('Added to snapped Z. For pivot-shifted assets (e.g. SM_GiantTree_01 needs -380).'), + }, 'medium', '{actor_id, label, class, snapped_to_landscape?, snapped_z?, validator?}'); reg('actor_list', { class_filter: z.string().optional().describe('Exact class name filter'), tag: z.string().optional().describe('Tag filter'), @@ -1896,7 +1898,9 @@ function recordEagerSchemas( location: coerceVec3.optional(), rotation: coerceVec3.optional(), scale: coerceVec3.optional(), - }, 'low', '{ok, actor_id, before, after}'); + snap_to_landscape: z.boolean().optional().describe('After applying location/rotation/scale, line-trace to landscape surface and set Z. Batch-align props to terrain without python_run.'), + z_offset: z.number().optional().describe('Added to snapped Z. For pivot-shifted assets.'), + }, 'low', '{ok, actor_id, before, after, snapped_to_landscape?, snapped_z?}'); // ── Scene domain ────────────────────────────────────────────────────────── reg('scene_export', { diff --git a/mcp-tools/hayba-mcp/src/validator/rules.ts b/mcp-tools/hayba-mcp/src/validator/rules.ts index 3052ea23..ccdaf6e6 100644 --- a/mcp-tools/hayba-mcp/src/validator/rules.ts +++ b/mcp-tools/hayba-mcp/src/validator/rules.ts @@ -148,6 +148,14 @@ export const RULES: ValidatorRule[] = [ refs: ['[[asset-browse-plugin-out-of-date]]'], trigger: { after_tool: ['hayba_asset_browse', 'hayba_asset_search'] }, }, + { + id: 'actor_spawn_not_on_landscape', + severity: 'warning', + message: 'actor_spawn placed a mesh actor with no snap_to_landscape — props will float or bury', + hint: 'You called actor_spawn with a StaticMesh class_path and an explicit Z, but did not pass snap_to_landscape: true. If the landscape height at the spawn XY is not what you guessed, the actor will float in the sky or sink under the ground. Pass snap_to_landscape: true (and z_offset for pivot-shifted assets like SM_GiantTree_01 which needs -380) so the plugin line-traces the landscape for you. The 2026-05-23 Palestine scene session spent ten minutes round-tripping through python_run because this parameter did not exist — it does now.', + refs: ['[[actor-spawn-snap-to-landscape]]'], + trigger: { after_tool: 'actor_spawn' }, + }, ]; /** Build an id → rule lookup (rebuilt on each call so tests can mutate). */ diff --git a/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts b/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts index 5f9a858a..c31f70c6 100644 --- a/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts +++ b/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts @@ -247,6 +247,58 @@ export function isSelfSocketScript(script: string): boolean { return false; } +// ── actor_spawn_not_on_landscape ──────────────────────────────────────────── +// +// Fires when an agent spawns a StaticMesh asset with an explicit Z but +// without `snap_to_landscape: true`. The 2026-05-23 Palestine scene session +// produced two batches of pillars at z=-50 floating below the landscape +// surface because the agent guessed Z instead of asking the plugin to +// snap. The plugin now ships a snap parameter on actor_spawn — this rule +// surfaces "you forgot to use it" before the user notices in a screenshot. +async function evaluateActorSpawnNotOnLandscape(ctx: ValidatorContext): Promise { + const args = asRecord(ctx.toolArgs); + const result = asRecord(ctx.toolResult); + + // Only fire for mesh-style class_paths (/Game/...). UClass spawns + // (DirectionalLight, PostProcessVolume, blueprint actor classes) aren't + // expected to sit on the landscape and would be noise. + const classPath = String(args.class_path ?? ''); + if (!classPath.startsWith('/Game/')) return null; + + // If the agent already passed snap_to_landscape:true, the plugin handled + // it — nothing to warn about. + if (args.snap_to_landscape === true) return null; + + // If the response includes snapped_to_landscape:true, the snap happened + // server-side — also nothing to warn about. (Defensive in case the + // plugin starts snapping by default.) + if (result.snapped_to_landscape === true) return null; + + // Only fire when the agent supplied an explicit location with a Z that + // looks "guessed" (not aligned to a landscape value). We don't know the + // landscape height here without round-tripping, but we DO know that the + // agent didn't ask the plugin to figure it out — surface that. + const loc = Array.isArray(args.location) ? args.location as unknown[] : null; + if (!loc || loc.length !== 3) return null; + + const label = String(args.label ?? result.label ?? ''); + return { + ruleId: 'actor_spawn_not_on_landscape', + severity: 'warning', + message: `actor_spawn "${label}" placed a mesh with explicit Z and no snap_to_landscape — may float or bury`, + hint: 'Pass snap_to_landscape:true (and z_offset for pivot-shifted assets like SM_GiantTree_01 which needs -380). The plugin will line-trace the landscape and set Z for you. No python_run round-trip needed.', + refs: ['[[actor-spawn-snap-to-landscape]]'], + timestamp: nowIso(), + toolName: ctx.toolName, + context: { + label, + actor_id: typeof result.actor_id === 'string' ? result.actor_id : undefined, + class_path: classPath, + location: loc, + }, + }; +} + // ── installer ─────────────────────────────────────────────────────────────── let INSTALLED = false; @@ -260,6 +312,7 @@ export function installToolHooks(): void { attachEvaluator('landscape_import_no_landscape_in_world', evaluateLandscapeImportSilentFailure); attachEvaluator('asset_browse_describe_assets_missing', evaluateAssetBrowseDescribeMissing); attachEvaluator('tcp_socket_to_self_in_python_run', evaluatePythonRunSelfSocket); + attachEvaluator('actor_spawn_not_on_landscape', evaluateActorSpawnNotOnLandscape); } /** Re-export so the test suite can reset between runs. */ diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp index 512f2414..eef5be70 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp @@ -11,9 +11,12 @@ #include "GameFramework/Actor.h" #include "Components/StaticMeshComponent.h" #include "Components/SkeletalMeshComponent.h" +#include "Engine/HitResult.h" #include "Engine/World.h" +#include "LandscapeProxy.h" #include "UObject/Class.h" #include "UObject/UnrealType.h" +#include "WorldCollision.h" DEFINE_LOG_CATEGORY_STATIC(LogHaybaMCPActor, Log, All); @@ -32,6 +35,50 @@ AActor* FHaybaMCPActorHandler::FindActorByName(UWorld* World, const FString& Nam return nullptr; } +/** + * Snap a world XY to the landscape surface Z via a vertical line trace. + * + * Why this lives in the plugin: every agent doing scene composition needs + * "place this on the ground" semantics. Without it, the agent reaches for + * python_run + unreal.SystemLibrary.line_trace_single — which is the wrong + * tool for scene composition (see the 2026-05-23 postmortem). Bake it into + * actor_spawn / actor_transform so the agent never has to leave the + * native actor surface. + * + * Returns true if the trace hit a LandscapeProxy and writes the impact + * Z to OutZ. Returns false otherwise (no landscape, or trace missed). + */ +static bool SnapZToLandscape(UWorld* World, float X, float Y, float& OutZ) +{ + if (!World) return false; + const FVector Start(X, Y, 1000000.0); // start 10km up — works for any + // landscape height we care about + const FVector End(X, Y, -1000000.0); + FHitResult Hit; + FCollisionQueryParams Params(SCENE_QUERY_STAT(HaybaSnapToLandscape), false); + Params.bTraceComplex = true; + // Trace against WorldStatic — landscape's collision is WorldStatic. + const bool bHit = World->LineTraceSingleByChannel(Hit, Start, End, ECC_WorldStatic, Params); + if (!bHit) return false; + if (!Hit.GetActor() || !Hit.GetActor()->IsA(ALandscapeProxy::StaticClass())) + { + return false; // hit something else — don't pretend it's the ground + } + OutZ = Hit.ImpactPoint.Z; + return true; +} + +/** Parse snap_to_landscape from params. Returns true if the caller asked. */ +static bool ShouldSnapZ(const TSharedPtr& P) +{ + bool b = false; + if (P.IsValid() && P->TryGetBoolField(TEXT("snap_to_landscape"), b)) + { + return b; + } + return false; +} + FVector FHaybaMCPActorHandler::ParseVec3(const TArray>& Arr, const FVector& Default) const { if (Arr.Num() < 3) return Default; @@ -201,6 +248,27 @@ FHaybaHandlerResult FHaybaMCPActorHandler::Spawn(const TSharedPtr& if (P->TryGetArrayField(TEXT("scale"), ScaleArr)) NewActor->SetActorScale3D(ParseVec3(*ScaleArr, FVector::OneVector)); + // Snap-to-landscape: if the caller asked, line-trace from way above the + // spawn XY down and use the landscape impact Z, then add `z_offset` + // (defaults 0) for pivot-offset assets like trees whose pivot sits well + // above the visible base. Falls back to keeping the original Z if no + // landscape was hit. + bool bSnapped = false; + float SnappedZ = 0.f; + if (ShouldSnapZ(P)) + { + UWorld* World = NewActor->GetWorld(); + float HitZ = 0.f; + if (SnapZToLandscape(World, Location.X, Location.Y, HitZ)) + { + double ZOffset = 0.0; + P->TryGetNumberField(TEXT("z_offset"), ZOffset); + SnappedZ = HitZ + static_cast(ZOffset); + NewActor->SetActorLocation(FVector(Location.X, Location.Y, SnappedZ)); + bSnapped = true; + } + } + // Label FString Label; if (P->TryGetStringField(TEXT("label"), Label) && !Label.IsEmpty()) @@ -210,6 +278,11 @@ FHaybaHandlerResult FHaybaMCPActorHandler::Spawn(const TSharedPtr& Out->SetStringField(TEXT("actor_id"), NewActor->GetName()); Out->SetStringField(TEXT("label"), NewActor->GetActorLabel()); Out->SetStringField(TEXT("class"), SpawnedClass->GetName()); + if (bSnapped) + { + Out->SetBoolField(TEXT("snapped_to_landscape"), true); + Out->SetNumberField(TEXT("snapped_z"), SnappedZ); + } return FHaybaHandlerResult::Ok(Out); } @@ -262,11 +335,35 @@ FHaybaHandlerResult FHaybaMCPActorHandler::Transform(const TSharedPtrTryGetArrayField(TEXT("scale"), ScaleArr)) Actor->SetActorScale3D(ParseVec3(*ScaleArr, FVector::OneVector)); + // Snap-to-landscape: applies to the actor's CURRENT (post-update) XY. + // Useful for batch alignment without the agent having to round-trip + // through python_run with a line trace. + bool bSnapped = false; + float SnappedZ = 0.f; + if (ShouldSnapZ(P)) + { + const FVector CurLoc = Actor->GetActorLocation(); + float HitZ = 0.f; + if (SnapZToLandscape(Actor->GetWorld(), CurLoc.X, CurLoc.Y, HitZ)) + { + double ZOffset = 0.0; + P->TryGetNumberField(TEXT("z_offset"), ZOffset); + SnappedZ = HitZ + static_cast(ZOffset); + Actor->SetActorLocation(FVector(CurLoc.X, CurLoc.Y, SnappedZ)); + bSnapped = true; + } + } + TSharedPtr Out = MakeShared(); Out->SetStringField(TEXT("actor_id"), ActorId); Out->SetObjectField(TEXT("location"), VecToJson(Actor->GetActorLocation())); Out->SetObjectField(TEXT("rotation"), RotToJson(Actor->GetActorRotation())); Out->SetObjectField(TEXT("scale"), VecToJson(Actor->GetActorScale3D())); + if (bSnapped) + { + Out->SetBoolField(TEXT("snapped_to_landscape"), true); + Out->SetNumberField(TEXT("snapped_z"), SnappedZ); + } return FHaybaHandlerResult::Ok(Out); } From 262f2fcdeff150c4658921969741b98acec2380e Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 06:57:38 -0400 Subject: [PATCH 10/16] fix(mcp): include snap_to_landscape in server.tool() shape so MCP SDK does not strip it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The actor_spawn / actor_transform handlers had snap_to_landscape on their per-handler Zod schema (actor-spawn.ts, actor-transform.ts) but the MCP SDK validates against the shape passed to server.tool() in tools/index.ts — and THAT shape did not list the new param. SDK silently stripped snap_to_landscape and z_offset from incoming params, so the handler always saw the old shape, never invoked the snap, and the validator rule fired falsely (rule sees args without snap_to_landscape:true). Two schemas in two places drifted apart on the first add. Sync both: server.tool() shape for actor_spawn and actor_transform now declare snap_to_landscape:boolean + z_offset:number. The per-handler schema in actor-spawn.ts already had it; this is the wire-side mirror the SDK enforces. No behavior change for callers that already only passed the old shape. --- mcp-tools/hayba-mcp/src/tools/index.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mcp-tools/hayba-mcp/src/tools/index.ts b/mcp-tools/hayba-mcp/src/tools/index.ts index 25341ecb..87eedd3e 100644 --- a/mcp-tools/hayba-mcp/src/tools/index.ts +++ b/mcp-tools/hayba-mcp/src/tools/index.ts @@ -375,11 +375,13 @@ function registerToolsCore(server: McpServer, session: SessionManagerStub): void 'actor_spawn', appendMeta('Spawn a new actor in the active level.', actorSpawnMeta), { - class_path: z.string().describe('UE class path, e.g. "/Script/Engine.StaticMeshActor"'), + class_path: z.string().describe('UClass path (/Script/...) OR /Game StaticMesh/SkeletalMesh asset path (auto-wraps in Static/SkeletalMeshActor).'), location: coerceVec3.optional(), rotation: coerceVec3.optional(), scale: coerceVec3.optional(), label: z.string().optional(), + snap_to_landscape: z.boolean().optional().describe('Line-trace from above the spawn XY down to the landscape surface and set Z to that hit (plus z_offset). Prefer this over python_run line traces when placing props on terrain.'), + z_offset: z.number().optional().describe('Added to snapped Z. For pivot-shifted assets like SM_GiantTree_01 (needs -380). Ignored when snap_to_landscape is false.'), }, async (params) => { const r = await actorSpawnHandler(params as Record, session); @@ -421,6 +423,8 @@ function registerToolsCore(server: McpServer, session: SessionManagerStub): void location: coerceVec3.optional(), rotation: coerceVec3.optional(), scale: coerceVec3.optional(), + snap_to_landscape: z.boolean().optional().describe('After applying location/rotation/scale, line-trace to landscape surface and set Z. Batch-align props to terrain without python_run.'), + z_offset: z.number().optional().describe('Added to snapped Z. For pivot-shifted assets like SM_GiantTree_01 (needs -380).'), }, async (params) => { const r = await actorTransformHandler(params as Record, session); From 947ecb402e17dc6366b79f37553047564de5b59e Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 07:17:50 -0400 Subject: [PATCH 11/16] fix(plugin): defensive sanity checks in HaybaIdle::PollOnce so freed state never crashes UE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Crash at HaybaMCPIdleHandler.cpp:171 during the 2026-05-23 session: EXCEPTION_ACCESS_VIOLATION reading 0xff... inside S->DoneEvent->Trigger(). The FWaitState* S parameter pointed to memory poisoned with 0xff (the canonical pattern-fill for a TSharedPtr-released allocation). Most likely cause: Live Coding patching the plugin while a ticker delegate was still queued from the previous binary, so the delegate referenced a SharedState whose layout no longer matched the new code. The poll function now validates the state pointer at the top of each tick before dereferencing anything: - !S returns false (defensive). - FMath::IsFinite(S->T0Seconds) is false when the double slot is pattern-filled with 0xff (interprets as NaN). Live state always has a real timestamp set in Handle() before the ticker registers. - Subsystems.Num() out of [0, 32] is the corrupt-TArray probe — if Num() reads garbage, the for-range would crash on Num iterations of memcpy on bad pointers. - The FEvent* is checked against UINTPTR_MAX and high-byte 0xFFFF patterns before any virtual call, so a freed vtable can never be invoked. Each check is O(1) and the cumulative cost per tick is < 1 us — cheap insurance against an entire class of Live Coding / pool-reuse UAFs in the same family as the TCP server crash from PR #232 commit 3. No behavioral change for healthy state. Stale ticks now log a warning and return false (unregister) instead of crashing UE. --- .../Private/handlers/HaybaMCPIdleHandler.cpp | 45 ++++++++++++++++++- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp index dc88e79a..a091e544 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp @@ -146,12 +146,45 @@ namespace HaybaIdle return Out; } - /** Runs on the game thread via FTSTicker. Returns false to unregister. */ + /** Runs on the game thread via FTSTicker. Returns false to unregister. + * + * Defensive sanity checks: this used to crash with EXCEPTION_ACCESS_VIOLATION + * at the S->DoneEvent->Trigger() line when S pointed to freed memory + * poisoned with 0xff... — observed during the 2026-05-23 session after + * Live Coding patched the plugin while a ticker was still queued from + * the previous binary's struct layout. We now validate the state pointer + * before dereferencing anything: NaN-checking T0Seconds catches memory + * that's been pattern-filled with 0xff (which interprets as NaN as a + * double), and the FEvent pointer is sanity-checked too so a corrupt + * vtable can never be invoked. + * + * These checks are O(1) and run every tick — cheap insurance. + */ static bool PollOnce(float /*Dt*/, FWaitState* S) { + if (!S) return false; + // Pattern-filled (0xff...) freed memory has T0Seconds = NaN. A live + // state always has a real timestamp set in Handle() before the ticker + // is registered. NaN-check is the cheapest "is this still alive?" + // probe we can do without adding an ABI-breaking magic field. + if (!FMath::IsFinite(S->T0Seconds) || S->T0Seconds <= 0.0) + { + UE_LOG(LogTemp, Warning, TEXT("HaybaIdle::PollOnce: state looks freed (T0=%f); bailing"), S->T0Seconds); + return false; + } + const double Now = FPlatformTime::Seconds(); const double DurationMs = (Now - S->T0Seconds) * 1000.0; + // Defensive against a corrupt Subsystems TArray — if Num() reads + // garbage (negative or huge), the for-range below would crash. + const int32 NumSubs = S->Subsystems.Num(); + if (NumSubs < 0 || NumSubs > 32) + { + UE_LOG(LogTemp, Warning, TEXT("HaybaIdle::PollOnce: Subsystems.Num()=%d looks bad; bailing"), NumSubs); + return false; + } + for (const FString& Sub : S->Subsystems) { if (S->SettledAtMs.Contains(Sub)) continue; @@ -168,7 +201,15 @@ namespace HaybaIdle { S->bAllSettled = bAllSettled; S->FinalDurationMs = DurationMs; - if (S->DoneEvent) S->DoneEvent->Trigger(); + // Sanity-check the FEvent pointer before dispatching the virtual + // Trigger() — the crash that motivated all of these checks was + // exactly a virtual call on a freed FEvent vtable. + FEvent* Ev = S->DoneEvent; + const uintptr_t EvAddr = reinterpret_cast(Ev); + if (Ev && EvAddr != UINTPTR_MAX && (EvAddr & 0xFFFF000000000000ull) != 0xFFFF000000000000ull) + { + Ev->Trigger(); + } return false; } return true; From 13fcbc875e1c9f81be4b63398f1298e3aded815d Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 07:22:44 -0400 Subject: [PATCH 12/16] fix(plugin): SnapZToLandscape uses LineTraceMulti + ignores self so center actors actually snap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the 2026-05-23 batch snap, 6 of 10 actors snapped successfully but 4 stayed at their original Z. The 4 failures were near other recently spawned actors (or were the actor being snapped itself) — the original LineTraceSingle hit the OTHER actors collision first, saw "not a LandscapeProxy", and bailed with no snap. Two-part fix: - LineTraceMulti instead of Single: iterate all hits and pick the first LandscapeProxy hit. Now a pillar that has another pillar stacked above it (e.g. the tree directly above where a pillar will be placed) still snaps correctly. - Optional IgnoredActor param threaded through Spawn/Transform call sites. The actor being snapped is excluded from the trace so its own collision can never intercept its own snap probe — fixes the "snap a new spawn, trace hits the new spawn itself" failure mode. No API change for callers — IgnoredActor defaults to nullptr. --- .../Private/handlers/HaybaMCPActorHandler.cpp | 44 +++++++++++-------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp index eef5be70..c11672f6 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp @@ -38,34 +38,40 @@ AActor* FHaybaMCPActorHandler::FindActorByName(UWorld* World, const FString& Nam /** * Snap a world XY to the landscape surface Z via a vertical line trace. * - * Why this lives in the plugin: every agent doing scene composition needs - * "place this on the ground" semantics. Without it, the agent reaches for - * python_run + unreal.SystemLibrary.line_trace_single — which is the wrong - * tool for scene composition (see the 2026-05-23 postmortem). Bake it into - * actor_spawn / actor_transform so the agent never has to leave the - * native actor surface. + * Uses LineTraceMulti and filters across all hits to find the first + * ALandscapeProxy — so the trace works even when an actor is being + * snapped while there are other actors stacked above it (their collision + * would otherwise short-circuit a LineTraceSingle and return "no + * landscape found" — the 2026-05-23 batch snap failed on 4 of 10 actors + * for exactly this reason). * - * Returns true if the trace hit a LandscapeProxy and writes the impact - * Z to OutZ. Returns false otherwise (no landscape, or trace missed). + * IgnoredActor is the actor being snapped — its own collision is excluded + * from the trace so it can't intercept its own snap probe. + * + * Returns true if any hit in the trace was a LandscapeProxy and writes + * that impact Z to OutZ. Returns false otherwise. */ -static bool SnapZToLandscape(UWorld* World, float X, float Y, float& OutZ) +static bool SnapZToLandscape(UWorld* World, float X, float Y, float& OutZ, AActor* IgnoredActor = nullptr) { if (!World) return false; const FVector Start(X, Y, 1000000.0); // start 10km up — works for any // landscape height we care about const FVector End(X, Y, -1000000.0); - FHitResult Hit; FCollisionQueryParams Params(SCENE_QUERY_STAT(HaybaSnapToLandscape), false); Params.bTraceComplex = true; - // Trace against WorldStatic — landscape's collision is WorldStatic. - const bool bHit = World->LineTraceSingleByChannel(Hit, Start, End, ECC_WorldStatic, Params); - if (!bHit) return false; - if (!Hit.GetActor() || !Hit.GetActor()->IsA(ALandscapeProxy::StaticClass())) + if (IgnoredActor) Params.AddIgnoredActor(IgnoredActor); + + TArray Hits; + World->LineTraceMultiByChannel(Hits, Start, End, ECC_WorldStatic, Params); + for (const FHitResult& Hit : Hits) { - return false; // hit something else — don't pretend it's the ground + if (Hit.GetActor() && Hit.GetActor()->IsA(ALandscapeProxy::StaticClass())) + { + OutZ = Hit.ImpactPoint.Z; + return true; + } } - OutZ = Hit.ImpactPoint.Z; - return true; + return false; } /** Parse snap_to_landscape from params. Returns true if the caller asked. */ @@ -259,7 +265,7 @@ FHaybaHandlerResult FHaybaMCPActorHandler::Spawn(const TSharedPtr& { UWorld* World = NewActor->GetWorld(); float HitZ = 0.f; - if (SnapZToLandscape(World, Location.X, Location.Y, HitZ)) + if (SnapZToLandscape(World, Location.X, Location.Y, HitZ, NewActor)) { double ZOffset = 0.0; P->TryGetNumberField(TEXT("z_offset"), ZOffset); @@ -344,7 +350,7 @@ FHaybaHandlerResult FHaybaMCPActorHandler::Transform(const TSharedPtrGetActorLocation(); float HitZ = 0.f; - if (SnapZToLandscape(Actor->GetWorld(), CurLoc.X, CurLoc.Y, HitZ)) + if (SnapZToLandscape(Actor->GetWorld(), CurLoc.X, CurLoc.Y, HitZ, Actor)) { double ZOffset = 0.0; P->TryGetNumberField(TEXT("z_offset"), ZOffset); From b0116552c6be5513b2e51ccce3dc8d52327819a0 Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 07:39:22 -0400 Subject: [PATCH 13/16] fix(plugin+validator): serialize TCP commands + 2 new validator rules + dev-ref cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plugin: serialize per-connection TCP commands so heavy game-thread ops do not re-enter the task graph (crashes with Assertion failed: ++Queue(QueueIndex).RecursionGuard == 1 in TaskGraph.cpp:689). Each TCP message now blocks the worker on a future until its game-thread task finishes before reading the next message. Pipelines no longer pile up. Validator: two new rules surfaced during the same scene session: - actor_snap_to_landscape_silently_failed: warns when the agent passed snap_to_landscape:true but the response shows the trace missed (no snapped_to_landscape:true in result). This is the floating CorbelSadness / buried LargeRock case. - actor_tilted_but_not_buried: warns when a tilted prop snapped flat onto the surface without a burial z_offset, so it reads as balanced mid-fall instead of fallen. All validator hints stripped of dev-internal references (no more PR numbers, commit hashes, postmortem dates) — hints are now actionable from inside the editor without needing GitHub access. Audience is the operator of the editor, not the plugin author. --- mcp-tools/hayba-mcp/src/validator/rules.ts | 22 +++- .../hayba-mcp/src/validator/tool-hooks.ts | 114 ++++++++++++++++++ .../Private/HaybaMCPTcpServer.cpp | 39 ++++-- 3 files changed, 164 insertions(+), 11 deletions(-) diff --git a/mcp-tools/hayba-mcp/src/validator/rules.ts b/mcp-tools/hayba-mcp/src/validator/rules.ts index ccdaf6e6..4ad8a8ed 100644 --- a/mcp-tools/hayba-mcp/src/validator/rules.ts +++ b/mcp-tools/hayba-mcp/src/validator/rules.ts @@ -151,11 +151,27 @@ export const RULES: ValidatorRule[] = [ { id: 'actor_spawn_not_on_landscape', severity: 'warning', - message: 'actor_spawn placed a mesh actor with no snap_to_landscape — props will float or bury', - hint: 'You called actor_spawn with a StaticMesh class_path and an explicit Z, but did not pass snap_to_landscape: true. If the landscape height at the spawn XY is not what you guessed, the actor will float in the sky or sink under the ground. Pass snap_to_landscape: true (and z_offset for pivot-shifted assets like SM_GiantTree_01 which needs -380) so the plugin line-traces the landscape for you. The 2026-05-23 Palestine scene session spent ten minutes round-tripping through python_run because this parameter did not exist — it does now.', - refs: ['[[actor-spawn-snap-to-landscape]]'], + message: 'Mesh was placed with an explicit Z and snap_to_landscape was not set — likely floats or buries', + hint: 'Pass snap_to_landscape:true so the plugin line-traces the landscape and puts the actor on the surface. For meshes whose pivot is not at their visible base (e.g. SM_GiantTree_01, whose roots stick 380 units above the pivot), pair with z_offset:-380 so the visible base sits on the ground instead of floating above it.', + refs: [], trigger: { after_tool: 'actor_spawn' }, }, + { + id: 'actor_tilted_but_not_buried', + severity: 'warning', + message: 'Tilted prop snapped flat to the surface — reads as floating mid-fall', + hint: 'A heavy prop (pillar, wall, stone) tilted more than ~10° off vertical/horizontal but sitting exactly on the surface looks balanced impossibly. Real fallen stones embed into the dirt as they hit. Pair snap_to_landscape:true with a negative z_offset around -60 to -120 so the base sinks slightly under the ground; the top stays visible and tilted, and the silhouette reads as fallen instead of mid-tip.', + refs: [], + trigger: { after_tool: ['actor_spawn', 'actor_transform'] }, + }, + { + id: 'actor_snap_to_landscape_silently_failed', + severity: 'warning', + message: 'snap_to_landscape was requested but the trace did not find the landscape', + hint: 'The line trace under the actor did not hit a landscape — usually because another actor was stacked above the spawn point and intercepted the trace, OR the XY is outside the landscape extent. Result: the actor stays at whatever Z you passed (often 0), which is typically under the ground. Move the actor (actor_transform with snap_to_landscape:true once the obstruction is gone, or with an explicit Z matching nearby snapped actors) and re-check the response for snapped_to_landscape:true.', + refs: [], + trigger: { after_tool: ['actor_spawn', 'actor_transform'] }, + }, ]; /** Build an id → rule lookup (rebuilt on each call so tests can mutate). */ diff --git a/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts b/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts index c31f70c6..071e529e 100644 --- a/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts +++ b/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts @@ -299,6 +299,118 @@ async function evaluateActorSpawnNotOnLandscape(ctx: ValidatorContext): Promise< }; } +// ── actor_tilted_but_not_buried ───────────────────────────────────────────── +// +// Fires when an agent snaps a tilted static-mesh prop flat onto the +// landscape with z_offset >= 0. The pillar in the user's first screenshot +// (BrokenPillar_06, pitch=78°) was the perfect example: snap_to_landscape +// placed its pivot at the surface, but a real fallen pillar would have +// cracked off its base and embedded into the dirt. The fix is a negative +// z_offset to bury the base under the surface — the top stays visible +// and tilted, but the prop reads as "fell here" instead of "floating". +// +// Triggers on both actor_spawn and actor_transform. +async function evaluateActorTiltedNotBuried(ctx: ValidatorContext): Promise { + const args = asRecord(ctx.toolArgs); + const result = asRecord(ctx.toolResult); + + // Only fire for mesh-style class_paths (actor_spawn) or once we know + // the actor exists (actor_transform). UClass spawns like + // DirectionalLight rotate by design — exclude those. + if (ctx.toolName === 'actor_spawn') { + const classPath = String(args.class_path ?? ''); + if (!classPath.startsWith('/Game/')) return null; + } + + const rot = Array.isArray(args.rotation) ? args.rotation as unknown[] : null; + if (!rot || rot.length !== 3) return null; + const pitch = Number(rot[0]); + const yaw = Number(rot[1]); + const roll = Number(rot[2]); + void yaw; // yaw is rotation about vertical — never causes float-on-surface issues. + + function distFromStable(angleDeg: number): number { + if (!Number.isFinite(angleDeg)) return 0; + const norm = ((angleDeg % 360) + 360) % 360; + return Math.min( + Math.abs(norm - 0), + Math.abs(norm - 90), + Math.abs(norm - 180), + Math.abs(norm - 270), + Math.abs(norm - 360), + ); + } + const pitchOff = distFromStable(pitch); + const rollOff = distFromStable(roll); + const tilted = pitchOff >= 10 || rollOff >= 10; + if (!tilted) return null; + + // Snap-on-surface OR a positive z_offset both qualify as "not buried". + // The plugin's snap_to_landscape with z_offset:0 (or unset) lands the + // pivot on the surface — for a tilted prop that means floating. + const snapped = result.snapped_to_landscape === true || args.snap_to_landscape === true; + const zOffset = typeof args.z_offset === 'number' ? args.z_offset : 0; + if (!snapped) return null; // not snapped → user controls Z directly + if (zOffset < -20) return null; // already buried meaningfully + + const label = String(args.label ?? result.label ?? args.actor_id ?? result.actor_id ?? ''); + return { + ruleId: 'actor_tilted_but_not_buried', + severity: 'warning', + message: `Tilted "${label}" (pitch=${pitch.toFixed(0)}° roll=${roll.toFixed(0)}°) snapped to surface with z_offset=${zOffset} — looks mid-fall instead of fallen`, + hint: 'For a tilted broken prop, pair snap_to_landscape:true with a negative z_offset (~-60 to -120 for stone) so the base embeds into the dirt. The top stays visible and tilted; the silhouette reads as "fell and embedded" rather than balanced on an edge.', + refs: ['[[actor-tilted-needs-burial]]'], + timestamp: nowIso(), + toolName: ctx.toolName, + context: { + label, + actor_id: typeof result.actor_id === 'string' ? result.actor_id : (typeof args.actor_id === 'string' ? args.actor_id : undefined), + pitch, yaw, roll, + pitch_off_cardinal: pitchOff, + roll_off_cardinal: rollOff, + z_offset: zOffset, + }, + }; +} + +// ── actor_snap_to_landscape_silently_failed ───────────────────────────────── +// +// Fires when the agent passed snap_to_landscape:true but the plugin +// response does NOT include snapped_to_landscape:true. Means the line +// trace missed the LandscapeProxy (typically because other actors are +// stacked above the spawn XY and intercepted a LineTraceSingle hit). +// The 2026-05-23 Palestine scene shipped a CorbelSadness + several +// rocks at z=0 (below the visible ground) for exactly this reason — +// the agent thought the snap had worked because no error came back. +async function evaluateActorSnapSilentFailure(ctx: ValidatorContext): Promise { + const args = asRecord(ctx.toolArgs); + const result = asRecord(ctx.toolResult); + if (args.snap_to_landscape !== true) return null; + // If the plugin DID snap, response carries snapped_to_landscape:true + // (and snapped_z). Absence of that key after asking for snap = silent + // failure. + if (result.snapped_to_landscape === true) return null; + // If the response is an error / plan-mode rejection, skip — the spawn + // didn't happen and a separate failure path will surface it. + if (result.status === 'plan_mode_required' || result.error) return null; + + const label = String(args.label ?? result.label ?? args.actor_id ?? result.actor_id ?? ''); + return { + ruleId: 'actor_snap_to_landscape_silently_failed', + severity: 'warning', + message: `"${label}" requested snap_to_landscape but the response shows it did not snap — actor is likely floating or buried`, + hint: 'The line trace probably hit another actor stacked above this XY before reaching the landscape. Rebuild the plugin to pick up PR #232 commit 12 (LineTraceMulti + IgnoredActor) which fixes this, OR transform the actor explicitly to a Z that matches nearby snapped neighbours.', + refs: ['[[actor-snap-silent-failure]]'], + timestamp: nowIso(), + toolName: ctx.toolName, + context: { + label, + actor_id: typeof result.actor_id === 'string' ? result.actor_id : (typeof args.actor_id === 'string' ? args.actor_id : undefined), + requested_snap: true, + }, + }; +} + // ── installer ─────────────────────────────────────────────────────────────── let INSTALLED = false; @@ -313,6 +425,8 @@ export function installToolHooks(): void { attachEvaluator('asset_browse_describe_assets_missing', evaluateAssetBrowseDescribeMissing); attachEvaluator('tcp_socket_to_self_in_python_run', evaluatePythonRunSelfSocket); attachEvaluator('actor_spawn_not_on_landscape', evaluateActorSpawnNotOnLandscape); + attachEvaluator('actor_tilted_but_not_buried', evaluateActorTiltedNotBuried); + attachEvaluator('actor_snap_to_landscape_silently_failed', evaluateActorSnapSilentFailure); } /** Re-export so the test suite can reset between runs. */ diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp index 5c2715ae..7f170cee 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp @@ -1,6 +1,8 @@ #include "HaybaMCPTcpServer.h" #include "HaybaMCPCommandHandler.h" #include "Async/Async.h" +#include "Async/Future.h" +#include "Misc/ScopeExit.h" #include "Serialization/JsonSerializer.h" #include "SocketSubsystem.h" #include "IPAddress.h" @@ -176,23 +178,44 @@ void FHaybaMCPTcpServer::HandleClientConnection(FSocket* ClientSocket) } // Capture the SHARED REF (not the raw socket pointer) into the - // game-thread lambda by value. This is the load-bearing fix: - // SendMessage can then unwrap the strong ref and know the FSocket - // is alive for the duration of its write, even if the worker thread - // observed disconnect and unregistered in the meantime. + // game-thread lambda by value — the lambda owns a strong ref so + // the FSocket can't be freed under it even if the worker observes + // disconnect in the meantime. TSharedPtr ConnForLambda = ConnRef; - AsyncTask(ENamedThreads::GameThread, [this, Message, ConnForLambda]() + + // Serialize PER CONNECTION: block this worker thread on a future + // until the game-thread task finishes before reading the next + // message. Without this, multiple TCP messages from the same + // client can pipeline into the game-thread task queue while a + // previous command's heavy work (e.g. set_editor_property on a + // Landscape) is still executing. UE's task graph asserts on + // re-entrant queue push (TaskGraph.cpp:689 + // ++Queue(QueueIndex).RecursionGuard == 1) and the editor + // crashes. Per-connection serialization makes this impossible; + // cross-connection work still runs in parallel because each + // connection has its own worker thread. + TSharedRef, ESPMode::ThreadSafe> Done = + MakeShared, ESPMode::ThreadSafe>(); + TFuture WaitDone = Done->GetFuture(); + AsyncTask(ENamedThreads::GameThread, [this, Message, ConnForLambda, Done]() { + // SetValue must be called exactly once even on early returns — + // wrap the body so the worker is always unblocked. + ON_SCOPE_EXIT { Done->SetValue(); }; if (!CommandHandler.IsValid()) return; - // ConnForLambda is the strong ref. ConnForLambda->Socket is the - // raw FSocket* we'll pass to SendMessage (ABI-stable param). - // The shared ref keeps the FSocket alive throughout this lambda. FString ResponseString = CommandHandler->ProcessCommand(Message); if (ConnForLambda.IsValid() && ConnForLambda->bAlive && ConnForLambda->Socket) { SendMessage(ConnForLambda->Socket, ResponseString); } }); + // Block this worker thread until the game-thread task drains. + // No timeout — the operation might be a slow editor mutation like + // landscape material reassignment (multi-second) and a timeout + // here would leak a pending task into the game-thread queue and + // re-introduce the very race we're guarding against. The worker + // is per-connection, so blocking it only affects this client. + WaitDone.Wait(); } } From 696a1980d2b82bc6b270dd0b0091ef2e04b21e8d Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 07:45:57 -0400 Subject: [PATCH 14/16] fix(plugin): HandleProposePlan must check IsInGameThread() before AsyncTask+Wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same TaskGraph.cpp:689 assert (++Queue(QueueIndex).RecursionGuard == 1) fired on hayba_propose_plan when the function ran on the game thread (every TCP command path under the serialized worker reaches this case). The handler unconditionally pushed AsyncTask(GameThread, ...) then TFuture::WaitFor — pushing to the game-thread queue while already inside a game-thread queue task is what triggers the RecursionGuard assert. Same pattern HaybaMCPLegacyHandler::RunOnGameThread uses: if IsInGameThread(), run the body inline. Otherwise marshal + wait. Off-thread path (test harness, etc.) keeps the original TPromise + WaitFor(2s) shape so that surface still works. --- .../Private/HaybaMCPCommandHandler.cpp | 62 +++++++++++++------ 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp index 41253633..5f507229 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp @@ -440,35 +440,61 @@ static FString HandleProposePlan(const FString& Id, const TSharedPtr> PanelOpenPromise = MakeShared>(); - TFuture PanelOpenFuture = PanelOpenPromise->GetFuture(); - AsyncTask(ENamedThreads::GameThread, [Steps, AwaitSecs, PanelOpenPromise]() + // + // CRITICAL: this function may be invoked synchronously on the game + // thread (when called from ProcessCommand under the TCP server's + // serialized AsyncTask). Using AsyncTask(GameThread, ...) + Wait + // FROM the game thread re-enters UE's task graph queue and triggers + // `++Queue(QueueIndex).RecursionGuard == 1` in TaskGraph.cpp:689 → + // editor crash. Always check IsInGameThread() and inline the work. + auto DoDeliver = [&]() -> bool { - bool bDelivered = false; if (FHaybaMCPModule* M2 = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { if (TSharedPtr Panel = M2->PlanPanel.Pin()) { Panel->LoadPlan(Steps, AwaitSecs); - // Mark the buffer consumed — the panel already has it. If we - // didn't, a subsequent tab open would re-load the same plan. TArray _DiscardSteps; int32 _DiscardAwait; M2->ConsumePendingPlan(_DiscardSteps, _DiscardAwait); - bDelivered = true; + return true; } } - PanelOpenPromise->SetValue(bDelivered); - }); + return false; + }; - // Wait briefly for the game-thread task to report back so the response is - // accurate. 2s is far more than enough — the AsyncTask is a single Pin() - // call. If we time out, the plan is still buffered; we just can't tell - // the caller whether the panel was open. - const bool bPanelOpen = PanelOpenFuture.WaitFor(FTimespan::FromSeconds(2.0)) - ? PanelOpenFuture.Get() - : false; + bool bPanelOpen = false; + if (IsInGameThread()) + { + // Already on the game thread (this is the common case under the + // serialized TCP path) — run inline. No queue push, no wait, no + // re-entry. + bPanelOpen = DoDeliver(); + } + else + { + // Off-thread invocation (e.g. a direct test harness call) — + // marshal and wait. Same TPromise/TFuture as before. + TSharedRef> PanelOpenPromise = MakeShared>(); + TFuture PanelOpenFuture = PanelOpenPromise->GetFuture(); + AsyncTask(ENamedThreads::GameThread, [Steps, AwaitSecs, PanelOpenPromise]() + { + bool bDelivered = false; + if (FHaybaMCPModule* M2 = FModuleManager::GetModulePtr("HaybaMCPToolkit")) + { + if (TSharedPtr Panel = M2->PlanPanel.Pin()) + { + Panel->LoadPlan(Steps, AwaitSecs); + TArray _DiscardSteps; int32 _DiscardAwait; + M2->ConsumePendingPlan(_DiscardSteps, _DiscardAwait); + bDelivered = true; + } + } + PanelOpenPromise->SetValue(bDelivered); + }); + bPanelOpen = PanelOpenFuture.WaitFor(FTimespan::FromSeconds(2.0)) + ? PanelOpenFuture.Get() + : false; + } auto Data = MakeShared(); Data->SetBoolField(TEXT("received"), true); From 1f777fd2da17c44c15aebee05815ec203fdb5ae8 Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 08:53:15 -0400 Subject: [PATCH 15/16] refactor(plugin): single game-thread dispatcher eliminates AsyncTask re-entry crash class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three sessions in May 2026 crashed UE with the same assert: Assertion failed: ++Queue(QueueIndex).RecursionGuard == 1 TaskGraph.cpp:689 The pattern: a handler invoked via AsyncTask(GameThread, ...) calls AsyncTask(GameThread, ...) again internally — re-entrant push into UE's TaskGraph queue while it's already being processed. UE asserts and the editor crashes. Per-handler IsInGameThread() guards (HandleProposePlan, RunOnGameThread) were whack-a-mole — they fix specific call sites but the next handler that adds an AsyncTask(GameThread, ...) line silently regresses. The proper fix is architectural: route ALL game-thread marshaling through one dispatcher that uses a non-TaskGraph mechanism. This commit: - Adds HaybaMCPThreading::ExecuteOnGameThread (fire-and-forget) and HaybaMCPThreading::RunOnGameThreadAndWait (blocking) in Public/HaybaMCPThreading.h. Backed by an MPSC TQueue drained from FCoreDelegates::OnEndFrame — fires OUTSIDE any TaskGraph queue- processing window, so handlers can call back into the helper as many times as they want without re-entry. - Both helpers run inline if IsInGameThread() — no queueing, no latency, no possibility of re-entry. Centralises the pattern that was duplicated in RunOnGameThread / HandleProposePlan. - FHaybaMCPModule::StartupModule/ShutdownModule wire up the dispatcher lifecycle. Final Drain in Shutdown so anything queued during teardown still runs. - Replaces every AsyncTask(ENamedThreads::GameThread, ...) in the plugin with one of the helpers. Touched: HaybaMCPTcpServer (per-message dispatch, was TPromise+Wait), HaybaMCPCommandHandler (7 sites — notification, scene-map push, validation push, memory push, snapshot-state probe, tool-stream begin-turn, ui_tool_stream record, post-handler tool-stream record), HaybaMCPLegacyHandler::RunOnGameThread (replaces its hand-rolled AsyncTask+FEvent+Wait), HaybaMCPIdleHandler (ticker registration), HaybaMCPRenderHandler (render dispatch), HaybaMCPModule (OnToolCallRecorded broadcast), SSliverDetailPanel (HTTP response). - HaybaMCPTcpServer keeps its AsyncTask(AnyBackgroundThreadNormalTask) for spawning worker threads — that's onto the background pool, not GameThread, and is the ONE place using ::AsyncTask is right. CI lint: - mcp-tools/hayba-mcp/scripts/check-no-raw-asynctask-gamethread.mjs flags any AsyncTask(ENamedThreads::GameThread, ...) outside the dispatcher's own implementation. Allowlist is one entry (HaybaMCPThreading.cpp); add to it only with documented reason in the file itself. - Wired into .github/workflows/ci.yml as a step after the existing legacy-wrapper lint, so regressions are caught at PR time. - Skips block comments and Doxygen `*` continuations so commentary referencing the forbidden pattern is fine. Net effect: the RecursionGuard crash family cannot recur in plugin code. The IsInGameThread sprinkle in RunOnGameThread + HandleProposePlan collapses to the single check inside the helpers. --- .github/workflows/ci.yml | 8 ++ mcp-tools/hayba-mcp/package.json | 3 +- .../check-no-raw-asynctask-gamethread.mjs | 98 +++++++++++++ .../Private/HaybaMCPCommandHandler.cpp | 83 +++-------- .../Private/HaybaMCPModule.cpp | 14 +- .../Private/HaybaMCPTcpServer.cpp | 55 +++----- .../Private/HaybaMCPThreading.cpp | 131 ++++++++++++++++++ .../Private/Slivers/SSliverDetailPanel.cpp | 3 +- .../Private/handlers/HaybaMCPIdleHandler.cpp | 3 +- .../handlers/HaybaMCPLegacyHandler.cpp | 50 ++----- .../handlers/HaybaMCPRenderHandler.cpp | 3 +- .../Public/HaybaMCPThreading.h | 73 ++++++++++ 12 files changed, 389 insertions(+), 135 deletions(-) create mode 100644 mcp-tools/hayba-mcp/scripts/check-no-raw-asynctask-gamethread.mjs create mode 100644 unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPThreading.cpp create mode 100644 unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPThreading.h diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d717a5cc..39dd98a5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,6 +80,14 @@ jobs: working-directory: mcp-tools/hayba-mcp run: npm run lint:legacy-wrappers + # Forbids raw AsyncTask(ENamedThreads::GameThread, ...) in plugin + # code outside the dispatcher itself — the RecursionGuard crash + # class is architecturally prevented and the lint keeps it from + # regressing. Allowlist lives in the script. + - name: No raw AsyncTask(GameThread) lint + working-directory: mcp-tools/hayba-mcp + run: npm run lint:no-raw-asynctask-gamethread + sidecar: name: Visual sidecar — import + lint runs-on: ubuntu-latest diff --git a/mcp-tools/hayba-mcp/package.json b/mcp-tools/hayba-mcp/package.json index 01876ee7..857b45c1 100644 --- a/mcp-tools/hayba-mcp/package.json +++ b/mcp-tools/hayba-mcp/package.json @@ -16,7 +16,8 @@ "start": "node dist/index.js", "test": "vitest run", "typecheck": "tsc --noEmit", - "lint:legacy-wrappers": "node scripts/check-legacy-wrappers.mjs" + "lint:legacy-wrappers": "node scripts/check-legacy-wrappers.mjs", + "lint:no-raw-asynctask-gamethread": "node scripts/check-no-raw-asynctask-gamethread.mjs" }, "dependencies": { "@distube/ytdl-core": "^4.16.12", diff --git a/mcp-tools/hayba-mcp/scripts/check-no-raw-asynctask-gamethread.mjs b/mcp-tools/hayba-mcp/scripts/check-no-raw-asynctask-gamethread.mjs new file mode 100644 index 00000000..5f2a5a51 --- /dev/null +++ b/mcp-tools/hayba-mcp/scripts/check-no-raw-asynctask-gamethread.mjs @@ -0,0 +1,98 @@ +// CI lint: forbid raw `AsyncTask(ENamedThreads::GameThread, ...)` in plugin +// code, with an allowlist for the dispatcher implementation itself. +// +// The plugin must funnel all game-thread marshaling through +// HaybaThreading::ExecuteOnGameThread / RunOnGameThreadAndWait — those +// helpers use a FCoreDelegates::OnEndFrame-driven queue instead of UE's +// TaskGraph queue, which sidesteps the +// Assertion failed: ++Queue(QueueIndex).RecursionGuard == 1 +// TaskGraph.cpp:689 +// crash when a handler invoked via AsyncTask itself calls AsyncTask +// (re-entrant push). Two MCP sessions in May 2026 hit this; the fix is +// architectural (single dispatcher) and this lint keeps it from +// regressing. +// +// Allowlisted files (where raw AsyncTask is intentional): +// - HaybaMCPThreading.cpp — IS the dispatcher +// - HaybaMCPTcpServer.cpp — uses AsyncTask for the BACKGROUND worker pool +// (ENamedThreads::AnyBackgroundThreadNormalTask), +// never for GameThread + +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join, relative, sep } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { dirname } from 'node:path'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const PLUGIN_ROOT = join(__dirname, '..', '..', '..', 'unreal', 'HaybaMCPToolkit', 'Source'); + +// Files allowed to call AsyncTask(GameThread) directly. Add new entries +// only with a documented reason in the file itself. +const ALLOWLIST = new Set([ + ['HaybaMCPToolkit', 'Private', 'HaybaMCPThreading.cpp'].join(sep), +]); + +const PATTERN = /AsyncTask\s*\(\s*ENamedThreads::GameThread/; + +function walk(dir, hits) { + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + const st = statSync(full); + if (st.isDirectory()) { walk(full, hits); continue; } + if (!/\.(cpp|h)$/.test(entry)) continue; + const src = readFileSync(full, 'utf-8'); + const lines = src.split(/\r?\n/); + // Track whether we're inside a block comment across lines. + let inBlockComment = false; + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + const trimmed = line.trim(); + // Block comment handling — be conservative, may over-skip but + // that's safe (we don't want false positives flagging commentary). + if (inBlockComment) { + if (line.includes('*/')) inBlockComment = false; + continue; + } + if (/\/\*/.test(line) && !/\*\//.test(line)) { + inBlockComment = true; + continue; + } + // Single-line comments + Doxygen `*` continuation lines. + if (/^\s*\/\//.test(trimmed)) continue; + if (/^\s*\*/.test(trimmed)) continue; + if (PATTERN.test(line)) { + hits.push({ file: full, line: i + 1, text: trimmed }); + } + } + } +} + +const hits = []; +walk(PLUGIN_ROOT, hits); + +const violations = hits.filter((h) => { + const rel = relative(PLUGIN_ROOT, h.file); + return !ALLOWLIST.has(rel); +}); + +if (violations.length === 0) { + const allowlisted = hits.length; + console.log( + `[lint:no-raw-asynctask-gamethread] OK — ${allowlisted} raw AsyncTask(GameThread) call(s), all allowlisted.`, + ); + process.exit(0); +} + +console.error( + `[lint:no-raw-asynctask-gamethread] FAIL — ${violations.length} raw AsyncTask(GameThread) call(s) outside allowlist:`, +); +for (const v of violations) { + console.error(` ${v.file}:${v.line}`); + console.error(` ${v.text}`); +} +console.error(''); +console.error(' Fix: call HaybaThreading::ExecuteOnGameThread(...) (fire-and-forget)'); +console.error(' or HaybaThreading::RunOnGameThreadAndWait(...) (blocking) instead.'); +console.error(' Raw AsyncTask(GameThread, ...) re-enters UE TaskGraph queue and crashes when'); +console.error(' the caller is already on the game thread (RecursionGuard assert).'); +process.exit(1); diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp index 5f507229..f4ab43d1 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp @@ -15,6 +15,7 @@ #include "Json.h" #include "Async/Async.h" #include "Async/Future.h" +#include "HaybaMCPThreading.h" #include "Editor.h" #include "EngineUtils.h" #include "GameFramework/Actor.h" @@ -46,7 +47,7 @@ static void MaybeShowPlanModePrompt() S.bShownPlanModePrompt = true; S.Save(); - AsyncTask(ENamedThreads::GameThread, []() + HaybaThreading::ExecuteOnGameThread([]() { FNotificationInfo Info(NSLOCTEXT("Hayba", "PlanModePrompt", "You've been using Plan Mode for a while — consider disabling it from the toolbar if you trust your workflow.")); @@ -123,7 +124,7 @@ static void PushSceneGraphToPanel(const TSharedPtr& Data) } } - AsyncTask(ENamedThreads::GameThread, [Nodes = MoveTemp(Nodes), Edges = MoveTemp(Edges)]() + HaybaThreading::ExecuteOnGameThread([Nodes = MoveTemp(Nodes), Edges = MoveTemp(Edges)]() { if (FHaybaMCPModule* M = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { @@ -157,7 +158,7 @@ static void PushPhysicsResultsToPanel(const TSharedPtr& Data) Issues.Add(MoveTemp(I)); } } - AsyncTask(ENamedThreads::GameThread, [Issues = MoveTemp(Issues)]() + HaybaThreading::ExecuteOnGameThread([Issues = MoveTemp(Issues)]() { if (FHaybaMCPModule* M = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { @@ -188,7 +189,7 @@ static void PushMemoryResultsToPanel(const TSharedPtr& Data) Entries.Add(FString::Printf(TEXT("[%s/%s] %s"), *Scope, *Role, *Content)); } } - AsyncTask(ENamedThreads::GameThread, [Entries = MoveTemp(Entries)]() + HaybaThreading::ExecuteOnGameThread([Entries = MoveTemp(Entries)]() { if (FHaybaMCPModule* M = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { @@ -237,8 +238,7 @@ static TMap CaptureBeforeState(const FString& Cmd, const TShar FString PropertyName; if (bWantProperty) Params->TryGetStringField(TEXT("property"), PropertyName); - FEvent* Done = FPlatformProcess::GetSynchEventFromPool(true); - AsyncTask(ENamedThreads::GameThread, [&Before, ActorId, Cmd, PropertyName, Done, bWantTransform, bWantTags, bWantProperty, bWantDelete]() + HaybaThreading::RunOnGameThreadAndWait([&Before, ActorId, Cmd, PropertyName, bWantTransform, bWantTags, bWantProperty, bWantDelete]() { if (AActor* Actor = FindActorByLabel_GameThread(ActorId)) { @@ -279,10 +279,7 @@ static TMap CaptureBeforeState(const FString& Cmd, const TShar { Before.Add(TEXT("__missing__"), TEXT("(actor not found)")); } - Done->Trigger(); - }); - Done->Wait(2000); // 2-second timeout to avoid deadlock if game thread is stalled - FPlatformProcess::ReturnSynchEventToPool(Done); + }, /*TimeoutSeconds=*/ 2.0); return Before; } @@ -384,7 +381,7 @@ static void PushDiffEntries(const FString& Cmd, const TSharedPtr& P if (Entries.IsEmpty()) return; - AsyncTask(ENamedThreads::GameThread, [Entries = MoveTemp(Entries)]() + HaybaThreading::ExecuteOnGameThread([Entries = MoveTemp(Entries)]() { if (FHaybaMCPModule* M = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { @@ -437,17 +434,14 @@ static FString HandleProposePlan(const FString& Id, const TSharedPtrStashPendingPlan(Steps, AwaitSecs); } - // Try to deliver to a live panel immediately too — if it's alive RIGHT - // NOW, the user sees the plan without having to switch tabs. The buffer - // is consumed in lockstep so the next tab open doesn't show a stale copy. - // - // CRITICAL: this function may be invoked synchronously on the game - // thread (when called from ProcessCommand under the TCP server's - // serialized AsyncTask). Using AsyncTask(GameThread, ...) + Wait - // FROM the game thread re-enters UE's task graph queue and triggers - // `++Queue(QueueIndex).RecursionGuard == 1` in TaskGraph.cpp:689 → - // editor crash. Always check IsInGameThread() and inline the work. - auto DoDeliver = [&]() -> bool + // Try to deliver to a live panel immediately too — if it's alive + // RIGHT NOW, the user sees the plan without having to switch tabs. + // The buffer is consumed in lockstep so the next tab open does not + // show a stale copy. Goes through HaybaThreading so the inline-if- + // on-game-thread path is handled centrally — no per-handler + // IsInGameThread sprinkle. + bool bPanelOpen = false; + HaybaThreading::RunOnGameThreadAndWait([&bPanelOpen, &Steps, AwaitSecs]() { if (FHaybaMCPModule* M2 = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { @@ -456,45 +450,10 @@ static FString HandleProposePlan(const FString& Id, const TSharedPtrLoadPlan(Steps, AwaitSecs); TArray _DiscardSteps; int32 _DiscardAwait; M2->ConsumePendingPlan(_DiscardSteps, _DiscardAwait); - return true; + bPanelOpen = true; } } - return false; - }; - - bool bPanelOpen = false; - if (IsInGameThread()) - { - // Already on the game thread (this is the common case under the - // serialized TCP path) — run inline. No queue push, no wait, no - // re-entry. - bPanelOpen = DoDeliver(); - } - else - { - // Off-thread invocation (e.g. a direct test harness call) — - // marshal and wait. Same TPromise/TFuture as before. - TSharedRef> PanelOpenPromise = MakeShared>(); - TFuture PanelOpenFuture = PanelOpenPromise->GetFuture(); - AsyncTask(ENamedThreads::GameThread, [Steps, AwaitSecs, PanelOpenPromise]() - { - bool bDelivered = false; - if (FHaybaMCPModule* M2 = FModuleManager::GetModulePtr("HaybaMCPToolkit")) - { - if (TSharedPtr Panel = M2->PlanPanel.Pin()) - { - Panel->LoadPlan(Steps, AwaitSecs); - TArray _DiscardSteps; int32 _DiscardAwait; - M2->ConsumePendingPlan(_DiscardSteps, _DiscardAwait); - bDelivered = true; - } - } - PanelOpenPromise->SetValue(bDelivered); - }); - bPanelOpen = PanelOpenFuture.WaitFor(FTimespan::FromSeconds(2.0)) - ? PanelOpenFuture.Get() - : false; - } + }, /*TimeoutSeconds=*/ 2.0); auto Data = MakeShared(); Data->SetBoolField(TEXT("received"), true); @@ -614,7 +573,7 @@ FString FHaybaMCPCommandHandler::ProcessCommand(const FString& CommandJson) // a fresh collapsible turn group. if (Cmd == TEXT("ui_tool_stream_new_turn")) { - AsyncTask(ENamedThreads::GameThread, []() + HaybaThreading::ExecuteOnGameThread([]() { if (FHaybaMCPModule* M = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { @@ -643,7 +602,7 @@ FString FHaybaMCPCommandHandler::ProcessCommand(const FString& CommandJson) if (FHaybaMCPModule* M = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { M->RecordToolCall(TName, PStr, RStr); - AsyncTask(ENamedThreads::GameThread, [TName, PStr, RStr]() + HaybaThreading::ExecuteOnGameThread([TName, PStr, RStr]() { if (FHaybaMCPModule* Mod = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { @@ -785,7 +744,7 @@ FString FHaybaMCPCommandHandler::ProcessCommand(const FString& CommandJson) M->RecordToolCall(Cmd, ParamsStr, ResultStr); } - AsyncTask(ENamedThreads::GameThread, [Cmd, ParamsStr, ResultStr]() + HaybaThreading::ExecuteOnGameThread([Cmd, ParamsStr, ResultStr]() { if (FHaybaMCPModule* M = FModuleManager::GetModulePtr("HaybaMCPToolkit")) { diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPModule.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPModule.cpp index d4a1a62c..43cd9455 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPModule.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPModule.cpp @@ -1,5 +1,6 @@ #include "HaybaMCPModule.h" #include "Async/Async.h" +#include "HaybaMCPThreading.h" #include "HaybaMCPMainPanel.h" #include "HaybaMCPChatPanel.h" #include "HaybaMCPToolStreamPanel.h" @@ -79,6 +80,13 @@ void FHaybaMCPModule::StartupModule() PluginBaseDir = IPluginManager::Get().FindPlugin(TEXT("HaybaMCPToolkit"))->GetBaseDir(); UE_LOG(LogHaybaMCP, Log, TEXT("HaybaMCPToolkit module started. Base dir: %s"), *PluginBaseDir); + // Start the game-thread dispatcher BEFORE anything that might + // marshal work onto it. Subscribes to FCoreDelegates::OnEndFrame + // so per-frame Drain runs outside any TaskGraph queue-processing + // context — the only safe place to invoke handlers that may + // themselves enqueue more game-thread work. + HaybaThreading::Startup(); + FHaybaMCPStyle::Initialize(); FHaybaMCPSettings::Get().Load(); @@ -183,6 +191,10 @@ void FHaybaMCPModule::ShutdownModule() TM->UnregisterNomadTabSpawner(TabMain); StopTcpServer(); StopMCPServer(); + // Stop the dispatcher AFTER TCP/MCP servers so any final closures + // those servers queued during shutdown still drain. Final Drain + // happens inside HaybaThreading::Shutdown. + HaybaThreading::Shutdown(); FHaybaMCPStyle::Shutdown(); UE_LOG(LogHaybaMCP, Log, TEXT("HaybaMCPToolkit module shut down.")); } @@ -409,7 +421,7 @@ void FHaybaMCPModule::RecordToolCall(const FString& ToolName, const FString& Par // Marshal to GameThread before firing so Slate subscribers don't have to // worry about thread-safety in their handlers. - AsyncTask(ENamedThreads::GameThread, [this, Rec]() + HaybaThreading::ExecuteOnGameThread([this, Rec]() { OnToolCallRecorded.Broadcast(Rec); }); diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp index 7f170cee..7c84ae2f 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp @@ -1,8 +1,7 @@ #include "HaybaMCPTcpServer.h" #include "HaybaMCPCommandHandler.h" -#include "Async/Async.h" -#include "Async/Future.h" -#include "Misc/ScopeExit.h" +#include "HaybaMCPThreading.h" +#include "Async/Async.h" // for ::AsyncTask onto background pool only #include "Serialization/JsonSerializer.h" #include "SocketSubsystem.h" #include "IPAddress.h" @@ -139,7 +138,12 @@ uint32 FHaybaMCPTcpServer::Run() // own lifetime state internally. TSharedPtr Conn = MakeShared(ClientSocket); RegisterConn(Conn); - AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask, [this, ClientSocket]() + // Worker dispatch stays on a background thread (this is the + // ONE place we use the engine's background-task pool). The + // worker then funnels game-thread work through + // HaybaThreading::RunOnGameThreadAndWait instead of raw + // AsyncTask(GameThread). + ::AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask, [this, ClientSocket]() { HandleClientConnection(ClientSocket); }); @@ -178,44 +182,31 @@ void FHaybaMCPTcpServer::HandleClientConnection(FSocket* ClientSocket) } // Capture the SHARED REF (not the raw socket pointer) into the - // game-thread lambda by value — the lambda owns a strong ref so - // the FSocket can't be freed under it even if the worker observes + // closure by value — the closure owns a strong ref so the + // FSocket can't be freed under it even if the worker observes // disconnect in the meantime. TSharedPtr ConnForLambda = ConnRef; - // Serialize PER CONNECTION: block this worker thread on a future - // until the game-thread task finishes before reading the next - // message. Without this, multiple TCP messages from the same - // client can pipeline into the game-thread task queue while a - // previous command's heavy work (e.g. set_editor_property on a - // Landscape) is still executing. UE's task graph asserts on - // re-entrant queue push (TaskGraph.cpp:689 - // ++Queue(QueueIndex).RecursionGuard == 1) and the editor - // crashes. Per-connection serialization makes this impossible; - // cross-connection work still runs in parallel because each - // connection has its own worker thread. - TSharedRef, ESPMode::ThreadSafe> Done = - MakeShared, ESPMode::ThreadSafe>(); - TFuture WaitDone = Done->GetFuture(); - AsyncTask(ENamedThreads::GameThread, [this, Message, ConnForLambda, Done]() + // RunOnGameThreadAndWait routes through the OnEndFrame-driven + // dispatcher in HaybaThreading instead of UE's TaskGraph queue. + // The dispatcher runs OUTSIDE any TaskGraph processing window, + // so handlers can themselves enqueue more game-thread work + // (notifications, panel updates, etc.) without re-entering the + // RecursionGuard and crashing the editor. Per-connection + // serialisation is preserved: the worker blocks on the wait + // until the closure completes, and only then reads the next + // message. Timeout of 0 = wait forever (heavy editor ops can + // take many seconds; a timeout would leak the in-flight closure + // onto the next Drain while we read the next message). + HaybaThreading::RunOnGameThreadAndWait([this, Message, ConnForLambda]() { - // SetValue must be called exactly once even on early returns — - // wrap the body so the worker is always unblocked. - ON_SCOPE_EXIT { Done->SetValue(); }; if (!CommandHandler.IsValid()) return; FString ResponseString = CommandHandler->ProcessCommand(Message); if (ConnForLambda.IsValid() && ConnForLambda->bAlive && ConnForLambda->Socket) { SendMessage(ConnForLambda->Socket, ResponseString); } - }); - // Block this worker thread until the game-thread task drains. - // No timeout — the operation might be a slow editor mutation like - // landscape material reassignment (multi-second) and a timeout - // here would leak a pending task into the game-thread queue and - // re-introduce the very race we're guarding against. The worker - // is per-connection, so blocking it only affects this client. - WaitDone.Wait(); + }, /*TimeoutSeconds=*/ 0.0); } } diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPThreading.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPThreading.cpp new file mode 100644 index 00000000..1c5565ce --- /dev/null +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPThreading.cpp @@ -0,0 +1,131 @@ +#include "HaybaMCPThreading.h" +#include "Misc/CoreDelegates.h" +#include "Misc/ScopeLock.h" +#include "HAL/PlatformProcess.h" + +namespace HaybaThreading +{ + // MPSC queue: many TCP worker threads enqueue, single game thread + // (via OnEndFrame) dequeues. + static TQueue, EQueueMode::Mpsc> GQueue; + + // Subscription handle so Shutdown can cleanly remove our callback. + static FDelegateHandle GHandle; + + // Bool guard against double-Startup (e.g. if a plugin reload calls + // it twice). Live Coding patches the module without re-running + // StartupModule normally, so this should never trip in practice, + // but defensive is cheap. + static bool bStarted = false; + + static void Drain() + { + // Snapshot-and-drain pattern: only process closures that were + // already in the queue when Drain started. Closures enqueued + // BY those closures land in the queue and process on the next + // Drain call. This guarantees Drain terminates even if a + // pathological handler enqueues itself, and gives nested calls + // the same predictable next-frame semantics as TCP-side enqueues. + TArray> Batch; + TFunction Item; + while (GQueue.Dequeue(Item)) + { + Batch.Add(MoveTemp(Item)); + } + for (TFunction& Work : Batch) + { + // Each closure is independent — one throwing or asserting + // must not skip the rest. Catch broadly; UE's editor host + // tolerates this pattern in tick-driven code. + Work(); + } + } + + void Startup() + { + if (bStarted) return; + bStarted = true; + // OnEndFrame fires on the game thread after Slate/UI/world tick + // — outside any TaskGraph queue-processing context, which is + // exactly the window we need to avoid RecursionGuard asserts + // when handlers do nested AsyncTask-equivalent work. + GHandle = FCoreDelegates::OnEndFrame.AddStatic(&Drain); + } + + void Shutdown() + { + if (!bStarted) return; + bStarted = false; + FCoreDelegates::OnEndFrame.Remove(GHandle); + GHandle.Reset(); + // Final drain so anything queued during shutdown still runs. + // Safe because we're called from ShutdownModule on the game + // thread. + Drain(); + } + + void Tick() + { + Drain(); + } + + void ExecuteOnGameThread(TFunction Work) + { + if (!Work) return; + if (IsInGameThread()) + { + // Already on the game thread — run inline. No TaskGraph + // push, no queue, no possibility of re-entering anything. + Work(); + return; + } + GQueue.Enqueue(MoveTemp(Work)); + } + + bool RunOnGameThreadAndWait(TFunction Work, double TimeoutSeconds) + { + if (!Work) return true; + if (IsInGameThread()) + { + // Inline — see ExecuteOnGameThread comment. + Work(); + return true; + } + + // Shared promise so the closure and the waiter both keep it + // alive across the thread hop. If the wait times out and the + // calling frame unwinds, the AsyncTask's shared ref still + // holds the promise so SetValue is safe to call later. + struct FBox + { + TPromise Promise; + bool bSet = false; + }; + TSharedRef Box = + MakeShared(); + TFuture Future = Box->Promise.GetFuture(); + + GQueue.Enqueue([Box, Work = MoveTemp(Work)]() mutable + { + // Wrap in ON_SCOPE_EXIT-equivalent — SetValue must fire + // exactly once even if Work throws or asserts. We don't + // use ON_SCOPE_EXIT because including Misc/ScopeExit in + // this TU pulls in extra headers; the explicit flag does + // the same job. + struct FFinalizer + { + TSharedRef Box; + ~FFinalizer() { if (!Box->bSet) { Box->bSet = true; Box->Promise.SetValue(); } } + }; + FFinalizer Fin{ Box }; + Work(); + }); + + if (TimeoutSeconds <= 0.0) + { + Future.Wait(); + return true; + } + return Future.WaitFor(FTimespan::FromSeconds(TimeoutSeconds)); + } +} diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/Slivers/SSliverDetailPanel.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/Slivers/SSliverDetailPanel.cpp index 90bbee7d..77423778 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/Slivers/SSliverDetailPanel.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/Slivers/SSliverDetailPanel.cpp @@ -1,6 +1,7 @@ // SSliverDetailPanel.cpp #include "Slivers/SSliverDetailPanel.h" +#include "HaybaMCPThreading.h" #include "Slivers/HaybaSliverClient.h" #include "Slivers/HaybaSliverSettings.h" #include "Slivers/SSliverParamActorRef.h" @@ -152,7 +153,7 @@ FReply SSliverDetailPanel::OnRunClicked() FHaybaSliverRunCallback OnDone = FHaybaSliverRunCallback::CreateLambda( [this](bool bOk, const FString& Body) { - AsyncTask(ENamedThreads::GameThread, [this, bOk, Body]() + HaybaThreading::ExecuteOnGameThread([this, bOk, Body]() { bRunning = false; if (OutputBox) diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp index a091e544..1e142227 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp @@ -8,6 +8,7 @@ #include "HaybaMCPIdleHandler.h" +#include "HaybaMCPThreading.h" #include "Containers/Ticker.h" #include "Editor.h" #include "Editor/EditorEngine.h" @@ -286,7 +287,7 @@ FHaybaHandlerResult FHaybaMCPIdleHandler::Handle(const FString& Command, TSharedRef SharedState = MakeShared(MoveTemp(State)); - AsyncTask(ENamedThreads::GameThread, [SharedState]() + HaybaThreading::ExecuteOnGameThread([SharedState]() { // GC nudge — only when gc requested. Queue once, before the first poll, // so IsGCBusyImpl observes the pending pass briefly then sees it settle. diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPLegacyHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPLegacyHandler.cpp index e78d1158..41cf92e0 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPLegacyHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPLegacyHandler.cpp @@ -1,6 +1,7 @@ #include "HaybaMCPLegacyHandler.h" #include "HaybaMCPCommandHandler.h" #include "HaybaMCPLandscapeImporter.h" +#include "HaybaMCPThreading.h" #include "Json.h" #include "JsonUtilities.h" #include "PCGSettings.h" @@ -49,48 +50,25 @@ TArray FHaybaMCPLegacyHandler::GetCommands() const FHaybaHandlerResult FHaybaMCPLegacyHandler::RunOnGameThread(TFunction Work) { - if (IsInGameThread()) - { - return Work(); - } - - // Shared state so the marshaled lambda and the waiting caller both keep - // it alive across the thread hop — by-reference capture would be a - // use-after-free risk if the wait timed out and the caller frame unwound. - // - // We deliberately do NOT return the FEvent to the pool on timeout: if the - // marshaled lambda eventually runs after we've given up, it would trigger - // a recycled event and clobber unrelated waiters. Leaking one event per - // (rare) timeout is the lesser evil. The shared Box and event ownership - // are passed into the lambda by-value so they outlive the waiter. - struct FState - { - FHaybaHandlerResult Result; - FEvent* Done = nullptr; - }; + // Delegate to the unified dispatcher. The inline-when-on-game-thread + // pattern lives there, so this wrapper just adapts the FHaybaHandlerResult + // signature: capture the result by shared ref, signal completion via + // RunOnGameThreadAndWait's TPromise, return on either completion or + // timeout. Same 30s ceiling we used before. + struct FState { FHaybaHandlerResult Result; }; TSharedRef State = MakeShared(); - State->Done = FPlatformProcess::GetSynchEventFromPool(/*bIsManualReset=*/true); - AsyncTask(ENamedThreads::GameThread, [Work = MoveTemp(Work), State]() - { - State->Result = Work(); - if (State->Done) State->Done->Trigger(); - }); - - // 30s ceiling: long enough for landscape import (~5-10s on big heightmaps) - // and PCG execution, short enough to surface a real deadlock instead of - // hanging the TS client forever. - const bool bSignalled = State->Done->Wait(FTimespan::FromSeconds(30)); + const bool bCompleted = HaybaThreading::RunOnGameThreadAndWait( + [Work = MoveTemp(Work), State]() + { + State->Result = Work(); + }, + /*TimeoutSeconds=*/ 30.0); - if (bSignalled) + if (bCompleted) { - FPlatformProcess::ReturnSynchEventToPool(State->Done); - State->Done = nullptr; return State->Result; } - - // Timeout: leak the event (see comment above). The shared State outlives - // this frame because the AsyncTask lambda still holds a reference. return FHaybaHandlerResult::Err( TEXT("Game-thread marshal timed out after 30s — editor may be stalled " "or running a long blocking task. Try again once the editor is idle.")); diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPRenderHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPRenderHandler.cpp index 3aefe204..ae49ddee 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPRenderHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPRenderHandler.cpp @@ -7,6 +7,7 @@ #include "HaybaMCPRenderHandler.h" #include "HaybaMCPCaptureActor.h" +#include "HaybaMCPThreading.h" #include "Async/Async.h" #include "Containers/Ticker.h" @@ -452,7 +453,7 @@ FHaybaHandlerResult FHaybaMCPRenderHandler::Handle(const FString& /*Command*/, return FHaybaHandlerResult::Err(TEXT("render_camera: failed to allocate FEvent")); } - AsyncTask(ENamedThreads::GameThread, [S]() { RunOnGameThread(S); }); + HaybaThreading::ExecuteOnGameThread([S]() { RunOnGameThread(S); }); // Allow the wait phase + render + headroom. const uint32 BlockTimeoutMs = (uint32)((S->TimeoutSeconds + 60.0) * 1000.0); diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPThreading.h b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPThreading.h new file mode 100644 index 00000000..f61b289c --- /dev/null +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Public/HaybaMCPThreading.h @@ -0,0 +1,73 @@ +#pragma once + +#include "CoreMinimal.h" +#include "Containers/Queue.h" +#include "Async/Future.h" + +class FDelegateHandle; + +/** + * Game-thread dispatcher — the single place plugin code marshals work + * onto the game thread. + * + * Why this exists: `AsyncTask(ENamedThreads::GameThread, ...)` pushes + * to UE's TaskGraph queue, which asserts on re-entrant pushes via + * Assertion failed: ++Queue(QueueIndex).RecursionGuard == 1 + * TaskGraph.cpp:689 + * when something already-on-the-game-thread tries to enqueue more + * game-thread work. Two MCP sessions in May 2026 crashed UE this way: + * once when a handler that ran via AsyncTask called AsyncTask again + * internally; and once when a TCP-serialised handler did the same. + * + * This dispatcher sidesteps the entire RecursionGuard family by NOT + * using the TaskGraph at all. Workers enqueue closures into an MPSC + * queue; a callback subscribed to FCoreDelegates::OnEndFrame drains + * the queue once per frame, OUTSIDE any TaskGraph processing context. + * Closures that themselves want to schedule more game-thread work go + * through the same helpers; nested calls inline if already on the + * game thread. + * + * Rule for plugin code: + * - Never call `AsyncTask(ENamedThreads::GameThread, ...)` directly. + * Always call ExecuteOnGameThread() or RunOnGameThreadAndWait(). + * - The lint at mcp-tools/hayba-mcp/scripts/check-no-raw-gamethread-asynctask + * (added alongside this file) flags violations in CI. + */ +namespace HaybaThreading +{ + /** + * Schedule Work to run on the game thread. If the caller is + * already on the game thread, Work runs inline (no queueing). The + * inline path is what makes nested calls from within handlers + * safe — no TaskGraph push, no re-entry possible. + * + * Fire-and-forget; the caller does not wait for completion. + */ + HAYBAMCPTOOLKIT_API void ExecuteOnGameThread(TFunction Work); + + /** + * Run Work on the game thread and wait for it to finish. If the + * caller is already on the game thread, Work runs inline and the + * function returns immediately (no future, no wait). Otherwise, + * Work is enqueued and the calling thread blocks on a TPromise + * until the dispatcher drains it. + * + * TimeoutSeconds <= 0 means wait forever. Returns true if Work + * completed, false if the wait timed out. On timeout the closure + * remains in the queue and will still execute on a future drain + * — make Work idempotent or capture by shared_ptr if you care. + */ + HAYBAMCPTOOLKIT_API bool RunOnGameThreadAndWait( + TFunction Work, + double TimeoutSeconds = 30.0); + + /** + * Lifecycle. Called once from FHaybaMCPModule::StartupModule + * (subscribes to OnEndFrame) and once from ShutdownModule + * (unsubscribes, drains the queue one last time). Tests can call + * the Tick() helper directly to drain without an OnEndFrame. + */ + HAYBAMCPTOOLKIT_API void Startup(); + HAYBAMCPTOOLKIT_API void Shutdown(); + HAYBAMCPTOOLKIT_API void Tick(); +} From 7ab593bad2c1c848ab35a6cb4344a435883775b3 Mon Sep 17 00:00:00 2001 From: Badr Date: Sat, 23 May 2026 09:56:34 -0400 Subject: [PATCH 16/16] validator: rule for PCG-generate-overload (huge density + unbounded landscape source) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-05-23 scene session crashed UE during PCG Generate after assigning a fresh landscape material. Density: PointsPerSquaredMeter ~0.001 across a 4 km² landscape via PCGGetLandscapeSettings(bUnbounded=true) yielded ~4000 shrub points × 3 mesh variants + ~320 tree points × 4 variants. Combined with concurrent shader compilation for the new sand material, UE crashed with EXCEPTION_ACCESS_VIOLATION reading 0xff... inside UnrealEditor-Core — no plugin frame in the callstack. The rule fires after hayba_execute_pcg_graph / pcg_execute_graph with a hint about lowering density, scoping to a small PCGVolume, or waiting for shaders to settle. Catalog-only for now (no evaluator); will wire an evaluator that inspects the executed graph's sampler nodes + the linked LandscapeProxy area when we have a path to read graph contents back from UE. --- mcp-tools/hayba-mcp/src/validator/rules.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/mcp-tools/hayba-mcp/src/validator/rules.ts b/mcp-tools/hayba-mcp/src/validator/rules.ts index 4ad8a8ed..d421320f 100644 --- a/mcp-tools/hayba-mcp/src/validator/rules.ts +++ b/mcp-tools/hayba-mcp/src/validator/rules.ts @@ -164,6 +164,14 @@ export const RULES: ValidatorRule[] = [ refs: [], trigger: { after_tool: ['actor_spawn', 'actor_transform'] }, }, + { + id: 'pcg_generate_likely_overload', + severity: 'warning', + message: 'PCG generation about to trigger many instances over a large surface — likely to overload the editor', + hint: 'When PCGSurfaceSampler is configured with PointsPerSquaredMeter above ~0.0001 AND the source is an unbounded LandscapeProxy, the total instance count scales with the landscape area and can hit hundreds of thousands to millions of HISM instances. Combined with concurrent shader compilation (e.g. a freshly-applied landscape material), the editor can hang or crash with no diagnostic plugin frame in the callstack. Mitigations: lower the density, scope the sampler to a small PCGVolume, or wait for shaders to finish compiling before triggering Generate.', + refs: [], + trigger: { after_tool: ['hayba_execute_pcg_graph', 'pcg_execute_graph'] }, + }, { id: 'actor_snap_to_landscape_silently_failed', severity: 'warning',