Skip to content

feat: explorer mcp server for scenes#9339

Open
pravusjif wants to merge 73 commits into
devfrom
feat/explorer-mcp-server
Open

feat: explorer mcp server for scenes#9339
pravusjif wants to merge 73 commits into
devfrom
feat/explorer-mcp-server

Conversation

@pravusjif

@pravusjif pravusjif commented Jul 9, 2026

Copy link
Copy Markdown
Member

Summary

Embeds an MCP (Model Context Protocol) server inside the Explorer so a coding agent (Claude Code, Codex, …) can see a running SDK7 scene (screenshot, player/scene state, scene console logs) and drive it (teleport, move, walk, look, camera, pointer clicks, chat, emotes, reload) — closing the edit → reload → verify loop for scene development without a human in the middle.

The primary target is a retail build launched by Creator Hub (no Unity Editor, no Unity license on the creator's machine); the same server runs unchanged in the Editor for internal dev and on PR builds for QA.

Scope & blast radius

  • Dormant by default. Compiled into every build but does nothing unless explicitly enabled at launch.
  • Enabled only via --mcp / --mcp-port <port> (accepted from the command line or a deep link). Default port 8123.
  • Feature-gated: FeatureId.MCP_SERVER (FeaturesRegistry, resolved as appArgs.HasFlag(MCP) || appArgs.HasFlag(MCP_PORT)); DynamicWorldContainer.CreateAsync adds McpServerPlugin only when the feature is enabled.
  • localhost-only: the HttpListener binds to 127.0.0.1 and is never reachable from the network.
  • Zero impact on the retail path — when the flag is absent the plugin is never constructed, no listener opens, no systems run.
  • Almost entirely additive (new DCL.McpServer assembly + two folded folders); no #if UNITY_EDITOR — Editor and build behave identically.

Architecture (brief)

  • Standalone DCL.McpServer asmdef rooted at Explorer/Assets/DCL/McpServer/, split into Core/ (protocol + transport + tool contract), Tools/ (one class per tool), Components/, Systems/, Utils/, Tests/. Systems/ folds into DCL.Plugins and Tests/ into DCL.EditMode.Tests via .asmref so they can reach code the standalone assembly doesn't reference.
  • Hand-rolled JSON-RPC 2.0 over System.Net.HttpListener (Streamable-HTTP), Newtonsoft, protocol spec 2025-06-18, tools capability only. McpHttpServer (server + Origin validation) → McpJsonRpcDispatcher (initialize / ping / tools/list / tools/call) → IMcpTool.
  • ECS bridge for main-thread work. Input-driving tools don't touch Unity directly: they write an intent component (McpMovementOverride, McpPointerClickIntent) + a UniTaskCompletionSource, consumed and removed by a BaseUnityLoopSystem (McpInputOverrideSystem in the input group, McpPointerClickSystem in presentation) so synthetic input/clicks flow through the real pipelines. Each tool awaits SwitchToMainThread itself before reading ECS/Unity; heavy work (base64) hops back to the pool.

Key decisions & rationale

Standalone embedded server — no integration with Unity's official MCP.
Unity's official MCP (in com.unity.ai.assistant) is Editor-only by construction: its transport chain runs through a relay binary into the Unity Editor process, with no runtime component for a built player, and its extension API is discovered by Editor-startup TypeCache scanning that doesn't exist in a player. It is also license-gated (Unity 6+, Assistant package, AI-Gateway sign-in, a per-tier connection entitlement) — creators have none of these — and its ~51 tools target Editor asset/script/shader work with no input simulation and no built-player interaction at all. For our primary scenario (retail build, no Editor) integration is simply impossible. Owning the server is cheap because the MCP layer is a thin protocol adapter; all real capability (back-buffer capture, input injection, ECS reads) must live in-process regardless of who serves the protocol.

Embedded MCP-over-HTTP because Creator Hub will drive multiple engines, not just Unity.
The product direction is a Creator Hub gateway: the coding agent talks to one endpoint (Creator Hub), which fans out as an MCP client to whichever engine's embedded server is running — Unity, Bevy, or Godot. For Hub to use one generic MCP client across all of them, every engine must expose the same MCP-over-HTTP surface. That requirement is what drove the transport choice and what set aside the parallel automation approaches:

  • Esteban's CDP automation-bridge (PR #5) turns the CDP channel into a generic in-player RPC server, but CDP (ws://1473, Fleck) is Unity/web-specific — Bevy and Godot will never ship it, so routing Unity through CDP would force Hub to maintain a bespoke non-MCP client just for Unity. It stays a sibling network-monitor concern, not the automation transport.
  • The DclHarness/ClaudeIPC prototype is an Editor-only, reflection-based file channel bound to a running Unity Editor — no retail/multi-engine path at all.

An embedded, standalone MCP server per engine is precisely what makes the multi-engine gateway possible.

Hand-rolled JSON-RPC — not the ModelContextProtocol C# SDK; no batching.
The SDK's most valuable piece (HTTP hosting) lives in ModelContextProtocol.AspNetCore (Kestrel / Generic Host), which Unity doesn't run — we'd still have to write our own HttpListener transport and the main-thread marshalling. Its reflection/assembly-scan tool discovery and runtime schema generation are exactly the patterns that get fragile under IL2CPP / managed-code stripping in retail builds, whereas our explicit registration is IL2CPP-safe by construction. It would also drag in System.Text.Json + Microsoft.Extensions.*, conflicting with the codebase's Newtonsoft + own composition-root DI. We borrow the SDK's design (schema-from-signature, DI-injected params, annotation hints), not its code — the whole dispatcher is ~150 lines. We track spec 2025-06-18, which removed JSON-RPC batching, so a single-object dispatcher (no batch-array path) is spec-correct, not a gap.

Tool annotations (readOnlyHint / destructiveHint / idempotentHint / openWorldHint, spec 2025-06-18) are emitted in tools/list — cheap, forward-compatible, and the machine-readable hook a future auth-gate of mutating tools (and profile filtering) will read.

Structured output seam (structuredContent + outputSchema) is built as a purely-additive result envelope and adopted only as an exemplar on the read-only state tools (get_player_state, get_scene_state, list_scene_entities) that already build a JObject; every other tool returns text only. Doing it now locks the output-field contract while zero external consumers depend on the prose format.

Ecosystem sanity-check. A sweep of third-party Unity MCP servers + Anthropic's tool-design guidance confirmed the foundation is spec-aligned. Almost the entire ecosystem is Editor-tooling with a sidecar process; being fully in-process inside a running player is unusual and correct — it sidesteps the domain-reload churn that is the ecosystem's dominant pain by construction. Curated ~16 workflow-shaped tools avoid the tool-list flooding that degrades LLM tool selection.

Security model

  • Listener binds to 127.0.0.1 only.
  • Browser-originated requests rejected unless Origin is localhost (drive-by / DNS-rebinding defense); requests without an Origin header (CLI clients, curl) are allowed.
  • No authentication token in v1 — security rests on localhost binding + Origin check. Deliberately no execute-arbitrary-C# tool (a foot-gun for a retail build with no auth). The trigger to add a token is the first mutating dev-only tool.

Tool catalog (16 tools)

Full arguments/returns and troubleshooting in docs/mcp-automation.md.

  • Seeing (6): screenshot, get_player_state, get_scene_state, get_scene_logs, list_scene_entities, get_entity_details.
  • Controlling (10): teleport, move_to, walk, look_at, set_camera_mode, set_camera_pose, send_chat, reload_scene, trigger_emote, click_entity.

Tests

EditMode tests (folded into DCL.EditMode.Tests) cover the load-bearing routing plus the input bridge:

  • McpJsonRpcDispatcherShould — known tool dispatch, unknown tool → -32602, notification (id == null) dropped.
  • McpToolsRegistryShouldtools/list serialization, TryGet / name-guard.
  • McpToolResultShould — result / error / structured-output envelope.
  • McpHttpServerShould, McpInputSchemaShould.
  • McpPointerClickSystemShould and per-tool tests (GetEntityDetailsToolShould, ListSceneEntitiesToolShould, GetPlayerStateToolShould, GetSceneStateToolShould).

Deliberately NOT in this PR / follow-ups

Iteration-1 holes are intentional — the foundation is built to extend organically:

  • Per-command cancellation: today only the server-lifetime token trips; a notifications/cancelled handler + a per-request CTS registry land with the first genuinely long-running/interruptible tool.
  • Output discipline on large dumps: done for get_entity_details (char cap) and list_scene_entities (limit + "narrow with X"); a component-name filter needs extending IWorldInfo (out of scope).
  • Resources capability for read-only state — deferred, and the reason is structural, not cosmetic. MCP's primitives differ by who invokes them: Tools are model-controlled (the agent calls them itself), while Resources are application-controlled — the host decides whether to surface/attach a Resource, so an agent cannot autonomously read one; it depends on the client wiring it in, which makes Resource output read as "strange" to an agent. On top of that, Resource support across coding-agent clients is still uneven. That combination is why the ecosystem converged on Tools, and why we keep tools-only (no resources capability declared): read-only state that is spec-naturally a Resource (scene state, entity list, log buffer) stays a polling tool for now. Two narrow slices are worth adopting later — resource_link in results (selectively; not for screenshot, which agents consume best as an inline image) and resources/subscribe (the one real win over polling, adopted only when a consumer needs push and the target client supports it).
  • Two-tier tool exposure (deferred — context economy is a client-side lever today; server-side profiles are the coarser complement, worth building only as the tool count grows).
  • Canonical cross-engine tool contract (owned by Creator Hub / Foundation — our names are already canonical-friendly verb_noun snake_case; the only engine-specific risk is set_camera_mode values drone/free, to be isolated behind a profile later).
  • --mcp-profile creator|dev, auth token, per-tool toggles — designed, not built.

Related: V2 issue #9393

How to enable / test

# macOS
open Decentraland.app --args --mcp
# Windows
Decentraland.exe --mcp-port 8124

Endpoint: http://127.0.0.1:<port>/unity-explorer-mcp. Connect an agent:

claude mcp add --transport http --scope user explorer http://127.0.0.1:8123/unity-explorer-mcp

Connection details: docs/mcp-automation.md#connecting-a-coding-agent.


Test Steps

[simpler SKILL pending for creators to use the MCP server in a more efficient way]

  1. Download the build from this PR.
  2. Start a local scene (e.g. the sdk7-scene-template).
  3. Close the Explorer that auto-opened. Leave the scene running in the console/terminal.
  4. Open the build with the following parameters (if you use a different scene, change the position value):
    • Windows: "C:\Users\[YOUR-USER]\Downloads\Decentraland_windows64\Decentraland.exe" --realm http://127.0.0.1:8000 --position 0,0 --local-scene true --skip-version-check true --mcp
    • macOS: open Decentraland.app --args --realm http://127.0.0.1:8000 --position 0,0 --local-scene true --skip-version-check true --mcp
  5. From a terminal in the scene folder, run the MCP setup command, e.g. claude mcp add --transport http --scope user explorer http://127.0.0.1:8123/mcp.
  6. Start your agent in the scene folder and prompt it to connect and iterate, e.g.:
You are going to work on the current decentraland scene that this folder contains, the scene is already running, you are not allowed to stop and restart its server, nor make any change in the `scene.json` file.

Connect to the Decentraland Explorer mcp server that should already be up and running at `http://127.0.0.1:8123/unity-explorer-mcp` and use its toolset to verify your modifications of the scene through scene interactions (screenshot, moving the player, interacting with pointer events, etc.).

I have sdk-skills installed, use those to build the scene.

I want you to scrap the current scene code and make a small simple labyrinth game, in 1 parcel, the walls can be cubes, and they should have collision.

The game should start when the player actually enters the labyrinth on point A and it finishes when he exits on point B (only exit of the labyrinth).

You should verify that the player can actually play and win

⚠️ QA — Camera transitions smoke test

@pravusjif pravusjif added the force-build Used to trigger a build on draft PR label Jul 9, 2026
@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

@pravusjif pravusjif added force-build Used to trigger a build on draft PR and removed force-build Used to trigger a build on draft PR labels Jul 10, 2026
@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Warnings not reduced: 14668 => 14766 — remove at least one warning to merge.

Warnings/errors in files changed by this PR (97)
File Line Rule Message
Assets/DCL/McpServer/Tests/McpToolsRegistryShould.cs 166 CSharpWarnings::CS8604 Possible null reference argument for parameter 'item' in 'System.Collections.Generic.List.Add'
Assets/DCL/PerformanceAndDiagnostics/Diagnostics/Container/DiagnosticsContainer.cs 16 CSharpWarnings::CS8618 Non-nullable field 'defaultLogHandler' is uninitialized. Consider adding the 'required' modifier or declaring the field as nullable.
Assets/DCL/PerformanceAndDiagnostics/Diagnostics/Container/DiagnosticsContainer.cs 17 CSharpWarnings::CS8618 Non-nullable property 'ReportHubLogger' is uninitialized. Consider adding the 'required' modifier or declaring the property as nullable.
Assets/DCL/PerformanceAndDiagnostics/Diagnostics/Container/DiagnosticsContainer.cs 19 CSharpWarnings::CS8618 Non-nullable property 'Settings' is uninitialized. Consider adding the 'required' modifier or declaring the property as nullable.
Assets/DCL/McpServer/Components/McpMovementOverride.cs 33 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Components/McpPointerClickIntent.cs 41 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Components/McpPointerClickIntent.cs 44 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Components/McpPointerClickIntent.cs 49 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Components/McpPointerClickIntent.cs 56 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Components/McpPointerClickIntent.cs 59 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Components/McpPointerClickIntent.cs 64 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/IMcpTool.cs 32 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpHttpServer.cs 35 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpHttpServer.cs 85 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpHttpServer.cs 163 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpHttpServer.cs 203 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpHttpServer.cs 208 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 46 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 48 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 71 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 71 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 91 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 92 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 107 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 112 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 130 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 130 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 132 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs 169 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonSchema.cs 22 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonSchema.cs 22 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonSchema.cs 25 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonSchema.cs 28 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonSchema.cs 31 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonSchema.cs 38 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonSchema.cs 52 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonSchema.cs 81 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpJsonSchema.cs 81 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpToolsRegistry.cs 62 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Core/McpToolsRegistry.cs 62 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/GetEntityDetailsTool.cs 41 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/GetPlayerStateTool.cs 32 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/GetPlayerStateTool.cs 64 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/GetPlayerStateTool.cs 65 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/GetSceneStateTool.cs 29 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/GetSceneStateTool.cs 59 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/ListSceneEntitiesTool.cs 36 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/ListSceneEntitiesTool.cs 55 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/ReloadSceneTool.cs 66 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context
Assets/DCL/McpServer/Tools/ScreenshotTool.cs 38 CSharpWarnings::CS8632 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context

…and 47 more (see the csharp-lint-reports artifact).

@decentraland-bot
decentraland-bot self-requested a review July 17, 2026 15:45

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

STEP 1 — Scope

New DCL.McpServer assembly (Core, Tools, Components, Systems, Utils, Tests) plus targeted changes to CinemachineExtensions, ApplyCinemachineCameraInputSystem, DynamicWorldContainer, FeaturesRegistry, AppArgsFlags, DiagnosticsContainer, and ReportCategory. Reviewed against CLAUDE.md, docs/code-style-guidelines.md, and the review-instructions prompt. Repository cloned and searched with rg for lifecycle owners, teardown mirrors, and existing conventions.

STEP 2 — Root-cause check

This is a new feature (embedded MCP server for coding-agent scene automation), not a bug fix. The diff directly builds the stated capability. No symptom-masking, swallowed exceptions, or workaround patterns detected. PASS.

STEP 3 — Design & integration

Owner search:

  • McpServerPlugin is a new IDCLGlobalPluginWithoutSettings registered in DynamicWorldContainer.CreateAsync behind FeatureId.MCP_SERVER. This is the correct registration point for a global feature plugin — confirmed by searching the ~50 other globalPlugins.Add(...) calls in the same file.
  • McpInputOverrideSystem runs in InputGroup after UpdateInputMovementSystem / UpdateInputJumpSystem. The movement input lifecycle is owned by these systems; the MCP override correctly injects into the same pipeline via a transient intent component.
  • McpPointerClickSystem runs in PresentationSystemGroup after ProcessPointerEventsSystem. The pointer-event lifecycle is owned by ProcessPointerEventsSystem; the MCP system injects a synthetic press through the same AppendPointerEventResultsIntent path, which is the correct seam.
  • No duplicate lifecycle management found. The intent components (McpMovementOverride, McpPointerClickIntent) are transient ECS components written by tools and consumed/removed by the systems — standard ECS intent pattern.

Teardown trace:

  • McpServerPlugin.Dispose() correctly disposes screenshotTool, server, and serverCts.
  • ⚠️ The DebugMenuConsoleLogEntryBus event subscription (logEntryBus.MessageAdded += logBuffer.Append, line 98) has no teardown path — logEntryBus is a local variable never stored as a field. This matches the identical pattern in DebugMenuPlugin (confirmed: same constructor pattern, same empty Dispose()), and both are process-lifetime plugins. No real-world leak, but the subscription can never be removed. P2 — store as field for future cleanup parity.

Design verdict: PASS — no misplaced logic, no duplicate lifecycle, no per-frame reconciliation of explicit moments.

STEP 4 — Member audit

Member Consumers Verdict
SetCameraModeTool.IsModeChangeAllowed() GetPlayerStateTool (camera.modeChangeAllowed), TrySwitchMode (internal) 2 consumers — justified
SetCameraModeTool.TrySwitchMode() SetCameraModeTool.ExecuteAsync, SetCameraPoseTool.ExecuteAsync 2 consumers — justified
CinemachineExtensions.ForceFreeCameraPose() SetCameraPoseTool.ExecuteAsync 1 consumer — follows the established Force*CameraLookAt pattern on ICinemachinePreset; acceptable as a cohesive extension group
CinemachineExtensions.ForceFreeCameraLookAt() ApplyCinemachineCameraInputSystem.ForceLookAt (Free case) 1 consumer — fixes a bug where Free mode fell through to ThirdPerson look-at logic
McpToolAnnotations.ReadOnly() / .Mutating() All 16 tools Multi-consumer

No single-use predicates re-checking caller-guaranteed invariants, no absent≠false conflation, no re-derived lookups found.

STEP 5 — Line-level findings

All findings are posted as inline comments with suggestion blocks below.

Summary of findings

# Severity File Issue
1 P1 McpHttpServer.cs:196 Method name typo WriteEmptyAncCloseWriteEmptyAndClose (naming standard)
2 P1 Utils/SceneLogBuffer.cs:4, Utils/JObjectExtensions.cs:6 Namespace DCL.McpServer.Tools doesn't match Utils/ folder — CLAUDE.md forbids suppressing CheckNamespace
3 P2 McpHttpServer.cs:36 Double semicolon
4 P2 McpHttpServer.cs:46-47 Redundant ?. after null guard
5 P2 McpHttpServer.cs:128 ContentLength64 check bypassed by chunked encoding (-1 > MAX is false)
6 P2 McpPointerClickSystem.cs:170 Inline epsilon 0.0001f — extract to named constant
7 P2 McpServerPlugin.cs:97-98 logEntryBus not stored as field — event subscription has no teardown path

Security assessment

  • Localhost binding (127.0.0.1 only) — correct, never network-reachable.
  • Origin validation — rejects browser-originated cross-origin requests; CLI tools (no Origin header) are allowed by design. Sufficient for a localhost dev tool.
  • No authentication token — acknowledged deferral in the PR. The server is opt-in (--mcp flag), localhost-only, and targets local development workflows. Acceptable for v1; token should land before any remote-reachable or multi-user scenario.
  • Input validation — all tool parameters are clamped/validated. SendChatTool caps message length at 500 chars. No injection vectors found.
  • Thread safety — every tool that touches ECS/Unity correctly switches to main thread. SceneLogBuffer uses lock for cross-thread access. ScreenshotTool uses Interlocked capture gate. No unsynchronized shared mutable state.

STEP 6 — Complexity

COMPLEX — introduces new ECS systems, components, plugin registration, assembly definitions, and input handling integration.

STEP 7 — QA assessment

QA_REQUIRED: YES — modifies runtime code, adds input simulation systems, camera control, and scene interaction tools.

STEP 8 — Non-blocking warnings

No warnings triggered (Main.unity not modified).

STEP 9 — Verdict

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Introduces new ECS systems (McpInputOverrideSystem, McpPointerClickSystem), components, a new assembly (DCL.McpServer), plugin registration, HTTP server, and input/camera handling integration.
QA_REQUIRED: YES


Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub

Comment thread Explorer/Assets/DCL/McpServer/Core/McpHttpServer.cs Outdated
Comment thread Explorer/Assets/DCL/McpServer/Core/McpHttpServer.cs Outdated
Comment thread Explorer/Assets/DCL/McpServer/Core/McpHttpServer.cs
Comment thread Explorer/Assets/DCL/McpServer/Core/McpHttpServer.cs Outdated
Comment thread Explorer/Assets/DCL/McpServer/Utils/SceneLogBuffer.cs Outdated
Comment thread Explorer/Assets/DCL/McpServer/Utils/JObjectExtensions.cs Outdated
Comment thread Explorer/Assets/DCL/McpServer/Systems/McpPointerClickSystem.cs Outdated
Comment thread Explorer/Assets/DCL/McpServer/Systems/McpServerPlugin.cs
@pravusjif pravusjif removed the force-build Used to trigger a build on draft PR label Jul 17, 2026
@pravusjif pravusjif moved this to QA in Creators Tools Jul 17, 2026

@Ludmilafantaniella Ludmilafantaniella left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested on Windows and Mac. ✅

Used the sdk7-scene-template as the local scene. Connected Claude Code to the Explorer MCP server and prompted it to build a labyrinth game from scratch without restarting the scene.

Windows:

  • MCP server connected successfully
  • Claude Code built the labyrinth, verified collisions and geometry via screenshots and locomotion
  • Scene logs confirmed: MAZE: entered at point A, game started and MAZE: exited at point B, WON in 141.6s

Mac:

  • MCP server connected (required --scope project instead of --scope user due to permissions)
  • Claude Code built and verified the labyrinth
  • Game completed successfully: You escaped the labyrinth! Time: 107.0s visible on screen ✅

In both cases the scene was never restarted and scene.json was untouched throughout. LGTM 🚀

Image Image Image

✅Smoke test performed:

  • ✔️ Backpack and wearables in world
  • ✔️ Emotes in world and in backpack
  • ✔️ Teleport with map/coordinates/Jump In
  • ✔️ Chat and multiplayer
  • ✔️ Camera
  • ✔️ Skybox

pravusjif and others added 10 commits July 17, 2026 23:39
# Conflicts:
#	Explorer/Assets/DCL/PerformanceAndDiagnostics/Diagnostics/ReportsHandling/ReportsHandlingSettingsProduction.asset
…stion

  The Content-Length guard in HandlePostAsync was bypassed by
  Transfer-Encoding: chunked requests (ContentLength64 == -1), letting
  ReadToEndAsync read an unbounded body into memory. Read into a fixed
  MAX_BODY_BYTES buffer and reject oversized bodies with 413.
…s teardown

  - McpHttpServer: cap POST body read into a fixed buffer so the 1MB limit
    can't be bypassed via Transfer-Encoding: chunked (ContentLength64 == -1)
  - McpServerPlugin: store DebugMenuConsoleLogEntryBus as a field and
    unsubscribe logBuffer.Append in Dispose()
  - McpHttpServer: cap POST body read via ReadBlockAsync so chunked requests
    can't bypass the Content-Length limit and exhaust memory
  - McpHttpServer: capture the listener into a local in the accept loop to
    avoid an NRE race with a concurrent Dispose()
  - McpHttpServer: document the deliberate stateless single-session design
    (unconditional, unvalidated Mcp-Session-Id)
  - McpToolsRegistry: hand out a DeepClone of the tools/list payload so
    concurrent dispatches don't race on the shared JObject
  - McpJsonRpcDispatcher: return -32600 (Invalid Request) for well-formed
    JSON that isn't an object, keeping -32700 for unparseable input
  - McpJsonRpcDispatcher: log a warning when the client's initialize
    protocolVersion differs from the server's
  - McpServerPlugin: store the log entry bus in a field and unsubscribe in
    Dispose()
@popuz
popuz removed the request for review from lorenzo-ranciaffi July 17, 2026 22:35
popuz and others added 12 commits July 18, 2026 20:24
…hed payload

  The implicit JObject conversion on McpToolsRegistry hid a DeepClone behind
  `Result(id, tools)` in the dispatcher — a surprising conversion with a
  per-request allocation. Serialize the tools/list payload once in Build() and
  hand it out via an explicit ToolsListPayload() that wraps the immutable JSON
  string in a fresh JRaw, so concurrent dispatches share it without cloning or
  re-parenting a JToken tree.

  - Drop `implicit operator JObject` and the per-request DeepClone
  - Store the sealed payload as a serialized string, built once in Build()
  - Update the dispatcher call site to tools.ToolsListPayload()
  - Re-parse the raw payload in registry tests via a Payload() helper
  Replace repeated routable.Value.X access with a single tuple
  deconstruction and a property-pattern guard in DispatchAsync.
  Centralize Formatting.Indented serialization in McpToolResult so tools
  can't drift the structured payload from its text mirror or forget the
  formatting. Convert the eight callsites and drop their now-unused
  Newtonsoft.Json import.
  The builder produces both input and output JSON Schemas (OutputSchema,
  VectorSchema), so the "Input" name misrepresented it. Pure rename of the
  type and its file/test.
  The three structured tools declare OutputSchema and build the payload by
  hand, so a field added to one and forgotten in the other would silently
  misdescribe the result. Add McpSchemaAssert.KeysMatch, a recursive
  schema-vs-payload key check, and wire it into the Get*/List* tool tests
  (populating a camera entity and a scene facade so nested objects are
  covered too).
… Build

  Dedup nullable type-token logic in McpJsonSchema via a private
  TypeToken(type, nullable) helper, dropping the (JToken)type cast in
  Property and sharing it with Object. Add symmetric OutputSchema
  validation in McpToolsRegistry.Build so a malformed output schema
  fails fast at registration instead of reaching the client.
  Change the extension to take HttpStatusCode instead of int, casting
  once inside the method. Drops the (int) cast and statusCode: label
  from all seven call sites and makes the status intent type-safe.
  Cap each tools/call at TOOL_CALL_TIMEOUT with a CancellationTokenSource
  linked to the server-lifetime token. A hung tool now returns a timeout
  error result instead of holding its HttpListenerContext open until
  shutdown. Distinguish shutdown cancellation (rethrow) from timeout
  (error result) via a when (ct.IsCancellationRequested) filter.
  Tools declared "switch to the main thread yourself" and 15 of 16 opened
  with an identical `await UniTask.SwitchToMainThread(ct)`. Hop once in
  McpJsonRpcDispatcher.CallToolAsync instead and flip the IMcpTool contract
  to "invoked on the main thread". Readers with no other await become
  non-async (UniTask.FromResult); tools that offload to the thread pool
  keep their return-trip hops.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: QA

Development

Successfully merging this pull request may close these issues.

4 participants