Add Phase 3 Batch 1: readiness gating + signal/autoload/input_map tools#5
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2579952 to
7e4d57b
Compare
…s, project_settings_set Closes Phase 2 exit criteria (readiness gating) and begins Phase 3 with 12 new MCP tools for signals, autoloads, input actions, and project settings writes. Test output compacted to summary-only by default (verbose opt-in). Readiness gating: - GDScript Connection computes readiness (ready/importing/playing/no_scene) and sends readiness_changed events + includes readiness in handshake - Python Session tracks readiness; require_writable() gates all 20 write handlers - editor_state response includes readiness field New tools (Phase 3 Batch 1): - project_settings_set — write settings with old_value tracking + save rollback - signal_list, signal_connect, signal_disconnect — undoable signal wiring - autoload_list, autoload_add, autoload_remove — persistent autoload management - input_map_list (filters builtins by default), input_map_add_action, input_map_remove_action, input_map_bind_event (key/mouse/joy_button) Quality improvements: - All ProjectSettings mutators roll back in-memory state on save failure - Test runner returns compact summary + failures only (verbose=true for full) - Deduplicated readiness logic (Connection.get_readiness() is static, shared) - Extracted _resolve_signal_params() to eliminate copy-paste in signal handler 228 Python + 153 GDScript = 381 total tests, all passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7e4d57b to
97715c3
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the Godot MCP server/plugin with editor readiness propagation and gating for write operations, and introduces new tool surfaces for signals, autoloads, input map management, and project setting mutation.
Changes:
- Add session readiness (
ready/importing/playing/no_scene) to handshake + events, and gate Python-side write handlers viarequire_writable(). - Add new MCP tools + handlers for
signal_*,autoload_*,input_map_*, andproject_settings_set. - Update Godot-side test runner + Python tools to support compact test summaries by default with
verbose=truefor full results.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_runtime_handlers.py | Extends stubbed runtime coverage for new handlers/tools and verbose test params. |
| tests/unit/test_readiness.py | Adds unit tests for require_writable() and session readiness defaults/serialization. |
| tests/integration/test_websocket.py | Verifies readiness comes from handshake and updates from readiness_changed events. |
| tests/integration/test_project_tools.py | Adds integration coverage for set_project_setting command roundtrip. |
| tests/integration/test_mcp_tools.py | Adds integration tests for new MCP tools + readiness gating behavior. |
| tests/conftest.py | Adds readiness field to plugin handshake in the test harness. |
| test_project/tests/test_signal.gd | Adds in-editor GDScript tests for signal list/connect/disconnect. |
| test_project/tests/test_script.gd | Adds cleanup to avoid stale-node interference in attach-script tests. |
| test_project/tests/test_project.gd | Adds in-editor tests for set_project_setting. |
| test_project/tests/test_node.gd | Adds cleanup to avoid stale group membership between runs. |
| test_project/tests/test_input.gd | Adds in-editor tests for input map list/add/remove/bind. |
| test_project/tests/test_autoload.gd | Adds in-editor tests for autoload list/add/remove. |
| src/godot_ai/transport/websocket.py | Stores readiness from handshake and handles readiness_changed events. |
| src/godot_ai/tools/testing.py | Adds verbose parameter to run_tests/get_test_results MCP tools. |
| src/godot_ai/tools/signal.py | Adds MCP tool definitions for signal operations. |
| src/godot_ai/tools/project.py | Adds project_settings_set MCP tool. |
| src/godot_ai/tools/input_map.py | Adds MCP tool definitions for input map operations. |
| src/godot_ai/tools/autoload.py | Adds MCP tool definitions for autoload operations. |
| src/godot_ai/sessions/registry.py | Adds readiness to Session + serialization output. |
| src/godot_ai/server.py | Registers new signal/autoload/input_map tools. |
| src/godot_ai/protocol/envelope.py | Extends handshake schema with readiness. |
| src/godot_ai/handlers/testing.py | Passes verbose through to plugin test runner commands. |
| src/godot_ai/handlers/signal.py | Adds shared Python handlers for signal commands + write gating. |
| src/godot_ai/handlers/script.py | Adds readiness gating to script write operations. |
| src/godot_ai/handlers/scene.py | Adds readiness gating to scene write operations. |
| src/godot_ai/handlers/resource.py | Adds readiness gating to resource assignment. |
| src/godot_ai/handlers/project.py | Adds project_settings_set handler + readiness gating. |
| src/godot_ai/handlers/node.py | Adds readiness gating to node write operations and selection set. |
| src/godot_ai/handlers/input_map.py | Adds shared Python handlers for input-map commands + write gating. |
| src/godot_ai/handlers/filesystem.py | Adds readiness gating to filesystem write/reimport operations. |
| src/godot_ai/handlers/autoload.py | Adds shared Python handlers for autoload commands + write gating. |
| src/godot_ai/handlers/_readiness.py | Introduces require_writable() gating helper for write operations. |
| plugin/addons/godot_ai/testing/test_runner.gd | Adds compact test summary output + verbose mode. |
| plugin/addons/godot_ai/plugin.gd | Registers new Godot-side handlers/commands. |
| plugin/addons/godot_ai/handlers/test_handler.gd | Passes verbose through to runner and results. |
| plugin/addons/godot_ai/handlers/signal_handler.gd | Implements signal list/connect/disconnect on the Godot side. |
| plugin/addons/godot_ai/handlers/project_handler.gd | Adds set_project_setting with rollback on save failure. |
| plugin/addons/godot_ai/handlers/input_handler.gd | Implements input map list/add/remove/bind with persistence. |
| plugin/addons/godot_ai/handlers/editor_handler.gd | Adds readiness field to editor_state output. |
| plugin/addons/godot_ai/handlers/autoload_handler.gd | Implements autoload list/add/remove via ProjectSettings. |
| plugin/addons/godot_ai/connection.gd | Computes readiness and sends it in handshake + readiness_changed events. |
| docs/implementation-plan.md | Updates plan checkboxes/progress notes for readiness + new tool batch. |
| CLAUDE.md | Updates dev workflow notes for compact test output + verbose mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if old_setting != null: | ||
| ProjectSettings.clear(key) | ||
| var err := ProjectSettings.save() | ||
| if err != OK: | ||
| InputMap.add_action(action) | ||
| ProjectSettings.set_setting(key, old_setting) | ||
| return McpErrorCodes.make(McpErrorCodes.INTERNAL_ERROR, "Failed to save project settings (error %d)" % err) |
There was a problem hiding this comment.
In the save-failure rollback path, remove_action() restores the ProjectSettings key but only recreates the InputMap action with InputMap.add_action(action) (default deadzone) and without restoring previously bound events. This leaves the in-memory editor state inconsistent with old_setting after an error. Consider reconstructing the action from old_setting (deadzone + events) when rolling back.
| var deadzone: float = 0.5 | ||
| if ProjectSettings.has_setting(key): | ||
| var existing = ProjectSettings.get_setting(key) | ||
| if existing is Dictionary: | ||
| deadzone = existing.get("deadzone", 0.5) | ||
| ProjectSettings.set_setting(key, { | ||
| "deadzone": deadzone, | ||
| "events": events, | ||
| }) | ||
| return ProjectSettings.save() |
There was a problem hiding this comment.
_save_action_events() mutates ProjectSettings in-memory before calling ProjectSettings.save(), but on save failure there is no rollback to the previous setting value. This can leave the editor’s in-memory ProjectSettings/input state inconsistent even though the tool returns an error (and bind_event() only rolls back the InputMap event). Capture the old setting and restore it (or clear it) when save() fails.
| var deadzone: float = 0.5 | |
| if ProjectSettings.has_setting(key): | |
| var existing = ProjectSettings.get_setting(key) | |
| if existing is Dictionary: | |
| deadzone = existing.get("deadzone", 0.5) | |
| ProjectSettings.set_setting(key, { | |
| "deadzone": deadzone, | |
| "events": events, | |
| }) | |
| return ProjectSettings.save() | |
| var had_setting := ProjectSettings.has_setting(key) | |
| var previous_setting = null | |
| var deadzone: float = 0.5 | |
| if had_setting: | |
| var existing = ProjectSettings.get_setting(key) | |
| if existing is Dictionary: | |
| deadzone = existing.get("deadzone", 0.5) | |
| if existing is Dictionary or existing is Array: | |
| previous_setting = existing.duplicate(true) | |
| else: | |
| previous_setting = existing | |
| ProjectSettings.set_setting(key, { | |
| "deadzone": deadzone, | |
| "events": events, | |
| }) | |
| var save_result := ProjectSettings.save() | |
| if save_result != OK: | |
| if had_setting: | |
| ProjectSettings.set_setting(key, previous_setting) | |
| else: | |
| ProjectSettings.clear(key) | |
| return save_result |
| from godot_ai.tools.autoload import register_autoload_tools | ||
| from godot_ai.tools.client import register_client_tools | ||
| from godot_ai.tools.editor import register_editor_tools | ||
| from godot_ai.tools.filesystem import register_filesystem_tools | ||
| from godot_ai.tools.input_map import register_input_map_tools | ||
| from godot_ai.tools.node import register_node_tools | ||
| from godot_ai.tools.project import register_project_tools | ||
| from godot_ai.tools.resource import register_resource_tools | ||
| from godot_ai.tools.scene import register_scene_tools | ||
| from godot_ai.tools.script import register_script_tools | ||
| from godot_ai.tools.session import register_session_tools | ||
| from godot_ai.tools.signal import register_signal_tools |
There was a problem hiding this comment.
The PR description says there are 12 new MCP tools, but based on what’s registered here (signal_* x3, autoload_* x3, input_map_* x4, project_settings_set x1) this PR appears to add 11 new tool endpoints (with existing testing tools extended via a new verbose parameter). Please align the PR description with the actual tool count, or point to the missing tool if one was intended.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Updated the PR description to correctly state 11 new MCP tools (signal_* ×3, autoload_* ×3, input_map_* ×4, project_settings_set ×1).
- CLAUDE.md: 234 Python tests, readiness gating in architecture section, require_writable step in "adding a new tool" - skill.md: 12 new tools in inventory table, signal/autoload/input_map handlers listed, write handler convention added - implementation-plan.md: 234 Python + 156 GDScript = 390 total tests, autoload path validation and compact test output noted Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- remove_action rollback restores deadzone + events (not just default action) - _save_action_events rolls back ProjectSettings on save failure - button param is now Optional[int] = None; only sent when explicitly provided Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix trace counter regression flagged by Copilot: pass _startup_trace_count as a Callable into PortResolver.find_pid_on_port / find_all_pids_on_port, so each scraper that actually runs bumps its own counter (Windows netstat → PowerShell fallthrough was over-counting on every successful netstat). - De-duplicate find_pid_on_port: now a one-liner over find_all_pids_on_port. - Make SERVER_PID_FILE one source of truth: defined on McpPortResolver, re-exported as a const alias in plugin.gd so tests still see GodotAiPlugin.SERVER_PID_FILE. - Skip the can_bind_local_port test when the port is held (was flaky). - Add test coverage for stop_server PID-aggregation, finalize handoff, and check_server_health watch-loop guard (Copilot review #4 + #5). - Delete three unused plugin.gd shims (_can_bind_local_port, _find_listener_pids_windows, _windows_powershell_candidates) — no callers in production or tests. - Trim narrate-the-change docstrings in port_resolver.gd / server_lifecycle.gd / plugin.gd shims; cut ~270 LOC of redundant test cases that already exist in test_netstat_parser.gd / test_plugin_lifecycle.gd via the forwarding shims. Net diff vs origin/beta drops from +698 LOC to +420 LOC. Production code net is now +85 LOC for the two new files plus their wiring; tests are +343 (genuinely new live-OS coverage + seam-validation smoke). Validated: ruff clean, pytest 748 pass, ci-check-gdscript clean, test_run 1073/1087 pass (0 failed, 14 skipped for env preconditions), ci-reload-test full reload cycle confirmed in editor logs. https://claude.ai/code/session_01Q2mfWoUAzPPjDEdGJygagu
…uplicate-ID reject Three small audit-v2 fixes in src/godot_ai/transport/websocket.py covering #343 sub-findings #2, #4, and #5. - #4 (P1): replace hardcoded `e.errno == 48` (macOS-only) with `errno.EADDRINUSE` so Linux (98) and Windows (10048) also hit the friendly "port already in use" branch instead of crashing the WS lifespan with an unhandled OSError. - #5 (P2): wrap `send_command`'s `ws.send` + `wait_for` in a try/finally that always pops `_pending`. Pre-fix, a `ws.send` raise (ConnectionClosed mid-send, transport error) leaked the Future entry forever; under churn the dict grew unbounded. Happy path still pops via the response receiver, so the finally pop is a no-op there. - #2 (P1): reject a second handshake whose session_id is already registered (close code 4001), instead of silently overwriting the routing map. session_id format is `<slug>@<4hex>` — 16 bits of suffix is locally guessable, so without this any local peer could hijack an active session by impersonating its ID. Legitimate plugin reconnect after `editor_reload_plugin` first triggers ConnectionClosed -> unregister, so the new connect still lands cleanly. Tests: - tests/unit/test_websocket_server_lifecycle.py — parametrized errno coverage (macos 48, linux 98, windows 10048; one runs per CI host) plus a non-EADDRINUSE OSError propagation case. - tests/integration/test_websocket.py — TestDuplicateHandshake pins the reject + reconnect-after-clean-disconnect paths; TestPendingFutureCleanup pins the timeout-pop and send-failure-pop behaviors. Live-smoked against a real Godot 4.6 editor: full GDScript suite 1037/1040 passed, duplicate-handshake rejected with close 4001, original session unaffected. https://claude.ai/code/session_016ijmCD5S6QfwJGJcc5Wirp
…ake reject Two small audit-v2 fixes in src/godot_ai/transport/websocket.py covering #343 sub-findings #2 and #5. (Sub-finding #4 — errno.EADDRINUSE portability — landed separately via PR #373.) - #5 (P2): wrap `send_command`'s `ws.send` + `wait_for` in a try/finally that always pops `_pending`. Pre-fix, a `ws.send` raise (ConnectionClosed mid-send, transport error) leaked the Future entry forever; under churn the dict grew unbounded. Happy path still pops via the response receiver, so the finally pop is a no-op there. - #2 (P1): reject a second handshake whose session_id is already registered (close code 4001), instead of silently overwriting the routing map. session_id format is `<slug>@<4hex>` — 16 bits of suffix is locally guessable, so without this any local peer could hijack an active session by impersonating its ID. Legitimate plugin reconnect after `editor_reload_plugin` first triggers ConnectionClosed -> unregister, so the new connect still lands cleanly. Tests: - tests/integration/test_websocket.py — TestDuplicateHandshake pins the reject + reconnect-after-clean-disconnect paths; TestPendingFutureCleanup pins the timeout-pop and send-failure-pop behaviors. Live-smoked against a real Godot 4.6 editor: full GDScript suite 1037/1040 passed, duplicate-handshake rejected with close 4001, original session unaffected. https://claude.ai/code/session_016ijmCD5S6QfwJGJcc5Wirp
…ake reject Two small audit-v2 fixes in src/godot_ai/transport/websocket.py covering #343 sub-findings #2 and #5. (Sub-finding #4 — errno.EADDRINUSE portability — landed separately via PR #373.) - #5 (P2): wrap `send_command`'s `ws.send` + `wait_for` in a try/finally that always pops `_pending`. Pre-fix, a `ws.send` raise (ConnectionClosed mid-send, transport error) leaked the Future entry forever; under churn the dict grew unbounded. Happy path still pops via the response receiver, so the finally pop is a no-op there. - #2 (P1): reject a second handshake whose session_id is already registered (close code 4001), instead of silently overwriting the routing map. session_id format is `<slug>@<4hex>` — 16 bits of suffix is locally guessable, so without this any local peer could hijack an active session by impersonating its ID. Legitimate plugin reconnect after `editor_reload_plugin` first triggers ConnectionClosed -> unregister, so the new connect still lands cleanly. Tests: - tests/integration/test_websocket.py — TestDuplicateHandshake pins the reject + reconnect-after-clean-disconnect paths; TestPendingFutureCleanup pins the timeout-pop and send-failure-pop behaviors. Live-smoked against a real Godot 4.6 editor: full GDScript suite 1037/1040 passed, duplicate-handshake rejected with close 4001, original session unaffected. https://claude.ai/code/session_016ijmCD5S6QfwJGJcc5Wirp
…ake reject (#374) * Harden WebSocket transport: pending-Future leak + duplicate-ID handshake reject Two small audit-v2 fixes in src/godot_ai/transport/websocket.py covering #343 sub-findings #2 and #5. (Sub-finding #4 — errno.EADDRINUSE portability — landed separately via PR #373.) - #5 (P2): wrap `send_command`'s `ws.send` + `wait_for` in a try/finally that always pops `_pending`. Pre-fix, a `ws.send` raise (ConnectionClosed mid-send, transport error) leaked the Future entry forever; under churn the dict grew unbounded. Happy path still pops via the response receiver, so the finally pop is a no-op there. - #2 (P1): reject a second handshake whose session_id is already registered (close code 4001), instead of silently overwriting the routing map. session_id format is `<slug>@<4hex>` — 16 bits of suffix is locally guessable, so without this any local peer could hijack an active session by impersonating its ID. Legitimate plugin reconnect after `editor_reload_plugin` first triggers ConnectionClosed -> unregister, so the new connect still lands cleanly. Tests: - tests/integration/test_websocket.py — TestDuplicateHandshake pins the reject + reconnect-after-clean-disconnect paths; TestPendingFutureCleanup pins the timeout-pop and send-failure-pop behaviors. Live-smoked against a real Godot 4.6 editor: full GDScript suite 1037/1040 passed, duplicate-handshake rejected with close 4001, original session unaffected. https://claude.ai/code/session_016ijmCD5S6QfwJGJcc5Wirp * Apply pending ruff format to test_mcp_tools and test_gdscript_no_adjacent_string_concat Drive-by: `ruff format --check` was failing on these two test files on beta. Reformatting them keeps the format-check CI step green for the audit-v2 transport-hardening PR. No behavioral change — purely whitespace / line-wrapping deltas produced by `ruff format`. https://claude.ai/code/session_016ijmCD5S6QfwJGJcc5Wirp --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
ready/importing/playing/no_scene), sends it in handshake + events. Pythonrequire_writable()gates all 20 write handlers, rejecting withEDITOR_NOT_READYwhen unsafe.project_settings_set,signal_list/connect/disconnect,autoload_list/add/remove,input_map_list/add_action/remove_action/bind_eventrun_testsreturns summary + failures only by default;verbose=truefor full per-test resultsProjectSettingsmutators restore in-memory state on save failureConnection.get_readiness()), extracted shared signal param validation,input_map_listfilters builtins by default228 Python + 153 GDScript = 381 total tests, all passing.
Test plan
ruff check src/ tests/— lint cleanpytest -v— 228 Python tests passrun_testsvia MCP — 153 GDScript tests pass (10 suites)editor_statereadiness field,signal_list/connect/disconnect,autoload_add/list/remove,input_map_add_action/bind_event(key + joy_button 0)/remove_action,project_settings_setroundtrip,input_map_listdefault vsinclude_builtin=true🤖 Generated with Claude Code