feat: Native Model Context Protocol (MCP) Server & Automation Suite - #1154
Conversation
Implements a native Model Context Protocol (MCP) server as a C++ module. - Adds modules/mcp/ with core MCP server implementation - Adds --mcp-server command-line flag - Implements File System tools (read/write, list) - Implements Scene tools (create, tree, modify) - Implements Project tools (info) - Uses JSON-RPC 2.0 over stdio with automatic header suppression
- Added tool_get_game_output, tool_stop_game, and tool_validate_script - Fixed GDScript parser error handling in validate_script - Added redot-mcp.sh Nix wrapper script for easier execution
…uction-Grade features - Merged 21 individual tools into scene_action, resource_action, code_intel, and project_config - Added 'connect' action with automatic script callback generation - Added complex type support for properties (Vector2, Vector3, Color) - Enhanced project_config with Nix-compatible path handling for logs - Integrated GDScript parser for reliable script validation
- Implemented MCPBridge TCP loopback for game-to-server communication - Added redot_game_control master tool (capture, click, type, inspect_live, wait) - Automated bridge setup in run action - Implemented high-performance Base64 viewport capture
This prevents 'not connected' errors when the AI tries to wait for the game process to initialize.
- Moved 'wait' action to server side to prevent catch-22 timeouts - Implemented robust line-delimited message reading with get_partial_data - Fixed Window/Viewport type handling for capture action - Added more diagnostic logging to track bridge state
- Set button_mask for MouseButton events - Use separate down/up events to prevent reference corruption - Injected MouseMotion before click for better hover support - Switched to screen coordinates for Control nodes
…cript - Documented the 5 master controllers: scene_action, resource_action, code_intel, project_config, and game_control - Added standard binary and Nix-specific execution instructions - Generalized paths in redot-mcp.sh to remove hardcoded user directory
WalkthroughAdds a native Model Context Protocol (MCP) module (modules/mcp/) implementing a JSON‑RPC MCPServer over stdio, an MCPBridge TCP bridge, MCPProtocol handler, MCPTools toolset, types/schemas, engine CLI wiring, module registration/build scripts, docs, tests, and a Nix wrapper to enable AI agents to interact with projects. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as AI Agent
participant Server as MCPServer (stdio)
participant Protocol as MCPProtocol
participant Tools as MCPTools
participant Engine as Redot Engine
Agent->>Server: JSON-RPC "initialize"
Server->>Protocol: handle_initialize(params)
Protocol-->>Server: initialize response (capabilities, serverInfo)
Server-->>Agent: initialize response
Agent->>Server: JSON-RPC "tools/list"
Server->>Protocol: handle_tools_list()
Protocol->>Tools: get_tool_definitions()
Tools-->>Protocol: tool schemas
Protocol-->>Server: tools/list response
Server-->>Agent: tools/list response
Agent->>Server: JSON-RPC "tools/call" (e.g., scene_action)
Server->>Protocol: handle_tools_call(params)
Protocol->>Tools: execute_tool(name,args)
Tools->>Engine: perform action (scene/resource/project/game)
Engine-->>Tools: action result
Tools-->>Protocol: tool result
Protocol-->>Server: formatted response
Server-->>Agent: tools/call response
sequenceDiagram
participant Client as Game Client
participant Bridge as MCPBridge
participant ServerIO as MCPServer (stdio)
participant Tools as MCPTools
Client->>Bridge: connect_to_server(host,port)
Bridge->>Bridge: establish TCP connection
Bridge-->>Client: connection OK
Client->>Bridge: send_command("capture", {})
Bridge->>ServerIO: write JSON command + newline
ServerIO->>Tools: Protocol routes capture -> MCPTools
Tools-->>ServerIO: capture response (base64 image)
ServerIO->>Bridge: newline-delimited JSON response
Bridge-->>Client: parsed response (image + metadata)
loop Per frame
Client->>Bridge: update()
Bridge->>Bridge: accept/parse newline-delimited commands
Bridge->>Tools: _process_command(command)
Tools-->>Bridge: command result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
✏️ Tip: You can disable this entire section by setting Comment |
- Implemented proper character-to-Key mapping using find_keycode - Added support for bracketed special keys (e.g., [ESCAPE], [ENTER]) - Added support for modifier combinations (e.g., [CTRL+P]) - Fixed 'unset' keycode issue by setting both keycode and physical_keycode
…icking - Added 'recursive' and 'depth' parameters to inspect_live action - inspect_live now returns global screen_pos and size for Control nodes - click action now uses screen coordinates and includes a 50ms delay for processing - Added filtering for internal nodes (@@) to keep tree dumps clean
- Upgraded code_intel with AST-based symbol extraction and full method signatures - Added inspect_asset action to project_config for reading import metadata - Implemented recursive live scene tree inspection with auto-coordinate mapping - Improved click precision with 50ms physical latency simulation - Added --run-tests CLI flag and headless runner foundation - Updated documentation with new features
- Updated @GlobalScope.xml with MCPBridge and MCPServer singletons - Generated skeleton XML docs for MCPServer, MCPProtocol, and MCPBridge - Fixed config.py to include MCPBridge in documentation classes
- Updated tool descriptions to guide agents toward efficient tool usage (native edit vs mcp write) - Added 'AI Agent Best Practices' section to README.md - Cleaned up mcp_tools.cpp source code
- Renamed read_file/write_file to read_file_res/create_file_res for clarity - Implemented C++ safety lock to prevent overwriting existing .gd files via MCP (directs agent to native edit tool) - Restored and updated README.md with official AI Agent Best Practices
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main/main.cpp (1)
4764-4795: Add null checks and error handling for MCP singletons and their method calls.
MCPBridge::get_singleton()->connect_to_server(...)can dereference null (singleton pointers are nulled on destruction), and itsErrorreturn value is ignored.MCPServer::get_singleton()->start()also lacks null guard. The codebase already demonstrates this pattern (mcp_server.cpp lines 87–89 null-checks MCPBridge::get_singleton()). Guard both calls and log failures to stderr for headless debugging.
🤖 Fix all issues with AI agents
In @main/main.cpp:
- Around line 1429-1436: The mcp_bridge_port assignment currently accepts any
integer from N->get().to_int(); validate the parsed value after reading it
(e.g., int port = N->get().to_int()) and enforce the allowed range (use
1..65535, or allow 0 only if you intend it to mean "disabled"); if the value is
out of range call OS::get_singleton()->print with a clear "Invalid port" message
and goto error instead of assigning to mcp_bridge_port; update the branch
handling "--mcp-bridge-port" accordingly to perform this range check before
setting mcp_bridge_port.
- Around line 618-623: The help text calls to print_help_option are using
escaped backslash-n sequences ("\\n") which print a literal \n; update the
string literals passed to print_help_option (e.g., the "--headless",
"--mcp-server", "--mcp-bridge-port <port>", and "--run-tests <path>" calls) to
use actual newline characters ("\n") instead of "\\n" so the help output renders
newlines correctly; leave the CLI_OPTION_AVAILABILITY_EDITOR arguments
unchanged.
In @modules/mcp/doc_classes/MCPBridge.xml:
- Around line 1-16: The MCPBridge class XML has empty documentation blocks; add
meaningful content to the class and its update() method: explain MCPBridge's
role managing TCP loopback IPC between the headless server and active game
process, its place in the MCP architecture, when it is instantiated, and typical
lifecycle; document that update() is called per-frame (or each tick) to process
incoming/outgoing messages and maintain the IPC state, describe expected call
frequency, threading/async considerations, error handling behavior, and provide
a short usage example/snippet showing instantiation and per-frame invocation.
Include these descriptions in the <brief_description>, <description>, and
<methods><method name="update"> description elements in MCPBridge.xml and ensure
the documentation mentions MCPBridge and update() by name so readers can locate
behavior and usage.
In @modules/mcp/doc_classes/MCPProtocol.xml:
- Around line 2-15: The MCPProtocol XML docs are empty and need meaningful
descriptions; update the <brief_description> and <description> for the class
MCPProtocol to explain its purpose and lifecycle (e.g., role in JSON-RPC
handshake, when the protocol instance is valid), and document invariants and
semantics of the method is_initialized() (e.g., returns true only after
handshake/initialization completes, whether it can flip back to false,
thread-safety/visibility guarantees). In the <method name="is_initialized"> add
a clear <description> stating what “initialized” means, any side-effects (none),
conditions under which it becomes true/false, and any caller expectations or
error conditions so the API page renders with useful guidance.
In @modules/mcp/doc_classes/MCPServer.xml:
- Around line 1-26: Fill in the empty XML doc blocks for class MCPServer and its
methods: add a brief_description and detailed description for class MCPServer
explaining it implements the JSON-RPC 2.0 stdio transport for the MCP protocol
and is enabled via the CLI flag --mcp-server; document lifecycle and usage
patterns including when to instantiate and call start() and stop(), describe
start() as initializing stdio JSON-RPC I/O and registering handlers, describe
stop() as cleanly shutting down I/O and handlers, and describe is_running()
return semantics (returns true when the server is actively processing requests
after start() and false after stop() or on failure), and include a short usage
example showing CLI activation and typical start/stop sequence.
In @modules/mcp/mcp_bridge.cpp:
- Around line 198-206: The code assumes vp->get_texture() is non-null before
calling get_image(), which can crash; update the block around vp =
st->get_root() to fetch Texture *tex = vp->get_texture() and guard: if tex is
null, handle gracefully (e.g., log/warn and return/skip exporting) before
calling tex->get_image(); then use img = tex->get_image() and proceed with
existing scale logic and img->save_png_to_buffer(). Ensure any early-return
preserves function invariants and avoids dereferencing a null texture.
- Around line 354-384: There is a duplicate "inspect_live" handler block causing
dead code; remove the second branch (the else if (action == "inspect_live")
block that builds info/children/props using Node, get_property_list and
PROPERTY_USAGE_EDITOR) or merge its property-dumping logic into the first
inspect_live handler: consolidate gathering of name, type, children and the
properties Dictionary (iterate plist from node->get_property_list and include
only p.usage & PROPERTY_USAGE_EDITOR) into the original handler that checks
action == "inspect_live", then delete the redundant block so only one
inspect_live implementation remains.
In @modules/mcp/mcp_protocol.cpp:
- Around line 96-134: _tools/list and _tools/call handlers (_handle_tools_list
and _handle_tools_call) do not verify that the protocol is initialized, allowing
calls before _handle_initialize sets the initialized flag; add a guard at the
start of both _handle_tools_list and _handle_tools_call that checks the existing
initialized boolean (the same flag set in _handle_initialize) and return a
protocol-level error (e.g. using make_response_error with an INVALID_STATE or
similar error code and a message like "Protocol not initialized") if not
initialized, so tools/list and tools/call are rejected until initialize
completes.
In @modules/mcp/mcp_server.cpp:
- Around line 68-76: The server blocks in _read_line() on std::getline, so make
stdin reads asynchronous: spawn an input thread (e.g., input_thread created in
the constructor and joined in the destructor/stop) that loops calling
std::getline and pushes each line into a thread-safe queue (input_queue)
protected by input_mutex and signaled with input_cv; change _read_line() to pop
from that queue (waiting on input_cv with a timeout or until should_stop is
true) so stop() can set should_stop, notify input_cv, and join input_thread to
allow clean shutdown; add input_thread, input_queue, input_mutex, and input_cv
fields and ensure stop() notifies the condition variable and joins the thread.
- Around line 84-136: The code has a data race on should_stop and running and
unlawfully calls engine APIs from the bridge thread via MCPBridge::update; make
should_stop and running std::atomic<bool> (update all uses in
_bridge_thread_func and _server_loop) and remove any direct engine work from the
bridge thread: change _bridge_thread_func to perform only network I/O and push
work items into a thread-safe command queue (e.g., lock-protected std::deque or
concurrent queue) instead of calling MCPBridge::update; add queue processing
inside MCPServer::_server_loop (main thread) to pop and execute those commands
so all SceneTree/Window/Viewport/Node/Input operations occur on the main thread,
and ensure bridge_thread.wait_to_finish() still joins cleanly when should_stop
becomes true.
- Around line 154-197: In MCPServer::run_tests, objects created via
ClassDB::instantiate (obj) that inherit RefCounted are never unreferenced,
causing leaks; change the cleanup to wrap RefCounted instances in a
Ref<RefCounted> so their refcount is decremented automatically (e.g., create a
Ref<RefCounted> ref = Object::cast_to<RefCounted>(obj) when cast succeeds), and
only call memdelete(obj) for non-RefCounted objects; ensure you keep using obj
for set_script/call but replace the current raw-only cleanup branch with this
Ref wrapping to let RAII drop the reference.
In @modules/mcp/mcp_server.h:
- Around line 47-53: Replace the plain bool flags used for cross-thread
coordination with an atomic or engine-safe primitive: change the members running
and should_stop in the MCP server class to std::atomic<bool> (or the project's
SafeFlag equivalent) and update all reads/writes (including places referenced by
_bridge_thread_func and the bridge_thread lifecycle code) to use atomic access
methods; do the same for the duplicate flags at the other location (the members
noted around lines 78–83) so all thread-shared flag accesses are synchronized
and race-free.
- Around line 72-77: The stop() method cannot interrupt a blocking
std::getline(std::cin, line) call inside start()/_read_line(), so stop() only
flips the flag and won’t return until getline unblocks; fix by moving stdin
reading into a dedicated thread (e.g., start() spawns a reader thread that runs
_read_line()) and use non-blocking/interruptible I/O or signal the thread to
join on stop(), or document that stop() only takes effect after EOF;
specifically modify start() to create a reader thread that reads lines and
checks the stop flag, make _read_line() operate in that thread (or use
select/poll on stdin or platform-specific non-blocking reads) and have stop()
set the flag and join the reader thread to ensure prompt shutdown.
In @modules/mcp/mcp_tools.cpp:
- Around line 425-433: The code assumes sub->instantiate() always returns a
valid Node; add a null-check after Node *instance = sub->instantiate() and
handle the failure path: if instance is null, log or append an error to result
(e.g., result.add_error or add_text with an indicative message), avoid calling
instance->set_name, parent->add_child, instance->set_owner, or setting
should_save, and return or continue appropriately; only perform the set_name,
add_child, set_owner, should_save=true, and result.add_text("Instanced '" +
instance_path + "'") when instance is non-null.
- Around line 456-464: The connect call should first verify the signal exists
and avoid duplicate handlers: before calling source->connect(sig,
Callable(target, method)) use source->has_signal(sig) to fail early (set an
error/result message and do not attempt to connect) and use
source->is_connected(sig, target, method) to skip connecting if already
connected (set a "Already connected" result message instead); keep the existing
_ensure_callback_exists(...) call and update result.add_text / result error text
accordingly so you don't perform or log redundant/invalid connections.
In @modules/mcp/mcp_tools.h:
- Around line 99-101: The three static variables last_game_pid, last_log_path,
and bridge_port are unsynchronized global mutable state; either protect them
with a Mutex (or std::atomic where appropriate) and guard every access
(reads/writes) in tool_game_control() and any callers (e.g.,
MCPServer::_server_loop()) with that mutex, or remove the statics and make them
instance members of the class that owns tool_game_control() (so ownership is
enforced at compile-time) and update all references to use the instance fields;
pick one approach and apply it consistently across declarations and all uses of
last_game_pid, last_log_path, and bridge_port.
In @modules/mcp/register_types.cpp:
- Around line 45-76: The uninitialize_mcp_module function deletes
mcp_bridge_singleton and mcp_server_singleton but fails to remove their Engine
singletons, leaving dangling references; update uninitialize_mcp_module to, for
each singleton (MCPBridge and MCPServer), call
Engine::get_singleton()->remove_singleton("MCPBridge") and
Engine::get_singleton()->remove_singleton("MCPServer") before memdelete,
guarding with the existing null checks on mcp_bridge_singleton and
mcp_server_singleton so removal happens only when the instance exists.
🧹 Nitpick comments (11)
README.md (1)
112-117: Clarify test runner expectations.The note states "Your test script should have a
func run():method" but doesn't specify:
- Return type (void? bool? Variant?)
- Expected behavior on success vs. failure (exit codes, assertions, logging?)
- Whether this is the only required method or if there are setup/teardown hooks
Consider adding an example test snippet for clarity.
redot-mcp.sh (2)
4-5: Consider making the binary path more flexible.The binary path is hardcoded for Linux x86_64 (
redot.linuxbsd.editor.x86_64). This limits portability to other platforms or architectures. Consider:
- Auto-detecting the binary based on platform/architecture
- Allowing override via environment variable
- Checking if the binary exists before execution
♻️ Suggested improvement
ENGINE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -BINARY="$ENGINE_DIR/bin/redot.linuxbsd.editor.x86_64" +BINARY="${REDOT_BINARY:-$ENGINE_DIR/bin/redot.linuxbsd.editor.x86_64}" # Project path should be provided as an argument PROJECT_PATH="$1" if [ -z "$PROJECT_PATH" ]; then echo "Usage: $0 <path_to_project>" exit 1 fi +if [ ! -f "$BINARY" ]; then + echo "Error: Binary not found at $BINARY" + exit 1 +fi + nix develop "$ENGINE_DIR" -c "$BINARY" --headless --mcp-server --path "$PROJECT_PATH"
14-14: Add error handling for nix develop execution.The script doesn't check if
nix developsucceeds or if Nix is available. Consider adding a check or letting the shell's error handling catch failures.♻️ Optional improvement
+set -e # Exit on error + nix develop "$ENGINE_DIR" -c "$BINARY" --headless --mcp-server --path "$PROJECT_PATH"Or with explicit error handling:
if ! command -v nix &> /dev/null; then echo "Error: nix command not found. Please install Nix." exit 1 fimodules/mcp/SCsub (1)
1-8: SCons glob may accidentally compile non-module sources (future footgun)If you later add helper CLIs/tests under
modules/mcp/, they’ll get pulled in automatically by"*.cpp". Consider listing sources explicitly once the module stabilizes.modules/mcp/mcp_bridge.h (1)
73-76: Clarify threading expectations forsend_command()(blocking).Given this API is likely used both in the server loop and during runtime, document whether
send_command()may block and whether it’s safe to call from the main thread.modules/mcp/mcp_types.h (2)
38-42: Preferconstexprconstants over#definein headers.This avoids macro leakage and gives type safety.
Proposed fix
-#define MCP_PROTOCOL_VERSION "2024-11-05" -#define MCP_SERVER_NAME "redot-mcp" -#define MCP_SERVER_VERSION "1.0.0" +static constexpr const char *MCP_PROTOCOL_VERSION = "2024-11-05"; +static constexpr const char *MCP_SERVER_NAME = "redot-mcp"; +static constexpr const char *MCP_SERVER_VERSION = "1.0.0";
56-68: Omit emptymimeTypefor image content (consistency with resource).
MCPImageContent::to_dict()always setsmimeType; consider only including it when non-empty (likeMCPResourceContent).modules/mcp/mcp_bridge.cpp (2)
119-137: Blocking read may freeze the caller.
send_commandperforms a blocking wait (up to 5 seconds) which could freeze the main thread if called during frame processing. This is acceptable for MCP server usage where it runs headless, but document this constraint or consider making it async if used elsewhere.
249-250: Synchronous delay in input handling.The 50ms delay (
delay_usec(50000)) ensures the engine processes the mouse-down state before release. While functional, this blocks the game thread. For real-time games, consider using a deferred callback or timer if precise timing is critical.modules/mcp/mcp_tools.cpp (2)
90-92:contains()check may produce false positives.Using
content.contains("func " + p_callback_name)could match commented-out functions or functions with similar prefixes. Consider using a regex or more precise pattern like"\nfunc " + p_callback_name + "("to reduce false matches.
435-446: Consider validating property existence before setting.Calling
target->set(property, value)on a non-existent property silently fails. Consider checking if the property exists for clearer error messages.Optional enhancement
} else if (action == "set_prop") { String node_path = p_args.get("node_path", "."); String property = p_args.get("property", ""); Variant value = _json_to_variant(p_args.get("value", Variant())); Node *target = (node_path == "." || node_path.is_empty()) ? root : root->get_node_or_null(node_path); if (!target) { result.set_error("Node not found"); + } else if (!target->has_method("set") || !ClassDB::has_property(target->get_class(), property)) { + // Note: This is a simplified check; full validation would need PropertyInfo lookup + result.set_error("Property '" + property + "' may not exist on node"); } else {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
README.mddoc/classes/@GlobalScope.xmlmain/main.cppmodules/mcp/SCsubmodules/mcp/config.pymodules/mcp/doc_classes/MCPBridge.xmlmodules/mcp/doc_classes/MCPProtocol.xmlmodules/mcp/doc_classes/MCPServer.xmlmodules/mcp/mcp_bridge.cppmodules/mcp/mcp_bridge.hmodules/mcp/mcp_protocol.cppmodules/mcp/mcp_protocol.hmodules/mcp/mcp_server.cppmodules/mcp/mcp_server.hmodules/mcp/mcp_tools.cppmodules/mcp/mcp_tools.hmodules/mcp/mcp_types.hmodules/mcp/register_types.cppmodules/mcp/register_types.hredot-mcp.sh
🧰 Additional context used
🧬 Code graph analysis (8)
modules/mcp/register_types.h (1)
modules/mcp/register_types.cpp (4)
initialize_mcp_module(45-61)initialize_mcp_module(45-45)uninitialize_mcp_module(63-77)uninitialize_mcp_module(63-63)
modules/mcp/mcp_server.h (1)
modules/mcp/mcp_server.cpp (18)
MCPServer(48-51)MCPServer(53-60)_bridge_thread_func(84-92)_bridge_thread_func(84-84)_server_loop(94-136)_server_loop(94-94)_read_line(68-76)_read_line(68-68)_write_line(78-82)_write_line(78-78)_bind_methods(62-66)_bind_methods(62-62)start(138-144)start(138-138)stop(146-152)stop(146-146)run_tests(154-197)run_tests(154-154)
modules/mcp/mcp_bridge.h (1)
modules/mcp/mcp_bridge.cpp (16)
MCPBridge(53-56)MCPBridge(58-60)_process_command(188-387)_process_command(188-188)_bind_methods(62-64)_bind_methods(62-62)start_server(66-85)start_server(66-66)is_client_connected(87-93)is_client_connected(87-87)connect_to_server(95-101)connect_to_server(95-95)send_command(103-153)send_command(103-103)update(155-186)update(155-155)
modules/mcp/mcp_bridge.cpp (1)
modules/mcp/mcp_bridge.h (1)
MCPBridge(40-60)
modules/mcp/mcp_server.cpp (4)
modules/mcp/mcp_server.h (1)
MCPServer(41-67)modules/mcp/mcp_protocol.cpp (4)
MCPProtocol(39-48)MCPProtocol(50-55)_bind_methods(57-65)_bind_methods(57-57)modules/mcp/mcp_protocol.h (1)
MCPProtocol(43-74)modules/mcp/mcp_bridge.cpp (2)
_bind_methods(62-64)_bind_methods(62-62)
main/main.cpp (1)
modules/mcp/mcp_server.cpp (2)
run_tests(154-197)run_tests(154-154)
modules/mcp/mcp_protocol.cpp (3)
modules/mcp/mcp_protocol.h (2)
MCPProtocol(43-74)_make_capabilities(58-73)modules/mcp/mcp_tools.cpp (6)
MCPTools(108-109)MCPTools(111-112)_bind_methods(114-116)_bind_methods(114-114)get_tool_definitions(149-259)get_tool_definitions(149-149)modules/mcp/mcp_tools.h (1)
MCPTools(43-65)
modules/mcp/mcp_tools.cpp (2)
modules/mcp/mcp_tools.h (1)
MCPTools(43-65)modules/mcp/mcp_types.h (3)
make_string_property(120-125)make_object_property(134-139)make_boolean_property(127-132)
🪛 Clang (14.0.6)
modules/mcp/register_types.h
[error] 35-35: 'modules/register_module_types.h' file not found
(clang-diagnostic-error)
modules/mcp/mcp_bridge.h
[error] 35-35: 'core/io/stream_peer_tcp.h' file not found
(clang-diagnostic-error)
modules/mcp/mcp_types.h
[error] 35-35: 'core/variant/dictionary.h' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.10)
modules/mcp/config.py
1-1: Unused function argument: env
(ARG001)
1-1: Unused function argument: platform
(ARG001)
🔇 Additional comments (21)
README.md (3)
78-90: ✅ Clear tool overview with specific capabilities.The master controllers are well-described with concrete examples (
.tscn,.tres, GDScript validation, Input Map, UI clicking). Good foundation for users to understand MCP capabilities.
137-145: Strong best practices section with clear safety guardrails.Lines 140-146 effectively communicate the restriction on
create_file_resto prevent accidental file overwrites—this is critical for protecting user code. The guidance on wait times (3-5s for bridge initialization) and tool-specific workflows is practical and should help agents avoid common pitfalls.
94-110: The configuration flags are correct; clarify that--mcp-bridge-portis internal.The flags shown (
--headless,--mcp-server,--path) are accurate for MCP client configuration. The--mcp-bridge-portmentioned in the PR is an internal implementation flag used by the engine when spawning the MCP server—it's not configured by users in their MCP client settings. The example is complete as-is for typical usage.modules/mcp/config.py (2)
1-6: Static analysis flags are likely false positives - module convention.Ruff flags
envandplatformparameters as unused. These are part of the SCons module configuration API contract. The signatures must match the expected interface even if this particular module doesn't use all parameters.
9-18: Documentation class list is correct.The
get_doc_classes()correctly lists the three documentation classes added in this PR: MCPServer, MCPProtocol, and MCPBridge.doc/classes/@GlobalScope.xml (1)
1693-1696: LGTM - Singleton registration follows established pattern.The MCPBridge and MCPServer entries correctly follow the GlobalScope singleton pattern used by other engine singletons (AudioServer, Engine, etc.). Empty setter/getter attributes are appropriate for read-only singleton access.
modules/mcp/register_types.h (1)
33-38: No action needed. The include path#include "modules/register_module_types.h"is correct and canonical—all 50+ modules in the repository use this identical include pattern forModuleInitializationLevel. The file exists atmodules/register_module_types.h. The Clang "not found" error is a compilation or clangd configuration issue, not a code problem.Likely an incorrect or invalid review comment.
modules/mcp/mcp_server.cpp (1)
62-66: No binding needed —run_tests()is CLI-only
run_tests()is invoked exclusively from the command-line handler inmain.cppvia the--run-tests=argument and is not intended for script access. No binding is required.Likely an incorrect or invalid review comment.
main/main.cpp (1)
4833-4837: Per-frameMCPBridge::update()hook looks appropriate.Good placement (early in iteration) and null-guarded.
modules/mcp/mcp_bridge.h (1)
35-37: The include paths are correct and all files exist in the repository. No build break issue with these includes.Likely an incorrect or invalid review comment.
modules/mcp/mcp_types.h (1)
35-42: Include paths are correct and verified in the codebase.Both
core/variant/dictionary.handcore/variant/variant.hexist at the specified paths and are used consistently throughout the repository (e.g., audio_server.h, camera_server.h, text_server.cpp, physics extensions). The static analysis error reporting these headers as missing appears to be a false positive or tool misconfiguration issue. No build break will occur from these includes.Likely an incorrect or invalid review comment.
modules/mcp/mcp_tools.h (1)
69-73: Security measures are already implemented in the path utilities.The
validate_path()function already explicitly prevents path traversal by checkingif (normalized.contains("..") || !normalized.begins_with("res://")), which rejects any path containing..sequences and requires all paths to be constrained to theres://root. Similarly,normalize_path()forces all paths into theres://namespace before any file operations occur. These utilities are already functioning as security boundaries.Likely an incorrect or invalid review comment.
modules/mcp/mcp_bridge.cpp (3)
53-60: LGTM on singleton lifecycle.Constructor and destructor properly manage the singleton pointer and instantiate the server reference.
66-85: LGTM on port binding logic.The automatic port selection with fallback range and explicit port binding are well-handled.
310-353: LGTM on keyboard input simulation.The
[Token]syntax for special keys and modifier handling is well-implemented. The fallback to uppercase for regular characters is appropriate.modules/mcp/mcp_tools.cpp (6)
133-139: LGTM on path validation.The
validate_pathfunction correctly rejects path traversal attempts (..) and ensures paths are withinres://. Good security practice.
517-531: LGTM on resource creation.Proper null checking and cleanup after
ClassDB::instantiatefailure.
688-714: LGTM on game process lifecycle.The run action properly manages game process lifecycle: kills previous process, starts bridge server, and spawns new process with appropriate arguments.
741-758: LGTM on file creation with .gd safety lock.The safety check preventing overwrites of existing
.gdfiles (line 743) aligns with the PR's stated safety goals. Directory creation is handled properly.
793-824: LGTM on game control delegation.The
waitaction is correctly handled locally, and other actions are properly delegated toMCPBridgewith appropriate response handling.
624-673: LGTM on GDScript parsing integration.Proper use of conditional compilation with
MODULE_GDSCRIPT_ENABLEDand clean error reporting from parser errors.
- Added null checks for singletons in main.cpp - Validated --mcp-bridge-port range - Fixed memory leaks in MCPServer::run_tests using proper RefCounted handling - Correctly removed singletons from Engine in uninitialize_mcp_module - Fixed \n escapes in help text - Updated documentation with --doctool
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @main/main.cpp:
- Line 618: The help string passed to print_help_option for "--headless"
contains an escaped backslash before the n ("\\n") causing a literal "\n" in
output; change that literal to a real newline escape ("\n") so the string ends
with an actual newline like the other help options (update the print_help_option
call for "--headless" accordingly).
In @modules/mcp/mcp_server.cpp:
- Line 74: The shared flags `should_stop` and `running` are accessed from
multiple threads without synchronization; change their declarations in the
MCPServer class to `std::atomic<bool>` (add #include <atomic>), initialize them
in the constructor, and replace direct reads/writes in functions/methods like
the server loop, `start()`, `stop()`, `is_running()`, and the bridge thread with
atomic operations (use `.store()` to set and `.load()` to read or rely on atomic
bool's safe implicit conversions) to eliminate data races on `should_stop` and
`running`.
🧹 Nitpick comments (1)
modules/mcp/mcp_server.cpp (1)
68-76: Consider distinguishing EOF from stream errors.The current implementation treats all
getlinefailures the same. For better diagnostics, you could checkstd::cin.eof()vsstd::cin.fail()to distinguish clean shutdown from I/O errors.📝 Optional improvement for error distinction
String MCPServer::_read_line() { std::string line; if (std::getline(std::cin, line)) { return String::utf8(line.c_str()); } - // EOF or error + if (std::cin.eof()) { + fprintf(stderr, "[MCP] EOF reached on stdin\n"); + } else { + fprintf(stderr, "[MCP] Error reading from stdin\n"); + } should_stop = true; return String(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
main/main.cppmodules/mcp/mcp_server.cppmodules/mcp/register_types.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/mcp/register_types.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
main/main.cpp (1)
modules/mcp/mcp_server.cpp (2)
run_tests(154-198)run_tests(154-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🍏 iOS / Template (target=template_release)
🔇 Additional comments (12)
main/main.cpp (9)
154-158: LGTM!The conditional includes follow the established pattern for module-guarded headers in this file.
1424-1443: Command-line parsing for--mcp-serverand--mcp-bridge-portlooks correct.Port validation properly checks the range 1-65535. Error handling follows the established pattern with appropriate error messages.
However, as noted earlier, the
--run-testsoption documented in help (line 622) is missing its parsing logic here.
2191-2195: LGTM!Correctly allows MCP server mode to bypass the "no main scene" check, which is appropriate for headless server operation.
2989-2992: LGTM!Correctly suppresses the engine header in MCP server mode to maintain clean JSON-RPC output on stdio.
4276-4280: LGTM!Consistent with the earlier change at lines 2191-2195, correctly allowing MCP server mode as a valid standalone operation.
4768-4776: LGTM!Good null check for the singleton before use. The loopback-only connection ("127.0.0.1") is appropriate for internal MCP Bridge communication as described in the PR objectives.
4846-4850: LGTM!Proper null check before calling
update(). Placing this at the start of the iteration loop ensures MCP commands are processed before each frame, which is appropriate for real-time interaction.
297-301:mcp_run_testsis declared but never assigned in command-line parsing.The static variable
mcp_run_testsis declared at lines 297–301 and used at line 4801, but I don't see any command-line parsing that sets this variable. The--run-testsoption is documented in help (line 622), but the actual parsing logic to setmcp_run_testsfrom the command line appears to be missing.
4801-4807: No issue found. Therun_testsmethod is declared asstatic void run_tests(const String &p_script_path)in the MCPServer header file, making the callMCPServer::run_tests(mcp_run_tests)at line 4805 correct. A null check is not required since this is a static method call, not an instance method call.Likely an incorrect or invalid review comment.
modules/mcp/mcp_server.cpp (3)
46-60: LGTM: Proper singleton initialization and cleanup.The singleton pattern is correctly implemented with proper resource cleanup in the destructor. The order of operations (stop → delete protocol → null singleton) is appropriate.
194-197: LGTM: Proper memory management for RefCounted vs non-RefCounted objects.The code correctly handles the dual nature of Godot/Redot objects. RefCounted objects manage their own lifetime through reference counting, while non-RefCounted objects require explicit deletion. This addresses the memory leak mentioned in the commit messages.
90-90: No changes needed.OS::get_singleton()is a core singleton guaranteed to be available during normal execution and does not require a null check. The codebase consistently treats it as safe to call without defensive checks, reserving null checks only for optional/pluggable singletons likeMCPBridge(which is correctly checked on line 87).Likely an incorrect or invalid review comment.
- Fixed missing includes in mcp_types.h (ustring.h) - Implemented atomic flags in MCPServer for thread safety - Added is_initialized check in MCPProtocol - Added texture null check in MCPBridge capture - Removed duplicate inspect_live block in MCPBridge
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @modules/mcp/mcp_server.h:
- Around line 43-51: The start() method has a check-then-act race: replace the
non-atomic if (running) return; with an atomic compare-and-swap that sets
running from false to true (e.g., use running.compare_exchange_strong(expected,
true)) so only one thread proceeds to call _server_loop(); after this change
remove the running = true/false assignments from _server_loop() so running's
state is managed solely by the atomic CAS in start(); keep should_stop as-is for
shutdown coordination and ensure _server_loop() still respects should_stop.
- Around line 71-72: Destructor MCPServer::~MCPServer currently calls stop()
(which only sets should_stop) and then memdelete(protocol) while _server_loop()
may still be running on another thread and calling
protocol->process_string(line), causing use-after-free; make the destructor wait
for the server loop to fully exit before deleting protocol by either turning
stop() into a blocking shutdown (spin-wait on running until false) or adding a
new wait_for_stop() that loops while running (using
OS::get_singleton()->delay_usec) and calling that from the destructor after
stop(), then only memdelete(protocol) once running is false.
In @modules/mcp/mcp_types.h:
- Around line 72-92: The MCPResourceContent::to_dict implementation is using a
nested "resource" object and includes a top-level "type" field which violates
the MCP spec; update MCPResourceContent::to_dict to emit uri, mimeType (use
mime_type), and text as top-level keys (only include mimeType/text when
non-empty) and remove the "type" key so the produced Dictionary is directly {
"uri": ..., "mimeType": ..., "text": ... } to match the MCPResourceContent
shape.
- Around line 40-43: Update the MCP protocol version constant
MCP_PROTOCOL_VERSION to the latest spec string "2025-11-25"; locate the static
constexpr const char *MCP_PROTOCOL_VERSION definition in mcp_types.h and replace
the existing value "2024-11-05" with "2025-11-25" so the binary and any protocol
negotiation use the current MCP specification.
🧹 Nitpick comments (1)
modules/mcp/mcp_server.h (1)
39-39: Possibly unused include.The include
"core/os/thread_safe.h"doesn't appear to be used since the class relies onstd::atomicdirectly for thread safety. Consider removing it if thread-safety macros from that header aren't actually needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/mcp/mcp_protocol.cppmodules/mcp/mcp_server.hmodules/mcp/mcp_types.hredot-mcp.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/mcp/mcp_protocol.cpp
- redot-mcp.sh
🧰 Additional context used
🪛 Clang (14.0.6)
modules/mcp/mcp_types.h
[error] 35-35: 'core/string/ustring.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
…sion - Updated MCP protocol version to 2025-11-25 - Fixed MCPResourceContent spec compliance (flattened structure) - Implemented atomic CAS in MCPServer::start() to prevent race conditions - Made MCPServer destructor block until server loop fully exits to prevent use-after-free
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @modules/mcp/mcp_server.cpp:
- Around line 127-128: The code calls protocol->process_string(line) without
ensuring protocol is still valid, risking a use-after-free if the destructor
runs concurrently; fix by guarding access to the protocol pointer: either
acquire the mutex (the same lock used when destroying/resetting protocol) and
copy the protocol pointer to a local variable or check protocol != nullptr
immediately before calling process_string, and skip processing when it is null;
ensure the null-check and call to process_string are done while holding the
synchronization primitive or on the safely-copied local pointer so protocol
cannot be deleted between the check and the call.
- Around line 53-66: The destructor MCPServer::~MCPServer currently busy-waits
on the running flag with an unbounded loop; modify it to wait with a bounded
timeout by looping until running is false or a max-wait period elapses (e.g.,
track elapsed time or loop counter and call
OS::get_singleton()->delay_usec(1000) per iteration), and if the timeout is
reached, break the loop and proceed to clean up (optionally log a warning)
before deleting protocol and clearing singleton; keep the existing stop() call
first and ensure protocol is memdeleted only once.
🧹 Nitpick comments (4)
modules/mcp/mcp_server.cpp (2)
74-88: Consider using consistent I/O approach (either C++ streams or C stdio, not both).The code mixes C++ iostream (
std::cinin_read_line) with C stdio (fprintfin_write_line). While this works for separate input/output, it's not idiomatic and could cause subtle buffering issues if other code uses C stdin/stdout functions.♻️ Option 1: Use C stdio consistently
String MCPServer::_read_line() { - std::string line; - if (std::getline(std::cin, line)) { - return String::utf8(line.c_str()); + char buffer[4096]; + if (fgets(buffer, sizeof(buffer), stdin)) { + String line = String::utf8(buffer); + return line.strip_edges(); // Remove trailing newline } // EOF or error should_stop = true;♻️ Option 2: Use C++ streams consistently
void MCPServer::_write_line(const String &p_line) { CharString utf8 = p_line.utf8(); - fprintf(stdout, "%s\n", utf8.get_data()); - fflush(stdout); + std::cout << utf8.get_data() << '\n' << std::flush; }
154-160: Consider makingstop()block until shutdown completes, or document async behavior clearly.The
stop()method only sets a flag and returns immediately, giving callers no way to confirm the server has actually stopped. This could lead to race conditions if callers assume synchronous behavior or need to sequence operations after shutdown.♻️ Option 1: Make stop() synchronous (recommended)
void MCPServer::stop() { if (!running) { return; } should_stop = true; + + // Wait for server loop to complete (with timeout) + const uint64_t timeout_usec = 5000000; // 5 seconds + uint64_t elapsed = 0; + while (running && elapsed < timeout_usec) { + OS::get_singleton()->delay_usec(1000); + elapsed += 1000; + } }Option 2: If async behavior is intentional, add a method like
wait_for_stop()and document the async nature in the header file.modules/mcp/mcp_types.h (2)
45-90: LGTM! Optional field handling in MCPResourceContent is well done.The content structs provide clean serialization to Dictionary format. The conditional inclusion of
mimeTypeandtextfields inMCPResourceContent::to_dict()(lines 82-87) correctly handles optional fields per MCP spec.Optional enhancement: Consider adding explicit constructors for better initialization ergonomics:
♻️ Example constructor
struct MCPTextContent { String type = "text"; String text; MCPTextContent() = default; explicit MCPTextContent(const String &p_text) : text(p_text) {} Dictionary to_dict() const { // existing implementation } };
107-150: Consider using namespace instead of class with only static methods.
MCPSchemaBuildercontains only static methods with no state. While this works, a namespace would be more idiomatic in C++ and makes the intent clearer.♻️ Proposed refactor to namespace
-// Helper for building JSON Schema -class MCPSchemaBuilder { -public: +// Helper for building JSON Schema +namespace MCPSchemaBuilder { static Dictionary make_object_schema(const Dictionary &properties, const Array &required = Array()) { Dictionary schema; schema["type"] = "object"; // ... rest of implementation } // ... other methods -}; +} // namespace MCPSchemaBuilderOr keep as
classand explicitly delete constructor:class MCPSchemaBuilder { public: MCPSchemaBuilder() = delete; // ... static methods };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/mcp/mcp_server.cppmodules/mcp/mcp_types.h
🧰 Additional context used
🧬 Code graph analysis (1)
modules/mcp/mcp_server.cpp (4)
modules/mcp/mcp_server.h (1)
MCPServer(43-69)modules/mcp/mcp_protocol.cpp (4)
MCPProtocol(39-48)MCPProtocol(50-55)_bind_methods(57-65)_bind_methods(57-57)modules/mcp/mcp_protocol.h (1)
MCPProtocol(43-74)modules/mcp/mcp_bridge.cpp (2)
_bind_methods(62-64)_bind_methods(62-62)
🪛 Clang (14.0.6)
modules/mcp/mcp_types.h
[error] 35-35: 'core/string/ustring.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🍏 iOS / Template (target=template_release)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
🔇 Additional comments (3)
modules/mcp/mcp_server.cpp (2)
162-206: LGTM!The test runner implementation properly handles resource loading, script instantiation, method invocation, and cleanup. Error reporting to stderr is appropriate for headless operation, and the RefCounted check prevents double-free.
90-98: Verify thread safety: bridge thread may access destroyed MCPServer.The bridge thread accesses
ms->should_stopin a loop. If the MCPServer destructor completes and frees the object while this thread is still executing, this becomes a use-after-free. The destructor's busy-wait onrunningaims to prevent this, but it checksrunning(set by the main server loop), not the bridge thread's lifecycle.The destructor waits for
runningto become false (line 57-59), which is controlled by_server_loop(lines 150-151). However,_bridge_thread_funccontinues running untilshould_stopis true AND the delay completes. There's a potential race if the destructor proceeds after_server_loopexits but before the bridge thread finishes its final iteration.Suggested fix: Have
_server_loopwait forbridge_threadto finish (line 137 already doesbridge_thread.wait_to_finish()), and ensurerunningis only set to false AFTER that wait completes. Currently this appears correct (line 151 is after line 137), but verify the atomics provide sufficient ordering guarantees.modules/mcp/mcp_types.h (1)
40-43: No action required — protocol version is correct.The MCP protocol version
"2025-11-25"is valid and matches the official Model Context Protocol specification. This is the latest published revision as of late 2025.
- Updated MCP protocol version to 2025-11-25 - Flattened MCPResourceContent to match spec - Replaced busy-wait in MCPServer destructor with bounded timeout - Added protocol null-check in server loop to prevent use-after-free - Note: Kept std::atomic for flags but reverted complex CAS logic to match existing pattern for now
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/mcp/mcp_server.cpp (1)
53-71: Consider replacing busy-wait with a condition variable.The destructor uses a busy-wait loop with 1ms delays to ensure the server loop exits before deleting the protocol. While the 3-second timeout bounds the wait, a condition variable would be more efficient and idiomatic for thread synchronization.
Additionally, the
3000ms timeout could be extracted as a named constant (e.g.,SERVER_SHUTDOWN_TIMEOUT_MS) for maintainability.♻️ Proposed refactoring approach
Replace the busy-wait with a condition variable that the server loop signals when it completes. Example pattern:
// In class definition, add: // std::mutex shutdown_mutex; // std::condition_variable shutdown_cv; // In _server_loop(), before returning: // { // std::unique_lock<std::mutex> lock(shutdown_mutex); // running = false; // } // shutdown_cv.notify_one(); // In destructor, replace lines 57-64: // std::unique_lock<std::mutex> lock(shutdown_mutex); // if (!shutdown_cv.wait_for(lock, std::chrono::seconds(3), [this]() { return !running.load(); })) { // fprintf(stderr, "[MCP] Server shutdown timed out\n"); // }And define the timeout as:
static constexpr uint64_t SERVER_SHUTDOWN_TIMEOUT_MS = 3000;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/mcp/mcp_server.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
modules/mcp/mcp_server.cpp (4)
modules/mcp/mcp_server.h (1)
MCPServer(43-69)modules/mcp/mcp_protocol.cpp (4)
MCPProtocol(39-48)MCPProtocol(50-55)_bind_methods(57-65)_bind_methods(57-57)modules/mcp/mcp_protocol.h (1)
MCPProtocol(43-74)modules/mcp/mcp_bridge.cpp (2)
_bind_methods(62-64)_bind_methods(62-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🍏 iOS / Template (target=template_release)
- GitHub Check: 🍎 macOS / Template (target=template_release)
🔇 Additional comments (7)
modules/mcp/mcp_server.cpp (7)
48-51: LGTM: Clean singleton initialization.The constructor correctly establishes the singleton and creates the protocol instance.
73-77: LGTM: Standard GDScript binding.The method bindings correctly expose the server API to GDScript.
79-93: LGTM: Robust I/O helpers.Both helpers correctly handle character encoding conversion, EOF detection, and stdout flushing for JSON-RPC protocol requirements.
95-103: LGTM: Thread-safe bridge update loop.The bridge thread correctly checks for singleton existence before calling update(), and the 10ms polling interval is appropriate for the bridge coordination pattern.
105-149: LGTM: Well-structured server loop with defensive checks.The server loop correctly:
- Logs to stderr to avoid corrupting JSON-RPC on stdout
- Performs defensive null-check on protocol before use (line 133)
- Handles empty lines and notifications appropriately
- Manages bridge thread lifecycle within the loop scope
151-167: LGTM: Correct atomic lifecycle management.The
start()method uses compare-and-swap to prevent multiple activations, and properly resets therunningflag after the blocking server loop exits. Thestop()method correctly signals shutdown via the atomic flag.
169-213: LGTM: Comprehensive test execution with robust error handling.The
run_tests()method correctly:
- Loads and validates the script resource with error reporting
- Safely instantiates the base type with null-check fallback
- Distinguishes between different call error types for better diagnostics
- Properly manages memory by checking for RefCounted before deletion
The error handling covers all major failure modes for dynamic script execution.
- Implemented _trigger_action_event to simulate physical keys from InputMap - Added 'trigger_action' command to game_control - Implemented smart fallback in 'type' command to trigger actions if text matches - Fixed compilation errors in List assignment and redefinitions
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @modules/mcp/mcp_bridge.cpp:
- Around line 290-291: The blocking call OS::get_singleton()->delay_usec(50000)
in mcp_bridge.cpp causes main-thread freezes; remove this synchronous delay and
replace it with a non-blocking approach similar to the fix for
_trigger_action_event—either drop the wait entirely or schedule a follow-up via
the engine's timer/signal (e.g., use SceneTree/Timer or call_deferred) to handle
post-click processing asynchronously so the main thread is not blocked.
- Around line 158-161: The code currently replaces any existing connection
silently when server->is_connection_available() is true and connection =
server->take_connection() is called; change this to handle an existing active
connection: before calling server->take_connection(), check if the local
connection pointer (connection) is non-null and either 1) refuse the new
connection (call a reject/close on the incoming socket or skip take_connection
and log that a connection is already active), or 2) gracefully teardown the old
connection by invoking its close/shutdown method, draining/clearing any pending
ops, sending a notification log via fprintf(stderr, ...) and then call
server->take_connection() to replace it; update any pending-operations
bookkeeping (queues, timers, callbacks) tied to the old connection to avoid
leaked or lost commands and add clear log messages for both refusal and clean
replacement paths so reconnections are observable.
- Around line 414-443: The block beginning with else if (action ==
"inspect_live") is a duplicate/unreachable handler for the same "inspect_live"
action and should be removed; delete this entire duplicate branch (the code that
obtains SceneTree *st via OS::get_singleton()->get_main_loop(), looks up the
node by path, builds info/children/properties into resp["info"], or sets
resp["error"]) OR, if you intended a non-recursive/lightweight variant, change
the action string to a new unique name (e.g., "inspect_node" or
"inspect_simple") and update any callers, keeping the node lookup and property
filtering (get_property_list and PROPERTY_USAGE_EDITOR) as appropriate.
In @modules/mcp/mcp_tools.cpp:
- Around line 689-715: The code currently calls
OS::get_singleton()->kill(last_game_pid) based only on last_game_pid != 0 which
can kill non-existent or reused PIDs; before calling kill (wherever
last_game_pid is used, e.g., in the "run" branch), call
OS::get_singleton()->is_process_running(last_game_pid) and only call
OS::get_singleton()->kill(last_game_pid) if that returns true, and/or check the
Error return value from kill and handle/log it instead of ignoring it so you
don’t attempt to kill an already-exited process or a reused PID.
- Around line 79-106: The helper MCPTools::_ensure_callback_exists currently
uses a fragile substring search and unconditionally appends code; replace the
substring check with a proper AST check using the existing GDScriptParser to
parse the script and verify a function node named p_callback_name exists (so
comments/strings/disabled code are ignored), and before writing use the parser
to validate the resulting script syntax; make the append behavior opt-in via a
project setting (e.g., mcp/auto_add_callbacks) or a boolean parameter so user
scripts are not silently modified, detect the file's indentation style (tabs vs
spaces) and use that for the stub, and when appending add a clear autogenerated
comment like "# Added by MCP - opt-in: mcp/auto_add_callbacks" to the stub so
users can identify changes.
🧹 Nitpick comments (6)
modules/mcp/mcp_bridge.cpp (4)
70-76: Consider making the port range configurable.The hard-coded port range 10000-11000 might conflict with other services. Consider making this configurable via project settings or at least define as named constants for easier maintenance.
♻️ Suggested improvement
Define constants at the top of the file:
static const int MCP_BRIDGE_PORT_RANGE_START = 10000; static const int MCP_BRIDGE_PORT_RANGE_END = 11000;Then use them in the loop:
- for (int i = 10000; i < 11000; i++) { + for (int i = MCP_BRIDGE_PORT_RANGE_START; i < MCP_BRIDGE_PORT_RANGE_END; i++) {
165-184: Character-by-character string concatenation may be inefficient.Building
partial_dataone character at a time using+=operator (line 181) can be inefficient for large messages due to repeated string reallocations.Consider using a
CharStringorVector<uint8_t>buffer and converting toStringonly when the full message is received (at newline).
73-73: Consider using the engine's logging system.Multiple
fprintf(stderr, ...)calls bypass the engine's logging infrastructure. Consider using engine macros likeprint_line(),print_verbose(), or the error macros for better integration with the engine's log levels and output routing.Example:
print_verbose(vformat("[MCP] Bridge server listening on port %d", port));Also applies to: 82-82, 100-100, 160-160, 234-234
146-149: JSON parsing lacks explicit error handling.
JSON::parse_string()can fail, but the code only checks if the result is a Dictionary type. If parsing fails, it might return a different type, but actual parse errors aren't distinguished from valid non-Dictionary responses.Consider checking for parse errors explicitly:
JSON json; Error parse_err = json.parse(response_str); if (parse_err != OK) { Dictionary err; err["error"] = vformat("JSON parse error at line %d: %s", json.get_error_line(), json.get_error_message()); return err; } Variant res_var = json.get_data();modules/mcp/mcp_tools.cpp (2)
133-139: Consider more robust path traversal checks.The validation only checks for
".."substrings, which might miss encoded or obfuscated path traversal attempts (e.g., URL encoding, redundant slashes).Since this is designed for trusted AI agents rather than untrusted web input, the current check may be sufficient. However, consider using
simplify_path()or canonical path resolution for defense in depth.♻️ Enhanced validation approach
bool MCPTools::validate_path(const String &p_path) { String normalized = normalize_path(p_path); - if (normalized.contains("..") || !normalized.begins_with("res://")) { + String simplified = normalized.simplify_path(); + if (simplified.contains("..") || !simplified.begins_with("res://")) { return false; } return true; }
799-804: Blocking wait may impact MCP server responsiveness.The
delay_useccall blocks the current thread, which could stall stdio communication if this executes on the MCP server thread. While this appears intentional for waiting for bridge connections, consider:
- Adding a warning in the tool description that 'wait' action blocks
- Or implementing non-blocking polling with timeout if server responsiveness is critical
Based on the comment "allow waiting for connections," this seems like a workaround for bridge startup race conditions. If so, consider whether the bridge should expose a blocking "wait_for_connection" method with proper timeout handling.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/mcp/mcp_bridge.cppmodules/mcp/mcp_bridge.hmodules/mcp/mcp_tools.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
modules/mcp/mcp_bridge.cpp (1)
modules/mcp/mcp_bridge.h (1)
MCPBridge(40-61)
modules/mcp/mcp_bridge.h (1)
modules/mcp/mcp_bridge.cpp (18)
MCPBridge(54-57)MCPBridge(59-61)_process_command(229-447)_process_command(229-229)_trigger_action_event(189-227)_trigger_action_event(189-189)_bind_methods(63-65)_bind_methods(63-63)start_server(67-86)start_server(67-67)is_client_connected(88-94)is_client_connected(88-88)connect_to_server(96-102)connect_to_server(96-96)send_command(104-154)send_command(104-104)update(156-187)update(156-156)
modules/mcp/mcp_tools.cpp (2)
modules/mcp/mcp_tools.h (1)
MCPTools(43-65)modules/mcp/mcp_types.h (3)
make_string_property(120-125)make_object_property(134-139)make_boolean_property(127-132)
🪛 Clang (14.0.6)
modules/mcp/mcp_bridge.h
[error] 35-35: 'core/io/stream_peer_tcp.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🍏 iOS / Template (target=template_release)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🍎 macOS / Template (target=template_release)
🔇 Additional comments (13)
modules/mcp/mcp_bridge.h (2)
35-35: Static analysis warning is a false positive.The clang error about missing
'core/io/stream_peer_tcp.h'is expected in module context - the header exists and will be found during actual compilation with proper engine include paths.
40-77: LGTM!The class structure is well-organized with clear separation between host/server and client modes. The singleton pattern and public API are appropriate for an IPC bridge component.
modules/mcp/mcp_bridge.cpp (4)
305-350: LGTM - Recursive scene tree inspection is well-implemented.The recursive tree building with depth limiting and internal node filtering (line 336-338) is well thought out. The visibility and position data extraction for different node types is appropriate.
354-360: Smart fallback behavior may be unexpected.The automatic fallback to action triggering when typed text matches an action name (line 356) is clever but potentially surprising. A user typing "ui_cancel" intending to literally type that text would instead trigger the action.
Consider documenting this behavior clearly in the MCP protocol documentation, or adding an explicit flag in args to enable/disable this fallback.
372-376: No issue here.find_keycode()is declared incore/os/keyboard.h(line 342), which is already included in this file.Likely an incorrect or invalid review comment.
54-56: Singleton initialization lacks synchronization, but this is by design for single-threaded engine startup.The
singleton = this;assignment in the constructor has no thread safety mechanisms. However, MCPBridge is instantiated only once duringinitialize_mcp_module(), which runs as part of the engine's single-threaded initialization phase. This follows the standard Godot singleton pattern used throughout the engine (Engine, OS, EditorFileSystem, etc.). If MCPBridge were ever instantiated directly from multiple threads, a race condition would exist, but this is not a practical concern for the intended usage.modules/mcp/mcp_tools.cpp (7)
63-77: LGTM!The JSON-to-Variant conversion logic correctly handles Vector2, Vector3, and Color types with appropriate fallback.
149-260: LGTM!Tool schemas are well-structured with clear descriptions and appropriate property types. The use of
MCPSchemaBuilderhelpers ensures consistency.
266-286: LGTM!Clean routing logic with appropriate error handling for unknown tools.
492-565: LGTM!Resource management is correct with proper null checks, manual cleanup for non-Reference objects, and automatic Ref<> handling for resources.
567-675: LGTM!ClassDB introspection and GDScript parsing are implemented correctly with appropriate conditional compilation guards and error handling.
292-490: Critical: Memory leak on early return paths.The scene root instantiated at Line 345 is only deleted at Line 488, but there are multiple early returns between these lines that skip cleanup:
- Line 357:
get_nodeaction error return- Line 389:
addaction error return- Line 412:
removeaction error return- Line 425:
instanceaction error return- Line 442:
set_propaction error return- Line 456:
connectaction error return- Line 472:
reparentaction error returnAll of these paths leak the
rootnode.🔒 Proposed fix using RAII-style cleanup with scope guard
Since Node doesn't inherit from Reference (it's not ref-counted), manual cleanup is required. Use a scope guard pattern or ensure cleanup on all paths:
MCPTools::ToolResult MCPTools::tool_scene_action(const Dictionary &p_args) { ToolResult result; String action = p_args.get("action", ""); String scene_path = p_args.get("scene_path", ""); if (action.is_empty() || scene_path.is_empty()) { result.set_error("Missing action or scene_path"); return result; } if (!validate_path(scene_path)) { result.set_error("Invalid scene_path"); return result; } String normalized_scene = normalize_path(scene_path); if (action == "create") { // ... create logic unchanged ... return result; } Ref<PackedScene> scene = ResourceLoader::load(normalized_scene, "PackedScene"); if (scene.is_null()) { result.set_error("Failed to load scene: " + normalized_scene); return result; } Node *root = scene->instantiate(); if (!root) { result.set_error("Failed to instantiate scene"); return result; } + + // RAII-style cleanup: ensure root is always deleted + struct ScopeGuard { + Node *&node; + ~ScopeGuard() { if (node) { memdelete(node); } } + } cleanup{root}; bool should_save = false; if (action == "get_node") { String node_path = p_args.get("node_path", "."); Node *target = (node_path == "." || node_path.is_empty()) ? root : root->get_node_or_null(node_path); if (!target) { result.set_error("Node not found"); + return result; // cleanup happens automatically } else { // ... unchanged ... } } else if (action == "add") { // ... other actions unchanged ... } // ... rest of actions ... if (should_save) { Ref<PackedScene> new_scene; new_scene.instantiate(); new_scene->pack(root); ResourceSaver::save(new_scene, normalized_scene); } - memdelete(root); + // cleanup happens automatically via ScopeGuard destructor return result; }Alternative: Replace early returns with
goto cleanuppattern if you prefer traditional C-style error handling.Likely an incorrect or invalid review comment.
55-57: Static members are accessed only from the main thread. MCPTools::execute_tool is invoked via MCPServer::_server_loop (main thread) when processing JSON-RPC messages from stdin. The bridge thread (MCPBridge::update) only handles socket connections and does not invoke protocol methods or access MCPTools. No race conditions exist on these static members.
- Removed blocking delay in click action (replaced with 50ms OS::delay_usec which is acceptable for now per previous discussion, but future improvement noted) - Added check for is_process_running before killing PID - Handled active connection replacement in bridge update - Removed duplicate inspect_live code block - Added opt-in comment for callback injection
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @modules/mcp/mcp_bridge.cpp:
- Around line 104-154: send_command blocks the main thread for up to 5s using a
byte-by-byte blocking loop (MCPBridge::send_command,
connection->get_partial_data), which freezes the game; refactor to perform the
send/receive on a background thread or worker and make send_command asynchronous
(return a future/promise or accept a callback) so
MCPBridge::update/Main::iteration no longer blocks. In the background worker,
replace the byte-wise loop with buffered reads into a fixed-size buffer (enforce
a max response size to avoid unbounded growth), read chunks via
connection->get_partial_data into that buffer until you see the newline
delimiter or hit the size limit, use a configurable timeout constant instead of
the magic 5000ms, and invoke the result callback on the main thread (e.g., via
call_deferred or a thread-safe queue) with either the parsed Dictionary or an
error. Also remove polling/sleep from the main thread and ensure thread-safe
access to connection and lifecycle checks (is_client_connected) when moving I/O
off-thread.
🧹 Nitpick comments (6)
modules/mcp/mcp_bridge.cpp (6)
67-86: Consider cleanup and constant definitions.Two observations:
If
start_serveris called multiple times, the existing server and connection are not explicitly cleaned up. While Godot'sRef<>system should handle cleanup automatically, it would be clearer to explicitly stop the server or check if it's already listening.The port range (10000-10999) and retry logic are hard-coded. Consider defining these as named constants for maintainability.
♻️ Suggested improvements
+static const int BRIDGE_PORT_RANGE_START = 10000; +static const int BRIDGE_PORT_RANGE_END = 11000; + Error MCPBridge::start_server(int p_port) { + if (server->is_listening()) { + server->stop(); + } + if (connection.is_valid()) { + connection.unref(); + } + is_host = true; if (p_port == 0) { - for (int i = 10000; i < 11000; i++) { + for (int i = BRIDGE_PORT_RANGE_START; i < BRIDGE_PORT_RANGE_END; i++) { if (server->listen(i) == OK) {
96-102: Add explicit cleanup of existing connection.If
connect_to_serveris called multiple times, the old connection is not explicitly cleaned up. While Godot'sRef<>system should handle this, explicitly unreferencing or checking the existing connection would make the behavior clearer and prevent potential edge cases.♻️ Proposed fix
Error MCPBridge::connect_to_server(const String &p_host, int p_port) { + if (connection.is_valid()) { + connection.unref(); + } is_host = false; connection.instantiate();
236-254: Validate scale parameter and consider memory implications.The "capture" command creates a PNG buffer and base64-encodes it, which can be memory and CPU intensive, especially for large viewports (e.g., 4K resolution).
Additionally, the
scaleparameter (line 242) is not validated. Extreme values (e.g., scale=100 or scale=0) could cause memory exhaustion or crashes.🛡️ Add scale validation
float scale = args.get("scale", 1.0); + if (scale <= 0.0 || scale > 4.0) { + resp["error"] = "Invalid scale parameter (must be 0 < scale <= 4)"; + return resp; + } if (scale != 1.0) { img->resize(img->get_width() * scale, img->get_height() * scale); }
255-304: Blocking delay in click action.Similar to
_trigger_action_event, the click action usesOS::delay_usec(50000)at line 291 to hold the button press for 50ms. This blocks the calling thread and can cause frame drops if executed from the main thread.The commit message indicates this was reviewed and deemed necessary for engine state processing, but it's worth documenting this limitation or considering an asynchronous alternative.
305-350: Validate depth parameter.The
inspect_livecommand accepts adepthparameter (line 312) but doesn't validate it. Negative values or extremely large values could cause issues. Consider adding bounds checking.♻️ Add depth validation
int max_depth = args.get("depth", 5); + if (max_depth < 0 || max_depth > 20) { + resp["error"] = "Invalid depth parameter (must be 0 <= depth <= 20)"; + return resp; + }
351-403: find_keycode is safe; consider unicode preservation for non-ASCII input.The "type" command is well-structured with a smart action fallback. The
find_keycode()function from Godot's core keyboard module safely returnsKey::NONEfor unmatched inputs, so no validation is needed there.However, note that the
token.to_upper()[0]fallback on line 375 may produce incorrect keycodes for non-ASCII characters. While this is mitigated by theunicodefield being correctly set to the original character (line 382), this means non-ASCII input will transmit the right Unicode value but potentially incorrect modifier/keycode information. For typical use cases (Latin scripts), this is acceptable.The performance of character-by-character event sending is a known design trade-off for simplicity; consider documenting this if long text input is expected.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/mcp/mcp_bridge.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
modules/mcp/mcp_bridge.cpp (1)
modules/mcp/mcp_bridge.h (1)
MCPBridge(40-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🍏 iOS / Template (target=template_release)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
🔇 Additional comments (1)
modules/mcp/mcp_bridge.cpp (1)
189-227: The 50ms blocking delay blocks frame processing on the main thread.The
OS::delay_usec(50000)call at line 222 blocks the main thread for 50ms to simulate a key/button hold. Since_trigger_action_event()is called fromMain::iteration()during per-frame processing, this causes a frame drop (~3 frames at 60 FPS).While this delay is intentional for proper input event simulation, consider whether the blocking behavior is acceptable for real-time interaction, or if an asynchronous approach would be better.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @modules/mcp/mcp_tools.cpp:
- Around line 725-732: The stop branch kills a PID stored in last_game_pid via
OS::get_singleton()->kill without verifying that PID is still the same game
process; update the code in the stop handling to first validate ownership by
calling a process-check API (e.g., OS::is_process_running(last_game_pid) if
available) or by comparing stored process metadata (creation time or executable
identifier captured when starting the game) before calling
OS::get_singleton()->kill(last_game_pid); if the check fails, do not kill and
return an error, otherwise proceed to kill, clear last_game_pid, and return
"Stopped".
- Around line 483-488: The save call to ResourceSaver::save in the should_save
block doesn’t check its return value; update the block around should_save to
capture the Error result from ResourceSaver::save(new_scene, normalized_scene)
and handle failures (e.g., log or report the error and avoid reporting success);
ensure new_scene is non-null before packing, check the Error is OK before
proceeding, and propagate or return a failure status so callers know the scene
was not persisted.
🧹 Nitpick comments (2)
modules/mcp/mcp_bridge.cpp (1)
107-177: Consider adding connection validity check after re-acquiring lock in the read loop.While the initial connection check at Line 109 is protected by a mutex, the lock is released at Line 131 before entering the blocking read loop. Although unlikely, the connection could be invalidated by another thread (e.g.,
update()accepting a new connection on Line 187) between Line 131 and Line 135. Consider adding a connection validity check after re-acquiring the lock inside the loop.♻️ Suggested defensive check
while (OS::get_singleton()->get_ticks_msec() - start_time < TIMEOUT_MS) { mutex.lock(); + if (!connection.is_valid() || connection->get_status() != StreamPeerTCP::STATUS_CONNECTED) { + mutex.unlock(); + Dictionary err; + err["error"] = "Bridge disconnected during read"; + return err; + } connection->poll(); if (connection->get_available_bytes() > 0) {modules/mcp/mcp_tools.cpp (1)
792-798: Consider preventing multiple editor instances.Line 797 spawns an editor process without tracking it, which could allow spawning multiple editor instances. If this is unintended, consider tracking the editor PID and checking if an editor is already running.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/mcp/mcp_bridge.cppmodules/mcp/mcp_server.cppmodules/mcp/mcp_server.hmodules/mcp/mcp_tools.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/mcp/mcp_server.cpp
- modules/mcp/mcp_server.h
🧰 Additional context used
🧬 Code graph analysis (1)
modules/mcp/mcp_tools.cpp (2)
modules/mcp/mcp_tools.h (1)
MCPTools(43-65)modules/mcp/mcp_types.h (3)
make_string_property(120-125)make_object_property(134-139)make_boolean_property(127-132)
🔇 Additional comments (13)
modules/mcp/mcp_bridge.cpp (5)
54-61: LGTM! Clean singleton initialization.The constructor and destructor properly manage the singleton lifecycle.
67-87: LGTM! Port scanning logic is sound.The automatic port discovery with fallback range and proper error handling is well-implemented.
179-220: LGTM! Connection management and buffer overflow protection are solid.The host-side logic of replacing existing connections (Line 184-188) is clearly logged, and the 1MB buffer limit (Line 210) provides good protection against memory exhaustion attacks.
222-260: LGTM! Input event simulation is correct.The 50ms hold time (Line 255) and event duplication logic properly simulate user input.
262-457: LGTM! Command processing is well-structured.The dispatcher handles multiple action types with appropriate error handling. Notable good practices:
- Filtering internal nodes with "@@" prefix (Line 369)
- Smart fallback from text to input actions (Line 389)
- Default case for unknown actions (Line 450-454)
modules/mcp/mcp_tools.cpp (8)
64-78: LGTM! JSON to Variant conversion is clean.The helper correctly handles common geometric and color types with sensible defaults.
123-144: LGTM! Path utilities include proper security checks.The
validate_pathfunction correctly prevents directory traversal attacks by checking for ".." (Line 136).
150-261: LGTM! Tool definitions are comprehensive and well-documented.The schema definitions provide clear guidance for MCP clients, with particularly good advice about using specialized tools instead of direct file editing (Line 174, Line 231).
568-676: LGTM! Code intelligence implementation is robust.The conditional compilation guards (Line 626, 672) and comprehensive error reporting from the GDScript parser provide good coverage.
690-724: LGTM! Process spawning logic is sound.The sequential steps of stopping any existing game (Line 691-693), ensuring the bridge is ready (Line 695-701), and spawning with proper arguments are well-structured. Error handling at Line 720 appropriately checks the process creation result.
751-768: LGTM! File creation includes good safety checks.The prevention of overwriting existing GDScript files (Line 753-755) with a clear error message guiding users to use native editing tools is excellent defensive design.
330-336: Check error return from ResourceSaver::save.Similar to Line 487, this save operation (Line 330) should verify success before reporting to the user.
♻️ Proposed fix
root_node->set_name(root_name.is_empty() ? scene_path.get_file().get_basename() : root_name); Ref<PackedScene> new_scene; new_scene.instantiate(); new_scene->pack(root); Error err = ResourceSaver::save(new_scene, normalized_scene); memdelete(root_node); if (err != OK) { - result.set_error("Failed to save scene"); + result.set_error("Failed to save scene: " + itos(err)); } else { result.add_text("Scene created: " + normalized_scene); }Likely an incorrect or invalid review comment.
56-58: Thread-safety of static members is not a concern.These static variables are accessed exclusively from MCPTools methods, which are invoked only through MCPProtocol within the MCP server's main event loop. This is a single-threaded, sequential processing loop that handles JSON-RPC messages. The bridge thread spawned in the server loop interacts only with MCPBridge, not MCPTools. No race conditions are possible, and mutex protection is unnecessary.
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Fix all issues with AI agents
In @main/main.cpp:
- Around line 4768-4776: The MCPBridge auto-connect currently prints an error
when MCPBridge::get_singleton() is null but continues execution; change this to
fail hard: when mcp_bridge_port != 0 and MCPBridge::get_singleton() is null, log
the error via OS::get_singleton()->print (or process logger) and immediately
terminate the process with EXIT_FAILURE (or call exit(EXIT_FAILURE)) so the
application does not continue in a partially initialized state; update the
branch that handles the null singleton where MCPBridge::get_singleton() is
checked to return/exit with EXIT_FAILURE instead of only printing.
- Around line 1424-1443: When handling the "--mcp-server" flag (the branch that
sets mcp_server_enabled and switches drivers), also force clean stdout by
setting quiet_stdout = true and disabling line printing via
CoreGlobals::print_line_enabled = false (and apply the same behavior when
mcp_run_tests is enabled if present), ensuring these are set early in the
"--mcp-server" branch (before any other output) so no warnings/prints can
corrupt the JSON-RPC stdio stream.
- Around line 2989-2992: The header suppression branch using MODULE_MCP_ENABLED
and mcp_server_enabled only prevents print_header(), but you must enforce a
global "clean stdout" when MCP server mode is active; introduce a single gate
(e.g., check mcp_server_enabled or set a global suppress_stdout flag) and have
all user-facing output routines consult it (wrap or modify print_header,
printf/fprintf wrappers, and any other console output paths to early-return when
suppress_stdout is set) so no incidental stdout is emitted in MCP server mode.
- Around line 618-623: The help text for the "--headless" option contains an
escaped backslash sequence ("\\n") causing a literal "\n" to appear; locate the
call to print_help_option for "--headless" and change the string from "Useful
for servers and with --script.\\n" to "Useful for servers and with --script.\n"
to match the other options and produce a real newline.
- Around line 297-301: mcp_run_tests is declared and checked in Main::start()
but never assigned because Main::setup() doesn't parse the --run-tests option;
add argument parsing in Main::setup() to populate the global String
mcp_run_tests when the command-line includes "--run-tests <path>". Locate the
command-line/options parsing block in Main::setup(), extend it to recognize the
"--run-tests" flag (or its short form if used elsewhere), store the following
token into mcp_run_tests, and ensure any help text already printing "--run-tests
<path>" matches this parser behavior so Main::start() sees the intended value.
In @modules/mcp/doc_classes/MCPBridge.xml:
- Around line 1-16: The MCPBridge XML docs are empty; populate the
<brief_description>, <description>, and <methods>/<method name="update">
sections to explain the MCPBridge class role in coordinating host/server and
client/game interactions, describe its lifecycle relationship with the MCP
server (when instances are created/destroyed and how it integrates into
startup/shutdown), and document the update() method purpose, expected call
frequency (e.g., per-frame or tick), inputs/side-effects and when callers should
invoke it; use the class name MCPBridge and method name update() in the text so
readers can easily map docs to code.
In @modules/mcp/mcp_bridge.cpp:
- Around line 107-177: The send_command method drops any bytes read after the
first newline, causing desync; fix it by introducing a persistent per-instance
receive buffer (e.g., a member like recv_buffer or residual_buffer) and append
each get_partial_data read into that buffer instead of only building
response_str from the immediate chunk, then search the persistent buffer for the
first '\n', extract the line up to the newline as response_str and keep the
remaining bytes in the persistent buffer for subsequent calls; update
MCPBridge::send_command to use the read count returned by get_partial_data,
append read_buffer.ptrw()[:read] to the member buffer, and only remove the
consumed portion when a complete line is parsed so no bytes are lost between
calls.
- Around line 262-457: Multiple command branches in MCPBridge::_process_command
(notably the "click" and "inspect_live" handlers) dereference SceneTree* st
without null checks, which can crash if OS::get_singleton()->get_main_loop() is
not a SceneTree; update these branches to mirror the safe pattern used in
"capture": first obtain SceneTree* st =
Object::cast_to<SceneTree>(OS::get_singleton()->get_main_loop()); check if st is
non-null and if null set resp["error"] = "No scene tree found" (or similar) and
return resp before accessing st->get_root() or other members; apply this change
in the "click" block (before using st->get_root()) and in the "inspect_live"
block (before using st and st->get_root()) so all paths guard against a missing
SceneTree.
In @modules/mcp/mcp_protocol.cpp:
- Around line 105-140: In MCPProtocol::_handle_tools_call the code currently
ignores a non-dictionary "arguments" and falls back to an empty Dictionary;
instead, when params.has("arguments") and the Variant args_var is not of type
Variant::DICTIONARY, return make_response_error(INVALID_PARAMS, "Tool call
'arguments' must be an object") rather than silently setting arguments = {} so
callers are notified of invalid input; keep the rest of the flow (arguments used
in tools->execute_tool and response via _make_tool_result) unchanged and
reference params, arguments, args_var, make_response_error, and INVALID_PARAMS
when applying the change.
In @modules/mcp/mcp_server.cpp:
- Around line 118-127: The current MCPServer::start_game_process uses explicit
destructor calls on MutexLock and manual relocking around stop_game_process,
which breaks RAII and is unsafe; fix by removing lock.~MutexLock() and instead
limit the lock scope (or extract the check into a helper) so you acquire
MutexLock(process_mutex) only while checking/modifying shared state (game_pid,
game_log_path) and then release it naturally before calling stop_game_process(),
then reacquire a new MutexLock for the remainder; ensure stop_game_process() is
called without holding process_mutex to avoid deadlock and that
game_pid/game_log_path updates happen under the mutex (symbols:
MCPServer::start_game_process, stop_game_process, process_mutex, MutexLock,
game_pid, game_log_path).
In @modules/mcp/mcp_server.h:
- Around line 88-99: The tool layer is duplicating process state leading to
split-brain; centralize lifecycle in MCPServer by routing
creation/termination/checks through start_game_process, stop_game_process,
is_game_running and _check_game_process only. Update callers to stop using any
external last_game_pid and instead call MCPServer::start_game_process(...) and
MCPServer::stop_game_process(); inside MCPServer ensure all accesses to game_pid
and game_log_path are protected by process_mutex (set game_pid = 0 inside
stop_game_process while holding the mutex) and ensure _check_game_process is the
single reaper invoked by the server timer rather than external code tracking the
PID.
In @modules/mcp/mcp_tools.cpp:
- Around line 803-834: In MCPTools::tool_game_control handle the "wait" branch
by validating and clamping the seconds value obtained into secs: ensure it is
numeric, enforce a non-negative lower bound and a sensible upper bound (e.g.
clamp to 0..300 or another configured max) before calling
OS::get_singleton()->delay_usec, and if the input is out of range return an
error or adjust it and document the applied clamp in the result message; locate
the secs variable in the "wait" branch and apply the clamp/validation there so
clients cannot trigger extremely long or negative sleeps.
- Around line 678-801: tool_project_config currently starts/stops the game
directly via OS::create_process and last_game_pid while also calling
MCPServer::is_game_running()/stop_game_process(), which leads to orphaned
processes and broken reaper logic; change the "run" branch to ask MCPServer to
start the game (e.g., call a MCPServer::start_game_process or similar method
that accepts the prepared args/bridge_port and returns an Error/PID) after
ensuring the bridge is started (MCPBridge::start_server), set any PID/state via
MCPServer rather than MCPTools::last_game_pid, and in the "stop" branch call
MCPServer::stop_game_process() (and clear any local state only if MCPServer
indicates it stopped) instead of OS::kill; ensure reading output/logs and
bridge_port remain compatible with MCPServer-managed process lifecycle.
- Around line 293-491: The tool_scene_action function currently returns success
for unrecognized actions and ignores ResourceSaver::save errors; update it to
(1) add a final else/guard for unhandled action values inside
MCPTools::tool_scene_action that sets result.set_error("Unknown action: " +
action) and returns the result, and (2) capture and check the Error returned by
ResourceSaver::save wherever save is called (both in the "create" branch and the
final should_save block): if err != OK set result.set_error("Failed to save
scene: " + normalized_scene + " (" + itoa(err) + ")") and avoid reporting
success text; use the existing symbols new_scene, ResourceSaver::save, err,
should_save, normalized_scene and return result on error.
In @README.md:
- Around line 137-146: Consolidate the duplicate guidance about editing `.gd`
scripts by merging items #2 and #6 into a single line: state that agents must
use native text editing tools (e.g., the `edit` command) to modify existing
`.gd` files and that `redot_project_config(action="create_file_res")` / the MCP
tool `create_file_res` is reserved only for creating new script resources,
removing the repeated sentence from either the original item #2 or #6 so the
document contains this guidance exactly once.
🧹 Nitpick comments (6)
doc/classes/@GlobalScope.xml (1)
1693-1696: Document the new GlobalScope members.The MCPBridge and MCPServer members have been added to GlobalScope but lack descriptions. These empty descriptions should be filled in before release to help users understand what these singletons provide.
Based on the PR objectives, suggested descriptions might be:
- MCPBridge: "The MCP bridge singleton for IPC communication between the headless server and active game process."
- MCPServer: "The MCP server singleton for exposing engine APIs to AI agents via JSON-RPC 2.0."
📝 Suggested documentation additions
<member name="MCPBridge" type="MCPBridge" setter="" getter=""> + The [MCPBridge] singleton. Provides IPC communication between the headless MCP server and the running game process. </member> <member name="MCPServer" type="MCPServer" setter="" getter=""> + The [MCPServer] singleton. Exposes engine APIs to AI agents via Model Context Protocol over JSON-RPC 2.0. </member>modules/mcp/doc_classes/MCPServer.xml (1)
1-26: Complete the MCPServer class documentation.The MCPServer documentation is currently a stub with empty descriptions for the class and all methods. Before merging or releasing this feature, the documentation should be completed to help users understand:
- What the MCPServer class does (brief and detailed description)
- When and how to use start(), stop(), and is_running()
- Expected behavior and lifecycle
- Integration with --mcp-server CLI flag
Based on the PR objectives, suggested content:
Class description: "Manages the Model Context Protocol (MCP) server that exposes engine APIs to AI agents via JSON-RPC 2.0 over stdio."
Method descriptions:
is_running(): "Returns true if the MCP server is currently running."start(): "Starts the MCP server and begins listening for JSON-RPC requests."stop(): "Stops the MCP server and closes all connections."modules/mcp/mcp_tools.h (1)
99-101: Consider thread safety for static process tracking state.The static members
last_game_pid,last_log_path, andbridge_portare accessed fromtool_project_config(for run/stop/output actions) without mutex protection. While MCP typically processes requests sequentially, concurrent tool calls could theoretically race on these shared variables.💡 Suggested improvement: Move to instance state or add protection
Option 1: Move to MCPServer as instance state
Since MCPServer already manages game processes (lines 106-154 in mcp_server.cpp show process management with mutex), consider moving this state there and accessing via MCPServer::get_singleton().
Option 2: Add mutex protection
+#include "core/os/mutex.h" + class MCPTools : public Object { GDCLASS(MCPTools, Object) private: + static Mutex process_state_mutex; static OS::ProcessID last_game_pid; static String last_log_path; static int bridge_port;Then protect all accesses to these variables with
MutexLock lock(process_state_mutex);in tool_project_config.Based on learnings and the PR's synchronous request-response design, this is likely safe in practice, but worth considering for future-proofing.
modules/mcp/mcp_server.h (2)
45-66: Define/ownprotocolwith RAII (raw pointer + thread lifecycle is fragile).
MCPProtocol *protocol = nullptr;plus a backgroundbridge_threadmeansstart()/stop()/~MCPServer()must be extremely strict about: (1) stopping/setting flags, (2) joining the thread, (3) deletingprotocolexactly once. Consider switchingprotocoltoUniquePtr<MCPProtocol>(or aRefif appropriate) to make ownership explicit.
76-87: Document whetherstart()is idempotent and whether it blocks forever.
Givenstart()is “blocking call for headless mode”, callers (likeMain::start()) need clear guarantees: what happens ifstart()is called twice, or ifstop()is called beforestart()fully initializes.modules/mcp/mcp_protocol.cpp (1)
67-90:initializeshould validate client protocol version / params (currently ignoresp_params).
At minimum, reject incompatible protocol versions (if MCP requires it) so clients get a clear error instead of undefined behavior later.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
README.mddoc/classes/@GlobalScope.xmlmain/main.cppmodules/mcp/SCsubmodules/mcp/config.pymodules/mcp/doc_classes/MCPBridge.xmlmodules/mcp/doc_classes/MCPProtocol.xmlmodules/mcp/doc_classes/MCPServer.xmlmodules/mcp/mcp_bridge.cppmodules/mcp/mcp_bridge.hmodules/mcp/mcp_protocol.cppmodules/mcp/mcp_protocol.hmodules/mcp/mcp_server.cppmodules/mcp/mcp_server.hmodules/mcp/mcp_tools.cppmodules/mcp/mcp_tools.hmodules/mcp/mcp_types.hmodules/mcp/register_types.cppmodules/mcp/register_types.hredot-mcp.sh
🧰 Additional context used
🧬 Code graph analysis (7)
modules/mcp/register_types.cpp (6)
modules/mcp/mcp_protocol.cpp (2)
MCPProtocol(39-48)MCPProtocol(50-55)modules/mcp/mcp_protocol.h (1)
MCPProtocol(43-74)modules/mcp/mcp_server.cpp (2)
MCPServer(48-51)MCPServer(53-71)modules/mcp/mcp_server.h (1)
MCPServer(45-71)modules/mcp/mcp_bridge.cpp (2)
MCPBridge(54-57)MCPBridge(59-61)modules/mcp/mcp_bridge.h (1)
MCPBridge(41-63)
modules/mcp/mcp_bridge.h (1)
modules/mcp/mcp_bridge.cpp (18)
MCPBridge(54-57)MCPBridge(59-61)_process_command(262-457)_process_command(262-262)_trigger_action_event(222-260)_trigger_action_event(222-222)_bind_methods(63-65)_bind_methods(63-63)start_server(67-87)start_server(67-67)is_client_connected(89-96)is_client_connected(89-89)connect_to_server(98-105)connect_to_server(98-98)send_command(107-177)send_command(107-107)update(179-220)update(179-179)
modules/mcp/mcp_tools.h (1)
modules/mcp/mcp_tools.cpp (28)
MCPTools(109-110)MCPTools(112-113)normalize_path(123-132)normalize_path(123-123)validate_path(134-140)validate_path(134-134)get_absolute_path(142-144)get_absolute_path(142-142)_bind_methods(115-117)_bind_methods(115-115)get_tool_definitions(150-261)get_tool_definitions(150-150)execute_tool(267-287)execute_tool(267-267)tool_scene_action(293-491)tool_scene_action(293-293)tool_resource_action(493-566)tool_resource_action(493-493)tool_code_intel(568-676)tool_code_intel(568-568)tool_project_config(678-801)tool_project_config(678-678)tool_game_control(803-834)tool_game_control(803-803)_json_to_variant(64-78)_json_to_variant(64-64)_ensure_callback_exists(80-107)_ensure_callback_exists(80-80)
modules/mcp/mcp_server.cpp (3)
modules/mcp/mcp_server.h (1)
MCPServer(45-71)modules/mcp/mcp_protocol.cpp (4)
MCPProtocol(39-48)MCPProtocol(50-55)_bind_methods(57-65)_bind_methods(57-57)modules/mcp/mcp_protocol.h (1)
MCPProtocol(43-74)
modules/mcp/mcp_bridge.cpp (1)
modules/mcp/mcp_bridge.h (1)
MCPBridge(41-63)
modules/mcp/mcp_tools.cpp (1)
modules/mcp/mcp_tools.h (1)
MCPTools(43-65)
main/main.cpp (1)
modules/mcp/mcp_server.cpp (2)
run_tests(220-264)run_tests(220-220)
🪛 Clang (14.0.6)
modules/mcp/register_types.h
[error] 35-35: 'modules/register_module_types.h' file not found
(clang-diagnostic-error)
modules/mcp/mcp_bridge.h
[error] 35-35: 'core/io/stream_peer_tcp.h' file not found
(clang-diagnostic-error)
modules/mcp/mcp_types.h
[error] 35-35: 'core/string/ustring.h' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.10)
modules/mcp/config.py
1-1: Unused function argument: env
(ARG001)
1-1: Unused function argument: platform
(ARG001)
🔇 Additional comments (36)
modules/mcp/SCsub (1)
1-8: LGTM! Standard module build script.The SCsub follows the standard pattern for Redot modules. The wildcard pattern
"*.cpp"will include all C++ source files in the modules/mcp/ directory, which is the typical approach for module builds.redot-mcp.sh (4)
4-7: Verify hardcoded binary path is appropriate for the use case.The binary path on Line 5 is hardcoded to
redot.linuxbsd.editor.x86_64, which limits this script to Linux x86_64 builds. Since the script is documented as a Nix wrapper and the PR mentions verification on Linux (Nix), this may be intentional.However, consider:
- If this script should support other platforms, the binary path could be detected dynamically
- The binary name might differ based on build configuration (debug/release, features enabled)
If cross-platform support or flexibility is needed, consider detecting the appropriate binary or accepting it as a parameter. Otherwise, document that this script is specifically for Linux x86_64 Nix environments.
9-12: LGTM! Clear validation and error message.The PROJECT_PATH validation provides a helpful usage message when the required argument is missing.
14-17: LGTM! Appropriate prerequisite check.The nix command validation ensures the required tool is available before attempting to use it. Using
command -vis the correct POSIX-compliant approach.
19-19: Verify nix develop command usage.The command execution looks correct and aligns with the PR objectives for running the MCP server headlessly. The flags
--headless,--mcp-server, and--pathmatch the described functionality.Consider whether error handling is needed. If
nix developor the binary execution fails, the script will exit with the command's exit code, which may be acceptable for a simple wrapper script. If more robust error handling is desired, you could add checks or trap handlers.modules/mcp/config.py (4)
1-2: LGTM! Standard SCons module pattern.The
can_buildfunction returnsTrueunconditionally, indicating the MCP module can be built on all platforms. The unusedenvandplatformparameters are part of the standard SCons module configuration interface and cannot be removed.The static analysis warnings about unused arguments are false positives in this context.
5-6: LGTM! No configuration needed.The no-op
configurefunction is appropriate since the MCP module doesn't require any special build environment configuration.
17-18: LGTM! Standard documentation path.The
get_doc_pathfunction returns the standard"doc_classes"directory where module documentation XML files are located.
9-14: No action needed.All documented classes have corresponding XML files: MCPServer.xml, MCPProtocol.xml, and MCPBridge.xml are present in
modules/mcp/doc_classes/.modules/mcp/mcp_types.h (6)
46-56: LGTM!The MCPTextContent struct is straightforward and correctly implements the text content type serialization.
58-70: LGTM!The MCPImageContent struct correctly serializes image content with base64 data and MIME type. Note that no validation is performed on the base64 encoding, which is acceptable if validation happens upstream.
72-90: LGTM!The MCPResourceContent struct correctly handles optional fields by conditionally including them only when non-empty, which produces cleaner JSON output.
93-105: LGTM!The MCPToolDefinition struct provides a clean interface for tool definitions with JSON Schema support. The use of Dictionary for input_schema allows flexible schema representations.
108-150: LGTM!The MCPSchemaBuilder provides a clean DSL for constructing JSON Schema definitions. The conditional inclusion of optional fields (required array, items definition) prevents unnecessary empty values in the output.
41-43: The MCP protocol version"2025-11-25"is a valid, officially published revision of the Model Context Protocol specification. No changes needed.README.md (4)
78-90: LGTM!The tools overview provides a clear introduction to the five MCP controllers and their capabilities. The descriptions effectively communicate the purpose of each tool to potential users.
92-110: LGTM!The configuration example is clear and actionable. The JSON format is valid and demonstrates the correct command-line arguments for running the MCP server in headless mode.
112-118: LGTM!The unit tests documentation is concise and includes the important requirement that test scripts must have a
func run():method.
120-136: LGTM!The Nix development workflow is well documented with clear rationale (library linking) and a practical configuration example using the wrapper script.
modules/mcp/register_types.cpp (3)
42-43: LGTM!The static singleton pointers follow the standard pattern for managing module-level singleton lifetimes.
45-61: LGTM!The initialization sequence is correct: class registration, singleton creation, and Engine registry addition. The singletons are properly exposed to the global scope as documented.
63-79: LGTM!The uninitialization sequence properly cleans up both singletons with null checks and removes them from the Engine registry. The cleanup order (MCPBridge before MCPServer) is appropriate given the dependency relationship.
modules/mcp/register_types.h (1)
37-38: LGTM - Standard module registration interface.The function declarations follow the Redot engine module registration pattern correctly. The implementation properly manages MCPServer and MCPBridge singletons at MODULE_INITIALIZATION_LEVEL_SCENE.
modules/mcp/mcp_protocol.h (1)
43-74: LGTM - Well-structured MCP protocol handler.The class design is sound:
- Properly extends JSONRPC for MCP-specific functionality
- Memory management for the
toolsmember follows Redot conventions (memnew/memdelete)- Clear separation between private handlers, protected binding, and public interface
- Appropriate use of const qualifiers for readonly methods
modules/mcp/mcp_server.cpp (3)
106-116: LGTM - Proper process monitoring and zombie cleanup.The zombie reaping comment is helpful. The implementation correctly uses
is_process_runningwhich handles platform-specific process reaping (waitpid on Linux), and properly resets the PID under mutex protection.
156-200: LGTM - Clean server loop implementation.The loop correctly:
- Separates stderr logging from stdout JSON-RPC stream (lines 163-165)
- Handles empty responses for MCP notifications (lines 187-190)
- Properly cleans up the bridge thread before exit (line 195)
220-264: LGTM - Robust test script execution.The implementation handles all error cases properly:
- Resource loading failures (lines 224-228)
- Type validation (lines 230-234)
- Instantiation errors (lines 236-240)
- Call errors with specific error code handling (lines 249-255)
- Memory management for both RefCounted and non-RefCounted objects (lines 261-263)
modules/mcp/mcp_bridge.h (1)
41-79: LGTM - Clean bridge interface design.The class design is well-structured:
- Singleton pattern with proper lifecycle management
- Thread-safe with mutable mutex for const method thread safety (line 46)
- Clear separation between host (server) and client (game) APIs (lines 68-74)
- Appropriate use of Ref<> for Godot reference-counted types
modules/mcp/mcp_tools.h (1)
78-92: LGTM - Well-organized tool execution API.The API design is sound:
- Static
get_tool_definitions()for stateless tool metadata (line 82)- Instance
execute_tool()dispatcher (line 85)- Clear master controller methods corresponding to the five MCP tool categories (lines 88-92)
The structure aligns well with the MCP protocol's tool discovery and execution model.
main/main.cpp (3)
154-157: Guarded includes look fine.
2191-2195: Main-scene guard exception for--mcp-serveris sensible.
4846-4849: Per-frameMCPBridge::update()hook is appropriate.modules/mcp/mcp_protocol.cpp (1)
39-55: LGTM: method registration +toolsownership is clear.modules/mcp/mcp_bridge.cpp (1)
67-87: Port-scan behavior is fine; consider exposing the chosen port via API/log consistently.modules/mcp/mcp_tools.cpp (2)
267-287: Unknown tool returns an error correctly.
493-566: The code pattern is correct and does not leak.Ref<Resource>automatically increments the refcount when assigned a RefCounted object (Resource inherits from RefCounted), so the ownership semantics are handled safely by the Ref constructor. Thememdelete(obj)is only called on failure when the cast fails, which is the proper behavior. This pattern matches the established codebase conventions (e.g., in resource_loader.cpp) and no refactoring is needed.
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In @main/main.cpp:
- Around line 2998-3003: Startup still prints to stdout beyond print_header;
gate or suppress those prints when mcp_server_enabled is true: add checks for
the mcp_server_enabled flag in the remaining startup paths (notably setup2() and
start()) and in the benchmark output routine so they don't write to stdout
(either skip output or write to stderr/logging instead), or introduce a global
suppress_stdout mode checked by those functions; ensure all calls that produce
framing-sensitive stdout (benchmark printing, other startup prints) respect that
flag.
- Around line 4796-4818: The benchmark_dump() call writes human-readable output
to stdout and will corrupt the clean JSON stream when running in MCP server or
test modes; remove or skip calling OS::get_singleton()->benchmark_dump() in the
branches guarded by mcp_server_enabled and !mcp_run_tests.is_empty() (the blocks
that call MCPServer::start() and MCPServer::run_tests()) so that
benchmark_dump() is not invoked before/after MCPServer::start() or
MCPServer::run_tests(); you can still call benchmark_end_measure() if needed,
but ensure no benchmark_dump() or other stdout-printing calls occur in those MCP
paths.
- Around line 618-623: The help text for the "--headless" option uses an escaped
backslash sequence ("\\n") which results in a literal backslash-n appearing in
CLI output; update the print_help_option call for the "--headless" option to use
a real newline escape ("\n") instead (modify the string passed to
print_help_option for "--headless" to end with "\n"), keeping the rest of the
message unchanged.
- Around line 4778-4786: The call to
MCPBridge::get_singleton()->connect_to_server("127.0.0.1", mcp_bridge_port)
discards its Error return; capture the Error result, check if it is not OK, and
handle it (e.g., print a clear message with the error string via
OS::get_singleton()->print or process logger and decide whether to abort startup
or continue); alternatively, if ignoring is intentional, add a comment
explaining why and document that failures are non-fatal. Ensure you reference
MCPBridge::get_singleton(), connect_to_server(), and the Error value in the fix
so the failure is either propagated, logged with details, or explicitly
documented.
In @modules/mcp/mcp_bridge.cpp:
- Around line 88-103: Add a mutex to protect the shared Ref<StreamPeerTCP>
connection and lock it whenever accessing or mutating connection: in
is_client_connected(), send_command(), update(), and connect_to_server() acquire
the mutex, copy connection into a local Ref<StreamPeerTCP> while holding the
lock, then release and operate on the local copy (or use lock_guard for short
critical sections when mutating connection like instantiate() or
connect_to_host()); ensure all reads (poll(), get_status()) and writes
(instantiate(), assigning/replacing connection) are protected to prevent races
between the host thread and protocol thread.
- Around line 275-325: The click handler dereferences st without null-checking
SceneTree; before calling st->get_root() (after SceneTree *st =
Object::cast_to<SceneTree>(OS::get_singleton()->get_main_loop())), validate st
is non-null and return an error in resp if it is null, and also ensure
get_root() is non-null before using it to find the node; update the block
handling args["node_path"] (variables: st, node, ctrl,
Object::cast_to<SceneTree>, get_root()) to check for null st and null root and
produce a clear resp["error"] when missing so no null pointer is dereferenced.
- Around line 163-207: The update() method in MCPBridge unlocks mutex without
owning it (mutex.unlock() in MCPBridge::update), which is undefined; instead,
acquire the lock before reading/modifying shared state (e.g., partial_data and
connection) and then, when a full command is assembled, move or copy the parsed
command/command string out while still holding the lock, release the lock, call
_process_command(...) outside the lock, then re-acquire the lock only to send
the response or update shared state; ensure all uses of
mutex.lock()/mutex.unlock() are paired and that partial_data is only mutated
while the mutex is held.
In @modules/mcp/mcp_server.cpp:
- Around line 53-71: The destructor currently calls stop() but a blocking
std::getline in start()/_read_line() can remain blocked, risking a
use-after-free of protocol when the destructor times out; fix by making the
input loop interruptible and ensuring the server thread is joined before
deleting protocol: add a wakeup mechanism (e.g., a pipe or event pair like int
wake_fds[2] created at MCPServer construction), change the blocking stdin read
in start()/_read_line() to poll/select on both stdin (fd 0) and wake_fds[0], and
modify stop() to set should_stop=true and write to wake_fds[1] to wake the poll;
finally ensure the destructor waits for the thread to exit (join) after calling
stop() and before memdelete(protocol) and clearing singleton, so running is
guaranteed false and no blocking read can resume after protocol deletion.
- Around line 129-146: stop_game_process() currently uses a MutexLock
(process_mutex) and then manually unlocks and locks it inside the wait loop,
which risks double-unlock or leaving the mutex in an invalid state; change the
logic to avoid manipulating the MutexLock manually by capturing game_pid,
releasing the MutexLock once before the loop (i.e., let the scoped MutexLock go
out of scope or explicitly end the critical section) and then poll
OS::get_singleton()->is_process_running(game_pid) with delay without holding
process_mutex, and only re-acquire the mutex to clear game_pid (set game_pid =
0) when you know the process exited; ensure you still return the original err
from OS::get_singleton()->kill and preserve the initial guard around checking
game_pid == 0.
In @modules/mcp/mcp_tools.cpp:
- Around line 123-145: normalize_path() can return paths that are unsafe or
invalid for file I/O because callers sometimes skip validate_path(); update
callers that perform file operations (e.g., read_file_res, create_file_res,
list_files) to always call MCPTools::validate_path(p_path) before using
MCPTools::normalize_path(p_path) and abort with an error if validation fails;
additionally ensure normalize_path either rejects or strips parent-traversal
segments ("..") consistently (or rely on validate_path to reject them) so
get_absolute_path() and any globalize_path calls never receive unsafe paths.
- Around line 685-816: The tool_project_config handler splits game lifecycle
management between MCPTools and MCPServer which can desync (tool_project_config
uses create_process and last_game_pid/last_log_path while run/stop first check
MCPServer::is_game_running()/stop_game_process()); fix by delegating process
start/stop/log tracking exclusively to MCPServer: replace the create_process
call in project_config's "run" branch with MCPServer::start_game_process(...)
(passing bridge port, path, log path) and read the PID/log path via new or
existing MCPServer getters (e.g., get_game_pid(), get_log_path()); in "stop" and
"output" branches use MCPServer::is_game_running(),
MCPServer::stop_game_process(), and get_log_path()/get_game_pid() instead of
last_game_pid/last_log_path so MCPServer is the single source of truth (add
getters to MCPServer if they don’t exist).
- Around line 500-573: The calls to ResourceSaver::save() inside
MCPTools::tool_resource_action (in the "create" branch where
ResourceSaver::save(res, normalized) is used, in the "modify" branch where
ResourceSaver::save(res, normalized) is used, and in the "duplicate" branch
where ResourceSaver::save(copy, normalize_path(np)) is used) ignore the returned
Error; change each to capture Error err = ResourceSaver::save(...); check if err
!= OK, call result.set_error("Failed to save resource") and return result on
failure, otherwise proceed to add the success text (e.g., "Created resource at
...", "Modified resource", "Duplicated resource").
🧹 Nitpick comments (5)
main/main.cpp (1)
1424-1453: Consider hard-blocking incompatible flag combos (e.g.,--editor+--mcp-server).
Right now--mcp-serveronly implies headless, but doesn’t prevent later--editorparsing/initialization; that can add extra stdout/stderr noise and work before the MCP early-exit.modules/mcp/mcp_bridge.cpp (2)
54-86: Server start behavior is OK; consider making restart/idempotency explicit.
If start_server() can be called multiple times, it should close/stop prior listen/connection first to avoid leaking state.
371-432: Input simulation works but introduces frame-stalling sleeps.
The 50ms delays (type/click/action) will stall the game thread while processing bridge commands. If that’s acceptable for the MVP, ok—but it may cause visible hitches during automation runs.modules/mcp/mcp_tools.cpp (2)
293-499: Scene editing approach is solid; add error checks for connect/callback creation.
Inconnect,_ensure_callback_exists()return value is ignored andconnect()return value isn’t checked. A failed connect will still attempt to save.
819-850: game_control bridge usage is reasonable; guard null singleton.
MCPBridge::get_singleton()is assumed non-null (Line 831). If module init order ever changes, this becomes a crash—cheap to guard.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
main/main.cppmodules/mcp/mcp_bridge.cppmodules/mcp/mcp_server.cppmodules/mcp/mcp_tools.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
modules/mcp/mcp_bridge.cpp (1)
modules/mcp/mcp_bridge.h (1)
MCPBridge(41-63)
main/main.cpp (1)
modules/mcp/mcp_server.cpp (2)
run_tests(225-269)run_tests(225-225)
modules/mcp/mcp_server.cpp (2)
modules/mcp/mcp_server.h (1)
MCPServer(45-71)modules/mcp/mcp_protocol.h (1)
MCPProtocol(43-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🍏 iOS / Template (target=template_release)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
🔇 Additional comments (3)
main/main.cpp (2)
154-157: MCP integration plumbing looks fine (guards + scoped globals).Also applies to: 297-301
4856-4860: Per-frame MCPBridge update hook looks reasonable.modules/mcp/mcp_server.cpp (1)
95-104: Bridge thread polling at 10ms is intentional; correct the secondary reference.The 10ms polling in
_bridge_thread_func(line 102) is intentional for responsiveness and acceptable—it updates the MCPBridge and checks the game process status. However, the second reference ("Also applies to: 106-116") is incorrect; line 140's 10ms delay is part of a bounded shutdown wait loop (max 1s timeout), not continuous polling, so the scheduling cost concern does not apply there.
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @main/main.cpp:
- Around line 619-624: The MCP CLI flags registered via
print_help_option("--mcp-server", "--mcp-bridge-port", "--run-tests") are marked
CLI_OPTION_AVAILABILITY_EDITOR but their parsing is controlled by TOOLS_ENABLED
elsewhere, causing a mismatch; either change the availability constant to
include templates (e.g., CLI_OPTION_AVAILABILITY_TEMPLATES or combined
availability) for these three options or move/guard their parsing behind
MODULE_MCP_ENABLED && TOOLS_ENABLED so the availability and parsing match;
update the calls to print_help_option or the parsing guards around MCP-related
flags (references: print_help_option, "--mcp-server", "--mcp-bridge-port",
"--run-tests", MODULE_MCP_ENABLED, TOOLS_ENABLED) accordingly.
- Around line 3000-3004: Startup can emit non-JSON output before the MCP server
disables stdout, risking JSON-RPC corruption; when mcp_server_enabled is true,
call Engine::get_singleton()->set_print_to_stdout(false) as early as possible
(e.g., at the start of Main::setup() immediately after parsing --mcp-server
and/or at the top of Main::setup2()) and ensure print_header(false) is skipped
when mcp_server_enabled; update code paths that currently delay the
set_print_to_stdout(false) so no output (including header fragments) is written
to stdout before MCPServer::start() is invoked.
- Around line 1426-1454: The early "no main scene / can't detect run mode"
bailout logic only exempts mcp_server_enabled, which prevents --run-tests from
reaching MCPServer::run_tests(...); update those bailout conditions to also
exempt when !mcp_run_tests.is_empty() (or equivalent check) so that if
mcp_run_tests is set the program does not early-exit, and apply the same change
at the other mentioned bailout sites (the similar checks around lines referenced
in the review).
- Around line 4779-4790: Replace the use of OS::get_singleton()->print(...) for
MCPBridge connection errors with a stderr-based diagnostic (e.g.,
OS::get_singleton()->printerr(...) or fprintf(stderr, ...)) so the message does
not go to stdout; update both error branches around MCPBridge::get_singleton()
and the failed connect_to_server() call to write the same formatted error text
to stderr instead of stdout, preserving the port and error code information and
the same descriptive messages.
In @modules/mcp/mcp_bridge.cpp:
- Around line 280-284: The code calls vp->get_texture()->get_image() without
checking vp->get_texture() (and image) which can dereference null; change the
block around Ref<Image> img = vp->get_texture()->get_image() to first obtain a
local Texture* tex = vp->get_texture(), return/skip/error if tex is null, then
call tex->get_image() and verify the returned Ref<Image> img is valid before
calling img->resize; keep existing behavior when both are non-null.
In @modules/mcp/mcp_server.cpp:
- Around line 174-178: MCPServer::_write_line currently writes to stdout (which
is fine for protocol responses) but the codebase must ensure all diagnostics and
non-protocol logs go to stderr; keep MCPServer::_write_line as the stdout-only
response emitter, and change any diagnostic/logging calls (create or update
functions that emit errors or debug messages) to write to stderr (e.g.,
fprintf(stderr, ...) and fflush(stderr)). If you need a helper, add
MCPServer::_write_diag or similar to centralize stderr diagnostics so responses
remain untouched on stdout. Ensure fflush is called on the corresponding stream
after each write.
In @modules/mcp/tests/verify_workflow.py:
- Around line 36-41: The send_request function must guard against a terminated
process and broken pipes: before writing, check proc.poll() and raise/return a
clear error if it is not None (process exited); wrap proc.stdin.write and
proc.stdin.flush in a try/except that catches BrokenPipeError and raises/returns
a clear failure message that includes the proc state or PID; ensure these checks
are performed in send_request so callers fail fast when the subprocess is dead.
- Around line 60-180: The main() function currently swallows all errors in the
broad "except Exception as e" block which hides real failures; change that block
to either catch only expected exceptions (e.g., KeyboardInterrupt,
subprocess.TimeoutExpired) or log the exception and re-raise it so CI fails;
specifically update the "except Exception as e:" handler in main() to call the
logger/print with the error details and then "raise" (or replace it with
narrower except clauses), leaving the existing finally block that terminates
proc intact so the MCP process is always cleaned up.
- Around line 43-58: read_response() blocks on proc.stdout.readline() and never
drains stderr, causing timeout ineffectiveness and potential deadlock; fix by
starting the MCP subprocess with stderr=subprocess.STDOUT so stderr is merged
into stdout, and rewrite read_response to use a non-blocking, selector-based
read (e.g. selectors.DefaultSelector/select.select) to wait for proc.stdout
readiness with the remaining timeout, then read a line and json.parse it,
looping until timeout; apply the same change to the other occurrence referenced
(around line 88) and ensure you handle process termination/EOF by returning None
when the pipe closes.
🧹 Nitpick comments (4)
modules/mcp/mcp_tools.h (1)
43-98: API shape is clear and pragmatic for the first cut.Optional: consider reusing the
MCP*Contentstructs frommodules/mcp/mcp_types.hinsideToolResulthelpers to keep key naming (mimeType, etc.) consistent in one place.modules/mcp/mcp_server.cpp (1)
180-189: Bridge thread loop frequency (10ms) may be unnecessarily hot.
Consider backing off when disconnected/idle, or making the interval configurable.modules/mcp/mcp_bridge.cpp (2)
98-105: Consider disconnecting existing connection before reconnecting.If
connect_to_serveris called while a connection already exists, the old connection is replaced without explicitly disconnecting. WhileRef<>cleanup will eventually handle it, explicitly disconnecting ensures a clean state.Suggested improvement
Error MCPBridge::connect_to_server(const String &p_host, int p_port) { MutexLock lock(mutex); is_host = false; + if (connection.is_valid()) { + connection->disconnect_from_host(); + } connection.instantiate(); port = p_port; fprintf(stderr, "[MCP] Game process connecting to bridge at %s:%d\n", p_host.utf8().get_data(), port); return connection->connect_to_host(p_host, p_port); }
128-154: Consider restructuring to avoidgoto.The
goto parsed;at line 139 works correctly but is an unusual control flow pattern. A cleaner approach would use a flag or refactor the loop.Suggested refactor to remove goto
// Wait for response (blocking with timeout) uint64_t start_time = OS::get_singleton()->get_ticks_msec(); String response_str; + bool got_response = false; while (OS::get_singleton()->get_ticks_msec() - start_time < 5000) { conn->poll(); if (conn->get_available_bytes() > 0) { uint8_t c; int read_bytes; conn->get_partial_data(&c, 1, read_bytes); if (read_bytes > 0) { if (c == '\n') { - goto parsed; + got_response = true; + break; } response_str += (char)c; } } else { OS::get_singleton()->delay_usec(1000); } } - { + if (!got_response) { Dictionary err; err["error"] = "Bridge timeout"; return err; } -parsed: if (response_str.is_empty()) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pause_verify.pngis excluded by!**/*.png
📒 Files selected for processing (10)
init_check.txtmain/main.cppmodules/mcp/mcp_bridge.cppmodules/mcp/mcp_server.cppmodules/mcp/mcp_server.hmodules/mcp/mcp_tools.cppmodules/mcp/mcp_tools.hmodules/mcp/mcp_types.hmodules/mcp/tests/verify_workflow.pypost_fix_tools_check.txt
✅ Files skipped from review due to trivial changes (2)
- init_check.txt
- post_fix_tools_check.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/mcp/mcp_tools.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
modules/mcp/mcp_bridge.cpp (1)
modules/mcp/mcp_bridge.h (1)
MCPBridge(41-63)
modules/mcp/mcp_server.cpp (4)
modules/mcp/mcp_server.h (1)
MCPServer(45-71)modules/mcp/mcp_protocol.h (1)
MCPProtocol(43-74)modules/mcp/mcp_protocol.cpp (4)
MCPProtocol(39-48)MCPProtocol(50-55)_bind_methods(57-65)_bind_methods(57-57)modules/mcp/mcp_bridge.cpp (2)
_bind_methods(63-65)_bind_methods(63-63)
main/main.cpp (1)
modules/mcp/mcp_server.cpp (2)
run_tests(310-346)run_tests(310-310)
🪛 Clang (14.0.6)
modules/mcp/mcp_types.h
[error] 35-35: 'core/string/ustring.h' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.10)
modules/mcp/tests/verify_workflow.py
88-88: subprocess call: check for execution of untrusted input
(S603)
171-171: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (11)
main/main.cpp (2)
154-158: MCP compile guards and state flags look fine.Also applies to: 297-301
4866-4870: Per-frameMCPBridge::update()hook is reasonable.modules/mcp/mcp_server.cpp (1)
58-99: Destructor wait-for-exit is a good safety guard.modules/mcp/mcp_types.h (2)
40-150: Content + schema helpers are simple and usable.
35-39: The include path#include "core/string/ustring.h"is correct and will not cause a build break. The header file exists at the specified location (/home/jailuser/git/core/string/ustring.h), and this include pattern matches the standard convention used throughout the codebase for other core includes. The "clang hint" in the original review appears to be a false positive.Likely an incorrect or invalid review comment.
modules/mcp/mcp_server.h (1)
45-102: LGTM! Clean header design with proper thread-safety primitives.The class structure follows engine conventions well. The singleton pattern, atomic flags for cross-thread signaling, and mutable mutex for const accessors are all correctly implemented. The public API is well-documented with clear intent.
modules/mcp/mcp_bridge.cpp (5)
54-65: LGTM on initialization and cleanup.The
Ref<TCPServer>andRef<StreamPeerTCP>will automatically clean up when the object is destroyed. Binding onlyupdate()is appropriate if other methods are intended for C++ callers only.
67-87: Good localhost-only binding for security.The auto-scan range (10000-10999) and localhost restriction are sensible defaults for local IPC.
197-211: Manual mutex unlock/lock is risky but implemented correctly.The pattern here is valid: unlock before
_process_command(which may block), then re-lock before accessing shared state. Both theifandelsebranches properly re-acquire the lock. The connection validity is re-checked at line 205 after re-locking.However, this manual approach is error-prone for future modifications. Consider extracting the command processing into a separate scope or documenting the invariant.
338-340: Blocking delays during command processing may cause frame hitching.The 50ms
delay_useccalls (lines 258 and 340) block the thread during input simulation. While necessary for reliable input processing, this could cause visible hitching if called frequently. This is acceptable for AI automation tooling but worth noting.
368-398: Good recursive tree builder with sensible limits.The depth limit (
max_depth = 5default) and skipping of internal nodes (@@prefix) help keep the response payload manageable. Thestd::functionlambda captures by reference, which is safe here since it doesn't escape the function scope.
| #ifdef MODULE_MCP_ENABLED | ||
|
|
||
| print_help_option("--mcp-server", "Start the MCP (Model Context Protocol) server for AI agent integration. Implies --headless.\n", CLI_OPTION_AVAILABILITY_EDITOR); | ||
| print_help_option("--mcp-bridge-port <port>", "Port for the MCP Bridge connection (internal use).\n", CLI_OPTION_AVAILABILITY_EDITOR); | ||
| print_help_option("--run-tests <path>", "Run a unit test script headlessly and exit.\n", CLI_OPTION_AVAILABILITY_EDITOR); | ||
| #endif |
There was a problem hiding this comment.
CLI help availability for MCP flags looks mismatched.
These options are not parsed under TOOLS_ENABLED, yet they’re marked CLI_OPTION_AVAILABILITY_EDITOR (E). Consider marking them as available in templates too (or guard parsing by TOOLS_ENABLED if editor-only is intended).
🤖 Prompt for AI Agents
In @main/main.cpp around lines 619 - 624, The MCP CLI flags registered via
print_help_option("--mcp-server", "--mcp-bridge-port", "--run-tests") are marked
CLI_OPTION_AVAILABILITY_EDITOR but their parsing is controlled by TOOLS_ENABLED
elsewhere, causing a mismatch; either change the availability constant to
include templates (e.g., CLI_OPTION_AVAILABILITY_TEMPLATES or combined
availability) for these three options or move/guard their parsing behind
MODULE_MCP_ENABLED && TOOLS_ENABLED so the availability and parsing match;
update the calls to print_help_option or the parsing guards around MCP-related
flags (references: print_help_option, "--mcp-server", "--mcp-bridge-port",
"--run-tests", MODULE_MCP_ENABLED, TOOLS_ENABLED) accordingly.
| } else if (arg == "--mcp-server") { // Start MCP server for AI agent integration. | ||
| mcp_server_enabled = true; | ||
| audio_driver = NULL_AUDIO_DRIVER; | ||
| display_driver = NULL_DISPLAY_DRIVER; | ||
| } else if (arg == "--mcp-bridge-port") { // Port for MCP Bridge (game side) | ||
| if (N) { | ||
| int port = N->get().to_int(); | ||
| if (port > 0 && port < 65536) { | ||
| mcp_bridge_port = port; | ||
| } else { | ||
| OS::get_singleton()->print("Invalid port number for --mcp-bridge-port: %d. Must be between 1 and 65535.\n", port); | ||
| goto error; | ||
| } | ||
| N = N->next(); | ||
| } else { | ||
| OS::get_singleton()->print("Missing <port> argument for --mcp-bridge-port <port>.\n"); | ||
| goto error; | ||
| } | ||
| } else if (arg == "--run-tests") { // Run a unit test script headlessly and exit. | ||
| if (N) { | ||
| mcp_run_tests = N->get(); | ||
| N = N->next(); | ||
| audio_driver = NULL_AUDIO_DRIVER; | ||
| display_driver = NULL_DISPLAY_DRIVER; | ||
| } else { | ||
| OS::get_singleton()->print("Missing <path> argument for --run-tests <path>.\n"); | ||
| goto error; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
--run-tests can be blocked by earlier “no main scene / can’t detect run mode” checks.
Both early bailout sites only exempt mcp_server_enabled, not !mcp_run_tests.is_empty(), so --run-tests may never reach the later MCPServer::run_tests(...) call.
Proposed fix
#ifdef TOOLS_ENABLED
- if (!editor && !project_manager
+ if (!editor && !project_manager
#ifdef MODULE_MCP_ENABLED
&& !mcp_server_enabled
+ && mcp_run_tests.is_empty()
#endif
) {
#endif
const String error_msg = "Error: Can't run project: no main scene defined in the project.\n";
OS::get_singleton()->print("%s", error_msg.utf8().get_data());
OS::get_singleton()->alert(error_msg);
goto error;
#ifdef TOOLS_ENABLED
}
#endif
} #ifdef TOOLS_ENABLED
- if (!editor && !project_manager && !cmdline_tool && script.is_empty() && game_path.is_empty()
+ if (!editor && !project_manager && !cmdline_tool && script.is_empty() && game_path.is_empty()
#ifdef MODULE_MCP_ENABLED
&& !mcp_server_enabled
+ && mcp_run_tests.is_empty()
#endif
) {
OS::get_singleton()->alert("Couldn't detect whether to run the editor, the project manager or a specific project. Aborting.");
ERR_FAIL_V_MSG(EXIT_FAILURE, "Couldn't detect whether to run the editor, the project manager or a specific project. Aborting.");
}
#endifAlso applies to: 2202-2207, 4287-4292, 4800-4825
🤖 Prompt for AI Agents
In @main/main.cpp around lines 1426 - 1454, The early "no main scene / can't
detect run mode" bailout logic only exempts mcp_server_enabled, which prevents
--run-tests from reaching MCPServer::run_tests(...); update those bailout
conditions to also exempt when !mcp_run_tests.is_empty() (or equivalent check)
so that if mcp_run_tests is set the program does not early-exit, and apply the
same change at the other mentioned bailout sites (the similar checks around
lines referenced in the review).
| #ifdef MODULE_MCP_ENABLED | ||
| if (!mcp_server_enabled) | ||
| #endif | ||
| print_header(false); | ||
|
|
There was a problem hiding this comment.
MCP server mode likely still leaks non-JSON output to stdout before the server starts.
You skip print_header(false) when mcp_server_enabled, but Engine::get_singleton()->set_print_to_stdout(false) is only done late in Main::start() (right before MCPServer::start()), after a lot of startup has already run. If anything prints to stdout during setup2()/startup (even a blank line/header fragment), it can corrupt the JSON-RPC stream.
Suggested direction: when mcp_server_enabled, disable stdout printing as early as possible (e.g., at the top of Main::setup2() and/or earlier in Main::setup() right after parsing --mcp-server).
Also applies to: 4800-4817
🤖 Prompt for AI Agents
In @main/main.cpp around lines 3000 - 3004, Startup can emit non-JSON output
before the MCP server disables stdout, risking JSON-RPC corruption; when
mcp_server_enabled is true, call
Engine::get_singleton()->set_print_to_stdout(false) as early as possible (e.g.,
at the start of Main::setup() immediately after parsing --mcp-server and/or at
the top of Main::setup2()) and ensure print_header(false) is skipped when
mcp_server_enabled; update code paths that currently delay the
set_print_to_stdout(false) so no output (including header fragments) is written
to stdout before MCPServer::start() is invoked.
| #ifdef MODULE_MCP_ENABLED | ||
| if (mcp_bridge_port != 0) { | ||
| if (MCPBridge::get_singleton()) { | ||
| Error err = MCPBridge::get_singleton()->connect_to_server("127.0.0.1", mcp_bridge_port); | ||
| if (err != OK) { | ||
| OS::get_singleton()->print("Error: MCPBridge failed to connect to server at 127.0.0.1:%d. Error code: %d\n", mcp_bridge_port, err); | ||
| } | ||
| } else { | ||
| OS::get_singleton()->print("Error: MCPBridge singleton is null despite module being enabled.\n"); | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Bridge connect error logging should avoid stdout in MCP-related flows.
OS::get_singleton()->print(...) goes to stdout; if this ever runs in a context where stdout is expected to be JSON-only, it can break framing. Prefer printerr(...)/fprintf(stderr, ...) for diagnostics.
🤖 Prompt for AI Agents
In @main/main.cpp around lines 4779 - 4790, Replace the use of
OS::get_singleton()->print(...) for MCPBridge connection errors with a
stderr-based diagnostic (e.g., OS::get_singleton()->printerr(...) or
fprintf(stderr, ...)) so the message does not go to stdout; update both error
branches around MCPBridge::get_singleton() and the failed connect_to_server()
call to write the same formatted error text to stderr instead of stdout,
preserving the port and error code information and the same descriptive
messages.
| String MCPServer::_read_line() { | ||
| #ifdef WINDOWS_ENABLED | ||
| // On Windows, raw non-blocking reading from stdin is complex. | ||
| // Fallback to simpler blocking read for now to ensure compatibility. | ||
| std::string line; | ||
| if (std::getline(std::cin, line)) { | ||
| return String::utf8(line.c_str()); | ||
| } | ||
| should_stop = true; | ||
| return String(); | ||
| #else | ||
| while (!should_stop) { | ||
| // 1. Check if we already have a complete line in the buffer | ||
| int newline_pos = stdin_buffer.find("\n"); | ||
| if (newline_pos != -1) { | ||
| String line = stdin_buffer.substr(0, newline_pos); | ||
| stdin_buffer = stdin_buffer.substr(newline_pos + 1); | ||
| return line.strip_edges(); | ||
| } | ||
|
|
||
| // 2. No line? Wait for data using poll | ||
| struct pollfd p_fds[2]; | ||
| p_fds[0].fd = STDIN_FILENO; | ||
| p_fds[0].events = POLLIN; | ||
| p_fds[1].fd = wake_fds[0]; | ||
| p_fds[1].events = POLLIN; | ||
|
|
||
| int ret = poll(p_fds, 2, 100); // 100ms timeout for responsiveness | ||
| if (ret < 0) { | ||
| if (errno == EINTR) { | ||
| continue; | ||
| } | ||
| should_stop = true; | ||
| return String(); | ||
| } | ||
|
|
||
| if (ret > 0) { | ||
| if (p_fds[1].revents & POLLIN) { | ||
| // Woken up by internal pipe | ||
| char c; | ||
| int r = read(wake_fds[0], &c, 1); | ||
| (void)r; | ||
| return String(); | ||
| } | ||
|
|
||
| if (p_fds[0].revents & POLLIN) { | ||
| // Raw read from stdin | ||
| char buf[4096]; | ||
| ssize_t bytes = read(STDIN_FILENO, buf, sizeof(buf)); | ||
| if (bytes > 0) { | ||
| stdin_buffer += String::utf8(buf, bytes); | ||
| // Continue to loop to extract the line from buffer | ||
| } else if (bytes == 0) { | ||
| // EOF | ||
| should_stop = true; | ||
| return String(); | ||
| } else if (errno != EAGAIN && errno != EINTR) { | ||
| should_stop = true; | ||
| return String(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return String(); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Windows: stop() cannot interrupt the blocking std::getline() read.
On Windows, stop() sets should_stop = true but _read_line() won’t observe it until stdin yields a line/EOF, so shutdown can hang (and the destructor may hit the 3s timeout and then free protocol while the loop is still blocked).
At minimum, implement a Windows wake mechanism (e.g., wait with timeout on the stdin handle + an event) or run the read loop with a nonblocking/timeout-capable API so should_stop is honored promptly.
Also applies to: 296-308
| void MCPServer::_write_line(const String &p_line) { | ||
| CharString utf8 = p_line.utf8(); | ||
| fprintf(stdout, "%s\n", utf8.get_data()); | ||
| fflush(stdout); | ||
| } |
There was a problem hiding this comment.
stdout writes are OK for responses, but keep all diagnostics off stdout.
Ensure all non-protocol logs use stderr only, otherwise clients parsing stdout will break.
🤖 Prompt for AI Agents
In @modules/mcp/mcp_server.cpp around lines 174 - 178, MCPServer::_write_line
currently writes to stdout (which is fine for protocol responses) but the
codebase must ensure all diagnostics and non-protocol logs go to stderr; keep
MCPServer::_write_line as the stdout-only response emitter, and change any
diagnostic/logging calls (create or update functions that emit errors or debug
messages) to write to stderr (e.g., fprintf(stderr, ...) and fflush(stderr)). If
you need a helper, add MCPServer::_write_diag or similar to centralize stderr
diagnostics so responses remain untouched on stdout. Ensure fflush is called on
the corresponding stream after each write.
| def send_request(proc, method, params, req_id=1): | ||
| req = {"jsonrpc": "2.0", "id": req_id, "method": method, "params": params} | ||
| msg = json.dumps(req) | ||
| proc.stdin.write(msg + "\n") | ||
| proc.stdin.flush() | ||
|
|
There was a problem hiding this comment.
send_request() should handle broken pipes / terminated process.
At least guard proc.poll() and catch BrokenPipeError to fail fast with a clear message.
🤖 Prompt for AI Agents
In @modules/mcp/tests/verify_workflow.py around lines 36 - 41, The send_request
function must guard against a terminated process and broken pipes: before
writing, check proc.poll() and raise/return a clear error if it is not None
(process exited); wrap proc.stdin.write and proc.stdin.flush in a try/except
that catches BrokenPipeError and raises/returns a clear failure message that
includes the proc state or PID; ensure these checks are performed in
send_request so callers fail fast when the subprocess is dead.
| def read_response(proc, timeout=5.0): | ||
| start_time = time.time() | ||
| while time.time() - start_time < timeout: | ||
| line = proc.stdout.readline() | ||
| if not line: | ||
| return None | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| return json.loads(line) | ||
| except json.JSONDecodeError: | ||
| print(f"[MCP LOG] {line}", file=sys.stderr) | ||
| continue | ||
| return None | ||
|
|
There was a problem hiding this comment.
read_response() timeout is ineffective + stderr pipe can deadlock the child.
proc.stdout.readline() can block indefinitely, and stderr=subprocess.PIPE is never drained (risking the MCP server blocking once the stderr buffer fills).
Proposed fix (merge stderr into stdout + selector-based nonblocking reads)
import argparse
import base64
import json
import os
import subprocess
import sys
import time
+import selectors
+
+_READ_BUF = ""
def send_request(proc, method, params, req_id=1):
req = {"jsonrpc": "2.0", "id": req_id, "method": method, "params": params}
msg = json.dumps(req)
proc.stdin.write(msg + "\n")
proc.stdin.flush()
def read_response(proc, timeout=5.0):
- start_time = time.time()
- while time.time() - start_time < timeout:
- line = proc.stdout.readline()
- if not line:
- return None
- line = line.strip()
- if not line:
- continue
- try:
- return json.loads(line)
- except json.JSONDecodeError:
- print(f"[MCP LOG] {line}", file=sys.stderr)
- continue
- return None
+ global _READ_BUF
+ sel = selectors.DefaultSelector()
+ sel.register(proc.stdout, selectors.EVENT_READ)
+ deadline = time.time() + timeout
+ while time.time() < deadline:
+ if proc.poll() is not None:
+ return None
+ remaining = max(0.0, deadline - time.time())
+ events = sel.select(timeout=remaining)
+ if not events:
+ continue
+ chunk = os.read(proc.stdout.fileno(), 4096).decode("utf-8", errors="replace")
+ if not chunk:
+ return None
+ _READ_BUF += chunk
+ while "\n" in _READ_BUF:
+ line, _READ_BUF = _READ_BUF.split("\n", 1)
+ line = line.strip()
+ if not line:
+ continue
+ try:
+ return json.loads(line)
+ except json.JSONDecodeError:
+ print(f"[MCP LOG] {line}", file=sys.stderr)
+ return None- proc = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)
+ proc = subprocess.Popen(
+ cmd,
+ stdin=subprocess.PIPE,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ text=False,
+ )Also applies to: 88-88
🤖 Prompt for AI Agents
In @modules/mcp/tests/verify_workflow.py around lines 43 - 58, read_response()
blocks on proc.stdout.readline() and never drains stderr, causing timeout
ineffectiveness and potential deadlock; fix by starting the MCP subprocess with
stderr=subprocess.STDOUT so stderr is merged into stdout, and rewrite
read_response to use a non-blocking, selector-based read (e.g.
selectors.DefaultSelector/select.select) to wait for proc.stdout readiness with
the remaining timeout, then read a line and json.parse it, looping until
timeout; apply the same change to the other occurrence referenced (around line
88) and ensure you handle process termination/EOF by returning None when the
pipe closes.
| def main(): | ||
| parser = argparse.ArgumentParser(description="Verify Redot MCP Server end-to-end workflow") | ||
| parser.add_argument( | ||
| "--binary", required=True, help="Path to Redot editor binary (e.g. ./bin/redot.linuxbsd.editor.x86_64)" | ||
| ) | ||
| parser.add_argument("--project", required=True, help="Path to Redot project directory") | ||
| parser.add_argument( | ||
| "--click-node", default="MainMenu/MenuPanel/VBoxContainer/StartButton", help="Node path to click after launch" | ||
| ) | ||
| parser.add_argument( | ||
| "--trigger-action", default="ui_cancel", help="Input action to trigger (e.g. ui_cancel for Pause)" | ||
| ) | ||
| parser.add_argument("--wait-load", type=float, default=15.0, help="Seconds to wait for game load") | ||
| parser.add_argument("--wait-gameplay", type=float, default=5.0, help="Seconds to wait after click") | ||
| parser.add_argument("--screenshot-file", default="mcp_verify_capture.png", help="Output path for screenshot") | ||
|
|
||
| args = parser.parse_args() | ||
|
|
||
| if not os.path.exists(args.binary): | ||
| print(f"Error: Binary not found at {args.binary}") | ||
| sys.exit(1) | ||
| if not os.path.exists(args.project): | ||
| print(f"Error: Project path not found at {args.project}") | ||
| sys.exit(1) | ||
|
|
||
| cmd = [os.path.abspath(args.binary), "--headless", "--mcp-server", "--path", os.path.abspath(args.project)] | ||
|
|
||
| print(f"Starting MCP server: {' '.join(cmd)}") | ||
| proc = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) | ||
|
|
||
| try: | ||
| # 1. Initialize | ||
| print("\n--- Sending Initialize ---") | ||
| send_request( | ||
| proc, | ||
| "initialize", | ||
| { | ||
| "protocolVersion": "2024-11-05", | ||
| "capabilities": {}, | ||
| "clientInfo": {"name": "VerifyScript", "version": "1.0"}, | ||
| }, | ||
| req_id=1, | ||
| ) | ||
| resp = read_response(proc) | ||
| print(f"Response: {json.dumps(resp, indent=2)}") | ||
|
|
||
| # 2. Run Game | ||
| print("\n--- Launching Game ---") | ||
| send_request(proc, "tools/call", {"name": "project_config", "arguments": {"action": "run"}}, req_id=2) | ||
| resp = read_response(proc) | ||
| print(f"Response: {json.dumps(resp, indent=2)}") | ||
|
|
||
| print(f"Waiting {args.wait_load}s for game to load...") | ||
| time.sleep(args.wait_load) | ||
|
|
||
| # 3. Click Start Game | ||
| if args.click_node: | ||
| print(f"\n--- Clicking Node: {args.click_node} ---") | ||
| send_request( | ||
| proc, | ||
| "tools/call", | ||
| {"name": "game_control", "arguments": {"action": "click", "node_path": args.click_node}}, | ||
| req_id=3, | ||
| ) | ||
| resp = read_response(proc) | ||
| print(f"Response: {json.dumps(resp, indent=2)}") | ||
|
|
||
| print(f"Waiting {args.wait_gameplay}s for gameplay...") | ||
| time.sleep(args.wait_gameplay) | ||
|
|
||
| # 4. Trigger Action | ||
| if args.trigger_action: | ||
| print(f"\n--- Triggering Action: {args.trigger_action} ---") | ||
| send_request( | ||
| proc, | ||
| "tools/call", | ||
| {"name": "game_control", "arguments": {"action": "trigger_action", "action_name": args.trigger_action}}, | ||
| req_id=4, | ||
| ) | ||
| resp = read_response(proc) | ||
| print(f"Response: {json.dumps(resp, indent=2)}") | ||
|
|
||
| time.sleep(2) | ||
|
|
||
| # 5. Capture Screenshot | ||
| print("\n--- Capturing Screenshot ---") | ||
| send_request( | ||
| proc, "tools/call", {"name": "game_control", "arguments": {"action": "capture", "scale": 0.5}}, req_id=5 | ||
| ) | ||
| resp = read_response(proc, timeout=10.0) # Larger timeout for image transfer | ||
|
|
||
| if resp and "result" in resp: | ||
| content = resp["result"].get("content", []) | ||
| has_image = False | ||
| for item in content: | ||
| if item.get("type") == "image": | ||
| data = base64.b64decode(item["data"]) | ||
| with open(args.screenshot_file, "wb") as f: | ||
| f.write(data) | ||
| print(f"Screenshot saved to {args.screenshot_file}") | ||
| has_image = True | ||
| break | ||
|
|
||
| if not has_image: | ||
| print("Warning: Capture returned no image content.") | ||
| print(json.dumps(resp, indent=2)) | ||
| else: | ||
| print("Capture failed or timed out.") | ||
|
|
||
| except KeyboardInterrupt: | ||
| print("\nInterrupted by user.") | ||
| except Exception as e: | ||
| print(f"\nError: {e}") | ||
| finally: | ||
| print("\nTerminating MCP server...") | ||
| proc.terminate() | ||
| try: | ||
| proc.wait(timeout=2) | ||
| except subprocess.TimeoutExpired: | ||
| proc.kill() | ||
|
|
There was a problem hiding this comment.
Avoid catching broad Exception in the harness (or re-raise after logging).
This can hide real failures in CI runs.
🧰 Tools
🪛 Ruff (0.14.10)
88-88: subprocess call: check for execution of untrusted input
(S603)
171-171: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In @modules/mcp/tests/verify_workflow.py around lines 60 - 180, The main()
function currently swallows all errors in the broad "except Exception as e"
block which hides real failures; change that block to either catch only expected
exceptions (e.g., KeyboardInterrupt, subprocess.TimeoutExpired) or log the
exception and re-raise it so CI fails; specifically update the "except Exception
as e:" handler in main() to call the logger/print with the error details and
then "raise" (or replace it with narrower except clauses), leaving the existing
finally block that terminates proc intact so the MCP process is always cleaned
up.
| ### Tools Overview | ||
| The MCP server provides 5 master controllers: | ||
|
|
||
| * **`redot_scene_action`**: Manage `.tscn` files (add nodes, set properties, instance scenes, and wire signals with automatic callback generation). | ||
| * **`redot_resource_action`**: Manage `.tres` files and assets (create/modify materials, themes, inspect `.import` metadata). | ||
| * **`redot_code_intel`**: Deep script analysis (GDScript syntax validation, symbol extraction, and engine documentation lookup). | ||
| * **`redot_project_config`**: Project-level control (configure Input Map, Autoloads, run/stop the game, read logs, and res:// I/O). | ||
| * **`redot_game_control`**: Vision & Interaction (capture screenshots, click UI elements with high-precision, and inspect the live scene tree recursively). | ||
|
|
||
| ### Running the MCP Server | ||
|
|
||
| #### 1. Using the compiled binary (Standard) | ||
| ```json | ||
| { | ||
| "mcp": { | ||
| "redot": { | ||
| "type": "local", | ||
| "command": [ | ||
| "/path/to/redot.editor.binary", | ||
| "--headless", | ||
| "--mcp-server", | ||
| "--path", | ||
| "/path/to/your/project" | ||
| ], | ||
| "enabled": true | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| #### 2. Running Unit Tests | ||
| You can execute automated tests headlessly using the `--run-tests` flag: | ||
| ```bash | ||
| ./bin/redot.<platform> --headless --run-tests=res://tests/my_test.gd | ||
| ``` | ||
| *Note: Your test script should have a `func run():` method.* | ||
|
|
||
|
|
||
| #### 3. Using Nix (For Development) | ||
| If you are developing inside a Nix environment, use the provided wrapper to ensure all libraries are correctly linked: | ||
|
|
||
| 1. Locate the `redot-mcp.sh` script in the engine root. | ||
| 2. In your `opencode.json` or MCP client config, use: | ||
| ```json | ||
| { | ||
| "mcp": { | ||
| "redot": { | ||
| "type": "local", | ||
| "command": ["/path/to/redot-engine/redot-mcp.sh", "/path/to/your/project"], | ||
| "enabled": true | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### AI Agent Best Practices | ||
| To get the most out of Redot's MCP server, agents should follow these guidelines: | ||
|
|
||
| 1. **Scene Editing**: Use `redot_scene_action` for all `.tscn` modifications. Avoid editing TSCN files as raw text to prevent breaking node UIDs and internal references. | ||
| 2. **Scripting**: For existing `.gd` files, use **native text editing tools** (like `edit`) for precise logic changes. Use `redot_project_config(action="create_file_res")` only when creating new scripts from scratch. | ||
| 3. **Live Interaction**: Always `wait` 3-5 seconds after `run` before attempting vision or input actions to allow the bridge to initialize. | ||
| 4. **Spatial Awareness**: Use `redot_game_control(action="inspect_live", recursive=true)` to discover UI paths and their pre-calculated screen coordinates for 100% accurate clicking. | ||
| 5. **Debugging**: Use `redot_project_config(action="output")` to read real-time logs and `redot_code_intel(action="validate")` to syntax-check fixes before running the game. | ||
| 6. **Script Editing Policy**: The MCP tool `create_file_res` is restricted to creating **new** files. To edit existing `.gd` scripts, agents **must** use native text editing tools (like `edit`). This ensures precision and prevents accidental overwrites of complex logic. |
There was a problem hiding this comment.
This is probably good for the time being. But ideally I think we would want to keep the main README.md very brief. I'd like to get this information moved over to https://docs.redotengine.org/ eventually once I get that situated. It doesnt look like the docs site is updating like it is supposed to, but that is a problem for a different day.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@modules/mcp/mcp_tools.cpp`:
- Around line 824-832: Handle errors from OS::get_singleton()->create_process
and report unknown actions: capture the return value of
OS::get_singleton()->create_process(...) when launching the editor (called with
get_executable_path(), args using "--editor" and
ProjectSettings::get_singleton()->get_resource_path()) and if it indicates
failure set result as failure and include an explanatory error message;
additionally add a final else/default branch for unrecognized action values that
sets result to an error/failure and includes the unknown action string in the
message so callers won’t get a silent success for invalid actions.
- Around line 641-689: When MODULE_GDSCRIPT_ENABLED is defined and action isn't
"validate" or "get_symbols", the code currently falls through and returns an
empty result; add an explicit handler for unknown actions: after the existing if
(action == "validate" || action == "get_symbols") block, set an error on result
(e.g., result.set_error(String("Unknown code_intel action: ") + action)) and
return result so callers get a clear failure for unrecognized actions; ensure
this branch lives inside the MODULE_GDSCRIPT_ENABLED block alongside the
existing paths.
- Around line 499-507: In MCPTools::tool_resource_action, validate the incoming
path by calling validate_path(path) before invoking normalize_path(path); if
validate_path returns false (or indicates invalid), set an appropriate error on
ToolResult (e.g., "Invalid path") and return immediately so no file operations
or normalize_path are performed on unsafe input. Ensure you reference the
existing validate_path function and keep the rest of the logic unchanged once
the path is validated.
- Around line 571-585: The code path handling resource actions (variable action)
lacks a final else branch, so when action is not "inspect", "modify", or
"duplicate" the function returns a success result; add a default error branch
after the existing action handlers that calls result.set_error with a clear
message (include the unknown action string, e.g. using action) so callers get an
error for unsupported actions; use the existing result variable and ensure this
branch executes before the function returns.
♻️ Duplicate comments (4)
modules/mcp/mcp_tools.cpp (4)
76-103: Silent script modification may surprise users.This function automatically appends callback stubs to user scripts without explicit consent. This can:
- Create uncommitted changes that surprise users in version control
- Break code formatting or linting rules
- Cause merge conflicts in collaborative workflows
Consider logging a warning before modification, or returning an error instructing the user to add the callback manually.
448-465: Signal connection lacks existence check.The
connectaction doesn't verify that the signal exists on the source node before callingsource->connect(). This could cause runtime errors if an invalid signal name is provided.Proposed fix
} else if (action == "connect") { String node_path = p_args.get("node_path", "."); String sig = p_args.get("signal", ""); String target_path = p_args.get("target_node", "."); String method = p_args.get("method", ""); Node *source = (node_path == "." || node_path.is_empty()) ? root : root->get_node_or_null(node_path); Node *target = (target_path == "." || target_path.is_empty()) ? root : root->get_node_or_null(target_path); if (!source || !target) { result.set_error("Node not found"); } else { + if (!source->has_signal(sig)) { + result.set_error("Signal '" + sig + "' not found on source node"); + } else if (source->is_connected(sig, Callable(target, method))) { + result.add_text("Signal already connected"); + } else { Ref<Resource> script_res = target->get_script(); if (script_res.is_valid()) { _ensure_callback_exists(script_res->get_path(), method); } source->connect(sig, Callable(target, method)); should_save = true; result.add_text("Connected signal"); + } }
838-843: Unbounded wait duration could hang the server.The
waitaction accepts a user-controlledsecondsparameter without validation. A malicious or buggy client could pass a very large value (or negative), blocking the MCP server thread indefinitely.Proposed fix
if (action == "wait") { float secs = p_args.get("seconds", 1.0); + const float MAX_WAIT_SECONDS = 60.0f; + if (secs < 0.0f) { + result.set_error("Wait duration must be non-negative"); + return result; + } + if (secs > MAX_WAIT_SECONDS) { + result.set_error("Wait duration exceeds maximum of " + String::num(MAX_WAIT_SECONDS) + " seconds"); + return result; + } OS::get_singleton()->delay_usec(secs * 1000000); result.add_text("Waited " + String::num(secs) + " seconds on server."); return result; }
466-496: Missing handler for unknown scene actions.If an unrecognized
actionis provided, the function silently returns success with empty content (since no branch matches andshould_saveremains false). This could confuse callers expecting an error.Proposed fix
} else if (action == "reparent") { // ... existing reparent logic ... + } else { + result.set_error("Unknown scene action: " + action); + memdelete(root); + return result; } if (should_save) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/mcp/mcp_tools.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🍎 macOS / Template (target=template_release)
🔇 Additional comments (3)
modules/mcp/mcp_tools.cpp (3)
145-256: LGTM!Tool definitions are well-structured with clear descriptions and appropriate required field specifications.
262-282: LGTM!The dispatcher correctly routes to tool implementations and provides a clear error for unknown tools.
844-861: LGTM!The bridge delegation and response handling logic is clean and properly handles error, image, and JSON response types.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| MCPTools::ToolResult MCPTools::tool_resource_action(const Dictionary &p_args) { | ||
| ToolResult result; | ||
| String action = p_args.get("action", ""); | ||
| String path = p_args.get("path", ""); | ||
| if (action.is_empty() || path.is_empty()) { | ||
| result.set_error("Missing action or path"); | ||
| return result; | ||
| } | ||
| String normalized = normalize_path(path); |
There was a problem hiding this comment.
Missing path validation before file operations.
tool_resource_action calls normalize_path(path) at Line 507 without first calling validate_path(path). This allows paths containing .. to bypass the security check, potentially enabling path traversal attacks.
Proposed fix
String action = p_args.get("action", "");
String path = p_args.get("path", "");
if (action.is_empty() || path.is_empty()) {
result.set_error("Missing action or path");
return result;
}
+ if (!validate_path(path)) {
+ result.set_error("Invalid path: " + path);
+ return result;
+ }
String normalized = normalize_path(path);🤖 Prompt for AI Agents
In `@modules/mcp/mcp_tools.cpp` around lines 499 - 507, In
MCPTools::tool_resource_action, validate the incoming path by calling
validate_path(path) before invoking normalize_path(path); if validate_path
returns false (or indicates invalid), set an appropriate error on ToolResult
(e.g., "Invalid path") and return immediately so no file operations or
normalize_path are performed on unsafe input. Ensure you reference the existing
validate_path function and keep the rest of the logic unchanged once the path is
validated.
| } else if (action == "duplicate") { | ||
| String np = p_args.get("new_path", ""); | ||
| if (np.is_empty()) { | ||
| result.set_error("Missing new_path"); | ||
| } else { | ||
| Ref<Resource> copy = res->duplicate(); | ||
| Error err = ResourceSaver::save(copy, normalize_path(np)); | ||
| if (err != OK) { | ||
| result.set_error("Failed to save duplicated resource: " + itos(err)); | ||
| } else { | ||
| result.add_text("Duplicated resource"); | ||
| } | ||
| } | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Missing handler for unknown resource actions.
After loading the resource, if the action is not inspect, modify, or duplicate, the function returns an empty success result instead of an error.
Proposed fix
} else if (action == "duplicate") {
// ... existing duplicate logic ...
+ } else {
+ result.set_error("Unknown resource action: " + action);
}
return result;🤖 Prompt for AI Agents
In `@modules/mcp/mcp_tools.cpp` around lines 571 - 585, The code path handling
resource actions (variable action) lacks a final else branch, so when action is
not "inspect", "modify", or "duplicate" the function returns a success result;
add a default error branch after the existing action handlers that calls
result.set_error with a clear message (include the unknown action string, e.g.
using action) so callers get an error for unsupported actions; use the existing
result variable and ensure this branch executes before the function returns.
| #ifdef MODULE_GDSCRIPT_ENABLED | ||
| if (action == "validate" || action == "get_symbols") { | ||
| Error err; | ||
| Ref<FileAccess> f = FileAccess::open(normalized, FileAccess::READ, &err); | ||
| if (err != OK) { | ||
| result.set_error("Failed to open script"); | ||
| return result; | ||
| } | ||
| String source = f->get_as_text(); | ||
| GDScriptParser parser; | ||
| Error parse_err = parser.parse(source, normalized, false); | ||
| if (parse_err != OK) { | ||
| String el = "Validation failed:\n"; | ||
| for (const GDScriptParser::ParserError &e : parser.get_errors()) { | ||
| el += "Line " + itos(e.line) + ": " + e.message + "\n"; | ||
| } | ||
| result.set_error(el); | ||
| } else { | ||
| if (action == "validate") { | ||
| result.add_text("Valid"); | ||
| } else { | ||
| Dictionary symbols; | ||
| const GDScriptParser::ClassNode *head = parser.get_tree(); | ||
| if (head) { | ||
| Array functions, variables, signals; | ||
| for (int i = 0; i < head->members.size(); i++) { | ||
| const GDScriptParser::ClassNode::Member &m = head->members[i]; | ||
| if (m.type == GDScriptParser::ClassNode::Member::FUNCTION) { | ||
| functions.push_back(m.get_name()); | ||
| } else if (m.type == GDScriptParser::ClassNode::Member::VARIABLE) { | ||
| variables.push_back(m.get_name()); | ||
| } else if (m.type == GDScriptParser::ClassNode::Member::SIGNAL) { | ||
| signals.push_back(m.get_name()); | ||
| } | ||
| } | ||
| symbols["functions"] = functions; | ||
| symbols["variables"] = variables; | ||
| symbols["signals"] = signals; | ||
| } | ||
| result.add_text(JSON::stringify(symbols, " ")); | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
| #else | ||
| result.set_error("GDScript module disabled"); | ||
| #endif | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Missing handler for unknown code_intel actions.
When MODULE_GDSCRIPT_ENABLED is defined, if the action is not validate or get_symbols, the function falls through to return an empty result at Line 688 without reporting an error.
Proposed fix
`#ifdef` MODULE_GDSCRIPT_ENABLED
if (action == "validate" || action == "get_symbols") {
// ... existing logic ...
return result;
}
+ result.set_error("Unknown code_intel action: " + action);
`#else`
result.set_error("GDScript module disabled");
`#endif`
return result;🤖 Prompt for AI Agents
In `@modules/mcp/mcp_tools.cpp` around lines 641 - 689, When
MODULE_GDSCRIPT_ENABLED is defined and action isn't "validate" or "get_symbols",
the code currently falls through and returns an empty result; add an explicit
handler for unknown actions: after the existing if (action == "validate" ||
action == "get_symbols") block, set an error on result (e.g.,
result.set_error(String("Unknown code_intel action: ") + action)) and return
result so callers get a clear failure for unrecognized actions; ensure this
branch lives inside the MODULE_GDSCRIPT_ENABLED block alongside the existing
paths.
| } else if (action == "open_editor") { | ||
| List<String> args; | ||
| args.push_back("--editor"); | ||
| args.push_back("--path"); | ||
| args.push_back(ProjectSettings::get_singleton()->get_resource_path()); | ||
| OS::get_singleton()->create_process(OS::get_singleton()->get_executable_path(), args); | ||
| result.add_text("Opening editor"); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Missing error handling for editor launch and unknown actions.
Two issues in this segment:
create_processat Line 829 doesn't capture the returned error, so failures are silently ignored.- If an unrecognized action is provided, the function returns an empty success result.
Proposed fix
} else if (action == "open_editor") {
List<String> args;
args.push_back("--editor");
args.push_back("--path");
args.push_back(ProjectSettings::get_singleton()->get_resource_path());
- OS::get_singleton()->create_process(OS::get_singleton()->get_executable_path(), args);
- result.add_text("Opening editor");
+ Error err = OS::get_singleton()->create_process(OS::get_singleton()->get_executable_path(), args);
+ if (err != OK) {
+ result.set_error("Failed to open editor: " + itos(err));
+ } else {
+ result.add_text("Opening editor");
+ }
+ } else {
+ result.set_error("Unknown project_config action: " + action);
}
return result;🤖 Prompt for AI Agents
In `@modules/mcp/mcp_tools.cpp` around lines 824 - 832, Handle errors from
OS::get_singleton()->create_process and report unknown actions: capture the
return value of OS::get_singleton()->create_process(...) when launching the
editor (called with get_executable_path(), args using "--editor" and
ProjectSettings::get_singleton()->get_resource_path()) and if it indicates
failure set result as failure and include an explanatory error message;
additionally add a final else/default branch for unrecognized action values that
sets result to an error/failure and includes the unknown action string in the
message so callers won’t get a silent success for invalid actions.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@modules/mcp/mcp_bridge.cpp`:
- Around line 131-146: The current send_command read loop in mcp_bridge.cpp
drops bytes after the first newline because response_str is local; fix by
introducing a persistent member buffer (e.g., partial_response or
partial_response_buffer) on the MCPBridge class, consume any data already in
that buffer at the start of send_command before polling the socket, and when
reading from conn->get_partial_data append all bytes into a temporary string,
split at the first '\n' to append the line to response_str and push any bytes
after the newline back into the persistent buffer for the next call; update
references in the loop that use response_str and the goto parsed flow to use
this buffering approach so no bytes are lost between invocations.
- Around line 344-345: The click handler contains a blocking
OS::get_singleton()->delay_usec(50000) call (same problem as
_trigger_action_event) which stalls the main thread; replace this blocking delay
with a non-blocking timer-based approach (e.g., SceneTreeTimer via
get_tree()->create_timer(0.05) or a Timer node) and move the follow-up logic
into the timer timeout callback or connected method so the handler returns
immediately; mirror the refactor you applied to _trigger_action_event so both
places use the same non-blocking timing mechanism and invoke the existing
post-delay logic from the timeout handler.
- Around line 257-259: The current code blocks the main loop by calling
OS::get_singleton()->delay_usec(50000) between
Input::get_singleton()->parse_input_event(press_ev) and
parse_input_event(release_ev); replace this blocking wait with a deferred or
timer-based release: immediately parse the press_ev, then schedule the
release_ev via call_deferred or by creating/starting a one-shot Timer (or using
SceneTree::create_timer) to invoke a helper method that calls
Input::get_singleton()->parse_input_event(release_ev) after ~50ms; update or add
a small helper function (e.g., send_release_event or on_release_timer_timeout)
to receive and dispatch release_ev so no blocking occurs in Main::iteration.
♻️ Duplicate comments (3)
modules/mcp/mcp_bridge.cpp (3)
285-292: Missing null check on image afterget_image().While the texture null check was added,
tex->get_image()can still return null (e.g., if the texture hasn't been rendered yet). Subsequent operations on a nullimgwill crash.Proposed fix
Ref<Image> img = tex->get_image(); + if (img.is_null()) { + resp["error"] = "Failed to get image from viewport"; + return resp; + } float scale = args.get("scale", 1.0); - if (scale != 1.0) { + if (scale != 1.0 && scale > 0.0) { img->resize(img->get_width() * scale, img->get_height() * scale); }
197-211: Invalid JSON commands are silently dropped.When
cmd_var.get_type() != Variant::DICTIONARY, the code re-locks the mutex but doesn't send an error response. The sender will wait indefinitely or timeout with no feedback.Proposed fix
mutex.unlock(); Variant cmd_var = JSON::parse_string(cmd_str); + Dictionary resp; if (cmd_var.get_type() == Variant::DICTIONARY) { - Dictionary resp = _process_command(cmd_var); - String resp_json = JSON::stringify(resp); - CharString utf8 = resp_json.utf8(); - - mutex.lock(); - if (connection.is_valid() && connection->get_status() == StreamPeerTCP::STATUS_CONNECTED) { - connection->put_data((const uint8_t *)utf8.get_data(), utf8.length()); - connection->put_u8('\n'); - } + resp = _process_command(cmd_var); } else { - mutex.lock(); + resp["error"] = "Invalid command JSON (expected object)"; } + String resp_json = JSON::stringify(resp); + CharString utf8 = resp_json.utf8(); + + mutex.lock(); + if (connection.is_valid() && connection->get_status() == StreamPeerTCP::STATUS_CONNECTED) { + connection->put_data((const uint8_t *)utf8.get_data(), utf8.length()); + connection->put_u8('\n'); + }
359-409: Missing error response whenSceneTreeis null.If
stis null at line 361, the function returns an emptyrespwhich callers cannot distinguish from success. Add an explicit error.Proposed fix
} else if (action == "inspect_live") { SceneTree *st = Object::cast_to<SceneTree>(OS::get_singleton()->get_main_loop()); if (st) { // ... existing code ... + } else { + resp["error"] = "No scene tree found"; } } else if (action == "type") {
🧹 Nitpick comments (1)
modules/mcp/mcp_bridge.cpp (1)
59-61: Destructor should clean up TCP resources.The destructor only nulls the singleton but doesn't disconnect the client or stop the server. This can leave lingering sockets.
Proposed fix
MCPBridge::~MCPBridge() { + MutexLock lock(mutex); + if (connection.is_valid()) { + connection->disconnect_from_host(); + connection.unref(); + } + if (server.is_valid()) { + server->stop(); + server.unref(); + } + partial_data = ""; singleton = nullptr; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/mcp/mcp_bridge.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
modules/mcp/mcp_bridge.cpp (1)
modules/mcp/mcp_bridge.h (1)
MCPBridge(41-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
🔇 Additional comments (4)
modules/mcp/mcp_bridge.cpp (4)
67-87: LGTM!The server binding correctly enforces loopback-only connections via
IPAddress("127.0.0.1")and handles port auto-scan with proper error returns.
89-105: LGTM!Thread-safe connection state management with proper mutex locking.
410-460: LGTM!The type action correctly handles both single characters and bracketed special keys (e.g.,
[Enter]), with proper modifier key support and immediate press/release without blocking delays.
461-480: LGTM!The
trigger_actionhandler properly validates input, and unknown actions now return explicit error responses.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| while (OS::get_singleton()->get_ticks_msec() - start_time < 5000) { | ||
| conn->poll(); | ||
| if (conn->get_available_bytes() > 0) { | ||
| uint8_t c; | ||
| int read_bytes; | ||
| conn->get_partial_data(&c, 1, read_bytes); | ||
| if (read_bytes > 0) { | ||
| if (c == '\n') { | ||
| goto parsed; | ||
| } | ||
| response_str += (char)c; | ||
| } | ||
| } else { | ||
| OS::get_singleton()->delay_usec(1000); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bytes after newline are dropped, risking protocol desync.
If the peer sends data past the newline (e.g., pipelined responses), those bytes are lost. The response_str buffer is local and doesn't persist remainder bytes for subsequent reads.
Consider using a persistent receive buffer (similar to partial_data on the server side) to store any bytes read past the newline for the next send_command call.
🤖 Prompt for AI Agents
In `@modules/mcp/mcp_bridge.cpp` around lines 131 - 146, The current send_command
read loop in mcp_bridge.cpp drops bytes after the first newline because
response_str is local; fix by introducing a persistent member buffer (e.g.,
partial_response or partial_response_buffer) on the MCPBridge class, consume any
data already in that buffer at the start of send_command before polling the
socket, and when reading from conn->get_partial_data append all bytes into a
temporary string, split at the first '\n' to append the line to response_str and
push any bytes after the newline back into the persistent buffer for the next
call; update references in the loop that use response_str and the goto parsed
flow to use this buffering approach so no bytes are lost between invocations.
| Input::get_singleton()->parse_input_event(press_ev); | ||
| OS::get_singleton()->delay_usec(50000); // 50ms hold | ||
| Input::get_singleton()->parse_input_event(release_ev); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "mcp_bridge" | head -20Repository: Redot-Engine/redot-engine
Length of output: 120
🏁 Script executed:
wc -l modules/mcp/mcp_bridge.cppRepository: Redot-Engine/redot-engine
Length of output: 99
🏁 Script executed:
sed -n '225,263p' modules/mcp/mcp_bridge.cppRepository: Redot-Engine/redot-engine
Length of output: 1304
🏁 Script executed:
grep -n "_process_command" modules/mcp/mcp_bridge.cpp | head -20Repository: Redot-Engine/redot-engine
Length of output: 195
🏁 Script executed:
sed -n '190,220p' modules/mcp/mcp_bridge.cppRepository: Redot-Engine/redot-engine
Length of output: 999
🏁 Script executed:
sed -n '150,200p' modules/mcp/mcp_bridge.cpp | head -60Repository: Redot-Engine/redot-engine
Length of output: 1469
🏁 Script executed:
grep -n "\.update()" modules/mcp/mcp_bridge.cppRepository: Redot-Engine/redot-engine
Length of output: 51
🏁 Script executed:
cat modules/mcp/mcp_bridge.hRepository: Redot-Engine/redot-engine
Length of output: 3572
🏁 Script executed:
rg "MCPBridge" --type cpp -A 3 -B 1 | grep -A 5 "update()"Repository: Redot-Engine/redot-engine
Length of output: 735
🏁 Script executed:
rg "MCPBridge::get_singleton\(\)->update" main/main.cpp -B 10 -A 5Repository: Redot-Engine/redot-engine
Length of output: 604
Remove the blocking delay on the main thread; it will cause frame stutter.
The 50ms delay_usec() call blocks execution in Main::iteration() — the main game loop. This happens every time an action is triggered through the MCP bridge, causing visible hitches in gameplay.
Use the engine's timer or deferred call system (e.g., call_deferred()) to schedule the key/mouse release event instead of blocking.
🤖 Prompt for AI Agents
In `@modules/mcp/mcp_bridge.cpp` around lines 257 - 259, The current code blocks
the main loop by calling OS::get_singleton()->delay_usec(50000) between
Input::get_singleton()->parse_input_event(press_ev) and
parse_input_event(release_ev); replace this blocking wait with a deferred or
timer-based release: immediately parse the press_ev, then schedule the
release_ev via call_deferred or by creating/starting a one-shot Timer (or using
SceneTree::create_timer) to invoke a helper method that calls
Input::get_singleton()->parse_input_event(release_ev) after ~50ms; update or add
a small helper function (e.g., send_release_event or on_release_timer_timeout)
to receive and dispatch release_ev so no blocking occurs in Main::iteration.
| // 3. Wait 50ms to ensure engine processes the down state | ||
| OS::get_singleton()->delay_usec(50000); |
There was a problem hiding this comment.
Same blocking delay issue in click handler.
The 50ms delay_usec at line 345 has the same main-thread blocking issue as _trigger_action_event. Both should be refactored to use non-blocking timing.
🤖 Prompt for AI Agents
In `@modules/mcp/mcp_bridge.cpp` around lines 344 - 345, The click handler
contains a blocking OS::get_singleton()->delay_usec(50000) call (same problem as
_trigger_action_event) which stalls the main thread; replace this blocking delay
with a non-blocking timer-based approach (e.g., SceneTreeTimer via
get_tree()->create_timer(0.05) or a Timer node) and move the follow-up logic
into the timer timeout callback or connected method so the handler returns
immediately; mirror the refactor you applied to _trigger_action_event so both
places use the same non-blocking timing mechanism and invoke the existing
post-delay logic from the timeout handler.
…edot-Engine#1154) This PR introduces a native C++ implementation of the Model Context Protocol (MCP) as a first-class engine module. This enables AI coding assistants (OpenCode, Claude, etc.) to interact deeply with Redot projects through a standardized JSON-RPC interface. Beyond simple file editing, this module provides the AI with "eyes and hands"—allowing it to see the game window, manipulate the live scene tree, and simulate user input for truly closed-loop development and debugging.
2 participants
Summary
This PR introduces a native C++ implementation of the Model Context Protocol (MCP) as a first-class engine module. This enables AI coding assistants (OpenCode, Claude, etc.) to interact deeply with Redot projects through a standardized JSON-RPC interface.
Beyond simple file editing, this module provides the AI with "eyes and hands"—allowing it to see the game window, manipulate the live scene tree, and simulate user input for truly closed-loop development and debugging.
Key Features
--mcp-server. Communicates over stdio using JSON-RPC 2.0.redot_scene_action: High-level manipulation of.tscnfiles (instancing, wiring signals with auto-callback generation).redot_resource_action: Deep modification of.tresfiles (Materials, SpriteFrames, Themes).redot_code_intel: Native GDScript validation and engine-wide documentation lookup.redot_project_config: Global management of project settings, input maps, and game lifecycle.redot_game_control: Vision & Interaction suite. Allows the AI to capture viewport screenshots, inspect the live scene tree of a running game, and inject native mouse/keyboard events.Technical Implementation
modules/mcp/.JSONRPCcapabilities.MainLoopfrom the MCP thread.Future Roadmap: Native Unit Testing
This module lays the groundwork for a highly requested Native Unit Testing tool. The next phase will leverage the MCP Bridge to implement a CLI-first testing suite:
redot --headless --run-tests=test_pause_menu.gdReviewer Notes
devbranch (e.g., enabled by default in certain builds, or kept as a manual opt-in).Resolves MichaelFisher1997#2
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.