From 0408e0b38bbc820f7e88671ef0655f4730970536 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 21 Aug 2026 18:43:46 +0500 Subject: [PATCH 1/6] fix(cli): resolve the backend env once per invocation, not once per reader (backend#2320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The env/base-URL resolution family, not the one site #540 named. Each of the last recuts produced one more finding about a site that answered "which backend?" differently from its neighbour (cli#528 review, #542 review, now #540) — the failure mode is additive, so only the SECOND site is ever a bug. - telemetry: recordCommandOutcome took the resolved env as a parameter instead of calling telemetryEnv(signedInEnv()) a second time. The label and the sink (spool path + POST destination) now derive from one value, which is what the comment above RecordCommandOutcome already claimed. This is #540's finding. - sessionEnv is now the single config -> session-env resolution point, and it normalises (trim + lower-case) like api.ResolveEnv. Returning cfg.CurrentEnv verbatim made it the one env-resolving function whose output was not normalised: invisible where the value only reaches api.BaseURL (which lower-cases again), 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. - `cluster doctor` built its API client from cfg.CurrentEnv raw and `auth status --check` compared it raw against an already-normalised target. Both go through sessionEnv now: a doctor probing prod with a dev token reports "session expired" for a session that is fine, and the installer, whose contract is --check's exit code, re-ran login against a working session. - internal/doctor.backendHost derives its host from api.BaseURL instead of restating the same three hosts in a second switch. Behaviour-identical (BaseURL already lower-cases); it removes the copy that drifts. - A guard test pins the closed set of sanctioned resolution sites, so the next one fails a check instead of a review. 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, so it is a three-component decision tracked on backend#2171. Co-Authored-By: Claude Opus 5 --- internal/cli/auth.go | 13 +- internal/cli/client.go | 24 +- internal/cli/doctor.go | 6 +- internal/cli/env_resolution_test.go | 330 ++++++++++++++++++++++++++++ internal/cli/telemetry.go | 51 +++-- internal/cli/telemetry_test.go | 16 +- internal/doctor/doctor.go | 26 ++- 7 files changed, 427 insertions(+), 39 deletions(-) create mode 100644 internal/cli/env_resolution_test.go diff --git a/internal/cli/auth.go b/internal/cli/auth.go index c4237ebe..bf975190 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -442,7 +442,7 @@ func newAuthStatusCmd() *cobra.Command { // (IsSilentError) so main() prints nothing. // // The target env is resolved exactly like `login` (--env, then $CLIENT_ENV, then -// prod), and must match the signed-in CurrentEnv — otherwise the probe would OK a +// prod), and must match the signed-in env as sessionEnv resolves it — otherwise the probe would OK a // stale session for the wrong backend and the installer would skip the very // `login` that switches env, provisioning into the wrong account (RFC-0001 §10). func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { @@ -454,10 +454,15 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { return &exitError{code: exitFailure} } target := api.ResolveEnv(envFlag) - if !cfg.SignedIn() || cfg.CurrentEnv != target { + // Compare the RESOLVED session env, not the raw cfg.CurrentEnv: target comes + // out of api.ResolveEnv already normalised, so comparing it against the stored + // string made this the one place a `"current_env": "Dev"` config failed a probe + // for the session it is actually signed in to. + signedIn := sessionEnv(cfg) + if !cfg.SignedIn() || signedIn != target { if p.Verbose() { - if cfg.SignedIn() && cfg.CurrentEnv != target { - p.Hintf("Signed in to %q, but this run targets %q — run `tracebloc login`.", cfg.CurrentEnv, target) + if cfg.SignedIn() && signedIn != target { + p.Hintf("Signed in to %q, but this run targets %q — run `tracebloc login`.", signedIn, target) } else { p.Hintf("Not signed in. Run `tracebloc login`.") } diff --git a/internal/cli/client.go b/internal/cli/client.go index d7246ee5..e5a3f830 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -143,12 +143,26 @@ func clientPrompter() prompter { } // sessionEnv resolves the backend env for the signed-in session: the env saved -// at login, falling back (legacy / empty config) to $CLIENT_ENV then prod. Shared -// by authedClient and logout so every authenticated call — including the revoke -// on sign-out — talks to the host the token was actually issued for. +// at login, falling back (legacy / empty config) to $CLIENT_ENV then prod. +// +// THE ONLY PLACE THAT DERIVES A SESSION ENV FROM A CONFIG. Every caller that +// wants "which backend is this signed-in session on?" — authedClient, logout's +// revoke, `cluster doctor`, `auth status --check`, the telemetry label — goes +// through here, so the answer cannot differ by caller. Reading cfg.CurrentEnv +// directly is the bug this function exists to prevent: it silently drops the +// $CLIENT_ENV fallback, and it skips the normalisation below. +// +// The result is normalised (trimmed, lower-cased) to match api.ResolveEnv, which +// lower-cases both its explicit argument and $CLIENT_ENV. Returning cfg.CurrentEnv +// verbatim made this the one env-resolving function in the CLI whose output was +// not normalised: harmless where the value only reaches api.BaseURL (which +// lower-cases again), but a false negative anywhere the value is COMPARED — a +// config carrying `"current_env": "Dev"` (migrateV1 stores a v1 `env` verbatim, +// and the file is hand-written in fixtures) failed `auth status --check --env dev` +// against a session that works perfectly. func sessionEnv(cfg *config.Config) string { - if cfg.CurrentEnv != "" { - return cfg.CurrentEnv + if e := strings.ToLower(strings.TrimSpace(cfg.CurrentEnv)); e != "" { + return e } return api.ResolveEnv("") } diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index cd3341a0..ef3edce7 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -111,7 +111,11 @@ func runClusterDoctor( // an error (5xx/403/decode) is a tracebloc-side problem, distinct from a // network failure to reach it at all. Conflating the two would blame the // user's network (and hand them a proxy remedy) for tracebloc's own error. - apiClient := newAPIClient(cfg.CurrentEnv) + // sessionEnv, not cfg.CurrentEnv: the session probe must target the same host + // authedClient would, or `doctor` reports on a backend no other command talks + // to. Reading CurrentEnv directly drops sessionEnv's $CLIENT_ENV fallback and + // its normalisation — a second resolution of the same question. + apiClient := newAPIClient(sessionEnv(cfg)) apiClient.Token = cfg.Current().Token if _, werr := apiClient.WhoAmI(ctx); werr != nil { var ae *api.APIError diff --git a/internal/cli/env_resolution_test.go b/internal/cli/env_resolution_test.go new file mode 100644 index 00000000..86e2ceda --- /dev/null +++ b/internal/cli/env_resolution_test.go @@ -0,0 +1,330 @@ +package cli + +// The environment / base-URL resolution family (backend#2171). +// +// The CLI answers "which backend am I talking to?" in several places, and each +// recut of the release train produced one more finding about a site that answered +// it differently from its neighbour: cli#528 (the telemetry label resolved through +// ResolveEnv while the client used the config), #542 (a spool path resolved twice), +// #540 (the label and the sink resolved twice). The pattern is always the same — +// a SECOND resolution of a question already answered — so these tests pin the +// invariants rather than the individual sites: +// +// 1. sessionEnv is the ONLY function that turns a config into a session env, and +// it normalises (trim + lower-case) like api.ResolveEnv does. +// 2. Callers take the resolved value; they never re-derive it. +// +// NOT covered here, deliberately: api.BaseURL's unknown/empty -> prod fail-open. +// That behaviour is shared with the installer's `_backend_url` and diverges from +// client-runtime's controller.py (which refuses), so changing it is a three- +// component decision tracked on backend#2171, not a CLI-local cleanup. + +import ( + "net/http" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/telemetry" +) + +// --- 1. sessionEnv is the single, normalising resolution point ---------------- + +// TestSessionEnvNormalisesTheStoredEnv: sessionEnv used to return cfg.CurrentEnv +// verbatim, which made it the one env-resolving function in the CLI whose output +// was not normalised — api.ResolveEnv lower-cases both its argument and +// $CLIENT_ENV, api.BaseURL lower-cases, spoolEnvSlug trims and lower-cases. +// +// Verbatim is invisible where the value only reaches api.BaseURL (which +// lower-cases again) and load-bearing everywhere else: a value that is COMPARED +// (`auth status --check`) or TRIMMED by one consumer and not another (BaseURL does +// not trim; " dev " therefore fell through to PROD). +func TestSessionEnvNormalisesTheStoredEnv(t *testing.T) { + for _, tc := range []struct { + stored string + want string + }{ + {"dev", "dev"}, + {"Dev", "dev"}, // migrateV1 stores a v1 `env` verbatim + {" dev ", "dev"}, // hand-written / fixture config + {"PROD", "prod"}, + {"banana", "banana"}, // unknown is preserved, not coerced — see BaseURL note above + } { + t.Run(tc.stored, func(t *testing.T) { + t.Setenv("CLIENT_ENV", "") + cfg := &config.Config{CurrentEnv: tc.stored} + if got := sessionEnv(cfg); got != tc.want { + t.Fatalf("sessionEnv(current_env=%q) = %q, want %q — sessionEnv must "+ + "normalise like api.ResolveEnv, or its callers disagree about the "+ + "same session", tc.stored, got, tc.want) + } + }) + } +} + +// TestSessionEnvFallsBackToClientEnvOnlyWhenUnset pins the precedence the task +// brief calls load-bearing: config `current_env` BEATS $CLIENT_ENV, and +// $CLIENT_ENV is consulted only when `current_env` is absent. The offboard e2e +// fixture writes `"current_env": "prod"` into its config, so a change that let +// $CLIENT_ENV win would silently repoint that suite. +func TestSessionEnvFallsBackToClientEnvOnlyWhenUnset(t *testing.T) { + t.Setenv("CLIENT_ENV", "dev") + + if got := sessionEnv(&config.Config{CurrentEnv: "prod"}); got != api.EnvProd { + t.Fatalf("sessionEnv(current_env=prod) with CLIENT_ENV=dev = %q, want %q — "+ + "the signed-in env must beat $CLIENT_ENV", got, api.EnvProd) + } + if got := sessionEnv(&config.Config{}); got != api.EnvDev { + t.Fatalf("sessionEnv(no current_env) with CLIENT_ENV=dev = %q, want %q — "+ + "$CLIENT_ENV is the legacy/empty-config fallback", got, api.EnvDev) + } + t.Setenv("CLIENT_ENV", "") + if got := sessionEnv(&config.Config{}); got != api.EnvProd { + t.Fatalf("sessionEnv(no current_env, no CLIENT_ENV) = %q, want %q", got, api.EnvProd) + } +} + +// --- 2. no caller re-derives the session env ---------------------------------- + +// TestClusterDoctorProbesTheSessionEnv: `cluster doctor` built its API client +// from cfg.CurrentEnv directly instead of sessionEnv — a second answer to the +// question authedClient already answers. The whitespace case makes the two +// answers differ for real: api.BaseURL lower-cases but does NOT trim, so +// newAPIClient(" dev ") lands on the prod default while every other +// authenticated command talks to dev-api. +// +// A doctor that probes prod with a dev token reports "session expired" for a +// session that is fine — the worst possible output from a diagnostic. +func TestClusterDoctorProbesTheSessionEnv(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + t.Setenv("CLIENT_ENV", "") + if err := (&config.Config{CurrentEnv: " dev ", Profiles: map[string]*config.Profile{ + " dev ": {Token: "tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + var gotEnv string + orig := newAPIClient + newAPIClient = func(env string) *api.Client { + gotEnv = env + // No BaseURL: the WhoAmI below must fail locally rather than reach any + // real host. What is under test is the env, not the probe's verdict. + return &api.Client{HTTP: &http.Client{Timeout: time.Millisecond}} + } + t.Cleanup(func() { newAPIClient = orig }) + + // doctor exits non-zero here (no cluster, failed probe); the assertion is on + // the env it built the client for, which is decided before any of that. + _, _ = runCmd(t, "cluster", "doctor") + + if gotEnv != api.EnvDev { + t.Fatalf("cluster doctor built its API client for %q, want %q — it must resolve "+ + "through sessionEnv like authedClient, not read cfg.CurrentEnv raw "+ + "(api.BaseURL(%q) is the PROD default, so this probes the wrong backend)", + gotEnv, api.EnvDev, gotEnv) + } +} + +// TestAuthCheckComparesTheResolvedEnv: `auth status --check` compared the raw +// cfg.CurrentEnv against api.ResolveEnv's already-normalised target, so a config +// carrying a non-normalised env failed the probe for the very session it is +// signed in to — and the installer, whose contract is this exit code, would then +// re-run `login` (or skip provisioning) against a session that works. +func TestAuthCheckComparesTheResolvedEnv(t *testing.T) { + probed := false + withTestBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/userinfo/" { + probed = true + _, _ = w.Write([]byte(`{"email":"ds@co","account":"Acme"}`)) + } + }) + // withTestBackend isolates the config dir; write a non-normalised env into it. + if err := (&config.Config{CurrentEnv: "Dev", Profiles: map[string]*config.Profile{ + "Dev": {Token: "tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + if _, err := runCmd(t, "auth", "status", "--check", "--env", "dev"); err != nil { + t.Fatalf("--check --env dev must accept a session stored as \"Dev\" "+ + "(sessionEnv resolves it to dev and the client talks to dev-api), got: %v", err) + } + if !probed { + t.Error("the backend was never probed — the env comparison rejected a session " + + "that differs from the target only in case") + } +} + +// TestAuthCheckStillRejectsARealEnvMismatch is the other half: normalising the +// comparison must not make it lenient about the thing it exists to catch. +func TestAuthCheckStillRejectsARealEnvMismatch(t *testing.T) { + probed := false + withTestBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/userinfo/" { + probed = true + } + }) + saveSignedIn(t, "tok") // CurrentEnv=dev + if _, err := runCmd(t, "auth", "status", "--check", "--env", "stg"); ExitCodeFromError(err) != 1 { + t.Fatalf("exit code = %d, want 1 — a dev session must not satisfy a stg target", + ExitCodeFromError(err)) + } + if probed { + t.Error("must not probe the backend on a genuine env mismatch") + } +} + +// TestTheRecordIsLabelledWithTheEnvItWasHanded is the #540 finding. +// +// RecordCommandOutcome resolves the env once and derives BOTH the record's label +// and the sink (spool path + POST destination) from that one value. +// recordCommandOutcome used to call telemetryEnv(signedInEnv()) again for the +// emitter, so the label came from a second, independent config read: two reads +// that merely tend to agree, and disagree the moment a `login` lands between them +// — which is precisely the "labelled stg, posted to prod" leak the comment above +// RecordCommandOutcome claims to prevent. +// +// The invariant is structural, because a race between two config reads is not a +// deterministic test: recordCommandOutcome must label the record with the env it +// was GIVEN. Handing it an env that disagrees with the config on disk is how we +// tell "used the parameter" from "read the config again". +func TestTheRecordIsLabelledWithTheEnvItWasHanded(t *testing.T) { + dir := t.TempDir() + t.Setenv("TRACEBLOC_CONFIG_DIR", dir) + t.Setenv("CLIENT_ENV", "") + // On disk: dev. A second resolution inside recordCommandOutcome would find + // this and label the record "dev". + body := `{"version":2,"current_env":"dev","profiles":{"dev":{"token":"x"}}}` + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + if got := signedInEnv(); got != api.EnvDev { + t.Fatalf("signedInEnv() = %q, want %q — the fixture no longer matches the "+ + "on-disk config layout, so this test would pass vacuously", got, api.EnvDev) + } + + var res map[string]string + sink := telemetry.Sink(func(r map[string]string, _ map[string]any) { res = r }) + + root := NewRootCmd(testBuildInfo()) + // Handed: stg — standing in for "the value the sink was built from", which is + // what the caller resolved before the config changed under it. + if err := recordCommandOutcome( + root, root, testBuildInfo(), 0, time.Second, + func(string) string { return "" }, api.EnvStg, sink, + ); err != nil { + t.Fatal(err) + } + if res == nil { + t.Fatal("nothing was delivered") + } + if got := res["deployment.environment"]; got != api.EnvStg { + t.Fatalf("deployment.environment = %q, want %q — 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", got, api.EnvStg) + } +} + +// --- 3. the guard: no NEW resolution site can appear unnoticed ---------------- + +// resolutionSites is the closed set of places in internal/cli allowed to answer +// "which backend?" from ambient state (the config file or $CLIENT_ENV). Everything +// else must take a resolved env as an argument. +// +// This is the rule the last three recuts each re-litigated one site at a time. It +// is here rather than in a reviewer's head because the failure mode is additive: +// every new site looks locally correct, and only the SECOND one is a bug. +var resolutionSites = map[string]string{ + // sessionEnv: config current_env, else $CLIENT_ENV, else prod. The one chain. + "client.go": "sessionEnv — the single config -> session-env resolution", + // The --env FLAG, a different question: the env the human/installer NAMED, + // which login persists and `auth status --check` validates against the session. + "auth.go": "api.ResolveEnv(envFlag) — the explicit --env flag, validated by IsKnownEnv", + // telemetryEnv/signedInEnv read the config, but both delegate the chain to + // sessionEnv; they only map the result onto a label. + "telemetry.go": "telemetryEnv/signedInEnv — delegate to sessionEnv, map to a label", +} + +func TestNoNewEnvironmentResolutionSiteAppears(t *testing.T) { + entries, err := os.ReadDir(".") + if err != nil { + t.Fatal(err) + } + // Reading ambient state to answer "which backend?": the config's current_env, + // or the $CLIENT_ENV chain. api.BaseURL/IsKnownEnv are pure mappings over an + // argument and are deliberately NOT listed — they resolve nothing. + needles := []string{"CurrentEnv", "ResolveEnv(", `Getenv("CLIENT_ENV")`} + + found := 0 + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + src, err := os.ReadFile(name) + if err != nil { + t.Fatal(err) + } + // Comments discuss these names constantly (they are the whole subject); + // only code counts. + code := stripGoComments(string(src)) + for _, n := range needles { + if !strings.Contains(code, n) { + continue + } + found++ + if why, ok := resolutionSites[name]; !ok { + t.Errorf("%s resolves an environment from ambient state (%q), but is not a "+ + "sanctioned resolution site.\n"+ + "Take the resolved env as an ARGUMENT instead — the CLI must answer "+ + "\"which backend?\" once per invocation and thread it. If this really is "+ + "a new resolution point, add it to resolutionSites with the reason and "+ + "say how it cannot disagree with sessionEnv.", name, n) + } else if why == "" { + t.Errorf("%s is allowlisted with no reason", name) + } + break + } + } + // A guard that matches nothing is not a guard: if the needles ever stop + // matching (a rename), fail loudly rather than pass vacuously. + if found == 0 { + t.Fatal("the guard matched no files at all — its needles are stale and it is " + + "no longer checking anything") + } + for name := range resolutionSites { + if _, err := os.Stat(name); err != nil { + t.Errorf("resolutionSites lists %s, which does not exist — stale allowlist", name) + } + } +} + +// stripGoComments removes // and /* */ comments so the guard reads code, not prose. +func stripGoComments(src string) string { + var b strings.Builder + for i := 0; i < len(src); { + switch { + case strings.HasPrefix(src[i:], "//"): + if n := strings.IndexByte(src[i:], '\n'); n >= 0 { + i += n + } else { + i = len(src) + } + case strings.HasPrefix(src[i:], "/*"): + if n := strings.Index(src[i+2:], "*/"); n >= 0 { + i += n + 4 + } else { + i = len(src) + } + default: + b.WriteByte(src[i]) + i++ + } + } + return b.String() +} diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go index 421b4cbd..ff9af68a 100644 --- a/internal/cli/telemetry.go +++ b/internal/cli/telemetry.go @@ -96,8 +96,9 @@ func commandPathOf(c *cobra.Command) string { // 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 +// unknown value to https://api.tracebloc.io (sessionEnv normalises cfg.CurrentEnv +// but does not validate it, so an unrecognised value reaches api.New intact). 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. // @@ -108,13 +109,17 @@ func commandPathOf(c *cobra.Command) string { // // 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. +// but in internal/api/client.go, not here. It is shared with the installer's +// `_backend_url` and contradicted by client-runtime's controller.py (which +// refuses), so it is a three-component decision tracked on backend#2171. 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("") + // Not signed in: $CLIENT_ENV, then the prod default — resolved BY sessionEnv + // over an empty config rather than by a second copy of its fallback, so the + // precedence chain lives in exactly one function. + resolved = sessionEnv(&config.Config{}) } if api.IsKnownEnv(resolved) { return strings.ToLower(resolved) @@ -126,12 +131,17 @@ func telemetryEnv(env string) string { // signedInEnv reads the environment the config points at, best-effort. A // missing or unreadable config is simply "not signed in". +// +// Delegates to sessionEnv — the same function authedClient and logout resolve +// through — so the record's label is derived from the session env by the same +// code that picks the host the CLI talks to, not by a parallel restatement of +// the rule that can drift from it. func signedInEnv() string { cfg, err := config.Load() if err != nil || cfg == nil { - return "" + cfg = &config.Config{} // unreadable config == not signed in } - return cfg.CurrentEnv + return sessionEnv(cfg) } // processInstanceID is the per-PROCESS id §2 asks for off-cluster. @@ -158,27 +168,38 @@ func processInstanceID() string { // was unhappy would be a strictly worse CLI. A malformed event is caught by the // tests below, where it is free. func RecordCommandOutcome(root, executed *cobra.Command, info BuildInfo, exitCode int, elapsed time.Duration) { - // The sink is built from the SAME resolved env the emitter is labelled with, - // not from a second read of the config. Two independent resolutions here is - // how a record ends up labelled `stg` and posted to prod. + // THE ONE RESOLUTION POINT for this invocation. The label, the spool and the + // POST destination are all derived from this single value: two independent + // resolutions is how a record ends up labelled `stg` and posted to prod. + // + // It is resolved here and threaded down as a parameter rather than re-read + // inside recordCommandOutcome, because "both call telemetryEnv(signedInEnv())" + // is not one resolution — it is two config reads that merely tend to agree, + // and a `login` landing between them makes them disagree (Bugbot on #540). env := telemetryEnv(signedInEnv()) - _ = recordCommandOutcome(root, executed, info, exitCode, elapsed, os.Getenv, pendingSink(env)) + _ = recordCommandOutcome(root, executed, info, exitCode, elapsed, os.Getenv, env, pendingSink(env)) } -// recordCommandOutcome is RecordCommandOutcome with its two ambient -// dependencies passed in, so the tests drive the real thing. +// recordCommandOutcome is RecordCommandOutcome with its ambient dependencies +// passed in, so the tests drive the real thing. +// +// TAKES THE RESOLVED env, and never resolves one itself — the same rule, and for +// the same reason, as deliver/pendingSink in telemetry_transport.go: the emitter's +// label must be the value the sink was built from, not a second look at the +// config that happens to land on it. func recordCommandOutcome( root, executed *cobra.Command, info BuildInfo, exitCode int, elapsed time.Duration, getenv func(string) string, + env string, sink telemetry.Sink, ) error { if !telemetryEnabled(getenv) { return nil } - emitter := telemetry.New(telemetryEnv(signedInEnv()), info.Version, processInstanceID()) + emitter := telemetry.New(env, info.Version, processInstanceID()) if sink != nil { emitter.SetSink(sink) } diff --git a/internal/cli/telemetry_test.go b/internal/cli/telemetry_test.go index 61de0406..05056775 100644 --- a/internal/cli/telemetry_test.go +++ b/internal/cli/telemetry_test.go @@ -49,8 +49,13 @@ func captureOutcome( delivered++ }) getenv := func(k string) string { return env[k] } + // Resolve the env the way RecordCommandOutcome does and hand the SAME value + // to the recorder, so these tests exercise the production path end to end + // (config -> label) now that recordCommandOutcome no longer resolves for + // itself. TestTheRecordIsLabelledWithTheEnvItWasHanded pins the threading. if err := recordCommandOutcome( - root, executed, testBuildInfo(), exitCode, 1500*time.Millisecond, getenv, sink, + root, executed, testBuildInfo(), exitCode, 1500*time.Millisecond, getenv, + telemetryEnv(signedInEnv()), sink, ); err != nil { t.Fatalf("recordCommandOutcome: %v", err) } @@ -298,8 +303,8 @@ func TestTheEnvironmentLabelMatchesTheBackend(t *testing.T) { 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) + // env via sessionEnv, which normalises cfg.CurrentEnv but never falls back to + // $CLIENT_ENV while it is set — 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. @@ -356,8 +361,9 @@ 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 + // (sessionEnv normalises but does not validate cfg.CurrentEnv, so the unknown + // value reaches api.New and api.BaseURL routes it 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) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 83ffa661..3c31b0ea 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -29,6 +29,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" + "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/resources" ) @@ -506,18 +507,25 @@ func checkBackendEgress(ctx context.Context, env map[string]string, probe func(c // backendHost maps CLIENT_ENV to the backend API host, mirroring the edge // runtime's own mapping (controller.py). Unset/unknown defaults to prod, the // chart's CLIENT_ENV default. +// +// DERIVED FROM api.BaseURL, not restated. The env→host mapping used to be a +// second copy of BaseURL's switch living in this package, which is how the two +// drift: the same three hosts written down twice, with nothing that fails when +// only one of them is edited. api.BaseURL already lower-cases, so TrimSpace is +// the only normalisation this adds — a CLIENT_ENV read off a container spec can +// carry surrounding whitespace that a --env flag cannot. +// +// The input is the CLUSTER's CLIENT_ENV (read off the jobs-manager Deployment), +// not this CLI's session env — a deliberately different question, which is why +// this takes a string rather than calling into the session resolution. func backendHost(clientEnv string) string { - // Normalize like the API client (api.ResolveEnv/BaseURL lower-case), so a - // non-lowercase CLIENT_ENV on the edge box doesn't fall through to prod and - // make the doctor probe the wrong backend. - switch strings.ToLower(strings.TrimSpace(clientEnv)) { - case "dev": - return "dev-api.tracebloc.io" - case "stg": - return "stg-api.tracebloc.io" - default: + u, err := url.Parse(api.BaseURL(strings.TrimSpace(clientEnv))) + if err != nil || u.Host == "" { + // Unreachable for BaseURL's closed set of return values; the prod default + // keeps this total rather than returning an empty host into a probe URL. return "api.tracebloc.io" } + return u.Host } // checkRequestsProxy verifies the requests-proxy deployment is present and From 3414a79a13d8dab44c959fab33943827d5bb70a2 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 21 Aug 2026 18:48:41 +0500 Subject: [PATCH 2/6] docs(telemetry): reflow the BaseURL-mirroring note left ragged by the previous edit Comment-only; no behaviour change. Co-Authored-By: Claude Opus 5 --- internal/cli/telemetry.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go index ff9af68a..7fd799ec 100644 --- a/internal/cli/telemetry.go +++ b/internal/cli/telemetry.go @@ -98,9 +98,9 @@ func commandPathOf(c *cobra.Command) string { // present-but-unrecognised value is prod, because api.BaseURL routes every // unknown value to https://api.tracebloc.io (sessionEnv normalises cfg.CurrentEnv // but does not validate it, so an unrecognised value reaches api.New intact). 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. +// 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 From 4452b7066674b921e73a0918c7b78ad835411041 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 21 Aug 2026 18:52:48 +0500 Subject: [PATCH 3/6] fix(auth): `auth status` must report the env the client dials, not the stored string (backend#2320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit's last site: `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 — compares the resolved one. A status command that disagrees with the client is worse than no status command. Co-Authored-By: Claude Opus 5 --- internal/cli/auth.go | 12 +++++++++--- internal/cli/env_resolution_test.go | 22 ++++++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index bf975190..37b08093 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -411,7 +411,13 @@ func newAuthStatusCmd() *cobra.Command { prof := cfg.Current() p.Section("tracebloc auth") p.Field("status", "signed in") - p.Field("backend", cfg.CurrentEnv) + // sessionEnv, not the raw stored string: this line is the human-facing + // answer to "which backend am I on?", and it must be the same answer + // --check computes and the same one authedClient dials. Printing the + // stored value let `auth status` say `Dev` while every request went to + // dev — a status command that disagrees with the client is worse than + // no status command. + p.Field("backend", sessionEnv(cfg)) if prof.Email != "" { p.Field("account", prof.Email) } @@ -469,8 +475,8 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { } return &exitError{code: exitFailure} } - // Signed in AND CurrentEnv == target: probe it. authedClient() builds the client - // for sessionEnv (== CurrentEnv == target) with the stored token — reuse it and + // Signed in AND the resolved session env == target: probe it. authedClient() + // builds the client for sessionEnv (== the value just compared) with the stored token — reuse it and // discard its message (the exit code is the contract here). client, _, err := authedClient() if err != nil { diff --git a/internal/cli/env_resolution_test.go b/internal/cli/env_resolution_test.go index 86e2ceda..2568c8c0 100644 --- a/internal/cli/env_resolution_test.go +++ b/internal/cli/env_resolution_test.go @@ -179,6 +179,28 @@ func TestAuthCheckStillRejectsARealEnvMismatch(t *testing.T) { } } +// TestAuthStatusShowsTheEnvTheClientWillUse: `auth status` printed the raw +// stored env as its "backend" field, so the human-facing answer to "which +// backend am I on?" could differ from the one --check computes and the one +// authedClient dials — two answers to the same question, in the same file. +func TestAuthStatusShowsTheEnvTheClientWillUse(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + t.Setenv("CLIENT_ENV", "") + if err := (&config.Config{CurrentEnv: " Dev ", Profiles: map[string]*config.Profile{ + " Dev ": {Token: "tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + out, err := runCmd(t, "auth", "status") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(out, api.EnvDev) || strings.Contains(out, " Dev ") { + t.Fatalf("auth status reported the stored env verbatim, not the resolved one.\n"+ + "want the %q the client actually dials, got:\n%s", api.EnvDev, out) + } +} + // TestTheRecordIsLabelledWithTheEnvItWasHanded is the #540 finding. // // RecordCommandOutcome resolves the env once and derives BOTH the record's label From 86e06c795211da4e0a54a6cf39f22374810cbb3b Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 21 Aug 2026 18:59:50 +0500 Subject: [PATCH 4/6] test(cli): keep the doctor env test off the network (backend#2320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My own bug, and worth the comment it now carries. Past the session probe, `cluster doctor` loads the real kubeconfig and calls the real doctor.Run, whose checkBackendEgress probes backendHost("") — a live GET to https://api.tracebloc.io/. On a developer machine with a real k3d cluster the test therefore made a production request; the run time (~16s vs 0.01s stubbed) is the tell. Stubbing loadClusterFn returns right after the session probe, which is all this test needs: the env is decided before it. Co-Authored-By: Claude Opus 5 --- internal/cli/env_resolution_test.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/cli/env_resolution_test.go b/internal/cli/env_resolution_test.go index 2568c8c0..8a127912 100644 --- a/internal/cli/env_resolution_test.go +++ b/internal/cli/env_resolution_test.go @@ -20,6 +20,7 @@ package cli // component decision tracked on backend#2171, not a CLI-local cleanup. import ( + "errors" "net/http" "os" "path/filepath" @@ -28,6 +29,7 @@ import ( "time" "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" "github.com/tracebloc/cli/internal/telemetry" ) @@ -118,8 +120,21 @@ func TestClusterDoctorProbesTheSessionEnv(t *testing.T) { } t.Cleanup(func() { newAPIClient = orig }) - // doctor exits non-zero here (no cluster, failed probe); the assertion is on - // the env it built the client for, which is decided before any of that. + // HERMETIC BY CONSTRUCTION, and this matters more than it looks. Past the + // session probe, `cluster doctor` loads the real kubeconfig and calls the real + // doctor.Run, whose checkBackendEgress probes backendHost("") — i.e. it issues + // a live GET to https://api.tracebloc.io/. On a developer machine with a real + // k3d cluster that is a genuine production request from a unit test. Failing + // loadClusterFn returns right after the session probe, which is everything + // this test needs: the env is decided before it. + origLoad := loadClusterFn + loadClusterFn = func(cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + return nil, errors.New("no cluster (stubbed: keeps this test off the network)") + } + t.Cleanup(func() { loadClusterFn = origLoad }) + + // doctor exits non-zero here (stubbed no-cluster); the assertion is on the env + // it built the client for, which is decided before that. _, _ = runCmd(t, "cluster", "doctor") if gotEnv != api.EnvDev { From f33a3d17ad8cc4a7736f63e8cb383e0b15903d47 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 21 Aug 2026 19:12:25 +0500 Subject: [PATCH 5/6] test(cli): make the resolution guard actually guard (backend#2320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings, all the same class — a check that verifies the FORM of a thing rather than the property the form exists to guarantee. Which is the class this PR is about, so the guard having it was the worst possible place for it. - Scan Go as Go. The hand-rolled comment stripper was fail-open on string literals: the `//` inside an `https://…` literal started a "comment" that ate the rest of the line, needle included, and a `/*` inside a literal like `"/*.json"` swallowed every needle below it to the next `*/` or EOF. Seven non-test files in internal/cli already carry an https:// literal, so this was one future line away. go/scanner with mode 0 drops comments and knows literals, so neither evasion exists. Literals are kept in the output — a needle inside a string is then a loud false positive, which is the cheap direction. - Walk the module root, not the test's own package dir. The guard covered 1 of the 17 packages under internal/, i.e. it was blind exactly where the next site is most likely to land: a new package written by someone who never reads internal/cli. Keys are now repo-relative, and internal/api, internal/config and internal/doctor join the allowlist with the reasons the PR body already gave. - An inert allowlist entry now FAILS. Checking only that a sanctioned file exists let my own change turn the telemetry.go entry into a licence: it matched no needle any more, so it checked nothing while silently pre-approving the next raw read in the very file whose double resolution this PR removes. The entry is gone and the staleness assertion stops the next one going inert unnoticed. - signedInEnv's docstring was false: it can no longer return "". Says so now, including that `if signedInEnv() == ""` cannot fire — and telemetryEnv's empty arm is marked production-unreachable-but-test-reachable rather than left to be traced. Also corrects an overclaim I made in the first draft of goCodeTokens' comment: go/scanner is lexical, so the error covers unterminated literals and comments — the faults that would desynchronise boundary tracking — not `func f( {`, which scans clean. Stated precisely rather than left flattering. Co-Authored-By: Claude Opus 5 --- internal/cli/env_resolution_test.go | 224 +++++++++++++++++++--------- internal/cli/telemetry.go | 26 +++- 2 files changed, 177 insertions(+), 73 deletions(-) diff --git a/internal/cli/env_resolution_test.go b/internal/cli/env_resolution_test.go index 8a127912..2758ebbc 100644 --- a/internal/cli/env_resolution_test.go +++ b/internal/cli/env_resolution_test.go @@ -21,6 +21,10 @@ package cli import ( "errors" + "fmt" + "go/scanner" + "go/token" + "io/fs" "net/http" "os" "path/filepath" @@ -269,99 +273,185 @@ func TestTheRecordIsLabelledWithTheEnvItWasHanded(t *testing.T) { // --- 3. the guard: no NEW resolution site can appear unnoticed ---------------- -// resolutionSites is the closed set of places in internal/cli allowed to answer -// "which backend?" from ambient state (the config file or $CLIENT_ENV). Everything -// else must take a resolved env as an argument. +// resolutionSites is the closed set of files in this MODULE allowed to answer +// "which backend?" from ambient state (the config file or $CLIENT_ENV). +// Everything else must take a resolved env as an argument. // // This is the rule the last three recuts each re-litigated one site at a time. It // is here rather than in a reviewer's head because the failure mode is additive: // every new site looks locally correct, and only the SECOND one is a bug. +// +// Keys are repo-relative because the walk is repo-wide. It used to walk only +// internal/cli, which left the guard blind in 16 of the 17 packages under +// internal/ — i.e. blind exactly where a new site is most likely to land, in a +// package written by someone who never reads internal/cli (Lukas on #551). var resolutionSites = map[string]string{ - // sessionEnv: config current_env, else $CLIENT_ENV, else prod. The one chain. - "client.go": "sessionEnv — the single config -> session-env resolution", + // The primitive: ResolveEnv is the --env/$CLIENT_ENV/prod chain, and the only + // os.Getenv("CLIENT_ENV") in the module. + "internal/api/client.go": "api.ResolveEnv — the primitive chain, and the only $CLIENT_ENV read", // The --env FLAG, a different question: the env the human/installer NAMED, - // which login persists and `auth status --check` validates against the session. - "auth.go": "api.ResolveEnv(envFlag) — the explicit --env flag, validated by IsKnownEnv", - // telemetryEnv/signedInEnv read the config, but both delegate the chain to - // sessionEnv; they only map the result onto a label. - "telemetry.go": "telemetryEnv/signedInEnv — delegate to sessionEnv, map to a label", + // which login persists (the one cfg.CurrentEnv WRITE) and `auth status --check` + // validates against the session. + "internal/cli/auth.go": "api.ResolveEnv(envFlag) — the explicit --env flag, validated by IsKnownEnv", + // sessionEnv: config current_env, else $CLIENT_ENV, else prod. The one chain + // every session-env consumer in internal/cli resolves through. + "internal/cli/client.go": "sessionEnv — the single config -> session-env resolution", + // Storage. Profiles are keyed by the RAW stored string, so this layer must not + // normalise; it hands the raw value out and sessionEnv normalises it. + "internal/config/config.go": "the on-disk current_env field, its accessors, and the v1 migration", + // The CLUSTER's CLIENT_ENV, read off the jobs-manager Deployment — a + // deliberately different question from this CLI's session env. + "internal/doctor/doctor.go": "the cluster's own CLIENT_ENV, for the egress probe's target host", + // internal/cli/telemetry.go is deliberately ABSENT, and the staleness check + // below is what keeps it that way: telemetryEnv/signedInEnv delegate the whole + // chain to sessionEnv and name no needle, so an entry for it would be inert — + // a licence to re-admit a raw read in the very file whose double resolution + // this PR exists to remove (Bugbot on #551). } +// envNeedles are the ways a file reads ambient state to answer "which backend?". +// api.BaseURL/IsKnownEnv are pure mappings over an argument and are deliberately +// absent — they resolve nothing. +// +// Deliberately BROAD (bare identifiers, and CLIENT_ENV unquoted so it matches +// help text too): a false positive is a loud line in a diff, a false negative is +// the bug this guard exists to catch. Fail closed. +var envNeedles = []string{"CurrentEnv", "ResolveEnv", "CLIENT_ENV"} + func TestNoNewEnvironmentResolutionSiteAppears(t *testing.T) { - entries, err := os.ReadDir(".") - if err != nil { - t.Fatal(err) - } - // Reading ambient state to answer "which backend?": the config's current_env, - // or the $CLIENT_ENV chain. api.BaseURL/IsKnownEnv are pure mappings over an - // argument and are deliberately NOT listed — they resolve nothing. - needles := []string{"CurrentEnv", "ResolveEnv(", `Getenv("CLIENT_ENV")`} + const root = "../.." // this test's package dir -> the module root - found := 0 - for _, e := range entries { - name := e.Name() - if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { - continue - } - src, err := os.ReadFile(name) + matched := map[string]string{} // repo-relative path -> the needle that hit + walkErr := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { if err != nil { - t.Fatal(err) + return err } - // Comments discuss these names constantly (they are the whole subject); - // only code counts. - code := stripGoComments(string(src)) - for _, n := range needles { - if !strings.Contains(code, n) { - continue + if d.IsDir() { + switch d.Name() { + case ".git", ".claude", ".worktrees", "vendor", "node_modules", "testdata": + return fs.SkipDir } - found++ - if why, ok := resolutionSites[name]; !ok { - t.Errorf("%s resolves an environment from ambient state (%q), but is not a "+ - "sanctioned resolution site.\n"+ - "Take the resolved env as an ARGUMENT instead — the CLI must answer "+ - "\"which backend?\" once per invocation and thread it. If this really is "+ - "a new resolution point, add it to resolutionSites with the reason and "+ - "say how it cannot disagree with sessionEnv.", name, n) - } else if why == "" { - t.Errorf("%s is allowlisted with no reason", name) + return nil + } + name := d.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + return nil + } + src, err := os.ReadFile(path) + if err != nil { + return err + } + code, err := goCodeTokens(string(src)) + if err != nil { + // "cannot tell" is not "clean": abort rather than read a file whose + // tokenisation failed as a string that matches nothing. + return fmt.Errorf("%s: %w", path, err) + } + rel, err := filepath.Rel(root, path) + if err != nil { + return err + } + rel = filepath.ToSlash(rel) + for _, needle := range envNeedles { + if strings.Contains(code, needle) { + matched[rel] = needle + break } - break } + return nil + }) + if walkErr != nil { + t.Fatalf("walking the module: %v", walkErr) } + // A guard that matches nothing is not a guard: if the needles ever stop // matching (a rename), fail loudly rather than pass vacuously. - if found == 0 { + if len(matched) == 0 { t.Fatal("the guard matched no files at all — its needles are stale and it is " + "no longer checking anything") } - for name := range resolutionSites { - if _, err := os.Stat(name); err != nil { - t.Errorf("resolutionSites lists %s, which does not exist — stale allowlist", name) + + for rel, needle := range matched { + why, ok := resolutionSites[rel] + switch { + case !ok: + t.Errorf("%s resolves an environment from ambient state (%q), but is not a "+ + "sanctioned resolution site.\n"+ + "Take the resolved env as an ARGUMENT instead — the CLI must answer "+ + "\"which backend?\" once per invocation and thread it. If this really is "+ + "a new resolution point, add it to resolutionSites with the reason and "+ + "say how it cannot disagree with sessionEnv.", rel, needle) + case why == "": + t.Errorf("resolutionSites[%q] is allowlisted with no reason", rel) + } + } + + // THE ALLOWLIST MUST STAY A RECORD, NOT BECOME A LICENCE. Checking only that a + // sanctioned file EXISTS is the defect class this whole PR is about: verifying + // the form of a thing instead of the property the form exists to guarantee. An + // entry whose file no longer resolves anything is inert — it checks nothing, + // while silently pre-approving the next raw read in that file. + for rel := range resolutionSites { + if _, err := os.Stat(filepath.Join(root, rel)); err != nil { + t.Errorf("resolutionSites lists %s, which does not exist — stale allowlist", rel) + continue + } + if _, ok := matched[rel]; !ok { + t.Errorf("stale allowlist entry: %s is sanctioned but no longer resolves an "+ + "environment from ambient state — remove it from resolutionSites. An inert "+ + "entry is a licence, not a record: it would silently re-admit a raw read "+ + "in that file.", rel) } } } -// stripGoComments removes // and /* */ comments so the guard reads code, not prose. -func stripGoComments(src string) string { +// goCodeTokens renders src as its Go TOKENS, with comments dropped — so the +// guard reads code, never prose (these names are the subject of half the +// comments in the package). +// +// SCANS GO AS GO, because the hand-rolled comment stripper this replaces was +// fail-open on string literals, and Lukas demonstrated both holes on #551: +// the "//" inside a "https://…" literal started a comment that ate the rest of +// the line (needle included), and a "/*" inside a literal like "/*.json" +// swallowed every needle below it to the next "*/" or EOF. Seven non-test files +// in internal/cli already carry an https:// literal, so that was one future line +// away, not a contrived shape. +// +// Literals are KEPT in the output rather than dropped: a needle inside a string +// then reads as a loud false positive, which is cheap — the opposite direction +// fails silently, which is the bug. +// +// SCOPE OF THE ERROR, stated precisely because the first version of this comment +// overclaimed it: go/scanner is LEXICAL, not syntactic, so this returns an error +// only on lexical faults — an unterminated string literal or an unterminated +// /* comment. `func f( {` scans clean and is reported as normal code. That is the +// right guarantee rather than a weak one: the faults it does catch are exactly +// the ones that would desynchronise literal/comment boundaries and hand the +// needles a misread file, which is the failure this function exists to prevent. +// A file that tokenises correctly but does not compile still yields correct +// needles, and `go build` is the check for whether it compiles. +func goCodeTokens(src string) (string, error) { + var fset token.FileSet + var sc scanner.Scanner + scanErrs := 0 + f := fset.AddFile("", fset.Base(), len(src)) + // mode 0: comment tokens are not emitted at all. + sc.Init(f, []byte(src), func(token.Position, string) { scanErrs++ }, 0) var b strings.Builder - for i := 0; i < len(src); { - switch { - case strings.HasPrefix(src[i:], "//"): - if n := strings.IndexByte(src[i:], '\n'); n >= 0 { - i += n - } else { - i = len(src) - } - case strings.HasPrefix(src[i:], "/*"): - if n := strings.Index(src[i+2:], "*/"); n >= 0 { - i += n + 4 - } else { - i = len(src) - } - default: - b.WriteByte(src[i]) - i++ + for { + _, tok, lit := sc.Scan() + if tok == token.EOF { + break } + if lit != "" { + b.WriteString(lit) + } else { + b.WriteString(tok.String()) + } + b.WriteByte(' ') + } + if scanErrs > 0 { + return "", fmt.Errorf("%d scan error(s) — cannot tell what this file reads", scanErrs) } - return b.String() + return b.String(), nil } diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go index 7fd799ec..f76f7566 100644 --- a/internal/cli/telemetry.go +++ b/internal/cli/telemetry.go @@ -116,9 +116,15 @@ func commandPathOf(c *cobra.Command) string { func telemetryEnv(env string) string { resolved := env if resolved == "" { - // Not signed in: $CLIENT_ENV, then the prod default — resolved BY sessionEnv - // over an empty config rather than by a second copy of its fallback, so the - // precedence chain lives in exactly one function. + // No stored session: $CLIENT_ENV, then the prod default — resolved BY + // sessionEnv over an empty config rather than by a second copy of its + // fallback, so the precedence chain lives in exactly one function. + // + // NOT REACHABLE FROM PRODUCTION any more: the only production caller passes + // signedInEnv(), which since it delegates to sessionEnv never returns "". + // Kept, and not dead, because telemetryEnv is a pure mapping that the tests + // call directly with "" — and because a mapping that panics or mislabels on + // an empty input would be a worse contract than one that resolves it. resolved = sessionEnv(&config.Config{}) } if api.IsKnownEnv(resolved) { @@ -129,17 +135,25 @@ func telemetryEnv(env string) string { return api.EnvProd } -// signedInEnv reads the environment the config points at, best-effort. A -// missing or unreadable config is simply "not signed in". +// signedInEnv resolves the environment the config points at, best-effort. // // Delegates to sessionEnv — the same function authedClient and logout resolve // through — so the record's label is derived from the session env by the same // code that picks the host the CLI talks to, not by a parallel restatement of // the rule that can drift from it. +// +// ALWAYS RETURNS A RESOLVED ENV, never "". It used to return "" for a missing or +// unreadable config, and the old comment called that "not signed in"; delegating +// to sessionEnv means that case now resolves through $CLIENT_ENV to prod like any +// other empty config. So "not signed in" no longer names an output — it means +// "resolved from $CLIENT_ENV/prod rather than from a stored session", and the two +// are indistinguishable here by design (the label is about the host, not the +// session). Do NOT write `if signedInEnv() == ""` on the strength of a stale +// reading of this: it cannot fire. func signedInEnv() string { cfg, err := config.Load() if err != nil || cfg == nil { - cfg = &config.Config{} // unreadable config == not signed in + cfg = &config.Config{} // unreadable == no stored session } return sessionEnv(cfg) } From a670403cf11d1e4fb48b5c5ec745bc4f231dc2fb Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 21 Aug 2026 19:17:43 +0500 Subject: [PATCH 6/6] =?UTF-8?q?test(cli):=20one=20needle=20set,=20one=20ma?= =?UTF-8?q?tcher,=20one=20allowlist=20=E2=80=94=20each=20checked=20both=20?= =?UTF-8?q?ways=20(backend#2320)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopts Lukas's unified shape for the guard, which is better than what I pushed in f33a3d1 in two concrete ways: - `matchesAnyNeedle` is now THE matcher, called from the detection sweep AND the allowlist audit. Two copies of "does this file resolve an env?" is the same shape as the two copies of "which env?" this PR removes, and it would let detection and allowlisting drift apart exactly where nobody looks. - The allowlist audit is per ENTRY, not per suite, so staleness, the empty-reason check and the needles-went-stale backstop all fall out of one loop and the failure names the entry to delete. Renaming a needle now reports all five entries by name instead of a global counter hitting zero. Kept a narrow anchor the per-entry loop genuinely cannot see: an EMPTY allowlist makes that loop vacuous, so a needle rename plus an empty allowlist would pass in silence. It asserts both counts are non-zero. Eight reproductions, all red, all restored green — the control, both string-literal evasions, the sibling-package site, a re-inerted sanctioned entry, an empty reason, a needle rename, and a lexical fault elsewhere in the module. Co-Authored-By: Claude Opus 5 --- internal/cli/env_resolution_test.go | 91 ++++++++++++++++++----------- 1 file changed, 58 insertions(+), 33 deletions(-) diff --git a/internal/cli/env_resolution_test.go b/internal/cli/env_resolution_test.go index 2758ebbc..af879c7f 100644 --- a/internal/cli/env_resolution_test.go +++ b/internal/cli/env_resolution_test.go @@ -318,9 +318,33 @@ var resolutionSites = map[string]string{ // the bug this guard exists to catch. Fail closed. var envNeedles = []string{"CurrentEnv", "ResolveEnv", "CLIENT_ENV"} +// matchesAnyNeedle is THE matcher, called from both directions — the detection +// sweep and the allowlist audit. One function on purpose: two copies of "does +// this file resolve an env?" is the same shape as the two copies of "which env?" +// that this PR exists to remove, and it would let detection and allowlisting +// drift apart exactly where nobody looks. +func matchesAnyNeedle(code string) (string, bool) { + for _, needle := range envNeedles { + if strings.Contains(code, needle) { + return needle, true + } + } + return "", false +} + +// envCodeOf tokenises one file, or reports why it could not. +func envCodeOf(path string) (string, error) { + src, err := os.ReadFile(path) + if err != nil { + return "", err + } + return goCodeTokens(string(src)) +} + func TestNoNewEnvironmentResolutionSiteAppears(t *testing.T) { const root = "../.." // this test's package dir -> the module root + // --- detection: what actually resolves an env, module-wide --------------- matched := map[string]string{} // repo-relative path -> the needle that hit walkErr := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { if err != nil { @@ -337,11 +361,7 @@ func TestNoNewEnvironmentResolutionSiteAppears(t *testing.T) { if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { return nil } - src, err := os.ReadFile(path) - if err != nil { - return err - } - code, err := goCodeTokens(string(src)) + code, err := envCodeOf(path) if err != nil { // "cannot tell" is not "clean": abort rather than read a file whose // tokenisation failed as a string that matches nothing. @@ -351,12 +371,8 @@ func TestNoNewEnvironmentResolutionSiteAppears(t *testing.T) { if err != nil { return err } - rel = filepath.ToSlash(rel) - for _, needle := range envNeedles { - if strings.Contains(code, needle) { - matched[rel] = needle - break - } + if needle, ok := matchesAnyNeedle(code); ok { + matched[filepath.ToSlash(rel)] = needle } return nil }) @@ -364,45 +380,54 @@ func TestNoNewEnvironmentResolutionSiteAppears(t *testing.T) { t.Fatalf("walking the module: %v", walkErr) } - // A guard that matches nothing is not a guard: if the needles ever stop - // matching (a rename), fail loudly rather than pass vacuously. - if len(matched) == 0 { - t.Fatal("the guard matched no files at all — its needles are stale and it is " + - "no longer checking anything") - } - + // --- every match must be sanctioned -------------------------------------- for rel, needle := range matched { - why, ok := resolutionSites[rel] - switch { - case !ok: + if _, ok := resolutionSites[rel]; !ok { t.Errorf("%s resolves an environment from ambient state (%q), but is not a "+ "sanctioned resolution site.\n"+ "Take the resolved env as an ARGUMENT instead — the CLI must answer "+ "\"which backend?\" once per invocation and thread it. If this really is "+ "a new resolution point, add it to resolutionSites with the reason and "+ "say how it cannot disagree with sessionEnv.", rel, needle) - case why == "": - t.Errorf("resolutionSites[%q] is allowlisted with no reason", rel) } } - // THE ALLOWLIST MUST STAY A RECORD, NOT BECOME A LICENCE. Checking only that a - // sanctioned file EXISTS is the defect class this whole PR is about: verifying + // --- and every sanctioned entry must still EARN its place ---------------- + // + // THE ALLOWLIST MUST STAY A RECORD, NOT BECOME A LICENCE. Asking only whether a + // sanctioned file EXISTS is the defect class this whole PR is about: checking // the form of a thing instead of the property the form exists to guarantee. An - // entry whose file no longer resolves anything is inert — it checks nothing, - // while silently pre-approving the next raw read in that file. - for rel := range resolutionSites { + // entry whose file no longer resolves anything is inert — it verifies nothing, + // while silently pre-approving the next ambient read in that file. My own + // consolidation did exactly that to the telemetry.go entry (Bugbot on #551), + // in the one file whose double resolution this PR removes. + // + // Per ENTRY rather than per suite, so the failure names the entry to delete. + // This also subsumes the "needles went stale" backstop: rename a needle and + // every entry goes inert at once, which is loud and specific rather than a + // single global counter hitting zero. + for rel, why := range resolutionSites { if _, err := os.Stat(filepath.Join(root, rel)); err != nil { - t.Errorf("resolutionSites lists %s, which does not exist — stale allowlist", rel) + t.Errorf("resolutionSites lists %s, which cannot be read — stale allowlist", rel) continue } + if why == "" { + t.Errorf("resolutionSites[%q] is allowlisted with no reason", rel) + } if _, ok := matched[rel]; !ok { - t.Errorf("stale allowlist entry: %s is sanctioned but no longer resolves an "+ - "environment from ambient state — remove it from resolutionSites. An inert "+ - "entry is a licence, not a record: it would silently re-admit a raw read "+ - "in that file.", rel) + t.Errorf("%s is allowlisted as a resolution site but resolves nothing any more "+ + "— drop the entry, or the guard silently permits the next ambient read "+ + "here.", rel) } } + + // The one thing the per-entry loop cannot see: an EMPTY allowlist makes it + // vacuous, and a needle rename plus an empty allowlist would then pass in + // silence. Anchor both. + if len(resolutionSites) == 0 || len(matched) == 0 { + t.Fatalf("the guard checked nothing: %d sanctioned entries, %d files matched", + len(resolutionSites), len(matched)) + } } // goCodeTokens renders src as its Go TOKENS, with comments dropped — so the