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/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 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 9d2ad6ba..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); @@ -1880,12 +1884,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 +1902,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', { @@ -1948,6 +1956,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"'), 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/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'); diff --git a/mcp-tools/hayba-mcp/src/validator/rules.ts b/mcp-tools/hayba-mcp/src/validator/rules.ts index 3052ea23..d421320f 100644 --- a/mcp-tools/hayba-mcp/src/validator/rules.ts +++ b/mcp-tools/hayba-mcp/src/validator/rules.ts @@ -148,6 +148,38 @@ 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: '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: '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', + 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 5f9a858a..071e529e 100644 --- a/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts +++ b/mcp-tools/hayba-mcp/src/validator/tool-hooks.ts @@ -247,6 +247,170 @@ 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, + }, + }; +} + +// ── 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; @@ -260,6 +424,9 @@ 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); + 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/HaybaMCPCommandHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp index c4dfca9a..f4ab43d1 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPCommandHandler.cpp @@ -14,6 +14,8 @@ #include "HaybaMCPDiffPanel.h" #include "Json.h" #include "Async/Async.h" +#include "Async/Future.h" +#include "HaybaMCPThreading.h" #include "Editor.h" #include "EngineUtils.h" #include "GameFramework/Actor.h" @@ -45,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.")); @@ -122,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")) { @@ -156,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")) { @@ -187,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")) { @@ -236,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)) { @@ -278,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; } @@ -383,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")) { @@ -426,20 +424,47 @@ 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 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")) { - if (TSharedPtr Panel = M->PlanPanel.Pin()) + if (TSharedPtr Panel = M2->PlanPanel.Pin()) { Panel->LoadPlan(Steps, AwaitSecs); + TArray _DiscardSteps; int32 _DiscardAwait; + M2->ConsumePendingPlan(_DiscardSteps, _DiscardAwait); + bPanelOpen = true; } } - }); + }, /*TimeoutSeconds=*/ 2.0); 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); } @@ -548,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")) { @@ -577,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")) { @@ -719,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/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..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); }); @@ -427,4 +439,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/Private/HaybaMCPTcpServer.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp index 21f9ba9d..7c84ae2f 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.cpp @@ -1,12 +1,23 @@ #include "HaybaMCPTcpServer.h" #include "HaybaMCPCommandHandler.h" -#include "Async/Async.h" +#include "HaybaMCPThreading.h" +#include "Async/Async.h" // for ::AsyncTask onto background pool only #include "Serialization/JsonSerializer.h" #include "SocketSubsystem.h" #include "IPAddress.h" 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) { @@ -25,10 +36,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, @@ -75,9 +82,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) @@ -91,7 +131,19 @@ uint32 FHaybaMCPTcpServer::Run() { const int32 NewCount = ClientCount.Increment(); UE_LOG(LogHaybaMCPTCP, Log, TEXT("Client accepted (active: %d)"), NewCount); - AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask, [this, ClientSocket]() + + // 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); + RegisterConn(Conn); + // 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); }); @@ -103,31 +155,74 @@ uint32 FHaybaMCPTcpServer::Run() void FHaybaMCPTcpServer::HandleClientConnection(FSocket* ClientSocket) { - while (bIsRunning) + // 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(ClientSocket, Message)) { const int32 NewCount = ClientCount.Decrement(); UE_LOG(LogHaybaMCPTCP, Log, TEXT("Client disconnected (active: %d)"), NewCount); - ClientSocket->Close(); - ISocketSubsystem::Get(PLATFORM_SOCKETSUBSYSTEM)->DestroySocket(ClientSocket); + 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; } - AsyncTask(ENamedThreads::GameThread, [this, Message, ClientSocket]() + // Capture the SHARED REF (not the raw socket pointer) into the + // 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; + + // 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]() { - if (CommandHandler.IsValid()) + if (!CommandHandler.IsValid()) return; + FString ResponseString = CommandHandler->ProcessCommand(Message); + if (ConnForLambda.IsValid() && ConnForLambda->bAlive && ConnForLambda->Socket) { - FString ResponseString = CommandHandler->ProcessCommand(Message); - SendMessage(ClientSocket, ResponseString); + SendMessage(ConnForLambda->Socket, ResponseString); } - }); + }, /*TimeoutSeconds=*/ 0.0); } } bool FHaybaMCPTcpServer::ReadMessage(FSocket* Socket, FString& OutMessage) { + // 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; @@ -172,6 +267,18 @@ bool FHaybaMCPTcpServer::ReadMessage(FSocket* Socket, FString& OutMessage) void FHaybaMCPTcpServer::SendMessage(FSocket* Socket, const FString& Message) { + // 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) return; + FTCHARToUTF8 Utf8Msg(*Message); uint32 Length = Utf8Msg.Length(); @@ -181,10 +288,18 @@ 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) + { + 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..8cfd44e9 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/HaybaMCPTcpServer.h @@ -2,23 +2,45 @@ #include "CoreMinimal.h" #include "HAL/Runnable.h" +#include "HAL/CriticalSection.h" #include "Networking.h" class FHaybaMCPCommandHandler; +/** + * 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. + * + * 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; + bool bAlive = true; + + explicit FHaybaMCPConnection(FSocket* InSocket) : Socket(InSocket) {} + ~FHaybaMCPConnection(); + + FHaybaMCPConnection(const FHaybaMCPConnection&) = delete; + FHaybaMCPConnection& operator=(const FHaybaMCPConnection&) = delete; +}; + class FHaybaMCPTcpServer : public FRunnable { public: 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(); @@ -39,6 +61,18 @@ class FHaybaMCPTcpServer : public FRunnable bool bIsRunning = false; FThreadSafeCounter ClientCount; + // 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); 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/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; } 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/HaybaMCPActorHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp index 41e9e6a8..c11672f6 100644 --- a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp +++ b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPActorHandler.cpp @@ -4,12 +4,19 @@ #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" +#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); @@ -28,6 +35,56 @@ AActor* FHaybaMCPActorHandler::FindActorByName(UWorld* World, const FString& Nam return nullptr; } +/** + * Snap a world XY to the landscape surface Z via a vertical line trace. + * + * 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). + * + * 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, 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); + FCollisionQueryParams Params(SCENE_QUERY_STAT(HaybaSnapToLandscape), false); + Params.bTraceComplex = true; + if (IgnoredActor) Params.AddIgnoredActor(IgnoredActor); + + TArray Hits; + World->LineTraceMultiByChannel(Hits, Start, End, ECC_WorldStatic, Params); + for (const FHitResult& Hit : Hits) + { + if (Hit.GetActor() && Hit.GetActor()->IsA(ALandscapeProxy::StaticClass())) + { + OutZ = Hit.ImpactPoint.Z; + return true; + } + } + return false; +} + +/** 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; @@ -109,35 +166,115 @@ 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; - // Scale + // 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 (post-spawn so it applies to both paths) const TArray>* ScaleArr; 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, NewActor)) + { + 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()) @@ -146,7 +283,12 @@ 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()); + if (bSnapped) + { + Out->SetBoolField(TEXT("snapped_to_landscape"), true); + Out->SetNumberField(TEXT("snapped_z"), SnappedZ); + } return FHaybaHandlerResult::Ok(Out); } @@ -199,11 +341,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, Actor)) + { + 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); } diff --git a/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp b/unreal/HaybaMCPToolkit/Source/HaybaMCPToolkit/Private/handlers/HaybaMCPIdleHandler.cpp index dc88e79a..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" @@ -146,12 +147,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 +202,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; @@ -245,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/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); 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/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; +}; 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(); +}