add: Creator Hub Scene Agent - #1499
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Deep Review: Creator Hub Scene Agent (PR #1499)
An impressive and well-structured addition — the four-layer architecture (Chat Panel → Agent Runner → MCP Server → Explorer Gateway) is a solid foundation. The code is thoughtful, comments are extensive, and the existing test coverage (unit + E2E passing) gives confidence. That said, the deep review uncovered several findings that should be addressed before merge. Organized by severity:
P0 — Blockers
1. [Security] Signal-killed child process treated as successful turn
ai.ts:734 — code === null (process killed by signal — e.g. OOM-killer, external SIGKILL, or aiStop() race) calls finish(true). This means a turn that was forcibly terminated is reported to the user as "done successfully", and any partial mutations it made are not flagged for review. Should be finish(false) or finish(false, 'assistant was interrupted').
2. [Security] Unbounded request body in MCP HTTP server
scene-mcp.ts:658-665 — readBody() concatenates chunks with no size limit. While the server is localhost-only with bearer auth, a compromised or buggy MCP client can OOM the Electron main process. Add a body-size cap (e.g. 4 MB) and abort with 413.
P1 — Major
3. [Correctness] Stale scene reads immediately after mutations
scene-composite.ts:64-77 reads the composite from disk synchronously, but mutations flow through the live engine (operations.dispatch() → autosave ~100ms later). If the AI calls set_component then immediately scene_state, it reads pre-mutation data. The sceneOpChain serialization in scene-mcp.ts:139-147 orders mutations, but read tools bypass the chain entirely. Consider either routing reads through the engine or adding a small flush-wait after mutations.
4. [Security] filterEnvForChild() only strips AI-specific vars
ai.ts:180-216 — Combined with --permission-mode bypassPermissions / --sandbox danger-full-access, the spawned CLI inherits the user's full environment (AWS credentials, GH_TOKEN, NPM_TOKEN, SSH agent, etc.). The system prompt is the only guardrail, which is not resistant to prompt injection. This is an inherent design trade-off of the feature, but worth documenting explicitly — at minimum, add a code comment acknowledging this surface area and consider adding a user-facing warning in the settings UI about what "full capability mode" means.
5. [Correctness] Unvalidated entity as Entity casts on AI-supplied input
inspector/server.ts:297,303,309,316,319,329,345 — Every entity as Entity and parent as Entity cast takes an arbitrary number from the AI model and brands it without validation. An invalid entity id (e.g. a hallucinated number) will silently corrupt the CRDT or throw deep in the ECS. Add engine.entityExists() guard before casting.
6. [Reliability] No Explorer gateway reconnection after crash
explorer-gateway.ts:119-139 — The initial MCP connection retries, but once gateway.client is established there is no disconnect/error handler. If Unity Explorer crashes mid-session, callExplorerTool throws on every subsequent call with no recovery. The gateway stays "running" with a dead client until a manual stop_preview.
7. [Reliability] launching serialization is project-unaware
explorer-gateway.ts:292-298 — A single module-level launching promise serializes concurrent calls. If a second call arrives for a different project while the first is in-flight, it silently returns the first project's result. The guard should check whether the in-flight launch matches the requested projectDir.
8. [Reliability] Session leak — no TTL or cap on MCP sessions
scene-mcp.ts:617 — Sessions are created on each initialize but only removed on transport close. A client that opens sessions without closing them (or crashes) leaks server + transport pairs indefinitely. Add a max-sessions cap and/or idle TTL.
9. [Security] MCP config file permissions in tmpdir
scene-mcp.ts:785-803 — The --mcp-config JSON file containing the bearer token is written inside a mkdtempSync directory (0700), but the file itself uses default permissions (typically 0644). On shared systems, other users can read the token. Use fs.writeFileSync(file, content, { mode: 0o600 }).
10. [Type Safety] content as never suppresses all type checking
scene-mcp.ts:532,602 — content as never bypasses the MCP SDK's content type entirely. A schema change in @modelcontextprotocol/sdk will compile silently and crash at runtime. Map content blocks to the SDK's expected type, or use a narrower assertion.
P2 — Minor (non-blocking)
11. [Type Safety] Unvalidated JSON.parse with structural cast
ai.ts:492-494 — The sessions file is parsed and cast to Record<string, ProjectSessions> with no schema validation. A corrupt file will be spread into state. Add a shape guard.
12. [Correctness] jsonSchemaToZodShape drops nested object schemas
scene-mcp.ts:549-585 — type: "object" is treated as z.record(z.unknown()), losing any properties/required. The advertised schema is weaker than reality (tolerable since Explorer re-validates, but imprecise).
13. [Performance] Synchronous disk I/O on main process
scene-composite.ts:64-77 uses fs.readFileSync; skills.ts uses synchronous FS throughout. These block the Electron main process during large scene reads or skill cache refreshes. Consider async alternatives.
14. [Code Quality] Duplicated explorer-tool response handling
scene-mcp.ts:524-535 and 596-607 contain near-identical response-mapping logic. Extract a shared formatExplorerResult() helper.
15. [Code Quality] Boilerplate across scene-op tool handlers
scene-mcp.ts:301-455 — Eight tools repeat the same requestSceneOp → check !res.ok → fail() → ok() pattern. A registerSceneOpTool() helper would cut ~100 lines.
16. [Code Quality] Inconsistent error stringification
Mixed use of String(e), e instanceof Error ? e.message : e, and template wrapping across scene-mcp.ts and explorer-gateway.ts. Consolidate into a small errMsg(e) helper.
17. [Correctness] GET/DELETE without session returns plain text, not JSON-RPC
scene-mcp.ts:740 — Returns "Missing or unknown session id" as plain text. Per JSON-RPC 2.0 / MCP spec, this should be a JSON-RPC error envelope.
18. [Cleanup] Stale MCP config file not cleaned on shutdown
scene-mcp.ts:784 — configPath is never cleared by stopSceneMcpServer(), so stale token-containing files accumulate in os.tmpdir().
19. [Code Quality] Magic number for stderr truncation
ai.ts:728 — 8000 should be a named constant like MAX_STDERR_BYTES for consistency with other constants in the file.
20. [Code Quality] userSeq counter outside Redux state
slice.ts:102 — Module-level let userSeq = 0 resets on HMR while the store doesn't, risking id collisions in dev.
Consumer Impact
No breaking changes. The creator-hub package is "private": true. Changes to @dcl/inspector are purely additive (new RPC handlers). New AppSettings fields have defaults. Safe to merge from a compatibility standpoint.
CI Status
✅ lint, typecheck, unit tests, E2E (macOS) — all passing.
⏳ asset-packs/version — pending (non-blocking).
Review Agents Used
- TypeScript conventions reviewer
- Security sentinel
- Architecture strategist
- MCP/Agent-native reviewer
- Patterns & simplicity reviewer
Verdict
Request Changes — The two P0 items (signal-killed turn reported as success; unbounded request body) and the P1 items (stale reads, unvalidated entity casts, no gateway reconnection, session leak, file permissions) should be addressed before merge. The P2 items are non-blocking suggestions for follow-up.
The overall architecture is well-designed and the code quality is high — this is a strong foundation for the AI assistant feature. The fixes needed are localized and straightforward.
Reviewed by Jarvis 🤖 · Requested by Nicolas Echezarreta (<@U04FKDVJ8JW>) via Slack
Test @dcl/inspector package
|
Test this pull request on macos-latestDownload the correct version for your architecture:Click here if you don't know which version to downloadFor running this unsigned version of the app, you will need to run the xattr command on it:
|
Test this pull request on windows-latestDownload the correct version for your architecture: |
2 participants
Plan
Adds an AI assistant to the scene editor. It drives your own installed claude or codex CLI to build scenes for you. It reads the scene, writes SDK7 code, adds and edits entities and smart items, and can run the preview to check its work. It uses your CLI subscription so there is no API key to set up
What you get:
src/Turning it on:
Off by default. Settings > Experimental > AI scene assistant
Notes:
No keys are stored. If you have not signed into a CLI you can opt in to using the API key from your environment (
ANTHROPIC_API_KEY/OPENAI_API_KEY)