Followup changes for #446 (#453)

* Followup changes for #446

* Review nits: docstring/comment polish from PR #453 feedback

- mcp_dock.gd: clarify the Vector2i wrap with a comment explaining that
  `Vector2i * float` returns `Vector2`, and use `Vector2(560, 460)` as
  the literal so the cast reads as intentional instead of doubled.
- telemetry.gd: normalize tab indentation and "MCP dock" casing in the
  opt-out priority docstring.
- test_telemetry_setting.gd: remove the contradictory "may or may not
  remove the key depending on Godot version" caveat; passing null to
  EditorSettings.set_setting has been the documented unset path for
  many years.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: David Sarno <david@lighthaus.us>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: 3 people

docs/TELEMETRY.md

@@ -18,7 +18,8 @@ goes, and how to opt out. All telemetry code is open source and lives in
 - **Non-blocking**: events go through a bounded in-process queue and a
   single daemon worker. Telemetry failures never propagate to tool
   callers.
-- **Easy opt-out**: one environment variable. See "Opting out" below.
+- **Easy opt-out**: Respects opt-out via environment variable or through
+  in-editor settings menu. See "Opting out" below.
 
 ## What we collect
 
@@ -83,7 +84,7 @@ export GODOT_AI_DISABLE_TELEMETRY=true
 export DISABLE_TELEMETRY=true
 ```
 
-If either of the above environment variables are enabled, the opt-out is
+If either of the above environment variables is enabled, the opt-out is
 saved to Godot's editor settings and will persist between runs. Similarly,
 if an environment variable is explicitly set and disabled, that will be
 persisted to the editor settings.
@@ -93,12 +94,11 @@ startup.
 
 ### Effect
 
-On opt-out (whether via UI or env var): the collector enters disabled
-mode. No records are enqueued, no UUID is generated, no worker thread is
-spawned, and no data directory is created. Existing local telemetry
-files (`customer_uuid.txt`, `milestones.json`) are deleted on the next
-server startup. The plugin-side helper honors the same variables and
-stops buffering events. Opt-out is fully side-effect-free.
+On opt-out, the collector enters disabled mode. No records are enqueued,
+no UUID is generated, no worker thread is spawned, and no data directory
+is created. Existing local telemetry files (`customer_uuid.txt`,
+`milestones.json`) are deleted on the next server startup. The plugin-side
+helper honors the same variables and stops buffering events.
 
 ## Endpoint configuration
 

plugin/addons/godot_ai/client_configurator.gd

@@ -34,25 +34,16 @@ const SERVER_NAME := "godot-ai"
 ## the Windows-reservation diagnostics this is the escape hatch for.
 const DEFAULT_HTTP_PORT := 8000
 const DEFAULT_WS_PORT := 9500
-const SETTING_HTTP_PORT := "godot_ai/http_port"
-const SETTING_WS_PORT := "godot_ai/ws_port"
-const SETTING_STARTUP_TRACE := "godot_ai/log_startup_timing"
 const STARTUP_TRACE_ENV := "GODOT_AI_STARTUP_TRACE"
 const MIN_PORT := 1024
 const MAX_PORT := 65535
-
-## Comma-separated list of tool domains to drop from the server at spawn
-## time. Maps 1:1 onto the `--exclude-domains` CLI flag. Set via the dock's
-## "Tools" tab; a change requires a server restart (the dock handles this
-## by triggering a plugin reload). Unknown names are warned about on the
-## Python side and skipped, so an EditorSetting left over from a previous
-## plugin version can't wedge the spawn.
-const SETTING_EXCLUDED_DOMAINS := "godot_ai/excluded_domains"
+const SETTING_WS_PORT := "godot_ai/ws_port"
+const SETTING_STARTUP_TRACE := "godot_ai/log_startup_timing"
 
 
 ## Active HTTP port: user override (if in range) or `DEFAULT_HTTP_PORT`.
 static func http_port() -> int:
-	return _read_port_setting(SETTING_HTTP_PORT, DEFAULT_HTTP_PORT)
+	return _read_port_setting(McpSettings.SETTING_HTTP_PORT, DEFAULT_HTTP_PORT)
 
 
 ## Active WebSocket port: user override (if in range) or `DEFAULT_WS_PORT`.
@@ -83,7 +74,7 @@ static func ensure_settings_registered() -> void:
 	var es := EditorInterface.get_editor_settings()
 	if es == null:
 		return
-	_register_port_setting(es, SETTING_HTTP_PORT, DEFAULT_HTTP_PORT)
+	_register_port_setting(es, McpSettings.SETTING_HTTP_PORT, DEFAULT_HTTP_PORT)
 	_register_port_setting(es, SETTING_WS_PORT, DEFAULT_WS_PORT)
 	_register_bool_setting(es, SETTING_STARTUP_TRACE, false)
 
@@ -128,9 +119,9 @@ static func startup_trace_enabled() -> bool:
 ## `--exclude-domains` don't see an empty argument.
 static func excluded_domains() -> String:
 	var es := EditorInterface.get_editor_settings()
-	if es == null or not es.has_setting(SETTING_EXCLUDED_DOMAINS):
+	if es == null or not es.has_setting(McpSettings.SETTING_EXCLUDED_DOMAINS):
 		return ""
-	var raw := str(es.get_setting(SETTING_EXCLUDED_DOMAINS))
+	var raw := str(es.get_setting(McpSettings.SETTING_EXCLUDED_DOMAINS))
 	var parts := PackedStringArray()
 	for p in raw.split(","):
 		var t := p.strip_edges()

plugin/addons/godot_ai/mcp_dock.gd

@@ -381,12 +381,12 @@ func _on_redock() -> void:
 
 
 func _build_margin_container(margin: int = 12) -> MarginContainer:
-	var marginContainer := MarginContainer.new()
-	marginContainer.add_theme_constant_override("margin_left", margin)
-	marginContainer.add_theme_constant_override("margin_right", margin)
-	marginContainer.add_theme_constant_override("margin_top", margin)
-	marginContainer.add_theme_constant_override("margin_bottom", margin)
-	return marginContainer
+	var margin_container := MarginContainer.new()
+	margin_container.add_theme_constant_override("margin_left", margin)
+	margin_container.add_theme_constant_override("margin_right", margin)
+	margin_container.add_theme_constant_override("margin_top", margin)
+	margin_container.add_theme_constant_override("margin_bottom", margin)
+	return margin_container
 
 
 func _build_ui() -> void:
@@ -571,7 +571,7 @@ func _build_ui() -> void:
 
 	var clients_open_btn := Button.new()
 	clients_open_btn.text = "Clients & Settings"
-	clients_open_btn.tooltip_text = "Open the MCP settings window — configure AI clients or disable tool domains to fit under a client's hard tool-count cap (e.g. Antigravity's 100)."
+	clients_open_btn.tooltip_text = "Open the MCP settings window — configure AI clients, choose telemetry preferences, or disable tool domains to fit under a client's hard tool-count cap (e.g. Antigravity's 100)."
 	clients_open_btn.pressed.connect(_on_open_clients_window)
 	clients_row.add_child(clients_open_btn)
 
@@ -595,7 +595,8 @@ func _build_ui() -> void:
 
 	_clients_window = Window.new()
 	_clients_window.title = "MCP Clients & Settings"
-	_clients_window.min_size = Vector2i(560, 460) * EditorInterface.get_editor_scale()
+	## `Vector2i * float` yields Vector2; wrap the result back to Vector2i.
+	_clients_window.min_size = Vector2i(Vector2(560, 460) * EditorInterface.get_editor_scale())
 	_clients_window.visible = false
 	_clients_window.close_requested.connect(_on_clients_window_close_requested)
 	add_child(_clients_window)
@@ -991,7 +992,7 @@ func _on_log_logging_enabled_changed(enabled: bool) -> void:
 func _on_port_apply_requested(new_port: int) -> void:
 	var es := EditorInterface.get_editor_settings()
 	if es != null:
-		es.set_setting(ClientConfigurator.SETTING_HTTP_PORT, new_port)
+		es.set_setting(McpSettings.SETTING_HTTP_PORT, new_port)
 	## Every saved client config now points at the old port. Re-sweep so the
 	## drift banner appears in the same frame the user committed the change —
 	## the plugin reload below will run a second sweep on its own first paint,
@@ -1015,19 +1016,13 @@ func _refresh_server_label() -> void:
 # --- Telemetry setting persistence ---
 
 
-func _env_truthy(var_name: String) -> bool:
-	var val: String = OS.get_environment(var_name).strip_edges().to_lower()
-	return val in ["1", "true", "yes", "on"]
-
-
 ## Returns true if GODOT_AI_DISABLE_TELEMETRY or DISABLE_TELEMETRY is set
 ## to a truthy value, false if either is set and non-truthy, null if neither
 ## env var is present at all.
 func _is_telemetry_disabled_via_env() -> Variant:
-	var disabled: bool = _env_truthy("GODOT_AI_DISABLE_TELEMETRY") or _env_truthy("DISABLE_TELEMETRY")
-	if OS.has_environment("GODOT_AI_DISABLE_TELEMETRY") or OS.has_environment("DISABLE_TELEMETRY"):
-		return disabled
-	return null
+	if not (OS.has_environment("GODOT_AI_DISABLE_TELEMETRY") or OS.has_environment("DISABLE_TELEMETRY")):
+		return null
+	return McpSettings.env_truthy("GODOT_AI_DISABLE_TELEMETRY") or McpSettings.env_truthy("DISABLE_TELEMETRY")
 
 
 ## Reads the telemetry preference, applying env-var override when present.
@@ -1044,15 +1039,15 @@ func _load_telemetry_setting() -> void:
 		## the env var honour the last-set value.
 		enabled = not bool(env_disabled)
 		if es != null:
-			es.set_setting("godot_ai/telemetry_enabled", enabled)
+			es.set_setting(McpSettings.SETTING_TELEMETRY_ENABLED, enabled)
 	else:
 		## No env var: read (or create) the EditorSettings key.
-		if es != null and es.has_setting("godot_ai/telemetry_enabled"):
-			enabled = bool(es.get_setting("godot_ai/telemetry_enabled"))
+		if es != null and es.has_setting(McpSettings.SETTING_TELEMETRY_ENABLED):
+			enabled = bool(es.get_setting(McpSettings.SETTING_TELEMETRY_ENABLED))
 		else:
 			enabled = true
 			if es != null:
-				es.set_setting("godot_ai/telemetry_enabled", true)
+				es.set_setting(McpSettings.SETTING_TELEMETRY_ENABLED, true)
 
 	_telemetry_pending_enabled = enabled
 	_telemetry_saved_enabled = enabled
@@ -1117,7 +1112,7 @@ func _apply_dev_mode_visibility() -> void:
 # --- Button handlers ---
 
 
-func _do_plugin_reload():
+func _do_plugin_reload() -> void:
 	EditorInterface.set_plugin_enabled("res://addons/godot_ai/plugin.cfg", false)
 	EditorInterface.set_plugin_enabled("res://addons/godot_ai/plugin.cfg", true)
 
@@ -1920,14 +1915,14 @@ func _on_tools_apply() -> void:
 	var canonical_excluded := ToolCatalog.canonical(_tools_pending_excluded)
 	var es := EditorInterface.get_editor_settings()
 	if es != null:
-		es.set_setting(ClientConfigurator.SETTING_EXCLUDED_DOMAINS, canonical_excluded)
-		es.set_setting("godot_ai/telemetry_enabled", _telemetry_pending_enabled)
+		es.set_setting(McpSettings.SETTING_EXCLUDED_DOMAINS, canonical_excluded)
+		es.set_setting(McpSettings.SETTING_TELEMETRY_ENABLED, _telemetry_pending_enabled)
 	_tools_saved_excluded = _tools_pending_excluded.duplicate()
 	_telemetry_saved_enabled = _telemetry_pending_enabled
 	_refresh_tools_ui_state()
-	## Plugin reload respawns the server with the new `--exclude-domains`
-	## flag (see `plugin.gd::_build_server_flags`). Mirrors the port-change
-	## Apply flow.
+	## Plugin reload respawns the server with the new `--exclude-domains` flag
+	## (see `plugin.gd::_build_server_flags`) and telemetry option. Mirrors the
+	## port-change Apply flow.
 	_on_reload_plugin()
 
 

plugin/addons/godot_ai/plugin.gd

@@ -534,7 +534,7 @@ static func _mcp_disabled_for_headless_launch() -> bool:
 
 
 static func _mcp_disabled_for_headless(args: PackedStringArray, display_name: String, allow_value: String) -> bool:
-	if _env_truthy(allow_value):
+	if McpSettings.truthy(allow_value):
 		return false
 	return _args_request_headless(args) or display_name.to_lower() == "headless"
 
@@ -551,12 +551,6 @@ static func _args_request_headless(args: PackedStringArray) -> bool:
 	return false
 
 
-static func _env_truthy(value: String) -> bool:
-	match value.strip_edges().to_lower():
-		"1", "true", "yes", "on":
-			return true
-		_:
-			return false
 
 
 func _disable_plugin() -> void:
@@ -1538,7 +1532,10 @@ func start_dev_server() -> void:
 			var new_pp := worktree_src if prev_pythonpath.is_empty() else worktree_src + sep + prev_pythonpath
 			OS.set_environment("PYTHONPATH", new_pp)
 
+		var injected_telemetry: bool = _lifecycle._inject_telemetry_env()
 		var pid := OS.create_process(cmd, inner_args)
+		if injected_telemetry:
+			OS.unset_environment("GODOT_AI_DISABLE_TELEMETRY")
 
 		## Restore PYTHONPATH immediately — the spawned child has already
 		## copied the env, so the editor's own process state returns to

plugin/addons/godot_ai/telemetry.gd

@@ -8,11 +8,16 @@
 ## and the bounded-queue worker stay in one place (Python), not
 ## duplicated in GDScript.
 ##
-## Opt-out: honors `GODOT_AI_DISABLE_TELEMETRY` and `DISABLE_TELEMETRY`
-## environment variables on the GDScript side too, so events are never
-## buffered (let alone sent) when the operator opts out — useful when
-## a tester wants to confirm the disable flag is effective before any
-## handshake has happened.
+## Opt-out options priority:
+##   1. `GODOT_AI_DISABLE_TELEMETRY` / `DISABLE_TELEMETRY` env vars —
+##      checked first so CI / operators can force-disable without touching
+##      EditorSettings.
+##   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.
 ##
 ## Buffering: events recorded before the WebSocket is connected go into
 ## a small bounded buffer and flush on the next `record_event` call once
@@ -83,7 +88,7 @@ var _pending: Array = []  # of {name: String, data: Dictionary}
 
 func _init(connection) -> void:
 	_connection = connection
-	_disabled = _resolve_disabled()
+	_disabled = not McpSettings.telemetry_enabled()
 	## Subscribe to ``connection_state_changed`` so events buffered before
 	## the WebSocket handshake (e.g. ``record_dock_startup`` from
 	## ``plugin._enter_tree``) actually leave the editor. Without this,
@@ -93,17 +98,6 @@ func _init(connection) -> void:
 	if _connection != null and _connection.has_signal("connection_state_changed"):
 		_connection.connection_state_changed.connect(_on_connection_state_changed)
 
-static func _truthy(value: String) -> bool:
-	## Match ``plugin.gd::_env_truthy`` semantics — ``strip_edges()`` so
-	## a stray ``" true "`` from shell quoting still parses as truthy.
-	return value.strip_edges().to_lower() in ["1", "true", "yes", "on"]
-
-static func _resolve_disabled() -> bool:
-	if _truthy(OS.get_environment("GODOT_AI_DISABLE_TELEMETRY")):
-		return true
-	if _truthy(OS.get_environment("DISABLE_TELEMETRY")):
-		return true
-	return false
 
 func record_event(name: String, data: Dictionary = {}) -> void:
 	if _disabled:

plugin/addons/godot_ai/utils/server_lifecycle.gd

@@ -391,15 +391,12 @@ static func _live_package_path_for_message(live: Dictionary) -> String:
 ## "godot_ai/telemetry_enabled" is set to 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"):
 		return false
-	var es := EditorInterface.get_editor_settings()
-	var telemetry_enabled: bool = (
-		bool(es.get_setting("godot_ai/telemetry_enabled"))
-		if es != null and es.has_setting("godot_ai/telemetry_enabled")
-		else true
-	)
-	if not telemetry_enabled:
+	if not McpSettings.telemetry_enabled():
 		OS.set_environment("GODOT_AI_DISABLE_TELEMETRY", "true")
 		return true
 	return false

plugin/addons/godot_ai/utils/settings.gd

@@ -0,0 +1,39 @@
+@tool
+class_name McpSettings
+extends RefCounted
+
+## Shared EditorSettings key constants for the godot_ai/* namespace.
+##
+## Centralised here so lightweight files (e.g. telemetry.gd) can reference
+## settings keys without pulling in the full client_configurator.gd dep tree.
+## All keys must keep their raw string values stable across releases because
+## they are persisted in the user's editor_settings-4.tres.
+
+const SETTING_HTTP_PORT := "godot_ai/http_port"
+## Comma-separated list of tool domains excluded from the server at spawn time.
+const SETTING_EXCLUDED_DOMAINS := "godot_ai/excluded_domains"
+const SETTING_TELEMETRY_ENABLED := "godot_ai/telemetry_enabled"
+
+
+## Returns true if the string value is truthy
+## ("1", "true", "yes", "on", case-insensitive, whitespace-trimmed).
+static func truthy(value: String) -> bool:
+	return value.strip_edges().to_lower() in ["1", "true", "yes", "on"]
+
+
+## Returns true if the named environment variable is set to a truthy value.
+static func env_truthy(var_name: String) -> bool:
+	return truthy(OS.get_environment(var_name))
+
+
+## Returns true if telemetry should be active, checking in priority order:
+##   1. GODOT_AI_DISABLE_TELEMETRY / DISABLE_TELEMETRY env vars
+##   2. The godot_ai/telemetry_enabled EditorSetting written by the dock UI
+## Defaults to true when neither source has set a preference.
+static func telemetry_enabled() -> bool:
+	if env_truthy("GODOT_AI_DISABLE_TELEMETRY") or env_truthy("DISABLE_TELEMETRY"):
+		return false
+	var es := EditorInterface.get_editor_settings()
+	if es != null and es.has_setting(SETTING_TELEMETRY_ENABLED):
+		return bool(es.get_setting(SETTING_TELEMETRY_ENABLED))
+	return true

plugin/addons/godot_ai/utils/settings.gd.uid

@@ -0,0 +1 @@
+uid://pefrtofs7ijw