Skip to content

Commit fd89867

Browse files
dsarnoclaudegithub-actions[bot]crithitstudio
authored
Merge beta into main: ship #428 + #429 (#430)
* Fix self-update runner snapshot scan (#415) * Pin UV_LINK_MODE=copy in uvx-bridge entries to dodge Windows pywin32 lock (#302) * Fall back $HOME -> %USERPROFILE% in path-template expand() so OpenCode auto-configure works on Windows (#318) User on Windows hit "Cannot write to /.config/opencode/opencode.json" and "ERROR: Could not create directory: '/.config'" when auto-configuring OpenCode. The leading "/" came from $HOME substituting to the empty string -- Windows typically only sets USERPROFILE, not HOME. The same module's _home() helper already does this fallback for ~/ expansion; bring $HOME into line so the two spellings mean the same thing on every platform. Running as admin doesn't help because the path itself is malformed (rooted at the drive root). OpenCode is the only descriptor that uses $HOME on Windows (because opencode debug paths reports ~/.config/opencode/opencode.json on every platform); other clients use $APPDATA / $USERPROFILE and dodged the trap. Existing test_opencode_client_uses_home_config_on_windows already exercises the right shape -- passes on Mac/Linux and on GitHub Actions Windows runners (HOME set by default), which is how this slipped through. After this change it passes on a stock Windows box too. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Dedup $HOME fallback to _home() + add mocked-env test for USERPROFILE path (#319) * Dedup $HOME fallback to _home() and lock USERPROFILE coverage with a mocked-env test Follow-up to #318. Two review points from Copilot: 1. The new $HOME -> USERPROFILE branch was a fresh fallback path instead of reusing _home(), so home-directory resolution had two spellings that could drift again. Fold $HOME's fallback through _home() so $HOME and ~ are guaranteed to mean the same thing. 2. test_opencode_client_uses_home_config_on_windows did not actually exercise the new branch on CI: GitHub Actions Windows runners set HOME by default, so the regression check passed without ever touching the USERPROFILE fallback. Add a focused test that explicitly clears HOME and sets USERPROFILE to a known value, runs both $HOME/foo and ~/foo through expand(), and asserts the fallback fires for both spellings on every CI platform. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Restore HOME/USERPROFILE via unset-when-empty pattern to avoid leaking defined-empty env vars Copilot pointed out that OS.set_environment(name, "") creates a defined-empty env var rather than leaving it unset, so restoring via the captured saved_* values would silently promote previously-unset vars to defined-empty for the rest of the test run. Mirror the unset-when-saved-was-empty pattern already used by the GODOT_AI_MODE tests in this file so the original unset/set state is preserved exactly. Also switch the test's HOME-clearing setup from set_environment("HOME", "") to unset_environment("HOME") for the same reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Detect bare-key TOML sections in _toml_strategy to fix codex duplicate-key error (#320) * Detect bare-key TOML sections in clients/_toml_strategy so codex reconfigure doesn't append a duplicate When a user's TOML file has a bare-key section like [mcp_servers.godot-ai] (valid TOML — bare keys allow [A-Za-z0-9_-]+), McpTomlStrategy._all_headers only considered the quoted form [mcp_servers."godot-ai"] that _primary_header always emits. _find_section returned empty, configure fell through to the append-at-end path, and the resulting file had two godot-ai sections — codex's TOML parser rejects this with `duplicate key`. The same bug made check_status report NOT_CONFIGURED on bare-key files (so the dock looked like the client was unconfigured) and made remove a silent no-op. Add _bare_key_header / _is_bare_key helpers and include the bare form in _all_headers when every segment of toml_section_path matches [A-Za-z0-9_-]+. _primary_header keeps emitting the quoted form for new writes (no churn on existing-quoted-form files); the matcher just tolerates either spelling now. Add a regression test covering the codex shape: a fixture file with a bare-key parent section plus a nested .tools subtable that the user might add to set per-tool approval_mode. The test asserts check_status detects the entry, configure updates it in place (no duplicate; subtable customisation survives), and remove cleans both spellings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Extend remove() to clean subtables in the namespace, not just the parent header Copilot pointed out that the original fix in this PR is only partial: remove() matches the parent header [mcp_servers.godot-ai] but stops at the next bracketed line, which on the codex shape is the user's [mcp_servers.godot-ai.tools.session_list] subtable. So remove reports success but leaves the subtable behind. TOML treats that subtable as implicitly defining mcp_servers.godot-ai, so a later configure rewriting [mcp_servers."godot-ai"] produces a duplicate-key error again — the exact shape the original bug took. Add _subtable_prefixes / _matches_subtable_prefix helpers (mirrors of the existing _matches_any_header style) and let remove() consume both the parent header and any subtable header in the namespace before moving on. Configure is unchanged — it only owns the matched parent section's body, leaving subtables alone so user customisations like per-tool approval_mode survive across reconfigure. Tighten the regression test: assert subtable header AND its body (approval_mode line) are gone after remove, then round-trip a configure-after-remove and assert exactly one godot-ai section in the final file. Pre-fix this round-trip would surface the original duplicate-key shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Split animation_handler.gd along the four-domain seam (#344) The 1674-line animation_handler.gd had four clearly-separated domains (write ops, presets, read introspection, value coercion) that the audit in #297 flagged as a deferred split (finding #13). With the reliability work and characterization tests in place, extract: - animation_presets.gd (480 LOC): preset_fade / slide / shake / pulse, the _resolve_preset_target classifier, and the _direction_offset static helper. - animation_values.gd (454 LOC): list_animations, get_animation, validate_animation, plus the shared keyframe value coercion (coerce_value_for_track / resolve_track_prop_context / coerce_for_type), transition parsing, enum-to-string helpers, and serialize_value. Adds a player_root_node helper that DRYs up the root-node fallback that was open-coded in three places. - animation_handler.gd shrinks to 869 LOC (under the 900-LOC cap from the issue) and keeps every public op as the dispatcher's registration target. preset_* and read methods are thin proxies into the submodules so plugin.gd dispatcher entries and test_animation.gd's _handler.method(...) call sites need no changes. The submodules hold a WeakRef back to the handler. The handler owns them strongly via _presets / _values; the WeakRef breaks the cycle so plugin teardown's _handlers.clear() actually decrefs to zero. Both files follow the const X := preload(...) + no-bare-class_name convention from CLAUDE.md. Validated locally: ruff clean, all 722 Python tests pass, script/ci-check-gdscript clean, and a SceneTree smoke confirms the handler wires up its submodules, the WeakRef back-pointers resolve, and the static helpers + proxy methods all behave as expected. https://claude.ai/code/session_011QxzADbf9zHwfZicjzdvCw Co-authored-by: Claude <noreply@anthropic.com> * Revert "Split animation_handler.gd along the four-domain seam (#344)" (#368) * Revert "Split animation_handler.gd along the four-domain seam (#344)" This reverts commit d915c4d. * Empty commit to retrigger CI (flake on Godot tests / macOS) --------- Co-authored-by: Claude <noreply@anthropic.com> * Bump version to 2.4.0 * Add Kimi Code CLI client support (#396) Register Kimi Code CLI as a new MCP client using the CLI strategy: - kimi mcp add --transport http for configuration - kimi mcp remove for removal - kimi mcp list for status checks Also update docs and README to reflect the expanded client list. * Rename Kimi Code CLI display name to Kimi Code (#404) * Rename Kimi Code CLI display name to Kimi Code The display_name "Kimi Code CLI" caused the dock status to render "Kimi Code CLI CLI not found" because _cli_strategy.gd appends " CLI not found" using the display name. Drop the redundant suffix to match the convention of every other CLI client in the registry (Claude Code, Codex, Qwen Code, Kilo Code, Roo Code, etc.). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Track kimi_code.gd.uid Every other client in the registry has its .uid file checked in; #396 missed this one. Without it, fresh checkouts have Godot regenerate a different uid, which can break preload references. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Drop redundant " CLI" suffix from cli_strategy error messages Display names of CLI clients vary in whether they include "CLI" in the brand: Gemini CLI does, Kimi Code CLI did, Claude Code / Codex / Qwen Code don't. Hardcoding " CLI" in the strategy message produced "Gemini CLI CLI not found" / "Kimi Code CLI CLI not found". Drop the suffix so the message reads naturally regardless of the client's brand. "Claude Code not found" and "Gemini CLI not found" are both fine; the duplication is gone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Bump version to 2.4.1 * Fix v2.4.1 upgrade regression: keep class_name McpErrorCodes; smoke from real prior release zip v2.4.1 broke in-place self-update for every user on any earlier version. Symptom on update: dozens of "Parse Error: Could not resolve script res://addons/godot_ai/utils/error_codes.gd" lines across the disable -> extract -> enable window, followed by plugin.gd:225 "Nonexistent function 'new'" when the broken script references cascaded into DebuggerPlugin instantiation. Root cause: Godot's project-wide script-class registry holds the old `McpErrorCodes -> error_codes.gd` mapping at the moment the new files (which no longer declare `class_name`) extract on top. The registry and the on-disk content go through a transient inconsistency that the new files' `preload()` calls cannot survive. #412 swept all 400+ consumer sites onto the preload alias, but the *declaration* removal turned out to be the single load-bearing line for upgrade compatibility. Fix: restore `class_name McpErrorCodes` on `error_codes.gd` only. All consumers stay on the new `const ErrorCodes := preload(...)` alias #412 introduced. The registry stays consistent across any upgrade from <= v2.4.1 -> v2.4.2 because the declaration on this file is unchanged from the user's prior install. Lint baseline holds at 306; declarations aren't part of the violation count. Smoke gap (the reason v2.4.1 shipped broken): `script/local-self-update-smoke` builds both v(N) and v(N+1) from the *current* source tree. Both fixtures had the new preload code, so the harness never exercised an actual class_name -> preload transition. Add `--base-from-release-tag <tag>` (and `--base-from-zip <path>` for offline) flags that source v(N) from a real released `godot-ai-plugin.zip`. Verified the fix: - --base-from-release-tag v2.4.0 --next-version 2.4.2: PASS, 0 Parse Errors during the update window, all 6 verifier PASS markers. - --base-from-release-tag v2.4.1 --next-version 2.4.2: PASS, 0 Parse Errors -- users already on the broken v2.4.1 recover by upgrading to v2.4.2. Subsequent self-update PRs that touch class_name on any load-surface file MUST re-smoke against the prior released zip via these flags; running the harness against current source alone will continue to miss this class of transition bug. * Bump version to 2.4.2 * Fix self-update runner snapshot scan * Fix self-update fixture: merge stderr into stdout for parse-error scan `subprocess.run(..., capture_output=True)` then `proc.stdout + proc.stderr` yields an "all-stdout-then-all-stderr" buffer with no time-ordering. The window markers in `assert_no_update_parse_errors` are stdout-only `print()` calls, but Godot routes `SCRIPT ERROR: Parse Error` and friends to stderr via `OS::print_error`. With unmerged streams those errors live at offsets beyond the marker window and the assertion silently passes regardless of whether the bug is firing. Switch to `stdout=subprocess.PIPE, stderr=subprocess.STDOUT` so the kernel interleaves chronologically into one buffer. Same reason existing CI scripts (`.github/workflows/ci.yml`) use shell `> log 2>&1`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix historical-constraint test: warm class cache + env-var driver gate The historical test was silently passing for a different reason than it documents: with no headless `--import` warmup, the editor's first scan happens concurrently with the autoload spawning the runner, so the v2.3.2 `McpErrorCodes` class registration never makes it into the cache before the new files extract. The registry-skew window the bug relies on never opens, and the assertion that parse errors fire fails. Two fixes: - Add `prime_class_cache(project_dir, godot_bin)` to the fixture: a `--headless --import` pass that populates `.godot/global_script_class_cache.cfg` against the v2.3.2 base before the real editor pass runs. - Replace the autoload's `--import` cmdline gate with an explicit `_SELF_UPDATE_DRIVER_SKIP=1` env var. The warmup sets it so the autoload skips its runner work; the real editor pass leaves it unset so the autoload runs even when `--headless` is in use (which the forward test needs). The previous `OS.get_cmdline_args().has("--import")` check did not skip reliably in `--headless --import` and would have allowed the warmup to consume the staged update zip. The env-var pattern is unambiguous. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Apply ruff format Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Crithit Studio [Joey] <joeweis00@icloud.com> * Smoke harness: surface clear error for pre-v2.4.0 bases (#422) `patch_fixture_plugin` patches `utils/server_lifecycle.gd` and `utils/update_manager.gd`. Both files were added in v2.4.0 (extractions from plugin.gd and mcp_dock.gd respectively); pre-v2.4.0 release zips don't have them. Before this change, passing `--base-from-release-tag v2.3.2` died with a cryptic `FileNotFoundError` from inside `patch_lifecycle_expected_server_version` before reaching the manual step instructions. Add a pre-flight check that detects the missing v2.4.0+ extracted files and raises a clear `HarnessError` pointing at the integration test that already covers the v2.3.2 -> current upgrade transition via a different code path (extract zip directly, invoke runner.start() on the base's shipped runner, assert the documented historical parse-error cascade fires). Supporting v2.3.2 base end-to-end in the dock-click smoke would require parallel patching of v2.3.2's mcp_dock.gd (where the update flow lived before #297/#310 extracted it). That's significantly more code and brittle against changes to old code we're not iterating on, with little marginal value over the existing integration test. Verified: - `--base-from-release-tag v2.3.2 --no-launch` -> clean FAIL message - `--base-from-release-tag v2.4.0 --no-launch` -> fixture ready, manual step instructions print - no flag (current-as-base) -> fixture ready, manual step instructions print Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Drop stale bare-`Mcp*` lint reference from McpErrorCodes doc-comment Copilot review on #425 caught that the cited `tests/unit/test_plugin_self_update_safety.py` lint no longer exists -- that test file's own docstring documents the deny-by-default ratchet was removed because it measured call shape rather than the actual parse-hazard bug. The CLAUDE.md `never-delete-published-class_name` policy referenced in the prior sentence is the real guard; drop the misleading lint reference. https://claude.ai/code/session_01VgXf3Lqv2ypt36g6EqpRYg * Merge beta into main: land #415 + #422 self-update fixes (#426) (#427) * Harden ZIP extractor against Windows drive-letter and backslash entries (#428) * Mega cleanup: #413, #416, #418, #419 (close #399) (#429) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Crithit Studio [Joey] <joeweis00@icloud.com>
1 parent 3bda9eb commit fd89867

10 files changed

Lines changed: 384 additions & 28 deletions

File tree

plugin/addons/godot_ai/plugin.gd

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -708,6 +708,10 @@ static func _probe_live_server_status(port: int, timeout_ms: int = SERVER_STATUS
708708
result["name"] = str(parsed.get("name", ""))
709709
result["version"] = _extract_server_version(parsed)
710710
result["ws_port"] = int(parsed.get("ws_port", 0))
711+
## `package_path` was added in v2.4.4 (#416) so the dock's
712+
## "Incompatible server" banner can name the source of a version
713+
## skew. Older servers omit it; treat the missing field as "".
714+
result["package_path"] = str(parsed.get("package_path", ""))
711715
return result
712716

713717

@@ -1509,7 +1513,10 @@ func start_dev_server() -> void:
15091513
OS.set_environment("PYTHONPATH", prev_pythonpath)
15101514

15111515
if pid > 0:
1512-
var suffix := " (PYTHONPATH=%s)" % worktree_src if not worktree_src.is_empty() else ""
1516+
## Match `server_lifecycle.gd::start_server`'s log wording —
1517+
## "prefix" since we prepended to any pre-existing PYTHONPATH,
1518+
## not replaced it. See #429 review.
1519+
var suffix := " (PYTHONPATH prefix=%s)" % worktree_src if not worktree_src.is_empty() else ""
15131520
print("MCP | started dev server with --reload (PID %d): %s %s%s" % [pid, cmd, " ".join(inner_args), suffix])
15141521
else:
15151522
push_warning("MCP | failed to start dev server")

plugin/addons/godot_ai/update_reload_runner.gd

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,16 @@ var _paths_written = []
8484
## is missing or stale on disk. Surfaces FAILED_MIXED so the runner refuses
8585
## to re-enable the plugin against a half-installed tree.
8686
var _restore_failed := false
87+
## Test-only opt-out for the scan-watchdog `push_warning` lines. The
88+
## watchdog unit tests in `test_update_reload_runner.gd` invoke
89+
## `_on_scan_watchdog_timeout()` and the post-timeout
90+
## `_start_filesystem_scan` bypass branch directly to pin their behavior
91+
## — but those code paths' `push_warning` calls then appear as yellow
92+
## console noise in every `test_run`, training reviewers to ignore the
93+
## runner's real production warnings. Tests set this true; production
94+
## leaves it false so genuine scan timeouts during a real self-update
95+
## still surface loudly. See issue #413.
96+
var _suppress_scan_warnings := false
8797

8898

8999
func start(zip_path: String, temp_dir: String, detached_dock) -> void:
@@ -166,10 +176,11 @@ func _start_filesystem_scan(next_step: String = "_enable_new_plugin") -> void:
166176
## actually completed. Skip the wait; Godot's normal background scan
167177
## catches up after the plugin re-enables. See PR #381 review.
168178
if _scan_timed_out:
169-
push_warning(
170-
"MCP | skipping filesystem_changed wait after previous timeout (next_step=%s)"
171-
% deferred_step
172-
)
179+
if not _suppress_scan_warnings:
180+
push_warning(
181+
"MCP | skipping filesystem_changed wait after previous timeout (next_step=%s)"
182+
% deferred_step
183+
)
173184
call_deferred(deferred_step)
174185
return
175186

@@ -209,10 +220,11 @@ func _on_scan_watchdog_timeout() -> void:
209220
## `_waiting_for_scan == false`.
210221
if not _waiting_for_scan:
211222
return
212-
push_warning(
213-
"MCP | filesystem_changed didn't fire within %ds; proceeding without scan confirmation"
214-
% int(SCAN_WATCHDOG_SECS)
215-
)
223+
if not _suppress_scan_warnings:
224+
push_warning(
225+
"MCP | filesystem_changed didn't fire within %ds; proceeding without scan confirmation"
226+
% int(SCAN_WATCHDOG_SECS)
227+
)
216228
_scan_timed_out = true
217229
var fs := EditorInterface.get_resource_filesystem()
218230
if fs != null and fs.filesystem_changed.is_connected(_on_filesystem_changed):

plugin/addons/godot_ai/utils/server_lifecycle.gd

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -327,17 +327,23 @@ static func _incompatible_server_message(
327327
) -> String:
328328
var version := _live_version_for_message(live)
329329
var actual_ws_port := _live_ws_port_for_message(live)
330+
## `package_path` is a v2.4.4+ field — older servers omit it. Suffix
331+
## the message with "(loaded from <path>)" when present so the user
332+
## can tell *which* `src/godot_ai/` is serving the port without
333+
## walking the process tree. See #416.
334+
var package_path := _live_package_path_for_message(live)
335+
var path_suffix := " (loaded from %s)" % package_path if not package_path.is_empty() else ""
330336
if not version.is_empty():
331337
if actual_ws_port > 0 and actual_ws_port != expected_ws_port:
332338
return (
333-
"Port %d is occupied by godot-ai server v%s using WS port %d; "
339+
"Port %d is occupied by godot-ai server v%s using WS port %d%s; "
334340
+ "plugin expects v%s with WS port %d. Stop the old server or "
335341
+ "change both HTTP and WS ports."
336-
) % [port, version, actual_ws_port, expected_version, expected_ws_port]
342+
) % [port, version, actual_ws_port, path_suffix, expected_version, expected_ws_port]
337343
return (
338-
"Port %d is occupied by godot-ai server v%s; plugin expects v%s. "
344+
"Port %d is occupied by godot-ai server v%s%s; plugin expects v%s. "
339345
+ "Stop the old server or change both HTTP and WS ports."
340-
) % [port, version, expected_version]
346+
) % [port, version, path_suffix, expected_version]
341347
var status_code := int(live.get("status_code", 0))
342348
if status_code > 0:
343349
return (
@@ -366,6 +372,16 @@ static func _live_ws_port_for_message(live: Dictionary) -> int:
366372
return int(live.get("ws_port", 0))
367373

368374

375+
static func _live_package_path_for_message(live: Dictionary) -> String:
376+
## Only trust the path when the live snapshot confirms a godot-ai
377+
## server — a probe of some unrelated HTTP service could in theory
378+
## return a `package_path` JSON field, and we don't want to mislabel
379+
## that as "godot-ai loaded from …" in the incompatible banner.
380+
if live.has("name") and str(live.get("name", "")) != "godot-ai":
381+
return ""
382+
return str(live.get("package_path", ""))
383+
384+
369385
# ---- start_server / spawn watch / respawn -----------------------------
370386

371387
## Branch table (recorded version is the "is this ours?" signal — uvx
@@ -479,8 +495,48 @@ func start_server() -> void:
479495
push_warning("MCP | port %d is reserved by Windows (Hyper-V / WSL2 / Docker)" % port)
480496
return
481497

498+
## PYTHONPATH handling for dev checkouts: when the editor is launched
499+
## against a worktree whose `src/godot_ai/__version__` differs from the
500+
## root repo's editable install, the dev-venv python's `sitecustomize`
501+
## adds the *root repo's* `src/` to `sys.path`. The spawned server then
502+
## reports the root repo's version, the plugin's compatibility check
503+
## flags it as incompatible, and the user gets a Restart-Server loop
504+
## with no exit. `start_dev_server` already prepends the worktree's
505+
## `src/` for its --reload spawn; mirror that here for the auto-spawn
506+
## path so the same worktree-vs-root version skew is impossible. Gated
507+
## on `is_dev_checkout()` so production user installs (no nearby `src/`)
508+
## are untouched. See #418.
509+
var worktree_src := ""
510+
var prev_pythonpath := ""
511+
var pythonpath_set := false
512+
if ClientConfigurator.is_dev_checkout():
513+
worktree_src = ClientConfigurator.find_worktree_src_dir(
514+
ProjectSettings.globalize_path("res://")
515+
)
516+
if not worktree_src.is_empty():
517+
prev_pythonpath = OS.get_environment("PYTHONPATH")
518+
var sep := ";" if OS.get_name() == "Windows" else ":"
519+
var new_pp := (
520+
worktree_src
521+
if prev_pythonpath.is_empty()
522+
else worktree_src + sep + prev_pythonpath
523+
)
524+
OS.set_environment("PYTHONPATH", new_pp)
525+
pythonpath_set = true
526+
482527
_server_pid = OS.create_process(cmd, args)
483528
var spawned_pid := int(_server_pid)
529+
530+
## Restore PYTHONPATH immediately — the spawned child has already
531+
## copied the env, so the editor's own process state returns to
532+
## baseline. Leaving it set would leak to any later OS.create_process
533+
## from unrelated paths.
534+
if pythonpath_set:
535+
if prev_pythonpath.is_empty():
536+
OS.unset_environment("PYTHONPATH")
537+
else:
538+
OS.set_environment("PYTHONPATH", prev_pythonpath)
539+
484540
if spawned_pid > 0:
485541
_server_spawn_ms = Time.get_ticks_msec()
486542
_server_exit_ms = 0
@@ -491,7 +547,13 @@ func start_server() -> void:
491547
## editor start's adopt branch heals it to the real port owner.
492548
_host._write_managed_server_record(spawned_pid, current_version)
493549
_startup_path = McpStartupPathScript.SPAWNED
494-
print("MCP | started server (PID %d, v%s): %s %s" % [spawned_pid, current_version, cmd, " ".join(args)])
550+
## Log "PYTHONPATH prefix=" rather than "PYTHONPATH=" so the line
551+
## isn't misleading when an existing PYTHONPATH was present —
552+
## we prepended `worktree_src`, not replaced. Keeps the log
553+
## compact (worktree_src is the actionable piece; the full
554+
## prev_pythonpath can be 5+ entries long on dev machines).
555+
var suffix := " (PYTHONPATH prefix=%s)" % worktree_src if not worktree_src.is_empty() else ""
556+
print("MCP | started server (PID %d, v%s): %s %s%s" % [spawned_pid, current_version, cmd, " ".join(args), suffix])
495557
_host._start_server_watch()
496558
else:
497559
set_terminal_diagnosis(McpServerStateScript.CRASHED)

script/local-game-capture-diag

Lines changed: 55 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,21 @@ def _wait_for_session(url: str, sid: str, timeout: float = 15.0) -> dict:
125125
raise RuntimeError(f"No Godot session registered within {timeout}s. Last response: {last}")
126126

127127

128-
def _print_autoload_section(url: str, sid: str) -> bool:
129-
"""Return True if _mcp_game_helper is present in the autoload list."""
130-
found = False
128+
def _print_autoload_section(url: str, sid: str) -> dict:
129+
"""Inspect _mcp_game_helper in both the editor's in-memory autoload list
130+
and the on-disk [autoload] section of project.godot, and print an
131+
actionable HINT keyed to the split state.
132+
133+
The game subprocess reads project.godot from disk — so in-memory-only
134+
presence is a real failure mode (capture never wires up) that's distinct
135+
from "missing everywhere" and from "disk-only" (editor needs a restart).
136+
137+
Returns {"in_memory": bool, "on_disk": bool} so callers can include the
138+
split state in downstream FAIL diagnostics rather than collapsing it to
139+
a single ambiguous bool.
140+
"""
141+
in_memory = False
142+
on_disk = False
131143
print("\n[1/3] Autoload list (from autoload_manage):")
132144
try:
133145
autoloads = _tool_call(url, sid, "autoload_manage", {"op": "list"}, 100)
@@ -141,7 +153,7 @@ def _print_autoload_section(url: str, sid: str) -> bool:
141153
marker = " <-- target" if name == "_mcp_game_helper" else ""
142154
print(f" - {name}: {path}{marker}")
143155
if name == "_mcp_game_helper":
144-
found = True
156+
in_memory = True
145157
except Exception as exc:
146158
print(f" autoload_manage failed: {exc}")
147159

@@ -168,19 +180,45 @@ def _print_autoload_section(url: str, sid: str) -> bool:
168180
if in_section:
169181
print(f" {line}")
170182
any_lines = True
171-
if "_mcp_game_helper" in line:
172-
found = True
183+
## Godot's INI parser treats `;` and `#` as line-comment
184+
## prefixes. Skip those so a commented-out autoload entry
185+
## doesn't count as "found on disk" — its substring would
186+
## otherwise suppress the HINT even though the autoload is
187+
## effectively disabled.
188+
stripped = line.lstrip()
189+
if stripped.startswith((";", "#")):
190+
continue
191+
## Match on the key only, not the value. The autoload value
192+
## may legitimately point at a path containing the substring
193+
## (unlikely but possible), and matching on `key=` keeps the
194+
## check unambiguous.
195+
key = stripped.split("=", 1)[0].strip()
196+
if key == "_mcp_game_helper":
197+
on_disk = True
173198
if not any_lines:
174199
print(" (no [autoload] section in project.godot)")
175200
except Exception as exc:
176201
print(f" filesystem_manage read_text failed: {exc}")
177202

178-
if not found:
203+
if not in_memory and not on_disk:
179204
print(
180205
"\n HINT: _mcp_game_helper is missing. Disable + re-enable the Godot AI\n"
181206
" plugin in Project Settings → Plugins to recreate it."
182207
)
183-
return found
208+
elif in_memory and not on_disk:
209+
print(
210+
"\n HINT: _mcp_game_helper is in the editor's in-memory ProjectSettings\n"
211+
" but missing from project.godot on disk. The game subprocess reads\n"
212+
" project.godot, so capture will not wire up. Save project (Project →\n"
213+
" Save All / Ctrl-S) to persist the autoload to disk."
214+
)
215+
elif on_disk and not in_memory:
216+
print(
217+
"\n HINT: _mcp_game_helper is on disk but the editor's in-memory\n"
218+
" ProjectSettings doesn't have it. The editor likely needs a reload —\n"
219+
" Project → Reload Current Project, or restart the editor."
220+
)
221+
return {"in_memory": in_memory, "on_disk": on_disk}
184222

185223

186224
def _png_dimensions(png: bytes) -> tuple[int, int] | None:
@@ -286,7 +324,7 @@ def main() -> int:
286324
print(f"\nFAIL: --session activate '{args.session}' raised: {exc}")
287325
return 1
288326

289-
autoload_ok = _print_autoload_section(args.url, sid)
327+
autoload_state = _print_autoload_section(args.url, sid)
290328

291329
we_started_run = False
292330
try:
@@ -323,13 +361,19 @@ def main() -> int:
323361
print("\nFAIL: game_capture_ready stayed false.")
324362
print(f" is_playing={ready.get('is_playing')}")
325363
print(f" readiness={ready.get('readiness')}")
326-
print(f" autoload registered={autoload_ok}")
364+
## Surface in-memory vs on-disk separately so the bullet list
365+
## below doesn't contradict itself when only the editor has the
366+
## autoload registered (game subprocess reads disk).
367+
print(
368+
f" autoload registered (in-memory={autoload_state['in_memory']},"
369+
f" disk={autoload_state['on_disk']})"
370+
)
327371
print(
328372
"\n This is the same condition that produces the 'autoload never\n"
329373
" registered its debugger capture within 20s' timeout. The most\n"
330374
" common causes:"
331375
)
332-
print(" - autoload missing from project.godot (see above)")
376+
print(" - autoload missing from project.godot on disk (see above)")
333377
print(" - game launched outside project_run (manual F5)")
334378
print(" - game crashed before reaching the mcp:hello beacon")
335379
_dump_recent_logs(args.url, sid)

src/godot_ai/server.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,14 @@
77
from collections.abc import AsyncIterator, Iterable
88
from contextlib import asynccontextmanager
99
from dataclasses import dataclass
10+
from pathlib import Path
1011
from typing import Any
1112

1213
from fastmcp import FastMCP
1314
from starlette.requests import Request
1415
from starlette.responses import JSONResponse
1516

17+
import godot_ai as _godot_ai_pkg
1618
from godot_ai import __version__ as _SERVER_VERSION
1719
from godot_ai.asgi import StaleMcpSessionDiagnosticMiddleware
1820
from godot_ai.godot_client.client import GodotClient
@@ -56,6 +58,14 @@
5658

5759
logger = logging.getLogger(__name__)
5860

61+
## Filesystem location of the running `godot_ai` package — surfaced via the
62+
## /godot-ai/status probe so the editor's "Incompatible server" diagnostic
63+
## can tell the user *which* `src/godot_ai/` was actually loaded. In a
64+
## multi-worktree dev setup this is the only fast way to distinguish "root
65+
## .venv resolved to a stale branch" from "wrong PYTHONPATH" without
66+
## walking the process tree by hand. See issue #416.
67+
_SERVER_PACKAGE_PATH = str(Path(_godot_ai_pkg.__file__).resolve().parent)
68+
5969

6070
@dataclass
6171
class AppContext:
@@ -221,6 +231,12 @@ async def godot_ai_status(_request: Request) -> JSONResponse:
221231
"ws_port": ws_port,
222232
"tool_surface": "rollup",
223233
"exclude_domains": sorted(exclude),
234+
## `package_path` lets the editor's incompatible-server
235+
## banner pinpoint the source of a version skew (e.g.
236+
## "loaded from /Users/.../godot-ai-feature-branch/src" vs
237+
## "loaded from /Users/.../godot-ai/src") without the
238+
## user having to walk the process tree. See #416.
239+
"package_path": _SERVER_PACKAGE_PATH,
224240
}
225241
)
226242

test_project/tests/test_plugin_lifecycle.gd

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -846,6 +846,59 @@ func test_incompatible_server_message_names_ws_port_mismatch() -> void:
846846
assert_contains(message, "change both HTTP and WS ports")
847847

848848

849+
func test_incompatible_server_message_surfaces_package_path_when_present() -> void:
850+
## v2.4.4+ /godot-ai/status carries `package_path` (issue #416). When
851+
## the live snapshot includes it, the dock's banner must name the
852+
## loaded path so the user can identify a worktree-vs-root version
853+
## skew without walking the process tree.
854+
var message := GodotAiPlugin._incompatible_server_message(
855+
{
856+
"name": "godot-ai",
857+
"version": "1.4.4",
858+
"package_path": "/Users/foo/godot-ai-branch/src/godot_ai",
859+
},
860+
"2.4.4",
861+
18130,
862+
McpClientConfigurator.ws_port(),
863+
)
864+
assert_contains(message, "v1.4.4")
865+
assert_contains(
866+
message,
867+
"(loaded from /Users/foo/godot-ai-branch/src/godot_ai)",
868+
"package_path must be surfaced verbatim so the user can match it to a worktree",
869+
)
870+
assert_contains(message, "plugin expects v2.4.4")
871+
872+
873+
func test_incompatible_server_message_omits_path_suffix_when_old_server() -> void:
874+
## Old servers (pre-v2.4.4) omit `package_path`. The banner must
875+
## degrade gracefully — no trailing "(loaded from )" stub.
876+
var message := GodotAiPlugin._incompatible_server_message(
877+
{"version": "1.2.10"},
878+
"2.2.0",
879+
8000,
880+
McpClientConfigurator.ws_port(),
881+
)
882+
assert_false(
883+
"loaded from" in message,
884+
"missing package_path must not leave an empty parenthetical in the banner",
885+
)
886+
887+
888+
func test_incompatible_server_message_ignores_package_path_for_non_godot_ai_peer() -> void:
889+
## A non-godot-ai server on the port could in theory return a JSON
890+
## `package_path` field. Don't label it as "godot-ai loaded from …" —
891+
## the surface is for godot-ai version skew, not for misattribution.
892+
var message := GodotAiPlugin._incompatible_server_message(
893+
{"name": "other-server", "version": "9.9.9", "package_path": "/somewhere/else"},
894+
"2.4.4",
895+
8000,
896+
McpClientConfigurator.ws_port(),
897+
)
898+
assert_false("loaded from" in message)
899+
assert_false("/somewhere/else" in message)
900+
901+
849902
func test_incompatible_transition_refreshes_dock_client_statuses() -> void:
850903
var plugin := _ProofPlugin.new()
851904
var dock := _RefreshDock.new()

0 commit comments

Comments
 (0)