harden(plugin): stop JSON config re-sort, honor UI opt-out over falsey env (#549)

* harden(plugin): preserve JSON config fidelity + honor UI opt-out over 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>

* harden(plugin): address Copilot review on config/telemetry

- _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>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: dsarno

plugin/addons/godot_ai/clients/_json_strategy.gd

@@ -24,7 +24,7 @@ static func configure(client: McpClient, server_name: String, server_url: String
 	var existing: Variant = holder.get(server_name, null)
 	holder[server_name] = build_entry(client, server_url, existing)
 
-	if not McpAtomicWrite.write(path, JSON.stringify(config, "\t")):
+	if not McpAtomicWrite.write(path, JSON.stringify(_narrow_integral_numbers(config), "\t", false)):
 		return {"status": "error", "message": "Cannot write to %s" % path}
 	return {"status": "ok", "message": "%s configured (HTTP: %s)" % [client.display_name, server_url]}
 
@@ -60,7 +60,7 @@ static func remove(client: McpClient, server_name: String) -> Dictionary:
 	var holder := _walk_path(config, client.server_key_path)
 	if holder is Dictionary and holder.has(server_name):
 		holder.erase(server_name)
-		if not McpAtomicWrite.write(path, JSON.stringify(config, "\t")):
+		if not McpAtomicWrite.write(path, JSON.stringify(_narrow_integral_numbers(config), "\t", false)):
 			return {"status": "error", "message": "Cannot write to %s" % path}
 	return {"status": "ok", "message": "%s configuration removed" % client.display_name}
 
@@ -236,3 +236,28 @@ static func _walk_path(root: Dictionary, key_path: PackedStringArray) -> Variant
 			return null
 		cur = cur[key]
 	return cur
+
+
+## Godot's JSON.parse turns every JSON number into a float, so a later
+## JSON.stringify re-emits the user's integer fields as "8080.0" — which strict
+## consumers (Go's encoding/json into an int field, etc.) reject, and which
+## needlessly rewrites every number across the user's *other* entries. Re-narrow
+## exactly-representable integral floats back to int so they serialize without
+## the ".0". Walks dicts/arrays in place and returns the (same) value.
+##
+## Integers above 2^53 already lost precision when Godot parsed them to double,
+## so they're left as the float Godot produced rather than faking exactness —
+## byte-perfect preservation would require not parsing the file at all, and such
+## magnitudes don't occur in MCP client configs.
+static func _narrow_integral_numbers(value: Variant) -> Variant:
+	match typeof(value):
+		TYPE_FLOAT:
+			if is_finite(value) and value == floor(value) and absf(value) <= 9007199254740992.0:
+				return int(value)
+		TYPE_DICTIONARY:
+			for k in value:
+				value[k] = _narrow_integral_numbers(value[k])
+		TYPE_ARRAY:
+			for i in value.size():
+				value[i] = _narrow_integral_numbers(value[i])
+	return value

plugin/addons/godot_ai/telemetry.gd

@@ -15,9 +15,9 @@
 ##   2. The `godot_ai/telemetry_enabled` EditorSetting — set through the
 ##      MCP dock and persisted between sessions.
 ##
-## When telemetry is disabled, events are never buffered or sent. If an env
-## var is explicitly set to a non-truthy value, telemetry is enabled even if
-## the editor setting is false.
+## When telemetry is disabled, events are never buffered or sent. Only a
+## *truthy* env var force-disables; a falsey or absent env var falls through
+## to the EditorSetting (which defaults to enabled). See McpSettings.telemetry_enabled.
 ##
 ## Buffering: events recorded before the WebSocket is connected go into
 ## a small bounded buffer and flush on the next `record_event` call once

plugin/addons/godot_ai/utils/server_lifecycle.gd

@@ -371,14 +371,18 @@ static func _live_package_path_for_message(live: Dictionary) -> String:
 
 ## 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
+## nor DISABLE_TELEMETRY is already set to a *truthy* value (a falsey "0" does
+## NOT count — it must not suppress a dock UI opt-out), and (b) the effective
+## McpSettings.telemetry_enabled() is false. Returns true if the var was
 ## injected so the caller can unset it after spawning.
 func _inject_telemetry_env() -> bool:
-	## Guard: if the user or CI already set an env var (even to "false"), leave
-	## it alone. McpSettings.telemetry_enabled() only reads the EditorSetting
-	## when no env var overrides are present.
-	if OS.has_environment("GODOT_AI_DISABLE_TELEMETRY") or OS.has_environment("DISABLE_TELEMETRY"):
+	## If telemetry is already disabled by a *truthy* env var, leave the env as
+	## the user/CI set it — the post-spawn cleanup unsets what we inject, so
+	## injecting here would strip their own var from the editor process. A
+	## *falsey* value (e.g. DISABLE_TELEMETRY=0) must NOT count as "handled":
+	## fall through so a dock UI opt-out still reaches the spawned server. The
+	## truthy test mirrors McpSettings.telemetry_enabled() and the Python server.
+	if McpSettings.env_truthy("GODOT_AI_DISABLE_TELEMETRY") or McpSettings.env_truthy("DISABLE_TELEMETRY"):
 		return false
 	if not McpSettings.telemetry_enabled():
 		OS.set_environment("GODOT_AI_DISABLE_TELEMETRY", "true")

test_project/tests/test_clients.gd

@@ -820,6 +820,43 @@ func test_json_strategy_preserves_other_servers() -> void:
 	assert_true(parsed["mcpServers"].has("godot-ai"), "Our entry not added")
 
 
+func test_json_strategy_preserves_integer_fields() -> void:
+	## Godot parses every JSON number as a float; a naive round-trip re-emits the
+	## user's integer fields (ports, counts) as "8080.0", which strict consumers
+	## reject and which churns numbers across the user's other entries. The
+	## strategy must re-narrow integral numbers so ints stay ints. (#528 / TC-2)
+	var path := _scratch_dir.path_join("ints.json")
+	var seed := {
+		"mcpServers": {"someone-else": {"url": "http://other/", "port": 8080, "retries": 3}},
+		"numStartups": 47,
+		"weights": [1, 2, 3],
+	}
+	var f := FileAccess.open(path, FileAccess.WRITE)
+	f.store_string(JSON.stringify(seed))
+	f.close()
+
+	var client := _make_test_json_client(path)
+	var result := McpJsonStrategy.configure(client, "godot-ai", "http://127.0.0.1:8000/mcp")
+	assert_eq(result.get("status"), "ok")
+
+	var content_file := FileAccess.open(path, FileAccess.READ)
+	var content := content_file.get_as_text()
+	content_file.close()
+	# Integers must survive as integers — not be floatified to "8080.0".
+	assert_true(content.contains('"port": 8080'), "port int must be present")
+	assert_false(content.contains('"port": 8080.0'), "port must not become 8080.0")
+	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")
+	# Check each element regardless of trailing comma/newline so a floatified
+	# last element ("3.0" with no comma) is also caught.
+	for floatified in ["1.0", "2.0", "3.0"]:
+		assert_false(content.contains(floatified), "array int must not be floatified (%s)" % floatified)
+	# Still valid JSON, other entry preserved, our entry added.
+	var parsed = JSON.parse_string(content)
+	assert_true(parsed["mcpServers"].has("someone-else"))
+	assert_true(parsed["mcpServers"].has("godot-ai"))
+
+
 func test_json_strategy_refuses_to_overwrite_unparseable_file() -> void:
 	## Regression: if the config file exists but we can't parse it (trailing
 	## comma, stray comment, truncated write), `configure()` used to silently

test_project/tests/test_server_lifecycle.gd

@@ -429,3 +429,27 @@ func test_inject_skips_when_disable_telemetry_already_present() -> void:
 	host.free()
 
 	assert_false(injected, "must not inject when DISABLE_TELEMETRY is already in env")
+
+
+func test_inject_when_env_present_but_falsey_and_setting_disabled() -> void:
+	## A falsey env value (e.g. DISABLE_TELEMETRY=0) must NOT suppress a dock UI
+	## opt-out. The Python server parses the env truthily, so a falsey value
+	## leaves the server *enabled* — the plugin has to inject the disable flag
+	## so the opt-out actually reaches the spawned server. The old
+	## has_environment() guard treated any value (even "0") as "handled" and
+	## silently shipped telemetry against the user's UI choice. (#530)
+	_clear_telemetry_env_vars()
+	OS.set_environment(_TENV2, "0")
+	EditorInterface.get_editor_settings().set_setting(
+		McpSettings.SETTING_TELEMETRY_ENABLED, false
+	)
+	var host := _ManagerHostStub.new()
+	var manager := McpServerLifecycleManagerScript.new(host)
+
+	var injected := manager._inject_telemetry_env()
+	var env_present := OS.has_environment(_TENV1)
+	host.free()
+	_clear_telemetry_env_vars()
+
+	assert_true(injected, "a falsey DISABLE_TELEMETRY must not suppress the UI opt-out")
+	assert_true(env_present, "GODOT_AI_DISABLE_TELEMETRY must be injected for the spawn")