diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go index 67c5964..df2ff2e 100644 --- a/internal/cli/telemetry.go +++ b/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 diff --git a/internal/cli/telemetry_test.go b/internal/cli/telemetry_test.go index a376603..61de040 100644 --- a/internal/cli/telemetry_test.go +++ b/internal/cli/telemetry_test.go @@ -271,10 +271,12 @@ 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 @@ -282,8 +284,8 @@ func TestTheEnvironmentIsNeverGuessed(t *testing.T) { {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) {