Skip to content

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

Merged
LukasWodka merged 2 commits into
developfrom
fix/2149-telemetry-unknown-env
Aug 19, 2026
Merged

fix(telemetry): don't file a run signed into an unknown env under prod#531
LukasWodka merged 2 commits into
developfrom
fix/2149-telemetry-unknown-env

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Resolves the Bugbot Medium on the `cli` develop→staging mirror (#528), `internal/cli/telemetry.go`.

The bug

`telemetryEnv` sent a present-but-unrecognised signed-in environment through `api.ResolveEnv("")`, which returns `prod` when `$CLIENT_ENV` is unset. `New()` then treated `prod` as a known env and exported — filing a run signed into an unknown backend under prod. That is the guess §3.2 forbids, and it contradicts the function’s own doc ("an unrecognised value is not repaired here").

The fix

Distinguish the two cases:

  • empty (not signed in) → still resolves via `$CLIENT_ENV`, then the prod default (unchanged);
  • present but unknown → passed through unchanged, so `New()` sees an unknown env and disables export.

Tests

  • Corrected `TestTheEnvironmentIsNeverGuessed`, which asserted the buggy `prod` answer for a signed-in `"staging"`.
  • Added `TestASignedInUnknownEnvironmentDeliversNothing` (end-to-end: a config with `current_env: banana` delivers nothing).
  • Mutation-proved: both requirement tests fail against the old code with the expected message; full `go test ./...` green, `go vet` clean.

Fix is on `develop` so the staging hop re-prepare picks it up.


Note

Medium Risk
Changes how misconfigured and unknown environments are attributed in telemetry (more events filed under prod), which affects operational dashboards and filters but not API auth or data handling.

Overview
Telemetry environment labeling now follows the same rules as api.BaseURL, so deployment.environment reflects the backend the CLI actually calls instead of a separate resolution path.

telemetryEnv only uses api.ResolveEnv when there is no signed-in env; known values are normalized, and unrecognized env strings (signed-in or via CLIENT_ENV) are labeled prod, matching unknown-to-prod API routing. Signed-in unknown envs no longer fall through ResolveEnv, which could incorrectly apply $CLIENT_ENV (e.g. banana + CLIENT_ENV=dev labeled dev while requests went to prod).

Tests were renamed and expanded to pin prod labeling and continued export for unknown/misconfigured envs, including end-to-end cases that previously expected withholding.

Reviewed by Cursor Bugbot for commit bd8c35b. Bugbot is set up for automated code reviews on this repo. Configure here.

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 <noreply@anthropic.com>
@LukasWodka LukasWodka self-assigned this Aug 19, 2026

@saadqbal saadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism is right — passing the value through does disable export, and the new test genuinely fails against the old code. But I don't think the premise holds: an unrecognised current_env isn't an unknown backend, it's prod, because sessionEnv (internal/cli/client.go:150) returns cfg.CurrentEnv verbatim and api.BaseURL falls through default: to https://api.tracebloc.io. Detail inline.

Outside the diff, that's the root cause worth its own ticket: authedClient will happily send the stored token to production for any current_env it doesn't recognise, while the user believes they're on another backend.

On the alerts in tracebloc/backend#2123 — no defect here, just a coordination note. cli is continuous = false there, so it gets no silence/heartbeat alarm, and unknown-env runs emit nothing at all rather than a new dimension value, so none of the metric-filter patterns break. The only interaction is coverage: runs that actually hit prod but carry an unrecognised current_env drop out of the service.name=cli + deployment.environment=prod failure alarm. Nothing live today (pendingSink is still nil), but worth agreeing on before #1905 connects the transport.

One test suggestion if the behaviour stands: TestASignedInUnknownEnvironmentDeliversNothing sets CLIENT_ENV="" on purpose, so the case where a signed-in unknown env coexists with CLIENT_ENV=dev is untested — and that's the one where the old code produced a genuinely wrong label rather than a defensible one.

Comment thread internal/cli/telemetry.go Outdated
Comment thread internal/cli/telemetry.go Outdated
… uses

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 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
Contributor Author

Good catch, Asad — verified and you're right. `sessionEnv` returns `cfg.CurrentEnv` verbatim and `api.BaseURL` defaults any unknown value to `https://api.tracebloc.io\`, so a `current_env: banana` install genuinely operates on prod; `prod` was the accurate label and my first cut dropped exactly the failed-prod runs the feature is for.

Reversed the direction: `telemetryEnv` now mirrors `api.BaseURL` (resolve, then known→itself / unknown→prod) and no longer reads `$CLIENT_ENV` for a signed-in env — which fixes the genuine bug you named (label `dev` while the client is on prod). Added `TestASignedInUnknownEnvIgnoresClientEnv` for exactly that case (fails against the old code); the two end-to-end tests flip from "delivers nothing" to "labelled prod". Root cause (BaseURL silently routing unknown→prod) filed as backend#2171.

@saadqbal saadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both points addressed, and the first one solved better than what I suggested. telemetryEnv now mirrors api.BaseURL's resolution rather than trying to normalise around it, so the label always names the host the client actually contacts. I checked the matrix at this head: prod, PROD, "prod ", " prod", a tab, " ", banana and staging all label prod, and api.BaseURL sends every one of them to https://api.tracebloc.io; dev/DEV label dev. Signed-in-unknown with CLIENT_ENV=dev labels prod, not dev — the sharpest version of the bug. And trimming, which is what I'd suggested, would have been wrong: " dev" routes to prod, so labelling it dev would have re-created the mismatch in the other direction.

Worth noting the return is now a closed set of exactly dev/stg/prod, so no config value can reach deployment.environment at all. That also settles the tracebloc/backend#2123 side: no new dimension value, no metric-filter pattern affected, and unknown-env runs land in the prod bucket where they genuinely belong. cli is continuous = false there, so there's no heartbeat alarm to worry about either.

Tests check out against both predecessors: the pre-PR telemetryEnv fails TestASignedInUnknownEnvIgnoresClientEnv and TestAnUnknownClientEnvIsLabelledProd, and the previous head fails all four. go vet ./... clean, go test ./... green at this SHA.

Thanks for splitting out backend#2171 — right call for the sessionEnv half.

nit, take or leave: the doc comment says this function must match BaseURL and not diverge from it, but nothing enforces that. Add a case to BaseURL without adding it to IsKnownEnv and that env gets silently labelled prod — and it now exports, so it's a quiet misattribution rather than the silence it used to be. A table assertion on api.BaseURL(telemetryEnv(cfg.CurrentEnv)) == api.BaseURL(sessionEnv(cfg)) would make the mirror self-enforcing.

@LukasWodka
LukasWodka merged commit 7f1fc28 into develop Aug 19, 2026
38 of 39 checks passed
@LukasWodka
LukasWodka deleted the fix/2149-telemetry-unknown-env branch August 19, 2026 09:14
@LukasWodka

Copy link
Copy Markdown
Contributor Author

/fr-pass

Best-effort FR passed (triage; behavioral evidence limited while e2e journey red — backend#2206). Advancing to Ready for prod.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants