Detect bare-key TOML sections in _toml_strategy to fix codex duplicate-key error#320
Merged
Conversation
…nfigure 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>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the TOML client strategy so Codex config detection/removal can recognize bare-key section headers like [mcp_servers.godot-ai] in addition to the quoted form the plugin writes. It fits into the MCP client configuration layer by making TOML-based client status, reconfigure, and remove behavior more tolerant of valid user-authored config syntax.
Changes:
- Added bare-key header generation/detection helpers in
McpTomlStrategyand included bare headers in_all_headers. - Kept writes on the existing quoted-header form to avoid churn in already-quoted files.
- Added a regression test covering bare-key detection, in-place reconfigure, and remove behavior for a Codex-like TOML fixture.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
plugin/addons/godot_ai/clients/_toml_strategy.gd |
Extends TOML header matching to recognize bare-key section spellings alongside the existing quoted form. |
test_project/tests/test_clients.gd |
Adds a regression test for bare-key Codex TOML configs, including status, reconfigure, and remove flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+163
to
+165
| var bare := _bare_key_header(client) | ||
| if not bare.is_empty() and bare != primary: | ||
| out.append(bare) |
| assert_eq(after_remove.count("[mcp_servers.godot-ai]\n"), 0, | ||
| "remove must clean the bare-key section:\n%s" % after_remove) | ||
| assert_eq(after_remove.count("[mcp_servers.\"godot-ai\"]\n"), 0, | ||
| "remove must clean the quoted-key section:\n%s" % after_remove) |
…ent 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>
dsarno
added a commit
that referenced
this pull request
May 5, 2026
…ot) (#339) * 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> * Disconnect debugger screenshot timer on _clear_pending (#297 blind spot) `_clear_pending` previously only erased the dict entry on screenshot success/error, leaving the SceneTreeTimer + bound `_on_timeout` lambda (which captures `request_id`) alive until the timer naturally fired — up to `timeout_sec` (8s default) of dead state per request. Store the bound timeout callable alongside the timer so the cleanup path can disconnect `timer.timeout` and let the lambda + captured request_id drop immediately. `_on_timeout` continues to bypass `_clear_pending` because by the time it runs the timer has already fired and self-disconnected. Closes the "debugger timer leak on screenshot success" blind spot called out in #297. https://claude.ai/code/session_01EaT7NibPToHgffViz4eEvp * Apply Copilot review feedback on TOML section-boundary detection The pre-fix section-end check in remove() and _find_section() required the next header to satisfy `ends_with("]")`, which doesn't hold for the TOML `[next_section] # comment` form (valid TOML, hand-written by some users). The end-of-section loop would walk past such a header and let remove() clobber unrelated sections that came after. Factor a shared `_is_any_section_header(line)` helper that finds the closing `]` and permits whitespace or `#` after it — same shape used by the existing `_matches_any_header` / `_matches_subtable_prefix` helpers elsewhere in the file. remove() and _find_section() both consume it. Adds `test_toml_strategy_remove_tolerates_inline_comment_on_next_header` locking in the regression: file with `[other_section] # comment` after the godot-ai section, remove() must delete only the godot-ai section and preserve [other_section] and its body. script/ci-check-gdscript: clean. pytest: 722 passed. https://claude.ai/code/session_01DwmN9ouQq88KnabiW42y2C * Retrigger CI Game capture smoke / Windows failed in 13s on the previous run — the runtime points at a chickensoft-games/setup-godot@v2 infrastructure hiccup (every other Windows job on the same SHA passed, and the diff in this PR doesn't touch the capture path). Empty commit to retest. https://claude.ai/code/session_01DwmN9ouQq88KnabiW42y2C --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
dsarno
added a commit
that referenced
this pull request
May 6, 2026
51 commits forward from main, headlined by two architecture audits: audit-v1 (issue #297, PRs #298-#315): scene-path ancestry guard, update/config data-loss safeguards, lifecycle reliability, characterization tests, plugin.gd extraction (McpPortResolver + McpServerLifecycleManager), state-model cleanup, McpUpdateManager extraction, Runtime Protocol deletion + DirectRuntime retype, narrowed meta-tool JSON coercion, self-update preload-alias hardening, locked FastMCP middleware order. audit-v2 (issue #343, PRs #369-#390): origin allowlist (DNS-rebinding guard, #345/#375), path-traversal guards on script_* / filesystem_* writes (#347/#369), errno.EADDRINUSE portability across all OSes (#348/#373), SessionRegistry RLock removal (#350/#370), Pydantic-validated WebSocket event payloads (#351/#378), sole-survivor auto-failover on active disconnect (#352/#379), 30s filesystem_changed watchdog during update reload (#353/#381), FAILED_MIXED self-update visibility via mixed_state in editor_state (#354/#382), 32/tick inbound packet-drain cap with spillover logging (#356/#383), error-code vocabulary enrichment (#365/#385: NODE_NOT_FOUND, PROPERTY_NOT_ON_CLASS, VALUE_OUT_OF_RANGE, MISSING_REQUIRED_PARAM cut INVALID_PARAMS sites 471 -> 97), resolve-or-error helper extraction (#364/#389: 38+ duplicate sites migrated), resource-form lint for meta-tool reads (#363/#386), LogViewer + PortPickerPanel extraction from mcp_dock.gd (#360/#390), LogViewer.tick() buffer-clear recovery (#392). Conflict resolutions (both intentional, both took beta's side): - plugin/addons/godot_ai/animation_handler.gd plus new submodules animation_presets.gd and animation_values.gd: beta's 4-domain split retained. Main's revert (#368) was CI-flake hygiene, not a structural rejection per its own commit message ("Empty commit to retrigger CI (flake on Godot tests / macOS)"). Beta's #367 is the canonical state the audit found wanted. - plugin/addons/godot_ai/clients/_toml_strategy.gd: beta's version retained. Beta handles inline comments after section headers (`[next_section] # note`) via _is_any_section_header(); main's later re-derived bare-key fix uses the simpler bracket check and would regress on commented headers. The four main-only Windows / TOML hotfixes (#302, #318, #319, #320) all landed on beta independently under different commit hashes and are content-equivalent or beta-improved; no cherry-pick was needed before this merge. Validation: - CI: green on beta tip 72b35d7 (and on PR #392 separately) across ubuntu-latest / macos-latest / windows-latest for Python tests, Godot tests, release-smoke, and game-capture-smoke. - Operator smoke (macOS, 2026-05-06): all 10 phases of the audit-v1+v2 post-landing runbook green. Report on issue #343. 47 GDScript suites + 903 Python tests passing. Phase 7 interactive self-update verified end-to-end with the local-self-update-smoke harness; plugin advanced 2.3.2 -> 2.3.3, no .ips, no _exit_tree leak, server stop/start clean. - Operator smoke (Windows 11, beta tip d5aa29f, 2026-05-06): Phase 7 self-update green (7/7; macOS-only .ips check correctly downgraded to SKIP). Path-traversal guard rejects backslash variant. Cursor client configure cycle round-trips cleanly. editor_state ping 109ms. Pre-existing pytest UnicodeDecodeError on Windows tracked separately as #397 (also on main, not introduced by this merge). # Conflicts: # test_project/tests/test_clients.gd
dsarno
added a commit
that referenced
this pull request
May 11, 2026
* 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>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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_headersonly considered the quoted form[mcp_servers."godot-ai"]that_primary_headeralways emits._find_sectionreturned empty,configurefell through to the append-at-end path, and the resulting file had two godot-ai sections. Codex's TOML parser rejects that withduplicate key:Same root cause also made
check_statusreportNOT_CONFIGUREDfor bare-key files (the dock looked like the client was unconfigured even when an entry existed), and maderemovea silent no-op.Fix
_bare_key_header/_is_bare_keyhelpers in plugin/addons/godot_ai/clients/_toml_strategy.gd._all_headerswhen every segment oftoml_section_pathmatches the bare-key character class._primary_headeris unchanged — new writes keep emitting the quoted form, so existing quoted-form files don't churn. The matcher just tolerates either spelling.Reproduction (pre-fix)
~/.codex/config.tomlto add[mcp_servers.godot-ai]\nurl = "..."\nenabled = true(bare-key form).godot-aisections (one bare, one quoted).codexerrors on launch.Test plan
pytest -v— 733 / 733 green (Python tests unaffected; included as a smoke check).test_toml_strategy_detects_bare_key_section_no_duplicate_on_reconfigure. The test fixtures the codex shape (bare-key parent section + nested.tools.session_listsubtable that a user might add to setapproval_mode = "approve") and asserts:check_statusreturnsCONFIGURED_MISMATCHinstead ofNOT_CONFIGURED.configureupdates the section in place — exactly onegodot-aisection header in the resulting file (counting both spellings).removecleans the bare-key form (was a silent no-op).