harden(plugin): stop JSON config re-sort, honor UI opt-out over falsey env#549
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… falsey env Config/telemetry correctness fixes from the pre-release review. JSON client-config writes (#528 / TC-2): - Default JSON.stringify(sort_keys) is true, so every rewrite alphabetically reordered the user's whole file (unrelated MCP entries and, for ~/.claude.json, the entire CLI state). Pass sort_keys=false. - Godot parses every JSON number as a float, so the same round-trip re-emitted the user's integer fields as "8080.0" — rejected by strict consumers and churning every number in their other entries. A _narrow_integral_numbers pass re-narrows exactly-representable integral floats back to int before serializing so ints stay ints. (Values above 2^53 already lost precision at parse and are left as-is — byte-perfect preservation would mean not parsing the file, and such magnitudes don't occur in client configs.) Telemetry opt-out (#530 / TC-4): - _inject_telemetry_env() skipped injection whenever GODOT_AI_DISABLE_TELEMETRY / DISABLE_TELEMETRY was *present*, even set to a falsey "0". The Python server parses those truthily (falsey => enabled), so a dock UI opt-out silently shipped telemetry. Now only a *truthy* env var counts as "already disabled" (matching McpSettings.telemetry_enabled / the server); a falsey/absent value falls through so the opt-out reaches the spawn. The truthy guard is kept so post-spawn cleanup doesn't strip a user's own var. Fixed the contradicting telemetry.gd comment too. Tests (live Godot 4.6.x): clients 94/94 incl. new integer-preservation case; server_lifecycle 20/20 incl. new falsey-env opt-out case; parse clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dsarno
force-pushed
the
harden/config-telemetry
branch
from
142c81e to
2d162de
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the Godot plugin’s MCP client configuration and telemetry spawning behavior to avoid unintended user config churn and to ensure the dock UI telemetry opt-out is correctly propagated to the spawned Python server.
Changes:
- Prevent JSON config rewrites from re-sorting keys by writing with
JSON.stringify(..., sort_keys=false). - Add
_narrow_integral_numbers()to re-narrow integral floats back to ints before stringifying, avoiding8080.0-style churn. - Fix telemetry env injection logic to only treat truthy disable env vars as “already disabled”, and add regression tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
plugin/addons/godot_ai/clients/_json_strategy.gd |
Writes JSON without key sorting and narrows integral floats back to ints before stringifying. |
plugin/addons/godot_ai/utils/server_lifecycle.gd |
Treats only truthy disable env vars as “already disabled” when deciding whether to inject GODOT_AI_DISABLE_TELEMETRY. |
plugin/addons/godot_ai/telemetry.gd |
Aligns the header comment with the “truthy env disables, else fall through to EditorSetting” semantics. |
test_project/tests/test_clients.gd |
Adds coverage for preserving integer formatting in JSON configs. |
test_project/tests/test_server_lifecycle.gd |
Adds coverage for the falsey-env + UI opt-out telemetry injection case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
372
to
376
| ## Sets GODOT_AI_DISABLE_TELEMETRY in the process environment for the | ||
| ## upcoming OS.create_process call if: (a) neither GODOT_AI_DISABLE_TELEMETRY | ||
| ## nor DISABLE_TELEMETRY is already set, and (b) the EditorSettings key | ||
| ## "godot_ai/telemetry_enabled" is set to false. Returns true if the var was | ||
| ## injected so the caller can unset it after spawning. |
Comment on lines
+848
to
+850
| assert_false(content.contains('"retries": 3.0'), "retries must not be floatified") | ||
| assert_false(content.contains('"numStartups": 47.0'), "top-level int must not be floatified") | ||
| assert_false(content.contains("1.0,") or content.contains("2.0,"), "array ints must not be floatified") |
- _narrow_integral_numbers: include exactly 2^53 in the narrow bound (< -> <=).
2^53 is exactly representable as a double, so it should serialize as an int;
the previous strict-less bound left it as "9007199254740992.0", contradicting
the comment that only values *above* 2^53 are kept as floats.
- Update the _inject_telemetry_env outer doc comment to match the implementation
(only a *truthy* disable env var counts as "already disabled"; a falsey "0"
falls through so the UI opt-out still reaches the spawn).
- Strengthen the array-int test to check each element regardless of trailing
comma, so a floatified last element ("3.0" with no comma) is also caught.
Live Godot 4.6.x: clients 94/94, server_lifecycle 20/20.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.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.
Wave 2 hardening — review findings TC-2 / TC-4 (#528, #530).
_json_strategy.gdwroteJSON.stringify(config, "<tab>"), whosesort_keysdefault istrue, so every reconfigure alphabetically reordered the user's entire config file (unrelated MCP entries, and the whole CLI state in~/.claude.json). Now passessort_keys=false, so only the godot-ai entry changes._inject_telemetry_env()skipped injection whenever the telemetry env var was present — including a falseyDISABLE_TELEMETRY=0. The Python server parses those truthily (falsey means enabled), so a dock UI opt-out silently shipped telemetry. Now only a truthy env var counts as "already disabled" (matchingMcpSettings.telemetry_enabledand the server); a falsey/absent value falls through so the opt-out reaches the spawned server. The truthy guard is retained so post-spawn cleanup does not strip a user's own var. Fixed the contradictingtelemetry.gdcomment too.Test
New
test_server_lifecyclecase for the falsey-env opt-out (20/20).clients(93),settings,telemetry_toggle,plugin_telemetryall green; parse clean.Closes #528, #530.
🤖 Generated with Claude Code