fix(telemetry): address Codex review — env-resolved gate, opt-out race, CLI guards (MCP-2482)

Codex REQUEST_CHANGES on PR #684:

1. [P1] Respect env-resolved disabled state. The transition gated only on
   Config.IsTelemetryEnabled() (MCPPROXY_TELEMETRY), so DO_NOT_TRACK=1 / CI=true
   could emit a beacon when telemetry was never enabled. Add
   EffectiveTelemetryEnabled() = IsTelemetryEnabled() && !IsDisabledByEnv(), and
   use it for the transition, the New() resolvedEnabled seed, and the CLI.

2. [P1] Heartbeat-after-opt-out race. sendHeartbeat checked optedOut only at
   entry; a heartbeat in flight when the latch flipped still shipped a full usage
   payload. Re-check the latch immediately before transmit so no usage data
   leaves after opt-out.

3. [P2] CLI disable path. Routed through a single guarded entry point
   (Service.EmitOptOutBeacon — applies semver/dev, env, and anon-id guards and
   owns the send) instead of calling SendOptOutBeacon directly (which bypassed
   the guards). The CLI now confirms "Telemetry disabled." before the best-effort
   beacon (non-blocking) with a short 3s timeout.

Tests: env-disabled emits nothing; mid-flight opt-out suppresses transmit;
EmitOptOutBeacon guard matrix (dev/env/no-anon-id skip, eligible sends). CLI
smoke: enabled=1 beacon, DO_NOT_TRACK/CI=0.

Related #MCP-2482

Author: Dumbris

cmd/mcpproxy/telemetry_cmd.go

@@ -268,11 +268,12 @@ func runTelemetryDisable(cmd *cobra.Command, _ []string) error {
 		return fmt.Errorf("failed to load config: %w", err)
 	}
 
-	// Capture the resolved state BEFORE mutating, so we can detect a genuine
-	// enabled->disabled transition (MCP-2482). A second `disable` when already
-	// disabled must not emit another beacon.
-	wasEnabled := cfg.IsTelemetryEnabled()
-	anonID := cfg.GetAnonymousID()
+	// Capture the EFFECTIVE resolved state BEFORE mutating, so we only beacon on
+	// a genuine enabled->disabled transition (MCP-2482). Effective resolution
+	// includes env overrides (DO_NOT_TRACK / CI), so an install where telemetry
+	// was never actually enabled emits nothing. A second `disable` when already
+	// disabled also emits nothing (wasEnabled == false).
+	wasEnabled := telemetry.EffectiveTelemetryEnabled(cfg)
 
 	if cfg.Telemetry == nil {
 		cfg.Telemetry = &config.TelemetryConfig{}
@@ -285,24 +286,23 @@ func runTelemetryDisable(cmd *cobra.Command, _ []string) error {
 		return fmt.Errorf("failed to save config: %w", err)
 	}
 
-	// One-time opt-out beacon: best-effort, fire on the real enabled->disabled
-	// flip. When a daemon is running it does NOT auto-reload this file (there is
-	// no fsnotify watcher), so the CLI is responsible for the beacon in the
-	// CLI-driven path. If there is no anonymous ID there is nothing to dedup on.
-	if wasEnabled && anonID != "" {
-		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	// The disable is now persisted and effective — confirm immediately so the
+	// command never appears to hang on the (best-effort) beacon below.
+	fmt.Println("Telemetry disabled.")
+
+	// One-time opt-out beacon. When a daemon is running it does NOT auto-reload
+	// this file (there is no fsnotify watcher), so the CLI is responsible for the
+	// beacon in the CLI-driven path. Route it through the SAME guarded server-side
+	// entry point (EmitOptOutBeacon applies the dev-build/semver, env, and
+	// anon-id guards and owns the single send) rather than duplicating the send
+	// or bypassing a guard. A short timeout keeps this from blocking on a slow
+	// endpoint; the CLI is short-lived so the send must complete before exit.
+	if wasEnabled {
+		ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
 		defer cancel()
-		// Reuse the telemetry service's own sender so the destination is the
-		// resolved telemetry endpoint (never an arbitrary caller-supplied URL).
-		beaconSvc := telemetry.New(cfg, "", "", "", zap.NewNop())
-		if beaconErr := beaconSvc.SendOptOutBeacon(ctx); beaconErr != nil {
-			// Best-effort only — telemetry is already disabled on disk. Surface
-			// at a low level so scripts aren't tripped up.
-			fmt.Println("Note: opt-out signal could not be delivered (telemetry is still disabled).")
-		}
+		beaconSvc := telemetry.New(cfg, "", version, Edition, zap.NewNop())
+		beaconSvc.EmitOptOutBeacon(ctx)
 	}
-
-	fmt.Println("Telemetry disabled.")
 	return nil
 }
 

internal/telemetry/optout.go

@@ -31,17 +31,61 @@ type OptOutBeacon struct {
 	AnonymousID string `json:"anonymous_id"`
 }
 
+// EffectiveTelemetryEnabled resolves whether telemetry is ACTUALLY active for
+// cfg, honoring both the config bool (IsTelemetryEnabled — nil means enabled per
+// MCP-2477) AND the startup env overrides (DO_NOT_TRACK / CI / MCPPROXY_TELEMETRY
+// via IsDisabledByEnv). This must match the same effective resolution startup
+// uses: telemetry that was never enabled (e.g. DO_NOT_TRACK=1 or CI=true) must
+// never be considered "enabled", so disabling it produces no opt-out beacon.
+func EffectiveTelemetryEnabled(cfg *config.Config) bool {
+	if cfg == nil {
+		return false
+	}
+	if disabled, _ := IsDisabledByEnv(); disabled {
+		return false
+	}
+	return cfg.IsTelemetryEnabled()
+}
+
 // TelemetryDisableTransition reports whether the resolved telemetry state moved
-// from enabled to disabled between prior and next. "Resolved" follows
-// Config.IsTelemetryEnabled — a nil Telemetry block or a nil Enabled pointer
-// resolves to enabled (telemetry is opt-out), per MCP-2477. This is the single
+// from enabled to disabled between prior and next, using the SAME effective
+// resolution as startup (config bool + env overrides). This is the single
 // source of truth for the enabled->disabled flip, shared by the running daemon
 // (REST / reload paths) and the `mcpproxy telemetry disable` CLI.
 func TelemetryDisableTransition(prior, next *config.Config) bool {
 	if prior == nil || next == nil {
 		return false
 	}
-	return prior.IsTelemetryEnabled() && !next.IsTelemetryEnabled()
+	return EffectiveTelemetryEnabled(prior) && !EffectiveTelemetryEnabled(next)
+}
+
+// EmitOptOutBeacon applies the shared eligibility guards and, if eligible, sends
+// the opt-out beacon synchronously. It is the single guarded send entry point
+// used by BOTH the daemon (fire-and-forget in a goroutine from
+// NotifyConfigChanged) and the `mcpproxy telemetry disable` CLI — so neither
+// duplicates the send nor bypasses a guard.
+//
+// Guards (all must hold, mirroring the heartbeat eligibility):
+//   - valid semver build (dev builds never emit telemetry),
+//   - telemetry not disabled by environment (DO_NOT_TRACK / CI / MCPPROXY_TELEMETRY),
+//   - an anonymous install ID exists (nothing to dedup on otherwise).
+//
+// Returns true if a send was attempted (regardless of HTTP outcome). Best-effort:
+// callers disable telemetry whether or not this succeeds.
+func (s *Service) EmitOptOutBeacon(ctx context.Context) bool {
+	if !isValidSemver(s.version) {
+		return false
+	}
+	if disabled, _ := IsDisabledByEnv(); disabled {
+		return false
+	}
+	if s.config.GetAnonymousID() == "" {
+		return false
+	}
+	if err := s.SendOptOutBeacon(ctx); err != nil {
+		s.logger.Debug("opt-out beacon send failed (telemetry still disabled)", zap.Error(err))
+	}
+	return true
 }
 
 // SendOptOutBeacon posts a single opt-out beacon to the configured telemetry
@@ -160,7 +204,10 @@ func (s *Service) NotifyConfigChanged(newCfg *config.Config) {
 
 	s.mu.Lock()
 	prior := s.resolvedEnabled
-	next := newCfg.IsTelemetryEnabled()
+	// Use the SAME effective resolution as startup (config bool + env overrides)
+	// so an env-disabled install (DO_NOT_TRACK / CI) — which never emitted
+	// telemetry — never produces an opt-out beacon on a config flip.
+	next := EffectiveTelemetryEnabled(newCfg)
 	s.resolvedEnabled = next
 	// Keep the service's live config/endpoint current. ApplyConfig swaps the
 	// runtime's *config.Config pointer wholesale, so without this the service
@@ -181,19 +228,11 @@ func (s *Service) NotifyConfigChanged(newCfg *config.Config) {
 	// Stop all further telemetry immediately, before the (slower) network send.
 	s.optedOut.Store(true)
 
-	// Dev builds never emit telemetry; don't emit a beacon for them either.
-	if !isValidSemver(s.version) {
-		return
-	}
-	if newCfg.GetAnonymousID() == "" {
-		return
-	}
-
+	// Fire-and-forget through the shared guarded entry point (semver / env /
+	// anon-id guards live there). Never blocks the caller (the config save).
 	go func() {
 		ctx, cancel := context.WithTimeout(context.Background(), optOutBeaconTimeout)
 		defer cancel()
-		if err := s.SendOptOutBeacon(ctx); err != nil {
-			s.logger.Debug("opt-out beacon send failed (telemetry still disabled)", zap.Error(err))
-		}
+		s.EmitOptOutBeacon(ctx)
 	}()
 }

internal/telemetry/optout_test.go

@@ -272,3 +272,131 @@ func TestOptOut_StopsFurtherHeartbeats(t *testing.T) {
 		t.Fatalf("heartbeat emitted after opt-out: hits went %d -> %d", hitsAfterBeacon, got)
 	}
 }
+
+// hookStats is a RuntimeStats whose GetServerCount fires a hook, letting a test
+// flip state DURING buildHeartbeat to exercise the in-flight opt-out race.
+type hookStats struct {
+	onServerCount func()
+}
+
+func (h *hookStats) GetServerCount() int {
+	if h.onServerCount != nil {
+		h.onServerCount()
+	}
+	return 1
+}
+func (h *hookStats) GetConnectedServerCount() int      { return 0 }
+func (h *hookStats) GetToolCount() int                 { return 0 }
+func (h *hookStats) GetRoutingMode() string            { return "retrieve_tools" }
+func (h *hookStats) IsQuarantineEnabled() bool         { return false }
+func (h *hookStats) IsDockerAvailable() bool           { return false }
+func (h *hookStats) GetDockerIsolatedServerCount() int { return 0 }
+
+// TestSendHeartbeat_RechecksOptOutBeforeTransmit covers Codex fix #2: a heartbeat
+// already in flight when the opt-out latch flips must NOT transmit a usage
+// payload. The latch is flipped mid-buildHeartbeat (so the entry check has
+// already passed); the pre-transmit re-check must suppress the send.
+func TestSendHeartbeat_RechecksOptOutBeforeTransmit(t *testing.T) {
+	clearTelemetryEnv(t)
+
+	var hits atomic.Int32
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		hits.Add(1)
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	cfg := &config.Config{Telemetry: &config.TelemetryConfig{
+		Enabled: boolPtr(true), AnonymousID: "anon-xyz", Endpoint: server.URL,
+	}}
+	svc := New(cfg, "", "v1.2.3", "personal", zap.NewNop())
+	// Flip the opt-out latch while the payload is being built (after the entry
+	// guard, before transmit).
+	svc.SetRuntimeStats(&hookStats{onServerCount: func() { svc.optedOut.Store(true) }})
+
+	svc.sendHeartbeat(context.Background())
+
+	if got := hits.Load(); got != 0 {
+		t.Fatalf("usage heartbeat transmitted after mid-flight opt-out: %d sends", got)
+	}
+}
+
+// TestNotifyConfigChanged_NoBeaconWhenEnvDisabled covers Codex fix #1: when
+// telemetry is disabled by environment (DO_NOT_TRACK / CI), it was never
+// enabled, so a config enabled->disabled flip must emit NO opt-out beacon.
+func TestNotifyConfigChanged_NoBeaconWhenEnvDisabled(t *testing.T) {
+	clearTelemetryEnv(t)
+	t.Setenv("DO_NOT_TRACK", "1") // env-disabled: telemetry never ran
+
+	received := make(chan struct{}, 4)
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		received <- struct{}{}
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	enabled := &config.Config{Telemetry: &config.TelemetryConfig{
+		Enabled: boolPtr(true), AnonymousID: "anon-xyz", Endpoint: server.URL,
+	}}
+	disabled := &config.Config{Telemetry: &config.TelemetryConfig{
+		Enabled: boolPtr(false), AnonymousID: "anon-xyz", Endpoint: server.URL,
+	}}
+
+	svc := New(enabled, "", "v1.2.3", "personal", zap.NewNop())
+	svc.NotifyConfigChanged(disabled)
+
+	select {
+	case <-received:
+		t.Fatal("no beacon expected when telemetry was env-disabled (never enabled)")
+	case <-time.After(300 * time.Millisecond):
+	}
+}
+
+// TestEmitOptOutBeacon_Guards covers Codex fix #3: the single guarded send entry
+// (used by both the daemon and the CLI) must skip on a dev/non-semver build, when
+// env-disabled, and when there is no anonymous ID — and send only when eligible.
+func TestEmitOptOutBeacon_Guards(t *testing.T) {
+	newSvc := func(version, anonID string, endpoint string) *Service {
+		return New(&config.Config{Telemetry: &config.TelemetryConfig{
+			AnonymousID: anonID, Endpoint: endpoint,
+		}}, "", version, "personal", zap.NewNop())
+	}
+
+	t.Run("dev_build_skips", func(t *testing.T) {
+		clearTelemetryEnv(t)
+		if newSvc("dev", "anon", "http://127.0.0.1:0").EmitOptOutBeacon(context.Background()) {
+			t.Fatal("dev (non-semver) build must not attempt a beacon")
+		}
+	})
+
+	t.Run("env_disabled_skips", func(t *testing.T) {
+		clearTelemetryEnv(t)
+		t.Setenv("CI", "true")
+		if newSvc("v1.2.3", "anon", "http://127.0.0.1:0").EmitOptOutBeacon(context.Background()) {
+			t.Fatal("env-disabled (CI) build must not attempt a beacon")
+		}
+	})
+
+	t.Run("no_anon_id_skips", func(t *testing.T) {
+		clearTelemetryEnv(t)
+		if newSvc("v1.2.3", "", "http://127.0.0.1:0").EmitOptOutBeacon(context.Background()) {
+			t.Fatal("missing anonymous ID must not attempt a beacon")
+		}
+	})
+
+	t.Run("eligible_sends", func(t *testing.T) {
+		clearTelemetryEnv(t)
+		var hits atomic.Int32
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			hits.Add(1)
+			w.WriteHeader(http.StatusOK)
+		}))
+		defer server.Close()
+		if !newSvc("v1.2.3", "anon", server.URL).EmitOptOutBeacon(context.Background()) {
+			t.Fatal("eligible service should attempt a beacon")
+		}
+		if hits.Load() != 1 {
+			t.Fatalf("expected exactly one beacon, got %d", hits.Load())
+		}
+	})
+}

internal/telemetry/telemetry.go

@@ -255,7 +255,7 @@ func New(cfg *config.Config, cfgPath, version, edition string, logger *zap.Logge
 		envDisabledReason: envReason,
 		initialDelay:      5 * time.Minute,
 		heartbeatInterval: 24 * time.Hour,
-		resolvedEnabled:   cfg.IsTelemetryEnabled(),
+		resolvedEnabled:   EffectiveTelemetryEnabled(cfg),
 	}
 }
 
@@ -472,6 +472,15 @@ func (s *Service) sendHeartbeat(ctx context.Context) {
 	}
 	req.Header.Set("Content-Type", "application/json")
 
+	// MCP-2482: re-check the opt-out latch immediately before transmit. The
+	// entry check above can pass for a heartbeat already in flight when the user
+	// opts out mid-build; without this second check that heartbeat would still
+	// ship a full usage payload AFTER the opt-out. No usage data leaves once the
+	// latch is set.
+	if s.optedOut.Load() {
+		return
+	}
+
 	resp, err := s.client.Do(req)
 	if err != nil {
 		s.logger.Debug("Failed to send heartbeat", zap.Error(err))