fix(telemetry): don't file a run signed into an unknown env under prod

internal/cli/telemetry.go

@@ -91,16 +91,36 @@ func commandPathOf(c *cobra.Command) string {
 
 // telemetryEnv picks deployment.environment for the records.
 //
-// The signed-in environment wins because it is the backend these records are
-// about; $CLIENT_ENV and the prod default are api.ResolveEnv's existing answer,
-// reused rather than restated. An unrecognised value is not repaired here — the
-// emitter refuses to export under a guessed environment (§3.2), and that
-// refusal belongs in one place.
-func telemetryEnv(signedInEnv string) string {
-	if api.IsKnownEnv(signedInEnv) {
-		return strings.ToLower(signedInEnv)
+// It labels each record with the backend the client is ACTUALLY talking to,
+// resolved exactly the way api.BaseURL resolves it — because that is the host
+// these records are about. The mapping mirrors BaseURL: a known env is itself; a
+// present-but-unrecognised value is prod, because api.BaseURL routes every
+// unknown value to https://api.tracebloc.io (sessionEnv hands cfg.CurrentEnv to
+// api.New verbatim). So prod is the accurate label for that population, not a
+// guess — and NOT withheld: a misconfigured install that hits prod and fails is
+// exactly the run this feature exists to see.
+//
+// $CLIENT_ENV is consulted only when there is no signed-in env, matching
+// sessionEnv: once cfg.CurrentEnv is set the client ignores $CLIENT_ENV, so
+// resolving a signed-in unknown through $CLIENT_ENV would label the record for a
+// backend the client never contacts (the bug this replaces).
+//
+// NOTE: that api.BaseURL silently routes an unknown env to prod — so an install
+// believing it is on another backend sends its token there — is a real defect,
+// but in client.go, not here; tracked separately. This function must match that
+// behaviour until it changes, not diverge from it.
+func telemetryEnv(env string) string {
+	resolved := env
+	if resolved == "" {
+		// Not signed in: $CLIENT_ENV, then the prod default (as sessionEnv does).
+		resolved = api.ResolveEnv("")
+	}
+	if api.IsKnownEnv(resolved) {
+		return strings.ToLower(resolved)
 	}
-	return api.ResolveEnv("")
+	// Unrecognised: api.BaseURL sends it to prod, so prod is where these records
+	// belong.
+	return api.EnvProd
 }
 
 // signedInEnv reads the environment the config points at, best-effort. A

internal/cli/telemetry_test.go

@@ -271,19 +271,21 @@ func TestTheOffSpellingsDoNotOptOut(t *testing.T) {
 
 // --- environment ---------------------------------------------------------------
 
-func TestTheEnvironmentIsNeverGuessed(t *testing.T) {
-	// §3.2 — an unrecognised environment must not export under a repaired or
-	// guessed value. `staging` is the classic near miss: it is the git branch
-	// name, and `stg` is the environment value.
+func TestTheEnvironmentLabelMatchesTheBackend(t *testing.T) {
+	// The label is the backend api.BaseURL actually targets: a known env is
+	// itself; anything unrecognised is prod, because BaseURL routes it there.
+	// `staging` is the classic near miss — the git branch name, not the `stg`
+	// environment value — and it resolves to prod (where a client signed into
+	// "staging" really goes), NOT to stg.
 	for _, tc := range []struct {
 		signedIn string
 		want     string
 	}{
 		{api.EnvDev, api.EnvDev},
 		{api.EnvStg, api.EnvStg},
 		{"PROD", api.EnvProd},
-		{"staging", api.EnvProd}, // not repaired to stg — falls back to the default
-		{"", api.EnvProd},
+		{"staging", api.EnvProd}, // unknown -> prod, matching api.BaseURL
+		{"", api.EnvProd},        // not signed in, CLIENT_ENV empty -> prod
 	} {
 		t.Run("signed_in_"+tc.signedIn, func(t *testing.T) {
 			t.Setenv("CLIENT_ENV", "")
@@ -294,14 +296,34 @@ func TestTheEnvironmentIsNeverGuessed(t *testing.T) {
 	}
 }
 
-func TestAnUnknownEnvironmentDeliversNothing(t *testing.T) {
-	// The end-to-end consequence: the emitter refuses to export under a value no
-	// query filters on, and the wiring must not have talked it out of that.
+func TestASignedInUnknownEnvIgnoresClientEnv(t *testing.T) {
+	// The bug this pins (Asad, cli#528 review): the client resolves a signed-in
+	// env via sessionEnv, which returns cfg.CurrentEnv VERBATIM and never consults
+	// $CLIENT_ENV — so a config on "banana" talks to prod (api.BaseURL default)
+	// regardless of $CLIENT_ENV. The old code resolved the label through
+	// ResolveEnv, which DOES read $CLIENT_ENV, so it filed the run under "dev"
+	// while every request went to prod. The label must be prod, not dev.
+	t.Setenv("CLIENT_ENV", "dev")
+	if got := telemetryEnv("banana"); got != api.EnvProd {
+		t.Fatalf("telemetryEnv(%q) with CLIENT_ENV=dev = %q, want %q — the label "+
+			"must match the backend the client actually contacts (prod)", "banana", got, api.EnvProd)
+	}
+}
+
+func TestAnUnknownClientEnvIsLabelledProd(t *testing.T) {
+	// The end-to-end consequence: not signed in, CLIENT_ENV=staging. sessionEnv
+	// resolves that through ResolveEnv -> "staging", and api.BaseURL routes it to
+	// prod — so the run genuinely hits prod and its record must be filed under
+	// prod, the population this feature exists for, not withheld.
 	isolateConfig(t)
 	t.Setenv("CLIENT_ENV", "staging")
 	root := NewRootCmd(testBuildInfo())
-	if _, _, ok := captureOutcome(t, root, root, 0, nil); ok {
-		t.Fatal("delivered a record under an unrecognised environment")
+	res, _, ok := captureOutcome(t, root, root, 0, nil)
+	if !ok {
+		t.Fatal("withheld a record for a run that hits prod under an unknown CLIENT_ENV")
+	}
+	if res["deployment.environment"] != api.EnvProd {
+		t.Fatalf("deployment.environment = %q, want %q", res["deployment.environment"], api.EnvProd)
 	}
 }
 
@@ -332,6 +354,34 @@ func TestTheSignedInEnvironmentWins(t *testing.T) {
 	}
 }
 
+func TestASignedInUnknownEnvironmentIsLabelledProd(t *testing.T) {
+	// A run signed into an environment the CLI does not recognise talks to prod
+	// (sessionEnv hands cfg.CurrentEnv to api.New verbatim, api.BaseURL routes the
+	// unknown value to prod), so its record must be filed under prod — that
+	// failed-install-on-prod run is exactly what this feature exists to capture.
+	dir := t.TempDir()
+	t.Setenv("TRACEBLOC_CONFIG_DIR", dir)
+	t.Setenv("CLIENT_ENV", "") // so the label comes from the config, not the env
+	body := `{"version":2,"current_env":"banana","profiles":{"banana":{"token":"x"}}}`
+	if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(body), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	// Read it back before asserting: a config layout this fixture no longer
+	// matches must be a finding, not a quiet pass that exercises the empty path.
+	if got := signedInEnv(); got != "banana" {
+		t.Fatalf("signedInEnv() = %q, want %q — the on-disk config layout changed "+
+			"and this fixture (and possibly the reader) is stale", got, "banana")
+	}
+	root := NewRootCmd(testBuildInfo())
+	res, _, ok := captureOutcome(t, root, root, 0, nil)
+	if !ok {
+		t.Fatal("withheld a record for a run signed into an unknown env that hits prod")
+	}
+	if res["deployment.environment"] != api.EnvProd {
+		t.Fatalf("deployment.environment = %q, want %q", res["deployment.environment"], api.EnvProd)
+	}
+}
+
 // --- instance id ---------------------------------------------------------------
 
 func TestTheInstanceIDIsPerProcessAndNotTheHostname(t *testing.T) {