Skip to content

Commit 2f12899

Browse files
dsarnoclaude
andcommitted
Fix review findings: Windows uvx, error consistency, CI robustness
- find_uvx: check uvx.exe on Windows, remove dead PATH-building code - Install uv button: use powershell directly on Windows, not via bash - find_nodes: return EDITOR_NOT_READY error instead of empty success when no scene is open (consistent with other handlers) - node_get_properties: skip properties that return null unexpectedly (protects against custom script getter errors) - ci-godot-tests: fail hard with clear message if no Godot session connects after 60 attempts, instead of falling through to run_tests - Document port-check limitation (HTTP only, not WebSocket) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 95c3c85 commit 2f12899

6 files changed

Lines changed: 52 additions & 28 deletions

File tree

plugin/addons/godot_ai/client_configurator.gd

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -98,29 +98,27 @@ static func get_server_command() -> Array[String]:
9898

9999

100100
## Find the uvx executable, checking platform-specific locations.
101+
## On GUI-launched Godot, PATH is minimal, so we check well-known
102+
## install locations explicitly before falling back to which/where.
101103
static func find_uvx() -> String:
102104
var extra_paths := _get_platform_path_prepend()
103-
var search_names := ["uvx"]
104-
105-
for name in search_names:
106-
# Check extra platform paths first
107-
for dir in extra_paths:
108-
var full := dir.path_join(name)
109-
if FileAccess.file_exists(full):
110-
return full
111-
112-
# Check via which/where
113-
var cmd := "which" if OS.get_name() != "Windows" else "where"
114-
var output: Array = []
115-
var env_path := OS.get_environment("PATH")
116-
if not extra_paths.is_empty():
117-
var prepend := ":".join(extra_paths) if OS.get_name() != "Windows" else ";".join(extra_paths)
118-
env_path = prepend + (":" if OS.get_name() != "Windows" else ";") + env_path
119-
var exit_code := OS.execute(cmd, [name], output, true)
120-
if exit_code == 0 and output.size() > 0:
121-
var found: String = output[0].strip_edges()
122-
if not found.is_empty():
123-
return found
105+
var is_windows := OS.get_name() == "Windows"
106+
var exe_name := "uvx.exe" if is_windows else "uvx"
107+
108+
# Check well-known platform paths first (works even with minimal PATH)
109+
for dir in extra_paths:
110+
var full := dir.path_join(exe_name)
111+
if FileAccess.file_exists(full):
112+
return full
113+
114+
# Fallback: which/where using inherited PATH
115+
var cmd := "where" if is_windows else "which"
116+
var output: Array = []
117+
var exit_code := OS.execute(cmd, [exe_name], output, true)
118+
if exit_code == 0 and output.size() > 0:
119+
var found: String = output[0].strip_edges()
120+
if not found.is_empty():
121+
return found
124122

125123
return ""
126124

plugin/addons/godot_ai/handlers/node_handler.gd

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,13 @@ func get_node_properties(params: Dictionary) -> Dictionary:
7070
var properties: Array[Dictionary] = []
7171
for prop in node.get_property_list():
7272
var usage: int = prop.get("usage", 0)
73-
# Only include properties visible in the inspector (PROPERTY_USAGE_EDITOR)
7473
if not (usage & PROPERTY_USAGE_EDITOR):
7574
continue
75+
# Safe read: custom script getters can error; skip bad properties
76+
# rather than letting one bad read timeout the entire request.
7677
var value = node.get(prop.name)
78+
if value == null and prop.type != TYPE_NIL:
79+
continue
7780
properties.append({
7881
"name": prop.name,
7982
"type": type_string(prop.type),

plugin/addons/godot_ai/handlers/scene_handler.gd

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func find_nodes(params: Dictionary) -> Dictionary:
3939

4040
var scene_root := EditorInterface.get_edited_scene_root()
4141
if scene_root == null:
42-
return {"data": {"nodes": [], "message": "No scene open"}}
42+
return McpErrorCodes.make(McpErrorCodes.EDITOR_NOT_READY, "No scene open")
4343

4444
var results: Array[Dictionary] = []
4545
_find_recursive(scene_root, scene_root, name_filter, type_filter, group_filter, results)

plugin/addons/godot_ai/mcp_dock.gd

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -256,14 +256,11 @@ func _make_status_row(label_text: String, value_text: String, value_color: Color
256256

257257

258258
func _on_install_uv() -> void:
259-
var install_cmd: String
260259
match OS.get_name():
261260
"Windows":
262-
install_cmd = "powershell -ExecutionPolicy ByPass -c \"irm https://astral.sh/uv/install.ps1 | iex\""
261+
OS.execute("powershell", ["-ExecutionPolicy", "ByPass", "-c", "irm https://astral.sh/uv/install.ps1 | iex"], [], false)
263262
_:
264-
install_cmd = "curl -LsSf https://astral.sh/uv/install.sh | sh"
265-
OS.execute("bash", ["-c", install_cmd], [], false)
266-
# Re-check after a moment
263+
OS.execute("bash", ["-c", "curl -LsSf https://astral.sh/uv/install.sh | sh"], [], false)
267264
_refresh_setup_status.call_deferred()
268265

269266

plugin/addons/godot_ai/plugin.gd

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ func _exit_tree() -> void:
6767
func _start_server() -> void:
6868
## If a server is already listening on our HTTP port, use it.
6969
## This covers: CI (external server), another Godot instance, or manual start.
70+
## NOTE: We only check port 8000 (HTTP), not 9500 (WebSocket). If a foreign
71+
## process holds 8000, we'll assume it's a valid server. The WebSocket
72+
## connection will fail and retry if the server isn't actually ours.
7073
if _is_port_in_use(McpClientConfigurator.SERVER_HTTP_PORT):
7174
print("MCP | server already running on port %d, using existing" % McpClientConfigurator.SERVER_HTTP_PORT)
7275
return

script/ci-godot-tests

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,29 @@ if [ -z "${SESSION_ID:-}" ]; then
5656
exit 1
5757
fi
5858

59+
# Verify a Godot session is actually connected before running tests
60+
FINAL_COUNT=$(curl -s "$SERVER_URL" -X POST "${HEADERS[@]}" \
61+
-H "Mcp-Session-Id: $SESSION_ID" \
62+
-d '{"jsonrpc":"2.0","id":99,"method":"tools/call","params":{"name":"session_list","arguments":{}}}' \
63+
| python3 -c "
64+
import json, sys
65+
raw = sys.stdin.read()
66+
for line in raw.split('\n'):
67+
if line.startswith('data: '):
68+
data = json.loads(line[6:])
69+
sc = data.get('result',{}).get('structuredContent',{})
70+
if not sc:
71+
text = data.get('result',{}).get('content',[{}])[0].get('text','{}')
72+
sc = json.loads(text)
73+
print(sc.get('count', 0))
74+
break
75+
" 2>/dev/null || echo "0")
76+
77+
if [ "$FINAL_COUNT" -eq 0 ] 2>/dev/null; then
78+
echo "ERROR: No Godot session connected after 60 attempts. Plugin may not have loaded."
79+
exit 1
80+
fi
81+
5982
# Call run_tests
6083
echo "Running handler tests..."
6184
RESULT=$(curl -s "$SERVER_URL" -X POST "${HEADERS[@]}" \

0 commit comments

Comments
 (0)