MCP: Audit and harden native MCP server implementation (#1290)

* MCP: Audit and harden native MCP server implementation

Fixes several bugs, improves spec compliance, and fills in missing
functionality across the MCP module.

Bugfixes:
- Fix null-deref crash in capture action when viewport returns no image
  or PNG encoding fails (mcp_bridge.cpp)
- Fix double-wrapped JSON-RPC error responses: pre-init tools/* requests
  now return a proper top-level error via process_string override instead
  of being nested inside a result envelope (mcp_protocol.{h,cpp})
- Fix double-wrapping on INVALID_PARAMS paths in tools/call handler

Protocol compliance:
- Update protocol version from 2024-11-05 to 2025-06-18
- Add version negotiation: echo client version if supported, else return
  latest (mcp_protocol.cpp)
- Move initialized flag from initialize response to notifications/initialized
  per spec lifecycle requirements
- Declare tools.listChanged capability explicitly

Security and robustness:
- Harden validate_path: segment-based .. check instead of substring match,
  applied consistently to all path-accepting actions (instance_path,
  resource paths, code_intel paths were previously unvalidated)
- Fix bridge concurrency: update() no longer drops an active connection
  mid-command when a new peer connects
- Improve send_command: chunked reads via CharString accumulation with
  single-pass UTF-8 decode to handle multi-byte chars across TCP boundaries

New functionality:
- Implement code_intel search action (recursive .gd file content search)
- Implement project_config add_input action (writes InputEventKey to
  project.godot with runtime InputMap registration)
- Implement project_config add_autoload action (writes autoload entry
  to project.godot)

Quality improvements:
- Add MCP tool annotations (readOnlyHint, destructiveHint, etc.) to all
  5 tools per 2025-06-18 spec
- Clarify project_config schema descriptions (which params each action
  uses) to prevent agent confusion
- Improve _ensure_callback_exists: word-boundary detection handles
  static func and annotation prefixes
- Add signal-exists validation before connect() in scene_action
- Remove dead code (_make_text_content)
- Fill in class reference documentation for MCPServer, MCPBridge,
  MCPProtocol

AI tools were used in the development of this contribution, in
accordance with the project AI Policy. All changes have been manually
tested end-to-end with a live MCP handshake across all 5 tools.

* MCP: Validate autoload files and refresh input actions

Reject nonexistent autoload paths before modifying project settings. When an input action already exists, replace its live deadzone and event bindings so its runtime state matches the persisted project configuration.

Author: MichaelFisher1997

modules/mcp/doc_classes/MCPBridge.xml

@@ -1,15 +1,18 @@
 <?xml version="1.0" encoding="UTF-8" ?>
 <class name="MCPBridge" inherits="Object" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../doc/class.xsd">
 	<brief_description>
+		TCP bridge relaying commands between the MCP server and a running game process.
 	</brief_description>
 	<description>
+		[MCPBridge] is a bidirectional TCP relay. On the server (headless) side it listens on an ephemeral loopback port and sends commands produced by MCP tools. On the game side (started with the [code]--mcp-bridge-port[/code] flag) it connects back to that port, receives commands, executes them against the live scene tree/viewport, and returns results. The bridge is single-connection and automatically replaces a stale peer when a new game process connects.
 	</description>
 	<tutorials>
 	</tutorials>
 	<methods>
 		<method name="update">
 			<return type="void" />
 			<description>
+				Pumps the bridge: on the host side it accepts pending connections; on the game side it reads incoming commands, dispatches them, and writes responses. Called each frame from the main loop (game side) or the server's bridge thread (host side).
 			</description>
 		</method>
 	</methods>

modules/mcp/doc_classes/MCPProtocol.xml

@@ -1,15 +1,18 @@
 <?xml version="1.0" encoding="UTF-8" ?>
 <class name="MCPProtocol" inherits="JSONRPC" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../doc/class.xsd">
 	<brief_description>
+		MCP JSON-RPC protocol handler: capability negotiation, tool listing, and dispatch.
 	</brief_description>
 	<description>
+		Implements the Model Context Protocol request layer on top of the JSONRPC class. Handles the [code]initialize[/code] handshake with version negotiation, the [code]notifications/initialized[/code] lifecycle event, [code]ping[/code], [code]tools/list[/code], and [code]tools/call[/code]. Tool execution is delegated to the internal MCPTools class. The operation phase is gated until the [code]initialized[/code] notification is received, per spec.
 	</description>
 	<tutorials>
 	</tutorials>
 	<methods>
 		<method name="is_initialized" qualifiers="const">
 			<return type="bool" />
 			<description>
+				Returns [code]true[/code] after the client has sent the [code]notifications/initialized[/code] handshake message, marking the start of the operation phase.
 			</description>
 		</method>
 	</methods>

modules/mcp/doc_classes/MCPServer.xml

@@ -1,25 +1,30 @@
 <?xml version="1.0" encoding="UTF-8" ?>
 <class name="MCPServer" inherits="Object" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../doc/class.xsd">
 	<brief_description>
+		Stdio-based MCP (Model Context Protocol) server for AI agent integration.
 	</brief_description>
 	<description>
+		The MCP server exposes the running Redot project to AI coding assistants over a JSON-RPC stdio transport. It is started with the [code]--mcp-server[/code] command-line flag, which implies headless mode. The server reads newline-delimited JSON-RPC requests on stdin and writes responses to stdout. A TCP bridge ([MCPBridge]) is used to forward tool calls (screenshots, input, tree inspection) to a separately launched game process. See [url=https://modelcontextprotocol.io/]the MCP specification[/url] for protocol details.
 	</description>
 	<tutorials>
 	</tutorials>
 	<methods>
 		<method name="is_running" qualifiers="const">
 			<return type="bool" />
 			<description>
+				Returns [code]true[/code] while the server loop is processing requests.
 			</description>
 		</method>
 		<method name="start">
 			<return type="void" />
 			<description>
+				Starts the blocking server loop. Reads requests from stdin and dispatches them to the registered protocol handlers. Returns when the input stream is closed or [method stop] is requested.
 			</description>
 		</method>
 		<method name="stop">
 			<return type="void" />
 			<description>
+				Signals the server loop to stop and wakes the stdin poll.
 			</description>
 		</method>
 	</methods>

modules/mcp/mcp_bridge.cpp

@@ -133,31 +133,57 @@ Dictionary MCPBridge::send_command(const String &p_action, const Dictionary &p_a
 
 	// Wait for response (blocking with timeout)
 	uint64_t start_time = OS::get_singleton()->get_ticks_msec();
-	String response_str;
+	CharString raw_buffer;
+	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);
+		int avail = conn->get_available_bytes();
+		if (avail > 0) {
+			uint8_t buf[4096];
+			int to_read = avail < (int)sizeof(buf) ? avail : (int)sizeof(buf);
+			int read_bytes = 0;
+			conn->get_partial_data(buf, to_read, read_bytes);
 			if (read_bytes > 0) {
-				if (c == '\n') {
-					goto parsed;
+				// Append raw bytes to the buffer (resize + copy).
+				int old_len = raw_buffer.length();
+				raw_buffer.resize_uninitialized(old_len + read_bytes + 1);
+				char *dst = raw_buffer.ptrw();
+				memcpy(dst + old_len, buf, read_bytes);
+				dst[old_len + read_bytes] = 0;
+
+				// Only scan the newly appended bytes for the delimiter.
+				for (int i = old_len; i < old_len + read_bytes; i++) {
+					if (raw_buffer[i] == '\n') {
+						got_response = true;
+						break;
+					}
+				}
+				if (got_response) {
+					break;
 				}
-				response_str += (char)c;
 			}
 		} else {
 			OS::get_singleton()->delay_usec(1000);
 		}
 	}
 
-	{
+	if (!got_response) {
 		Dictionary err;
 		err["error"] = "Bridge timeout";
 		return err;
 	}
 
-parsed:
+	// Decode the full buffer at once so multi-byte UTF-8 sequences
+	// spanning TCP chunk boundaries are handled correctly.
+	int nl = -1;
+	for (int i = 0; i < raw_buffer.length(); i++) {
+		if (raw_buffer[i] == '\n') {
+			nl = i;
+			break;
+		}
+	}
+	int len = (nl >= 0) ? nl : raw_buffer.length();
+	String response_str = String::utf8(raw_buffer.get_data(), len);
 	if (response_str.is_empty()) {
 		Dictionary err;
 		err["error"] = "Empty response";
@@ -178,12 +204,22 @@ void MCPBridge::update() {
 	MutexLock lock(mutex);
 	if (is_host) {
 		if (server->is_connection_available()) {
+			// Only replace the connection if the current one is stale/disconnected,
+			// so we never drop a peer mid-command from the bridge thread.
+			bool current_ok = false;
 			if (connection.is_valid()) {
-				fprintf(stderr, "[MCP] Dropping existing connection for new client\n");
-				connection->disconnect_from_host();
+				connection->poll();
+				current_ok = (connection->get_status() == StreamPeerTCP::STATUS_CONNECTED);
+			}
+			if (!current_ok) {
+				connection = server->take_connection();
+				fprintf(stderr, "[MCP] Game process connected to bridge on host side\n");
+			} else {
+				// Drain and discard the extra pending connection.
+				Ref<StreamPeerTCP> extra = server->take_connection();
+				extra->disconnect_from_host();
+				fprintf(stderr, "[MCP] Bridge: extra pending connection discarded (active connection in use)\n");
 			}
-			connection = server->take_connection();
-			fprintf(stderr, "[MCP] Game process connected to bridge on host side\n");
 		}
 	} else {
 		// Client (Game) side
@@ -289,13 +325,21 @@ Dictionary MCPBridge::_process_command(const Dictionary &p_cmd) {
 				return resp;
 			}
 			Ref<Image> img = tex->get_image();
+			if (img.is_null()) {
+				resp["error"] = "Viewport returned no image (not rendered yet or headless)";
+				return resp;
+			}
 
 			float scale = args.get("scale", 1.0);
 			if (scale != 1.0) {
 				img->resize(img->get_width() * scale, img->get_height() * scale);
 			}
 
 			PackedByteArray png_buffer = img->save_png_to_buffer();
+			if (png_buffer.is_empty()) {
+				resp["error"] = "Failed to encode PNG";
+				return resp;
+			}
 			resp["image_base64"] = CryptoCore::b64_encode_str(png_buffer.ptr(), png_buffer.size());
 			resp["format"] = "png";
 			resp["width"] = img->get_width();

modules/mcp/mcp_protocol.cpp

@@ -72,8 +72,8 @@ void MCPProtocol::_bind_methods() {
 
 Dictionary MCPProtocol::_make_capabilities() const {
 	Dictionary caps;
-	// We support tools
 	Dictionary tools_cap;
+	tools_cap["listChanged"] = false;
 	caps["tools"] = tools_cap;
 	return caps;
 }
@@ -85,13 +85,27 @@ Dictionary MCPProtocol::_make_server_info() const {
 	return info;
 }
 
+String MCPProtocol::_negotiate_version(const String &p_client_version) {
+	if (p_client_version == MCP_PROTOCOL_VERSION_LATEST || p_client_version == MCP_PROTOCOL_VERSION_LEGACY) {
+		return p_client_version;
+	}
+	return MCP_PROTOCOL_VERSION_LATEST;
+}
+
 Variant MCPProtocol::_handle_initialize(const Variant &p_params) {
+	String client_version;
+	if (p_params.get_type() == Variant::DICTIONARY) {
+		Dictionary params = p_params;
+		Variant pv = params.get("protocolVersion", Variant());
+		if (pv.get_type() == Variant::STRING) {
+			client_version = pv;
+		}
+	}
+
 	Dictionary result;
-	result["protocolVersion"] = MCP_PROTOCOL_VERSION;
+	result["protocolVersion"] = _negotiate_version(client_version);
 	result["capabilities"] = _make_capabilities();
 	result["serverInfo"] = _make_server_info();
-
-	initialized = true;
 	return result;
 }
 
@@ -100,17 +114,22 @@ Array MCPProtocol::_get_tool_definitions() const {
 }
 
 Variant MCPProtocol::_handle_tools_list(const Variant &p_params) {
-	if (!is_initialized()) {
-		return make_response_error(INVALID_REQUEST, "Protocol not initialized");
+	// Defense-in-depth: process_string gates this, but guard anyway in case
+	// dispatch bypasses the override (e.g. via base pointer or a notification).
+	if (!initialized) {
+		Dictionary result;
+		result["tools"] = Array();
+		return result;
 	}
 	Dictionary result;
 	result["tools"] = _get_tool_definitions();
 	return result;
 }
 
 Variant MCPProtocol::_handle_tools_call(const Variant &p_params) {
-	if (!is_initialized()) {
-		return make_response_error(INVALID_REQUEST, "Protocol not initialized");
+	// Defense-in-depth: never execute a tool before the init handshake.
+	if (!initialized) {
+		return _make_tool_result_error("Protocol not initialized");
 	}
 	Dictionary params;
 	if (p_params.get_type() == Variant::DICTIONARY) {
@@ -120,15 +139,15 @@ Variant MCPProtocol::_handle_tools_call(const Variant &p_params) {
 		if (!arr.is_empty() && arr[0].get_type() == Variant::DICTIONARY) {
 			params = arr[0];
 		} else {
-			return make_response_error(INVALID_PARAMS, "Tool call parameters must be an object (or array with object)");
+			return _make_tool_result_error("Tool call parameters must be an object (or array with object)");
 		}
 	} else {
-		return make_response_error(INVALID_PARAMS, "Tool call parameters must be an object");
+		return _make_tool_result_error("Tool call parameters must be an object");
 	}
 
 	String tool_name = params.get("name", "");
 	if (tool_name.is_empty()) {
-		return make_response_error(INVALID_PARAMS, "Missing tool name");
+		return _make_tool_result_error("Missing tool name");
 	}
 
 	Dictionary arguments;
@@ -154,18 +173,55 @@ Dictionary MCPProtocol::_make_tool_result(const Array &p_content, bool p_is_erro
 	return result;
 }
 
-Dictionary MCPProtocol::_make_text_content(const String &p_text) const {
+Dictionary MCPProtocol::_make_tool_result_error(const String &p_message) const {
 	Dictionary content;
 	content["type"] = "text";
-	content["text"] = p_text;
-	return content;
+	content["text"] = "Error: " + p_message;
+
+	Array content_arr;
+	content_arr.push_back(content);
+
+	Dictionary result;
+	result["content"] = content_arr;
+	result["isError"] = true;
+	return result;
 }
 
 Variant MCPProtocol::_handle_initialized_notification(const Variant &p_params) {
-	// Notification, no return value needed (but Variant() is void)
+	// Per spec, the operation phase begins after this notification.
+	initialized = true;
 	return Variant();
 }
 
 Variant MCPProtocol::_handle_ping(const Variant &p_params) {
 	return Dictionary(); // Empty object
 }
+
+String MCPProtocol::process_string(const String &p_input) {
+	// Gate tools/* requests before initialization at the protocol level,
+	// returning a proper JSON-RPC error (not a double-wrapped result).
+	// Notifications (no id) are silently dropped so the tool never executes.
+	if (!initialized && !p_input.is_empty()) {
+		JSON json;
+		if (json.parse(p_input) == OK) {
+			Variant data = json.get_data();
+			if (data.get_type() == Variant::DICTIONARY) {
+				Dictionary req = data;
+				String method = req.get("method", "");
+				if (method == "tools/list" || method == "tools/call") {
+					if (req.has("id")) {
+						Variant id = req["id"];
+						if (id.get_type() == Variant::FLOAT && id.operator float() == (float)(id.operator int())) {
+							id = id.operator int();
+						}
+						Dictionary resp = make_response_error(INVALID_REQUEST, "Protocol not initialized", id);
+						return JSON::stringify(resp);
+					}
+					// Notification before init: drop silently (no response permitted).
+					return String();
+				}
+			}
+		}
+	}
+	return JSONRPC::process_string(p_input);
+}

modules/mcp/mcp_protocol.h

@@ -66,11 +66,12 @@ class MCPProtocol : public JSONRPC {
 	Dictionary _make_capabilities() const;
 	Dictionary _make_server_info() const;
 	Array _get_tool_definitions() const;
+	static String _negotiate_version(const String &p_client_version);
 	/// @}
 
 	/// Make MCP-formatted tool result
 	Dictionary _make_tool_result(const Array &p_content, bool p_is_error = false) const;
-	Dictionary _make_text_content(const String &p_text) const;
+	Dictionary _make_tool_result_error(const String &p_message) const;
 
 protected:
 	static void _bind_methods();
@@ -80,4 +81,8 @@ class MCPProtocol : public JSONRPC {
 	~MCPProtocol();
 
 	bool is_initialized() const { return initialized; }
+
+	/// Override to gate pre-initialization requests (avoids double-wrapped errors).
+	/// Shadows JSONRPC::process_string; called via MCPProtocol* static dispatch.
+	String process_string(const String &p_input);
 };