Skip to content

Mega cleanup: #413, #416, #418, #419 (close #399)#429

Merged
dsarno merged 2 commits into
betafrom
claude/mega-issue-cleanup-RdaHE
May 13, 2026
Merged

Mega cleanup: #413, #416, #418, #419 (close #399)#429
dsarno merged 2 commits into
betafrom
claude/mega-issue-cleanup-RdaHE

Conversation

@dsarno

@dsarno dsarno commented May 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Bundle four contained backlog fixes into a single PR against beta. None touch the self-update install/extract/enable path; runner behavior is unchanged in production (changes there are test-mode-only).

Issue What changes Risk
#419 — diag script script/local-game-capture-diag: split in-memory vs on-disk autoload detection, fix HINT suppression bugs, ignore comment-prefixed and value-substring matches none (script-only)
#413 — test noise Add _suppress_scan_warnings opt-out on update_reload_runner; tests set it via _arm_scan_state, production default stays false low (no production behavior change)
#418 — auto-spawn start_server walks up to a sibling src/godot_ai/ and prepends to PYTHONPATH, gated on is_dev_checkout() low (production user installs unchanged)
#416 — banner detail /godot-ai/status adds package_path; dock incompatible-server message surfaces "(loaded from <path>)" when present low (cosmetic + a new server field)
#399 — closed Closed as not-planned — see the closing comment for the three-layer rationale n/a

Details

#419 — local-game-capture-diag autoload-missing diagnostic

The HINT line was suppressed in two real failure shapes and the FAIL bullet list contradicted the prior autoload registered=True line:

  • Bug 1: commented-out lines (;_mcp_game_helper=…) tripped the substring match, so the HINT didn't print when the autoload was disabled on disk.
  • Bug 2: a single found bool OR'd in-memory presence with on-disk presence, so the in-memory-only case (autoload registered in editor but missing from project.godot) silently suppressed the HINT — even though that's exactly the case where the game subprocess won't see it.
  • Bug 3: when the bridge failed, the bullet list said "autoload missing from project.godot (see above)" immediately after printing autoload registered=True, which read as contradictory.

Fix: split into {"in_memory": bool, "on_disk": bool}, emit three different HINTs keyed to the split state, skip comment-prefixed lines, match on the autoload key (not the value).

#413 — push_warning noise from update_reload_runner tests

Two of the watchdog unit tests directly invoke _on_scan_watchdog_timeout and the post-timeout-bypass branch of _start_filesystem_scan to pin their behavior. Both code paths emit push_warning lines that surface as yellow noise three times per test_run, training reviewers to ignore the runner's real production warnings.

Fix: add _suppress_scan_warnings (default false). _arm_scan_state in the test fixture flips it true so the tested code paths stay quiet without losing assertion coverage. New regression test test_suppress_scan_warnings_default_is_off pins the production default so a future refactor can't quietly turn off the user-facing warning.

Verified live: zero filesystem_changed warnings across all 1257 GDScript tests after the change.

#418 — auto-spawn ignores worktree src/ in dev checkouts

plugin.gd::start_dev_server already walks up from res:// to find a sibling src/godot_ai/ and prepends it to PYTHONPATH so a worktree serves its own Python rather than the root repo's editable install. The auto-spawn path in server_lifecycle.gd::start_server didn't — so worktrees with different pyproject.toml versions than the root would loop in "Incompatible server / Restart Server" forever (as described in the issue's repro: root on 1.4.4, worktree on 2.4.2, server reports 1.4.4, plugin flags incompatible, Restart Server respawns the same incompatible server).

Fix: mirror the dev-server walk-up in start_server, gated on is_dev_checkout() so production user installs are byte-for-byte unchanged. Restore PYTHONPATH immediately after OS.create_process returns to avoid leaking the env to unrelated spawns.

#416 — incompatible-server banner omits running server's load path

Add package_path (the resolved godot_ai/__init__.py parent dir) to /godot-ai/status. The editor's _probe_live_server_status captures it, and _incompatible_server_message appends (loaded from <path>) when the field is present and the peer identifies as godot-ai. Degrades cleanly:

  • Pre-v2.4.4 servers omit the field → no empty (loaded from ) stub
  • Non-godot-ai peer that happens to return a package_path JSON field → not surfaced; we won't mislabel an unrelated server as "godot-ai loaded from …"

#399 — closed as not-planned

See the closing comment on the issue. tl;dr: the proposed class_name retirement shipped in v2.4.1 and triggered the exact 500+ error cascade it was meant to prevent, v2.4.2 restored the shim, and the underlying parse-hazard is now fixed at the runner via single-phase write-before-scan (#398, #415). The 306-violation ratchet has been replaced with behavioral pins.

Test plan


Generated by Claude Code

Bundle four contained, low-risk fixes from the open-issues backlog into a
single PR. None touch the self-update install/extract/enable path; runner
behavior is unchanged in production.

#419 — local-game-capture-diag autoload-missing diagnostic
  The HINT line was suppressed in two real failure shapes and the FAIL
  bullet list contradicted the prior "autoload registered=True" line.
  Fix: split in-memory vs on-disk detection, emit a HINT keyed to the
  split state, skip comment-prefixed lines (`;` / `#`) when scanning
  project.godot, and match on the autoload key (not the value) so a
  path containing the substring can't false-positive.

#413 — push_warning noise from update_reload_runner tests
  The watchdog unit tests directly invoke `_on_scan_watchdog_timeout`
  and the post-timeout-bypass branch of `_start_filesystem_scan` to
  pin their behavior. Both paths emit `push_warning` lines that then
  show up three times per `test_run`, training reviewers to ignore
  the runner's real production signals. Add a `_suppress_scan_warnings`
  flag (default false) that tests opt into via `_arm_scan_state`; live
  self-update timeouts still surface loudly. New regression test pins
  the production default. Verified live: zero `filesystem_changed`
  warnings across all 1257 GDScript tests after the change.

#418 — auto-spawn ignores worktree src/ in dev checkouts
  `plugin.gd::start_dev_server` already walks up from `res://` to find
  a sibling `src/godot_ai/` and prepends it to PYTHONPATH so a worktree
  serves its own Python rather than the root repo's editable install.
  The auto-spawn path in `server_lifecycle.gd::start_server` didn't,
  so worktrees with different `pyproject.toml` versions than the root
  would loop in "Incompatible server / Restart Server" forever. Mirror
  the dev-server walk-up here, gated on `is_dev_checkout()` so
  production user installs are byte-for-byte unchanged. Restore
  PYTHONPATH immediately after `OS.create_process` returns to avoid
  leaking the env to unrelated spawns.

#416 — incompatible-server banner omits running server's load path
  Add `package_path` (resolved `godot_ai/__init__.py` parent) to
  `/godot-ai/status`. The editor's probe captures it, and the dock's
  "Incompatible server" message now appends "(loaded from <path>)" so
  the user can pinpoint a worktree-vs-root version skew without
  walking the process tree by hand. Degrades cleanly when the running
  server is pre-v2.4.4 (no `package_path` field) or isn't godot-ai
  at all (don't mislabel an unrelated peer as "godot-ai loaded from").

#399 — closed as not-planned
  Original plan (drop `class_name McpErrorCodes`) shipped in v2.4.1
  and triggered the 500+ "Could not resolve script" cascade it was
  meant to prevent. v2.4.2 restored the class_name as a shim and the
  underlying parse-hazard is now fixed at the runner via
  single-phase write-before-scan (#398, #415). The 306-violation
  ratchet has been deleted in favor of behavioral pins; see
  `tests/unit/test_plugin_self_update_safety.py` for the corrected
  comment block.

Verification
  - ruff check src/ tests/        — clean
  - ruff format --check           — clean
  - pytest                         — 908 passed, 2 skipped
  - Godot test_run                 — 1240/1257 passed, 0 failed
  - plugin_lifecycle               — 67/67 (4 new tests for #416)
  - update_reload_runner           — 16/16 (1 new test for #413)
  - test_server_status             — 2/2 (1 new test for #416)
  - Live editor log                — zero `filesystem_changed` warnings
    from the runner across the full suite (was 3 per run pre-fix)
Copilot AI review requested due to automatic review settings May 13, 2026 12:58
@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR bundles several small fixes across the Godot plugin, Python server, tests, and a local diagnostic script to improve developer ergonomics and reduce confusion in multi-worktree setups, while keeping production behavior unchanged (notably: scan-warning suppression is test-only, and worktree PYTHONPATH handling is gated to dev checkouts).

Changes:

  • Add package_path to /godot-ai/status, plumb it through the plugin probe, and surface it in the “Incompatible server” banner (with regression tests).
  • Silence scan-watchdog push_warning noise during update_reload_runner unit tests via a new test-only suppression flag (with a default-off invariant test).
  • Improve script/local-game-capture-diag autoload diagnostics by splitting in-memory vs on-disk detection and fixing comment/key matching.
  • In dev checkouts, auto-spawn now prepends the worktree src/ to PYTHONPATH for the spawned server, then immediately restores PYTHONPATH.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/godot_ai/server.py Adds package_path to /godot-ai/status so the editor can diagnose which Python package directory is serving a port.
plugin/addons/godot_ai/plugin.gd Captures package_path from the status probe into the plugin’s live-server snapshot.
plugin/addons/godot_ai/utils/server_lifecycle.gd Surfaces (loaded from <path>) in incompatible-server messages and prepends worktree src/ to PYTHONPATH for dev-checkout auto-spawn (with env restore).
plugin/addons/godot_ai/update_reload_runner.gd Adds _suppress_scan_warnings to suppress watchdog warnings in tests while keeping production defaults unchanged.
test_project/tests/test_plugin_lifecycle.gd Adds coverage for banner formatting with/without package_path and for non-godot-ai peers.
test_project/tests/test_update_reload_runner.gd Sets _suppress_scan_warnings in the test fixture and pins the default to “off” for production runners.
tests/unit/test_server_status.py Extends status-route expectations and adds an invariant test ensuring package_path points at a real loaded package dir.
script/local-game-capture-diag Fixes autoload HINT suppression by tracking in-memory vs on-disk state, skipping comment-prefixed lines, and matching on the autoload key.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +550 to +551
var suffix := " (PYTHONPATH=%s)" % worktree_src if not worktree_src.is_empty() else ""
print("MCP | started server (PID %d, v%s): %s %s%s" % [spawned_pid, current_version, cmd, " ".join(args), suffix])
Copilot flagged that the spawn log line printed "(PYTHONPATH=<src>)"
even though the code prepends to any existing PYTHONPATH rather than
replacing it — misleading when debugging version skew. Rewrite both
auto-spawn (`server_lifecycle.gd`) and dev-server (`plugin.gd`) log
lines to say "PYTHONPATH prefix=<src>" so the operator can tell at a
glance that an existing PYTHONPATH was preserved underneath.

Kept compact rather than logging the full effective value because
`prev_pythonpath` is routinely 5+ entries on dev machines — the
actionable piece for "why is my server running the wrong code?" is
the worktree path being prepended, not the rest of the chain.
@dsarno dsarno merged commit 2c77c78 into beta May 13, 2026
11 checks passed
dsarno added a commit that referenced this pull request May 13, 2026
* 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>
dsarno added a commit that referenced this pull request May 14, 2026
…ify cleanup (#432)

Three substantive changes:

1. CI telemetry gating across all surfaces. Workflow `env:` blocks on ci.yml / release.yml / bump-and-release.yml; shared `script/_ci_env.sh` sourced by all 6 `script/ci-*` runners; `tests/conftest.py` `setdefault("GODOT_AI_DISABLE_TELEMETRY", "true")` before any `godot_ai` import. Three layers of defense so CI / release runs / contributor laptops / ad-hoc pytest invocations stop polluting the production telemetry dashboard.

2. README Privacy & Telemetry dropdown. Short `<details>` block at the end of the README, links to docs/TELEMETRY.md.

3. /simplify cleanup pass on the telemetry surface:
   - `_build_sub_action_extractor` memoizes `inspect.signature` at decoration time — saves per-tool-call overhead.
   - `TelemetryCollector` lazy-creates and reuses one `httpx.Client` across sends; closes on shutdown only if the worker actually exited (avoids closing the client mid-post when the join times out).
   - Shared `Telemetry._drain_editor_setting_dict` helper consolidates the GDScript pending-flush dance for `plugin_reload` and `self_update`.
   - `mcp_dock.gd` calls `plugin.record_dev_server_toggle()` instead of poking into `_plugin._telemetry` directly.
   - Shared `tests/unit/conftest.py::isolated_data_dir` replaces 4 copies of the same fixture body.

Despite the original framing, the bundled `Merge beta into main` commit brings no new content — the substantive beta-side commits (#428, #429, #422, #415) were already merged via #430. The merge commit re-links the histories so future merge-base calculations are clean.

Two real defects caught + fixed during CI:
- GDScript doesn't support Python-style keyword args in calls; reverted `record_self_update(status, error=error)` to positional.
- Shutdown race where the httpx client could be closed mid-`post()` if the worker join timed out; fixed by gating close on a successful join.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants