fix(cli): resolve the backend env once per invocation, not once per reader (backend#2320)

[saadqbal]

Closes backend#2320. Fixes the unresolved Bugbot thread on the staging mirror (#540, "Telemetry env resolved twice") at the root rather than at the one site it named.

Why the whole family

Each of the last recuts produced one more finding about a site that answered "which backend am I talking to?" differently from its neighbour — cli#528 review (label via ResolveEnv, client via the config), #542 review (deliver computing its own spool path and draining another env's queue), now #540. The failure mode is additive: every new site looks locally correct, and only the second one is a bug. So this consolidates onto one resolution point per question instead of patching the named site.

Every environment / base-URL resolution site in cli

(*) = changed here. Precedence is left-to-right.

#	Site	Reads	Precedence	Unset	Unknown
1	api.ResolveEnv(explicit)	explicit arg, $CLIENT_ENV	explicit → $CLIENT_ENV → prod	prod	returns it lower-cased (caller validates)
2	api.BaseURL(env)	its arg only (pure)	—	prod	prod, silent — backend#2171, untouched
3	api.IsKnownEnv(env)	its arg only (pure)	—	false	false
4	cli.sessionEnv(cfg)(*)	cfg.current_env, then $CLIENT_ENV	current_env → $CLIENT_ENV → prod	prod	preserved (not coerced) — BaseURL decides
5	cli.signedInEnv()(*)	config file → sessionEnv	delegates to #4	prod (via #4)	preserved
6	cli.telemetryEnv(env)(*)	its arg; #4 when empty	delegates to #4	prod	prod (mirrors #2 deliberately)
7	runLogin (--env)	--env, $CLIENT_ENV	#1, then IsKnownEnv rejects	prod	refuses, exit 1
8	runAuthCheck (--env) (*)	--env, $CLIENT_ENV, config	#1 for target, #4 for session, compared	prod	exit 1 (no match)
8b	auth status display (*)	config via #4	delegates to #4	via #4	preserved
9	authedClient / logout	config via #4	delegates to #4	via #4	via #2
10	cluster doctor session probe (*)	config via #4	delegates to #4	via #4	via #2
11	pendingSink(env) / deliver	its arg only	—	—	— (single-resolution already, #542)
12	telemetrySpoolPath / spoolEnvSlug	its arg only	—	unknown bucket	unknown bucket
13	doctor.backendHost(*)	the cluster'sCLIENT_ENV off the jobs-manager Deployment	—	prod	prod
14	config.migrateV1	a v1 file's env	env → prod	prod	stored verbatim

Sites 13 and 7 answer genuinely different questions (the cluster's configured env; the env a human named), so they stay separate by design — noted in comments so the next reader does not "consolidate" them.

What was wrong, and what changed

#540 — the label and the sink came from two config reads.RecordCommandOutcome resolved the env for pendingSink, then recordCommandOutcome called telemetryEnv(signedInEnv())again for the emitter. Two reads that merely tend to agree, and disagree the moment a login lands between them — exactly the "labelled stg, posted to prod" leak the comment above RecordCommandOutcome claims to prevent. recordCommandOutcome now takes the resolved env, the same rule deliver/pendingSink already follow.

The "Dev" case is a latent live bug, not tidying — the strongest single justification here, and worth stating plainly: api.IsKnownEnv lower-cases, but the old signedInEnv did not. So telemetryEnv("Dev") fell through to the unknown arm and labelled the record prod while the client dialled dev. That is the exact label-versus-destination split this telemetry work has been closing for three recuts, still open on a config one keystroke away from normal.

sessionEnv returned cfg.CurrentEnv verbatim — the one env-resolving function in the CLI whose output was not normalised, while ResolveEnv, BaseURL and spoolEnvSlug all lower-case (and two of them trim). Invisible where the value only reaches BaseURL; load-bearing where it is compared, or where one consumer trims and another does not. api.BaseURL does not trim, so " dev " fell through to PROD.

Three callers re-derived the session env instead of using sessionEnv:

• cluster doctor built its API client from cfg.CurrentEnv raw → with current_env: " dev " it probed prod with a dev token and reported "your session expired" for a session that is fine. The worst possible output from a diagnostic.
• auth status --check compared raw cfg.CurrentEnv against an already-normalised target → a "current_env": "Dev" config failed the probe for the very session it is signed in to, and the installer (whose contract is this exit code) would re-run login against a working session.
• auth status printed cfg.CurrentEnv as its "backend" field while runAuthCheck — the machine-facing answer to the same question, 40 lines below in the same file — compared the resolved one. A status command that disagrees with the client is worse than no status command.

doctor.backendHost restated api.BaseURL's env→host table in a second switch. Now derived from api.BaseURL. Behaviour-identical — BaseURL already lower-cases, so TrimSpace is the only normalisation retained.

A guard test (TestNoNewEnvironmentResolutionSiteAppears) pins the closed set of files allowed to resolve an env from ambient state, with a reason per entry. It walks the module root, tokenises each file with go/scanner (comments dropped, literals preserved), and audits the allowlist per entry — so a sanctioned file that stops resolving anything fails instead of quietly becoming a licence. One needle set, one matcher (matchesAnyNeedle), one allowlist, each exercised from both directions. This is the part that stops the next recut finding one more site.

Explicitly NOT changed

api.BaseURL's unknown/empty → prod fail-open. It is shared with the installer's _backend_url and contradicted by client-runtime's controller.py (which refuses with SystemExit(1)), so it is a three-component decision tracked on backend#2171 — where I have added the evidence this audit produced. Site 6 (telemetryEnv) deliberately mirrors it and says so.

Proof — each fix reverted, real red

Every fix was reverted individually against the new tests:

REVERT telemetry (re-resolve for the emitter)
--- FAIL: TestTheRecordIsLabelledWithTheEnvItWasHanded
deployment.environment = "dev", want "stg" — the emitter must be labelled with
the env it was handed (the one the sink was built from), not with a second read
of the config
REVERT doctor (newAPIClient(cfg.CurrentEnv))
--- FAIL: TestClusterDoctorProbesTheSessionEnv
cluster doctor built its API client for " dev ", want "dev" — ... (api.BaseURL(" dev ")
is the PROD default, so this probes the wrong backend)
--- FAIL: TestNoNewEnvironmentResolutionSiteAppears
doctor.go resolves an environment from ambient state ("CurrentEnv"), but is not a
sanctioned resolution site.
REVERT sessionEnv (verbatim)
--- FAIL: TestSessionEnvNormalisesTheStoredEnv/Dev sessionEnv("Dev") = "Dev", want "dev"
--- FAIL: TestSessionEnvNormalisesTheStoredEnv/_dev_ sessionEnv(" dev ") = " dev ", want "dev"
--- FAIL: TestSessionEnvNormalisesTheStoredEnv/PROD sessionEnv("PROD") = "PROD", want "prod"
--- FAIL: TestClusterDoctorProbesTheSessionEnv
--- FAIL: TestAuthCheckComparesTheResolvedEnv got: exit 1
REVERT auth check (compare raw CurrentEnv)
--- FAIL: TestAuthCheckComparesTheResolvedEnv got: exit 1
REVERT auth status (print raw CurrentEnv)
--- FAIL: TestAuthStatusShowsTheEnvTheClientWillUse
auth status reported the stored env verbatim, not the resolved one.
want the "dev" the client actually dials, got:
tracebloc auth
status: signed in
backend: Dev
account: ds@co

Precedence is covered explicitly: TestSessionEnvFallsBackToClientEnvOnlyWhenUnset pins that config current_envbeats$CLIENT_ENV and that $CLIENT_ENV is consulted only when current_env is absent — the offboard e2e fixture (test/integration/delete_e2e_test.go) writes "current_env": "prod", so a change letting $CLIENT_ENV win would silently repoint that suite. TestAuthCheckStillRejectsARealEnvMismatch pins that normalising the comparison did not make it lenient about the mismatch it exists to catch.

One fix has no red available and I am not pretending otherwise:doctor.backendHost is behaviour-identical for every input (api.BaseURL already lower-cases), so reverting it changes nothing observable. It is a de-duplication — the existing backendHost test table, whitespace case included, passes unchanged either way.

What I ran

• make check — green (vet, full go test ./..., fmt-check, file-budget, check-style, tool-pins)
• make lint — clean (errcheck, ineffassign, misspell, staticcheck all,-ST1005)
• make deadcode — clean. It reports a pre-existing stale-allowlist advisory (internal/doctor/doctor.go: Status.String); I confirmed it against a stashed pristine c1e918f tree, so it is not from this change and is not fixed here.
• go vet -tags integration ./test/... — the integration tree still compiles against the changed signature.

Could not run: the k3d offboard e2e. It self-skips when a k3d cluster named tracebloc already exists (delete_e2e_test.go:114, "refusing to delete a cluster this test didn't create") and this machine has one. Relying on CI for it — this is a skip, not a pass.

One self-inflicted finding, recorded rather than quietly fixed

The first version of TestClusterDoctorProbesTheSessionEnv stubbed newAPIClient but notloadClusterFn. Past the session probe, cluster doctor loads the real kubeconfig and calls the real doctor.Run, whose checkBackendEgress probes backendHost("") — a live GET https://api.tracebloc.io/. On a dev machine with a real k3d cluster (this one has one) that test therefore made a production request, unauthenticated but real. The tell is the run time: ~16s before, 0.01s once loadClusterFn is stubbed.

Fixed in 86e06c7 and the reasoning is in the test as a comment, because the trap is not obvious from the call site — cluster doctor looks like it stops when the session probe fails, and it does not. The other seven new tests are pure, httptest-backed, or file-only; none can reach the network.

The guard took four rounds, and that is the story

Review found four independent holes in the guard — two string-literal evasions in my hand-rolled comment stripper (an https:// literal ate the rest of its line; a "/*.json" literal swallowed every needle below it), a scope hole (it walked 1 of the 17 packages under internal/, i.e. it was blind exactly where the next site is most likely to land), and — the sharpest — an inert allowlist entry: my own consolidation stopped telemetry.go matching any needle, so its entry checked nothing while silently pre-approving the next raw read in the one file whose double resolution this PR exists to remove.

They are all one defect class, and it is the class this PR is about: a check that verifies the FORM of a thing rather than the property the form exists to guarantee. The allowlist checked existence when the property is still matches a needle. Worth naming, because it recurs — a gate that enforces enumeration but cannot tell what it enumerated; a thread count that reports 0 on a PR the platform refuses to merge.

As Lukas put it: four holes in one guard is not an argument against the guard, it is the guard being the highest-leverage thing in the PR. The fixes are in f33a3d1 and a670403; the production consolidation underneath was reviewed and taken as-is.

The test-of-the-test — eight reproductions, all red

#	probe	result
1	control: plain new site in internal/cli	internal/cli/zz_probe.go resolves an environment from ambient state ("CurrentEnv")
2	evasion A: needle shares a line with // inside "https://x/%s"	same failure — caught
3	evasion B: "/*.json" literal above the needle	same failure — caught
4	sibling package internal/cluster (invisible before)	internal/cluster/zz_probe.go … ("CLIENT_ENV")
5	re-inert a sanctioned entry (telemetry.go)	… is allowlisted as a resolution site but resolves nothing any more — drop the entry
6	allowlist entry with an empty reason	resolutionSites["internal/cli/client.go"] is allowlisted with no reason
7	rename every needle	all five entries named, plus the guard checked nothing: 5 sanctioned entries, 0 files matched
8	lexical fault elsewhere in the module (unterminated /*)	walking the module: …/zz_broken.go: 1 scan error(s) — cannot tell what this file reads

Each was reverted and the guard is green with the tree clean.

One correction to my own claim, since it would otherwise read as stronger than it is: go/scanner is lexical, so the error path covers unterminated literals and unterminated /* — the faults that would desynchronise boundary tracking — notfunc f( {, which scans clean and is reported as ordinary code. I checked; it passes. That is the right guarantee rather than a weak one (a file that tokenises correctly still yields correct needles, and go build is the check for whether it compiles), but the comment now says that instead of implying full syntax validation.

Is that all of them?

Yes, and here is the falsifiable form of that claim. A repo-wide sweep of every non-test.go file with comments stripped (comments discuss these names constantly, so only code counts), for CurrentEnv, ResolveEnv(, and CLIENT_ENV, leaves exactly five files:

internal/api/client.go ResolveEnv / Getenv("CLIENT_ENV") -- the primitive itself
internal/cli/auth.go ResolveEnv(envFlag), CurrentEnv -- the --env flag; the login WRITE
internal/cli/client.go CurrentEnv, ResolveEnv -- sessionEnv, the one session point
internal/config/config.go CurrentEnv -- storage; profiles are keyed by the raw string
internal/doctor/doctor.go env["CLIENT_ENV"] -- the CLUSTER's env, a different question

internal/cli/telemetry.go has dropped off that list entirely — post-change it names none of them in code, because it goes through sessionEnv. Nothing outside Go derives a backend host either: grep for _backend_url, CLIENT_ENV, dev-api, stg-api, api.tracebloc.io across *.sh/*.ps1/*.yml/*.yaml/*.tmpl in this repo returns nothing (the cluster installer that owns _backend_url lives in another repo; internal/installer only holds the https://tracebloc.io/i.sh URL, which is env-independent by design).

So the remaining reads are one primitive, one flag site, one session site, one storage layer, and one genuinely different question — no duplicates, and the guard test fails if a sixth appears.

[LukasWodka]

Requesting changes on the guard test only — the production consolidation is right, and I checked it rather than taking the table on trust.

What I verified holds.sessionEnv is now the single config→session chain, and the three re-derivation sites really do route through it (internal/cli/doctor.go:118, auth.go:420, auth.go:467). signedInEnv's single production caller is RecordCommandOutcome, so changing it from "returns "" when not signed in" to "always resolved" breaks no consumer — I grepped every caller. The "Dev" case is a genuine latent fix, not just tidying: api.IsKnownEnv lower-cases but the oldsignedInEnv did not, so telemetryEnv("Dev") fell to the unknown arm and labelled the record prod while the client dialled dev — the exact mislabelling the comment above RecordCommandOutcome claims to prevent. backendHost deriving from api.BaseURL is sound (url.Parse over the closed set of https://… returns, host-only, prod on the unreachable error path), and the doctor.go sanctioning is doing real work: it has two raw needle hits, both in prose, so the comment-stripping is load-bearing for it passing at all.

What I'm holding on.TestNoNewEnvironmentResolutionSiteAppears is described as "the part that stops the next recut finding one more site," and it has three demonstrated fail-open holes — I armed the control first, then evaded it three ways against 4452b70:

probe	expected	actual
new site, needle on its own line	🔴	🔴 caught
needle after // inside a string literal ("https://x/%s")	🔴	🟢 missed
a /* inside a string literal ("/*.json") hides every needle below it	🔴	🟢 missed
new site in a sibling package (internal/cluster)	🔴	🟢 missed

Details and the fix in the two inline comments. Neither is hypothetical: seven non-test files in internal/cli already carry a https:// literal, and the guard covers 1 of the 17 packages under internal/ while the description claims the repo. go/scanner closes the first two in ~10 lines and makes an unparseable file a finding rather than a silent pass; walking the module root closes the third and lets the allowlist carry the reasons the PR body already writes out for internal/api, internal/config and internal/doctor.

The third inline comment is non-blocking — a now-false docstring on signedInEnv.

Everything else here I'd own. Re-request me when the guard's own coverage is pinned and I'll re-run the evasion table.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit eb299b8. Configure here.}

[LukasWodka]

All four findings are addressed and I've verified each one against f33a3d17 rather than re-stamping — details on the three threads, which I've closed. Not approving yet only because Test is still pending; I'll approve on green.

The evasion table, re-run on the new head:

probe	before	now
new site, needle on its own line	🔴 caught	🔴 caught
needle after // inside a "https://…" literal	🟢 missed	🔴 caught
/* inside a "/*.json" literal hiding every needle below	🟢 missed	🔴 caught
new site in a sibling package (internal/cluster)	🟢 missed	🔴 caught
an inert allowlist entry (Bugbot's)	🟢 missed	🔴 caught

Plus the check that matters more than any single probe: the allowlist is now the derived set, not a hand-maintained one that happens to agree. I tokenised every non-test .go in the module independently and counted needles in code only — five files have them, and they are exactly your five entries, while the three that grep hits in prose (internal/cli/doctor.go, telemetry.go, telemetry_installer_spool.go) are exactly the three correctly absent. So the guard, the allowlist and reality agree by construction rather than by coincidence.

Baseline green, go build ./... clean, go test ./internal/cli/ green — so broadening the needles to bare identifiers didn't import a backlog.

Two small things worth having on the record, neither a request:

The scanErrs > 0 fail-closed branch is unreachable in practice — an unterminated literal fails go build for the test binary first, in every package reachable from internal/cli. Your docstring is already careful about this (naming lexical faults and noting func f( { scans clean), so it's a correctly-scoped guarantee about the tokeniser rather than a check CI will ever exercise.

And the signedInEnv docstring went further than I asked: naming the trap outright — "Do NOT write if signedInEnv() == "" … it cannot fire" — is what actually stops the next reader, since the failure mode was trusting the old comment.

For the record on how this went: four independent holes in one guard, all in the same fail-open direction, and the production consolidation underneath never needed a change. That's the guard being the highest-leverage thing in the PR, which is why it drew the attention.

[LukasWodka]

Approving a670403c. Green gate clear: every check passes (quality / format skips — no shell or Python in this diff), mergeable, and zero unresolved threads including my own three, which I closed on the previous head after verifying each fix.

a670403c restructures the guard onto one needle set, one matcher and one allowlist, checked from both directions — the shape I sketched on the scope thread. I re-ran the whole table against it rather than assuming a restructure preserved the coverage:

probe	result
baseline	🟢 passes
needle after // inside a "https://…" literal	🔴 caught
new site in a sibling package (internal/cluster)	🔴 caught
an inert allowlist entry (Bugbot's)	🔴 "is allowlisted … but resolves nothing any more"
a needle rename (typo all three)	🔴 five per-entry failures + the anchor
an empty allowlist	🔴 five detections + "the guard checked nothing: 0 sanctioned entries, 5 files matched"

The needle-rename row is the one I most wanted to see, because the comment makes a specific claim — that the per-entry inert check subsumes the old global "needles went stale" counter, and does it more loudly. It does: five named entries to delete plus the anchor, instead of one counter reaching zero. Claim verified rather than accepted.

go build ./... clean, go test ./internal/cli/ green.

What the guard is now, stated plainly because it's the part worth reusing elsewhere in the org: one declaration of the needles, one matchesAnyNeedle called by both the detection sweep and the allowlist audit (so the two cannot drift), tokenisation by go/scanner so literals are literals, a module-wide walk, per-entry staleness that names the entry to delete, and an anchor for the genuinely-vacuous case. Every one of those exists because a specific hole was demonstrated, not because it sounded rigorous.

Thanks for taking four rounds on the guard without pushing back on any of them — and for the two things you added beyond the asks: the signedInEnv docstring naming the == "" trap outright, and the goCodeTokens comment scoping its guarantee to lexical faults instead of overclaiming that it catches malformed Go. The production consolidation underneath never needed a change; the guard drew the attention because it was the highest-leverage thing here.

[LukasWodka]

/fr-pass