From e9e9b9b50acf75f7a55a5ea0382708ca7016c68a Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 19 Aug 2026 08:49:57 +0200 Subject: [PATCH 1/2] fix(telemetry): do not file a run signed into an unknown env under prod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit telemetryEnv repaired a present-but-unrecognised signed-in environment through api.ResolveEnv(""), which returns prod when $CLIENT_ENV is unset. New() then saw a known env and exported — filing a run signed into an unknown backend under prod, the exact guess §3.2 forbids and this function's own doc disclaims. Distinguish the two cases: empty (not signed in) still resolves via $CLIENT_ENV then the prod default; a present-but-unknown value is passed through unchanged so New() disables export. Correct TestTheEnvironmentIsNeverGuessed, which asserted the buggy prod answer for a signed-in "staging", and add an end-to-end regression (TestASignedInUnknownEnvironmentDeliversNothing). Bugbot (Medium), cli#528 staging mirror. Co-Authored-By: Claude Opus 5 --- internal/cli/telemetry.go | 20 +++++++++++++++----- internal/cli/telemetry_test.go | 29 ++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go index 67c5964..cc6b601 100644 --- a/internal/cli/telemetry.go +++ b/internal/cli/telemetry.go @@ -92,15 +92,25 @@ 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. +// about. When there is no signed-in environment, $CLIENT_ENV and the prod +// default are api.ResolveEnv's existing answer, reused rather than restated. +// +// A signed-in value that is present but unrecognised is NOT repaired here: it is +// passed through unchanged so New() sees an unknown environment and disables +// export (§3.2 — refusal to export under a guessed environment belongs in one +// place). Repairing it to the prod default instead would file a run signed into +// an unknown backend under prod — the exact guess §3.2 forbids. func telemetryEnv(signedInEnv string) string { if api.IsKnownEnv(signedInEnv) { return strings.ToLower(signedInEnv) } - return api.ResolveEnv("") + if signedInEnv == "" { + // Not signed in: $CLIENT_ENV, then the prod default. + return api.ResolveEnv("") + } + // Signed in to an unrecognised environment: pass it through so the emitter + // refuses to export, rather than guessing prod. + return signedInEnv } // 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..8ebda2d 100644 --- a/internal/cli/telemetry_test.go +++ b/internal/cli/telemetry_test.go @@ -282,7 +282,10 @@ 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 + // "staging" is signed-in but unknown: passed through unchanged, NOT + // repaired to stg and NOT guessed to prod. New() then disables export. + {"staging", "staging"}, + // Empty is "not signed in": CLIENT_ENV (empty here) then the prod default. {"", api.EnvProd}, } { t.Run("signed_in_"+tc.signedIn, func(t *testing.T) { @@ -332,6 +335,30 @@ func TestTheSignedInEnvironmentWins(t *testing.T) { } } +func TestASignedInUnknownEnvironmentDeliversNothing(t *testing.T) { + // The bug this pins: a run signed into an environment the CLI does not + // recognise must not be filed under prod. telemetryEnv used to repair the + // unknown value to the prod default, so New() saw a known env and exported. + // The signed-in value must reach New() unrepaired so export is refused (§3.2). + dir := t.TempDir() + t.Setenv("TRACEBLOC_CONFIG_DIR", dir) + t.Setenv("CLIENT_ENV", "") // so a leak would have to come 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()) + if _, _, ok := captureOutcome(t, root, root, 0, nil); ok { + t.Fatal("delivered a record for a run signed into an unrecognised environment") + } +} + // --- instance id --------------------------------------------------------------- func TestTheInstanceIDIsPerProcessAndNotTheHostname(t *testing.T) { From bd8c35b6139955f61c13b451b89f9c9b5a926228 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 19 Aug 2026 09:32:22 +0200 Subject: [PATCH 2/2] fix(telemetry): label the record with the backend the client actually uses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reversing the direction of the first commit, per @saadqbal's review. The premise there — an unknown signed-in env is an unknown backend, so withhold — does not hold: sessionEnv (client.go) hands cfg.CurrentEnv to api.New verbatim and api.BaseURL routes every unrecognised value to prod. So a run signed into an unknown env genuinely hits prod, prod is the ACCURATE label, and withholding drops exactly the failed-install-on-prod runs this feature exists to see. telemetryEnv now mirrors api.BaseURL: resolve (CurrentEnv, else $CLIENT_ENV/prod), then known -> itself, unknown -> prod. The real bug it fixes is the old code reading $CLIENT_ENV for a signed-in env while the client ignores it — filing a run under 'dev' while every request went to prod. Rename the param env -> drop the shadow of signedInEnv(). Tests flip from 'delivers nothing' to 'labelled prod', plus TestASignedInUnknownEnvIgnoresClientEnv pinning the divergence (fails against the old code). Root cause — BaseURL silently routing unknown envs to prod — filed separately. Co-Authored-By: Claude Opus 5 --- internal/cli/telemetry.go | 44 +++++++++++++--------- internal/cli/telemetry_test.go | 67 +++++++++++++++++++++++----------- 2 files changed, 72 insertions(+), 39 deletions(-) diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go index cc6b601..df2ff2e 100644 --- a/internal/cli/telemetry.go +++ b/internal/cli/telemetry.go @@ -91,26 +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. When there is no signed-in environment, $CLIENT_ENV and the prod -// default are api.ResolveEnv's existing answer, reused rather than restated. +// 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. // -// A signed-in value that is present but unrecognised is NOT repaired here: it is -// passed through unchanged so New() sees an unknown environment and disables -// export (§3.2 — refusal to export under a guessed environment belongs in one -// place). Repairing it to the prod default instead would file a run signed into -// an unknown backend under prod — the exact guess §3.2 forbids. -func telemetryEnv(signedInEnv string) string { - if api.IsKnownEnv(signedInEnv) { - return strings.ToLower(signedInEnv) +// $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 signedInEnv == "" { - // Not signed in: $CLIENT_ENV, then the prod default. - return api.ResolveEnv("") + if api.IsKnownEnv(resolved) { + return strings.ToLower(resolved) } - // Signed in to an unrecognised environment: pass it through so the emitter - // refuses to export, rather than guessing prod. - return signedInEnv + // 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 8ebda2d..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,11 +284,8 @@ func TestTheEnvironmentIsNeverGuessed(t *testing.T) { {api.EnvDev, api.EnvDev}, {api.EnvStg, api.EnvStg}, {"PROD", api.EnvProd}, - // "staging" is signed-in but unknown: passed through unchanged, NOT - // repaired to stg and NOT guessed to prod. New() then disables export. - {"staging", "staging"}, - // Empty is "not signed in": CLIENT_ENV (empty here) then the prod 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", "") @@ -297,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) } } @@ -335,14 +354,14 @@ func TestTheSignedInEnvironmentWins(t *testing.T) { } } -func TestASignedInUnknownEnvironmentDeliversNothing(t *testing.T) { - // The bug this pins: a run signed into an environment the CLI does not - // recognise must not be filed under prod. telemetryEnv used to repair the - // unknown value to the prod default, so New() saw a known env and exported. - // The signed-in value must reach New() unrepaired so export is refused (§3.2). +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 a leak would have to come from the config, not the env + 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) @@ -354,8 +373,12 @@ func TestASignedInUnknownEnvironmentDeliversNothing(t *testing.T) { "and this fixture (and possibly the reader) is stale", got, "banana") } root := NewRootCmd(testBuildInfo()) - if _, _, ok := captureOutcome(t, root, root, 0, nil); ok { - t.Fatal("delivered a record for a run signed into an unrecognised environment") + 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) } }