MCP: Audit and harden native MCP server implementation#1290
Merged
Arctis-Fireblight merged 4 commits intoJul 11, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR updates MCP documentation and runtime behavior, including protocol version negotiation, initialization gating, TCP bridge handling, tool validation and annotations, GDScript search, project input/autoload configuration, and capture error reporting. ChangesMCP runtime and tools
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPProtocol
participant JSONRPC
participant MCPTools
Client->>MCPProtocol: initialize with protocolVersion
MCPProtocol->>Client: negotiated version and capabilities
Client->>MCPProtocol: notifications/initialized
MCPProtocol->>MCPProtocol: enable tool requests
Client->>MCPProtocol: tools/list or tools/call
MCPProtocol->>JSONRPC: process request
JSONRPC->>MCPTools: list or execute tool
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
MichaelFisher1997
force-pushed
the
mcp-audit
branch
from
June 30, 2026 20:17
f664f8c to
1e774d1
Compare
MichaelFisher1997
marked this pull request as ready for review
July 11, 2026 01:34
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/mcp/mcp_tools.cpp`:
- Around line 1000-1015: Before persisting the autoload in the add_autoload
action, verify that the normalized path exists using FileAccess::exists; if it
does not, return an appropriate error through result.set_error and avoid
modifying ProjectSettings. Keep the existing validate_path check and place the
existence guard before set_setting.
- Around line 986-998: Update the runtime registration logic in the add_input
implementation: when InputMap already contains action_name, clear its existing
events and set its deadzone to the new value before adding events; only
add_action for new actions, ensuring runtime state matches the overwritten
ProjectSettings entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62d6e11e-315f-4485-922f-db25499426c0
📒 Files selected for processing (8)
modules/mcp/doc_classes/MCPBridge.xmlmodules/mcp/doc_classes/MCPProtocol.xmlmodules/mcp/doc_classes/MCPServer.xmlmodules/mcp/mcp_bridge.cppmodules/mcp/mcp_protocol.cppmodules/mcp/mcp_protocol.hmodules/mcp/mcp_tools.cppmodules/mcp/mcp_types.h
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.
Arctis-Fireblight
approved these changes
Jul 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A comprehensive audit and hardening of the native MCP (Model Context Protocol) server module (
modules/mcp/). The module was functional but had several bugs, spec compliance gaps, and unimplemented actions advertised in the tool schemas.Bugfixes
captureaction when viewport returns no image (headless/first-frame) or PNG encoding failstools/*requests returned a JSON-RPC error nested inside aresultenvelope instead of a top-levelerror. Fixed via aprocess_stringoverride that gates pre-init requests and notificationsProtocol compliance
2024-11-05to2025-06-18with proper version negotiation (echoes client version if supported)initializedflag frominitializeresponse tonotifications/initializedper spec lifecycletools.listChangedcapability explicitlySecurity & robustness
validate_path: segment-based..traversal check, applied consistently to all path-accepting actions (previouslyinstance_path, resource paths, and code_intel paths were unvalidated)update()no longer drops an active connection mid-commandsend_command: chunked reads viaCharStringaccumulation with single-pass UTF-8 decodeNew functionality
code_intelsearchaction: recursive.gdfile content searchproject_configadd_inputaction: writesInputEventKeytoproject.godotwith runtimeInputMapregistrationproject_configadd_autoloadaction: writes autoload entry toproject.godotQuality
readOnlyHint,destructiveHint, etc.) on all 5 tools per 2025-06-18 specproject_configschema descriptions (which params each action uses)_ensure_callback_existsword-boundary detection (handlesstatic func, annotation prefixes)connect()inscene_actionMCPServer,MCPBridge,MCPProtocol_make_text_content)Testing
All changes manually tested end-to-end with a live MCP handshake (
--mcp-server --headless) across all 5 tools:code_intelget_symbols,get_docs,validate,searchgame_controlwait,capture,inspect_live,click,trigger_action,typeproject_configget_info,list_files,run,stop,output,read_file_res,create_file_res,set_setting,add_input,add_autoloadresource_actioninspect,inspect_asset,create,modify,duplicatescene_actionget_node,create,add,set_prop,instance,reparent,remove,connectPre-init gating verified: single requests return JSON-RPC error, notifications are silently dropped.
Notes
linuxbsdplatform, editor target).Checklist
scons platform=linuxbsd target=editor)Summary by CodeRabbit