From 62115f4c639fb9047b9f9a5b4da1782e1c0112ba Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 11 Sep 2026 13:19:12 +0500 Subject: [PATCH 1/4] fix(login): reuse a valid session instead of always starting a device flow (cli#651) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tracebloc login` went straight to a device code even when the machine already held a valid session for the target env. On a headless host that is a dead end rather than an inconvenience: the credentials are on disk, but the command insists on a browser approval it has no way to complete — turning "re-run login to be safe" into a hard stop for any script or runbook. login now checks the session it already has before asking for one: - A session for the target env that the backend accepts ends the command at exit 0 with "Already signed in as ". The session is confirmed with a live WhoAmI rather than trusted off disk — a revoked token is still a token on disk. - `--force` is the opt-out (switching accounts, replacing a session believed stale) and skips the short-circuit entirely, including the probe. Same sense as `delete --force`: proceed despite the state that would otherwise stop you. - Every fall-through says WHY first, and only claims what it can tell apart: a LOCAL expires_at that has passed is named to the second (and is not presented to the backend), a 401/403 is reported as rejected, and anything else — DNS, a 5xx — is "couldn't check", not a verdict on the session. A 426 surfaces the upgrade instruction and starts no flow, since a fresh flow would hit the same version floor. "A session for this env" is resolved two ways, so credentials already on disk aren't stranded: the current session when sessionEnv resolves it to the target (the same predicate `auth status --check` uses, so login and the installer's probe cannot disagree), else that env's own profile — a machine on prod can hold a live dev token, and `login --env dev` adopts it and switches current_env. The profile is written back under the key it was found under, so a v1-migrated `"Dev"` config doesn't gain a second, lower-cased profile beside the real one. Tests cover each arm by asserting whether /device/code was requested at all — the browser demand is the behaviour that matters. Goldens regenerated for the new --force flag and copy. Closes tracebloc/cli#651 Co-Authored-By: Claude Opus 5 --- internal/cli/auth.go | 151 ++++++++- internal/cli/auth_test.go | 310 ++++++++++++++++++ internal/cli/testdata/golden/07-login.golden | 7 + .../cli/testdata/golden/zz-all-strings.golden | 7 + 4 files changed, 473 insertions(+), 2 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 8adee687..a6fd16ef 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -23,6 +23,7 @@ import ( // backend doesn't support browser sign-in yet. func newLoginCmd() *cobra.Command { var envFlag string + var force bool cmd := &cobra.Command{ Use: "login", Annotations: runtimeClassFor(classBackend), @@ -32,15 +33,23 @@ on any device (your laptop or phone), sign in the way you already do (password, Google, or GitHub), and approve the code. The CLI stores a user token in ~/.tracebloc (mode 0600). +Already signed in to this backend? login says so and exits 0 without a +browser step — so re-running it is safe in a script or a runbook, and a +headless box is never asked to approve a code it already has. Pass +--force to re-authenticate anyway: switching accounts, or replacing a +session you believe is stale. + Works on a headless / SSH box — the browser and the CLI need not share a machine. Honors HTTP(S)_PROXY / NO_PROXY for corporate-proxy networks.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { - return runLogin(cmd.Context(), printerFor(cmd), envFlag) + return runLogin(cmd.Context(), printerFor(cmd), envFlag, force) }, } cmd.Flags().StringVar(&envFlag, "env", "", "backend environment: dev|stg|prod (default: $CLIENT_ENV, then prod)") + cmd.Flags().BoolVar(&force, "force", false, + "start a new device flow even when this machine already holds a valid session") return cmd } @@ -52,7 +61,7 @@ var ( pollAfter = time.After ) -func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { +func runLogin(ctx context.Context, p *ui.Printer, envFlag string, force bool) error { cfg, err := config.Load() if err != nil { return &exitError{code: exitFailure, err: err} @@ -67,6 +76,20 @@ func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { "unknown backend environment %q — valid values are dev, stg, prod (default). "+ "Check --env / $CLIENT_ENV", env)} } + // Before asking for a browser approval, USE the credentials already on disk + // (cli#651). Without this, "re-run login to be safe" — a reasonable thing for + // a script or a runbook to do — is a hard stop on any host without a browser, + // even though the session is valid and every other command would accept it. + if !force { + reused, err := reuseStoredSession(ctx, p, cfg, env) + if err != nil { + return err + } + if reused { + return nil + } + } + client := newAPIClient(env) p.Detailf("backend %s — requesting a device code …", client.BaseURL) @@ -132,6 +155,130 @@ func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { return nil } +// storedSessionFor returns the profile KEY and the profile of the session this +// machine already holds for env, or ("", nil) when it holds none. +// +// Two arms, because "a session for env" has two shapes on disk and answering +// only the first would leave the credentials for the second unused: +// +// - the CURRENT session, when it RESOLVES to env. This is the same predicate +// `auth status --check` uses (sessionEnv, not the raw cfg.CurrentEnv), so +// login's short-circuit and the installer's probe cannot disagree about what +// "signed in to this env" means. +// - failing that, env's OWN profile. Profiles are per-env (R10), so a machine +// signed in to prod can still hold a live dev token; `login --env dev` must +// find it rather than run a flow for a credential already on disk. +// +// The KEY comes back with the profile because Profiles is keyed on the RAW env +// string — a v1-migrated config stores `"Dev"` verbatim (config.migrateV1) while +// sessionEnv normalises for the comparison. Writing the reused session back +// under the key it was FOUND under is what keeps a second, lower-cased profile +// from being minted alongside it. +func storedSessionFor(cfg *config.Config, env string) (string, *config.Profile) { + if cfg.SignedIn() && sessionEnv(cfg) == env { + return cfg.CurrentEnv, cfg.Current() + } + if p := cfg.Profiles[env]; p != nil && p.Token != "" { + return env, p + } + return "", nil +} + +// sessionExpired reports whether a stored profile's own recorded expiry has +// already passed, and renders it for the message. +// +// expires_at is "when known" (config.Profile) and the device grant does not +// return one today, so this is usually absent and the verdict comes from the +// backend instead. That split is exactly why the two copy paths differ: a LOCAL +// expiry we can name to the second, and a 401 we can only report as "rejected", +// because the backend returns the same status for a token that expired and one +// that was revoked. Naming a cause we cannot distinguish would be worse than +// reporting the one fact we have. +// +// An unparseable timestamp is NOT treated as expired: the backend, not a +// malformed config field, gets to invalidate a session. +func sessionExpired(prof *config.Profile) (bool, string) { + if prof.ExpiresAt == "" { + return false, "" + } + t, err := time.Parse(time.RFC3339, prof.ExpiresAt) + if err != nil || time.Now().Before(t) { + return false, "" + } + return true, t.Format(time.RFC3339) +} + +// reuseStoredSession is login's "you are already signed in" short-circuit +// (cli#651). It reports whether login is DONE: true means the machine holds a +// session for env that the backend just accepted, and there is nothing to sign +// in to. False means fall through to the device flow — every such path first +// says WHY, so a user who expected the short-circuit learns whether their +// session expired, was rejected, or simply couldn't be checked. +// +// The session is confirmed against the backend rather than trusted off disk: a +// token that has been revoked is still a token on disk, and reporting it as a +// live session would send the user into the next command to discover otherwise. +// When the backend can't be reached we fall through to the flow, which is what +// login does today — the flow surfaces the network failure in its own words. +func reuseStoredSession(ctx context.Context, p *ui.Printer, cfg *config.Config, env string) (bool, error) { + key, prof := storedSessionFor(cfg, env) + if prof == nil { + return false, nil + } + if expired, at := sessionExpired(prof); expired { + p.Hintf("The session saved on this machine expired at %s — signing in again.", at) + return false, nil + } + + client := newAPIClient(env) + client.Token = prof.Token + p.Detailf("backend %s — checking the session already on this machine …", client.BaseURL) + id, err := client.WhoAmI(ctx) + if err != nil { + // A 426 is the CLI being below the server's version floor, not a verdict on + // the session — and a fresh device flow would hit the same floor. Surface the + // upgrade instruction instead of burning a browser approval on it (the same + // call `auth status --check` makes). + var ue *api.UpgradeRequiredError + if errors.As(err, &ue) { + return false, &exitError{code: exitFailure, err: ue} + } + // Only a 401/403 is the backend REJECTING the credential. Anything else — + // DNS, a refused connection, a 5xx — means we couldn't verify, which is not + // the same thing and must not be reported as if the session were bad. + var ae *api.APIError + if errors.As(err, &ae) && (ae.StatusCode == http.StatusUnauthorized || ae.StatusCode == http.StatusForbidden) { + p.Hintf("The backend rejected the session saved on this machine — it expired or was revoked. Signing in again.") + return false, nil + } + p.Hintf("Couldn't check the session saved on this machine (%v) — signing in again.", err) + return false, nil + } + + // The session is live. Adopt it as the active one: `login --env dev` from a + // prod session is a request to SWITCH, and answering "already signed in" + // without moving current_env would leave every following command on prod. + // Written under the key the profile was found under, never a re-derived one. + cfg.CurrentEnv = key + prof.Email, prof.FirstName = id.Email, id.FirstName + if err := cfg.Save(); err != nil { + return false, &exitError{code: exitFailure, err: err} + } + if id.Email != "" { + p.Successf("Already signed in as %s.", id.Email) + } else { + p.Successf("Already signed in.") + } + // Visible, not demoted to Detailf, and deliberately NOT routed through + // withSignInAdvice: this is the actionable half of an outcome the user did not + // ask for. Someone who typed `login` to switch accounts needs the next step + // here, and unlike the advice withSignInAdvice guards, it contradicts nothing + // the installer prints — the installer reaches this line only on its success + // path, where its own next step is to carry on provisioning, not to re-auth. + p.Hintf("Run `tracebloc login --force` to sign in again — switching accounts, or replacing a session you believe is stale.") + return true, nil +} + // pollDisposition is what the poll loop does with a failed PollToken call. type pollDisposition int diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index f4b9f31b..9f648e6c 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -851,3 +851,313 @@ func TestLogin_ClearsStaleIdentityOnWhoAmIFailure(t *testing.T) { t.Errorf("stale identity leaked: FirstName=%q Email=%q (want both cleared)", prof.FirstName, prof.Email) } } + +// ── login's "already signed in" short-circuit (cli#651) ──────────────────────── +// +// The bug: `login` went straight to a device code even when the machine already +// held a valid session, which on a headless host is a dead end rather than an +// inconvenience — the credentials are on disk, but the command insists on a +// browser approval it has no way to complete. +// +// Every test below asserts on whether /device/code was requested, because THAT +// is the behaviour that matters: the copy is secondary to whether a browser step +// was demanded. + +// loginBackend serves the device flow + /userinfo/, counting device-code +// requests, and lets a test decide what /userinfo/ says about the session that +// is already on disk. +func loginBackend(t *testing.T, userinfo http.HandlerFunc) *int { + t.Helper() + var codes int + withTestBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/device/code": + codes++ + _, _ = w.Write([]byte(`{"device_code":"dc","user_code":"WDJB-MJHT","verification_uri":"https://x/activate","expires_in":600,"interval":5}`)) + case "/device/token": + _, _ = w.Write([]byte(`{"token":"fresh_tok"}`)) + case "/userinfo/": + userinfo(w, r) + default: + t.Errorf("unexpected request path %s", r.URL.Path) + } + }) + return &codes +} + +// okUserinfo accepts whatever token is presented. +func okUserinfo(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"email":"ds@co","first_name":"Dana","account":"Acme"}`)) +} + +// TestLogin_ReusesValidSession is the issue's reproduction: a second `login` +// against a live session must exit 0 without requesting a device code. +func TestLogin_ReusesValidSession(t *testing.T) { + codes := loginBackend(t, okUserinfo) + saveSignedIn(t, "live_tok") // CurrentEnv=dev + + out, err := runCmd(t, "login", "--env", "dev") + if err != nil { + t.Fatalf("login over a valid session should exit 0, got: %v", err) + } + if *codes != 0 { + t.Errorf("requested %d device codes; a valid session must not start a flow", *codes) + } + if !strings.Contains(out, "Already signed in as ds@co") { + t.Errorf("expected the already-signed-in line, got:\n%s", out) + } + if !strings.Contains(out, "--force") { + t.Errorf("expected the --force opt-out to be named, got:\n%s", out) + } + // The session is kept, not replaced by the flow's token. + cfg, _ := config.Load() + if got := cfg.Current().Token; got != "live_tok" { + t.Errorf("token = %q, want the existing live_tok", got) + } +} + +// TestLogin_ForceStartsAFlowOverAValidSession: --force is the opt-out for +// switching accounts or replacing a session believed stale, so it must skip the +// short-circuit entirely — including the probe. +func TestLogin_ForceStartsAFlowOverAValidSession(t *testing.T) { + codes := loginBackend(t, okUserinfo) + saveSignedIn(t, "live_tok") + + out, err := runCmd(t, "login", "--env", "dev", "--force") + if err != nil { + t.Fatalf("login --force: %v", err) + } + if *codes != 1 { + t.Errorf("requested %d device codes, want 1 under --force", *codes) + } + if strings.Contains(out, "Already signed in") { + t.Errorf("--force must not short-circuit, got:\n%s", out) + } + cfg, _ := config.Load() + if got := cfg.Current().Token; got != "fresh_tok" { + t.Errorf("token = %q, want the re-authenticated fresh_tok", got) + } +} + +// TestLogin_RejectedSessionSaysSoThenSignsIn: a 401 is the backend REJECTING the +// stored credential — name that, then run the flow the user came for. +func TestLogin_RejectedSessionSaysSoThenSignsIn(t *testing.T) { + codes := loginBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") == "Bearer stale_tok" { + w.WriteHeader(http.StatusUnauthorized) + return + } + okUserinfo(w, r) + }) + saveSignedIn(t, "stale_tok") + + out, err := runCmd(t, "login", "--env", "dev") + if err != nil { + t.Fatalf("login after a rejected session: %v", err) + } + if *codes != 1 { + t.Errorf("requested %d device codes, want 1 after a rejected session", *codes) + } + if !strings.Contains(out, "rejected the session saved on this machine") { + t.Errorf("expected the rejection to be named before the new flow, got:\n%s", out) + } + cfg, _ := config.Load() + if got := cfg.Current().Token; got != "fresh_tok" { + t.Errorf("token = %q, want fresh_tok", got) + } +} + +// TestLogin_UnverifiableSessionIsNotCalledRejected: a 5xx means we COULDN'T +// CHECK, which is a different situation from a rejected credential — reporting +// it as a rejection would tell the user their session is bad when the backend is +// simply down. (Same distinction runAuthCheck draws; cli#651 asks login to draw +// it too.) +func TestLogin_UnverifiableSessionIsNotCalledRejected(t *testing.T) { + codes := loginBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") == "Bearer live_tok" { + w.WriteHeader(http.StatusInternalServerError) // reachable but erroring + return + } + okUserinfo(w, r) + }) + saveSignedIn(t, "live_tok") + + out, err := runCmd(t, "login", "--env", "dev") + if err != nil { + t.Fatalf("login after an unverifiable session: %v", err) + } + if *codes != 1 { + t.Errorf("requested %d device codes, want 1 when the session can't be checked", *codes) + } + if !strings.Contains(out, "Couldn't check the session saved on this machine") { + t.Errorf("expected a 'couldn't check' line, got:\n%s", out) + } + if strings.Contains(out, "rejected") { + t.Errorf("a 500 must not be reported as a rejected session, got:\n%s", out) + } +} + +// TestLogin_LocallyExpiredSessionNamesTheExpiryWithoutProbing: when the stored +// profile carries an expires_at that has passed, we know the answer locally — +// say "expired" (the one cause we can actually distinguish) and don't spend a +// round-trip presenting a credential we know is dead. +func TestLogin_LocallyExpiredSessionNamesTheExpiryWithoutProbing(t *testing.T) { + // Only the OLD token's presentation counts: login's own post-flow + // confirmation hits /userinfo/ too, with the token it just obtained. + presented := false + codes := loginBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") == "Bearer old_tok" { + presented = true + } + okUserinfo(w, r) + }) + past := time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) + if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ + "dev": {Token: "old_tok", Email: "ds@co", ExpiresAt: past}, + }}).Save(); err != nil { + t.Fatal(err) + } + + out, err := runCmd(t, "login", "--env", "dev") + if err != nil { + t.Fatalf("login after an expired session: %v", err) + } + if presented { + t.Error("must not present a locally-expired token to the backend") + } + if *codes != 1 { + t.Errorf("requested %d device codes, want 1 after an expired session", *codes) + } + if !strings.Contains(out, "expired at "+past) { + t.Errorf("expected the expiry to be named, got:\n%s", out) + } +} + +// TestLogin_UnexpiredSessionIsStillProbed guards the other side of the expiry +// arm: an expires_at in the FUTURE must not be taken as proof on its own — a +// revoked token still has an unexpired timestamp on disk. +func TestLogin_UnexpiredSessionIsStillProbed(t *testing.T) { + probed := false + codes := loginBackend(t, func(w http.ResponseWriter, r *http.Request) { + probed = true + okUserinfo(w, r) + }) + future := time.Now().Add(time.Hour).UTC().Format(time.RFC3339) + if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ + "dev": {Token: "live_tok", Email: "ds@co", ExpiresAt: future}, + }}).Save(); err != nil { + t.Fatal(err) + } + + if _, err := runCmd(t, "login", "--env", "dev"); err != nil { + t.Fatalf("login: %v", err) + } + if !probed { + t.Error("an unexpired session must still be confirmed with the backend") + } + if *codes != 0 { + t.Errorf("requested %d device codes; the confirmed session must short-circuit", *codes) + } +} + +// TestLogin_UpgradeRequiredDoesNotStartAFlow: a 426 says this CLI is below the +// server's version floor — a device flow would hit the same floor, so surface the +// upgrade instruction instead of demanding a browser approval that cannot help. +func TestLogin_UpgradeRequiredDoesNotStartAFlow(t *testing.T) { + codes := loginBackend(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUpgradeRequired) // 426 + _, _ = w.Write([]byte(`{"error":"upgrade_required","min_version":"1.2.3"}`)) + }) + saveSignedIn(t, "live_tok") + + _, err := runCmd(t, "login", "--env", "dev") + if err == nil || !strings.Contains(err.Error(), "too old") { + t.Fatalf("a 426 must surface the upgrade message, got: %v", err) + } + if *codes != 0 { + t.Errorf("requested %d device codes; a 426 must not start a flow", *codes) + } +} + +// TestLogin_ReuseAdoptsTheTargetEnvsOwnProfile: profiles are per-env (R10), so a +// machine signed in to prod can hold a live dev token. `login --env dev` must use +// it AND switch current_env — answering "already signed in" without moving the +// pointer would leave every following command talking to prod. +func TestLogin_ReuseAdoptsTheTargetEnvsOwnProfile(t *testing.T) { + codes := loginBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer dev_tok" { + t.Errorf("probed with %q, want the dev profile's token", r.Header.Get("Authorization")) + } + okUserinfo(w, r) + }) + if err := (&config.Config{CurrentEnv: "prod", Profiles: map[string]*config.Profile{ + "prod": {Token: "prod_tok", Email: "ds@co"}, + "dev": {Token: "dev_tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + if _, err := runCmd(t, "login", "--env", "dev"); err != nil { + t.Fatalf("login --env dev over a stored dev session: %v", err) + } + if *codes != 0 { + t.Errorf("requested %d device codes; the stored dev session must be reused", *codes) + } + cfg, _ := config.Load() + if cfg.CurrentEnv != "dev" { + t.Errorf("current_env = %q, want dev (the short-circuit must still switch env)", cfg.CurrentEnv) + } + if got := cfg.Profiles["prod"].Token; got != "prod_tok" { + t.Errorf("prod token = %q, want prod_tok left intact (R10)", got) + } +} + +// TestLogin_ReuseKeepsTheRawProfileKey: Profiles is keyed on the RAW env string, +// and a v1-migrated config stores it verbatim ("Dev"). Reusing that session must +// write back under the key it was FOUND under — deriving a fresh, lower-cased key +// would mint a second profile beside the real one and strand the token. +func TestLogin_ReuseKeepsTheRawProfileKey(t *testing.T) { + codes := loginBackend(t, okUserinfo) + if err := (&config.Config{CurrentEnv: "Dev", Profiles: map[string]*config.Profile{ + "Dev": {Token: "live_tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + if _, err := runCmd(t, "login", "--env", "dev"); err != nil { + t.Fatalf("login: %v", err) + } + if *codes != 0 { + t.Errorf("requested %d device codes; a `Dev`-keyed session still resolves to dev", *codes) + } + cfg, _ := config.Load() + if len(cfg.Profiles) != 1 { + t.Errorf("profiles = %v, want the single existing one (no duplicate key)", cfg.Profiles) + } + if p := cfg.Profiles["Dev"]; p == nil || p.Token != "live_tok" { + t.Errorf("the `Dev` profile lost its token: %+v", cfg.Profiles) + } +} + +// TestLogin_NoStoredSessionStillSignsIn: the short-circuit must be invisible on +// the path it doesn't apply to — a machine with no session gets the device flow +// exactly as before, with no extra probe. +func TestLogin_NoStoredSessionStillSignsIn(t *testing.T) { + probes := 0 + codes := loginBackend(t, func(w http.ResponseWriter, r *http.Request) { + probes++ + okUserinfo(w, r) + }) + + if _, err := runCmd(t, "login", "--env", "dev"); err != nil { + t.Fatalf("login on a fresh machine: %v", err) + } + if *codes != 1 { + t.Errorf("requested %d device codes, want 1", *codes) + } + // One probe only: login's own post-flow confirmation. A signed-out machine + // has nothing to check beforehand. + if probes != 1 { + t.Errorf("%d /userinfo/ calls, want 1 (the post-flow confirmation only)", probes) + } +} diff --git a/internal/cli/testdata/golden/07-login.golden b/internal/cli/testdata/golden/07-login.golden index d08e9359..6b972750 100644 --- a/internal/cli/testdata/golden/07-login.golden +++ b/internal/cli/testdata/golden/07-login.golden @@ -15,6 +15,12 @@ on any device (your laptop or phone), sign in the way you already do (password, Google, or GitHub), and approve the code. The CLI stores a user token in ~/.tracebloc (mode 0600). +Already signed in to this backend? login says so and exits 0 without a +browser step — so re-running it is safe in a script or a runbook, and a +headless box is never asked to approve a code it already has. Pass +--force to re-authenticate anyway: switching accounts, or replacing a +session you believe is stale. + Works on a headless / SSH box — the browser and the CLI need not share a machine. Honors HTTP(S)_PROXY / NO_PROXY for corporate-proxy networks. @@ -23,6 +29,7 @@ Usage: Flags: --env string backend environment: dev|stg|prod (default: $CLIENT_ENV, then prod) + --force start a new device flow even when this machine already holds a valid session -h, --help help for login Global Flags: diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index af957237..ebf3e7bb 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -136,6 +136,8 @@ screen. %s/%d are runtime placeholders. "A training run is allocated up to:" "Add --help to any command for the flags." "Add/resize a node to meet the job's requests, or lower RESOURCE_REQUESTS on jobs-manager." +"Already signed in as %s." +"Already signed in." "Already signed out." "Applies to your next training run; a run already going keeps its size." "Applying the resource change…" @@ -173,6 +175,7 @@ screen. %s/%d are runtime placeholders. "Copying %s" "Correlation id: %s" "Couldn't check for active training runs (%v) — continuing; the confirmation below still guards you." +"Couldn't check the session saved on this machine (%v) — signing in again." "Couldn't clear the stored active-client pointer (%v) — the on-disk config still names the revoked client; run `tracebloc logout` or remove it by hand." "Couldn't connect to your secure environment — check your kubeconfig/context." "Couldn't determine this client's namespace — skipped the Helm uninstall. If a release is still installed, re-run with --namespace ." @@ -352,6 +355,7 @@ screen. %s/%d are runtime placeholders. "Review" "Revoked this machine's credential — your secure environment %q stays on tracebloc as a record." "Run '%s --help' for the available commands." +"Run `tracebloc login --force` to sign in again — switching accounts, or replacing a session you believe is stale." "SELECT '%s',%s,COUNT(*),%s,%s FROM `%s`.`%s`" "SELECT r.table_name, COALESCE(r.task,'') FROM `%s`.`%s` r JOIN (SELECT table_name, MAX(started_at) ms FROM `%s`.`%s` WHERE task IS NOT NULL GROUP BY table_name) m ON r.table_name = m.table_name AND r.started_at = m.ms WHERE r.task IS NOT NULL" "SELECT table_name FROM information_schema.tables WHERE table_schema='%s' ORDER BY table_name" @@ -390,11 +394,13 @@ screen. %s/%d are runtime placeholders. "Target verified with tracebloc: %s (namespace %s) — cluster %s." "Task:" "Text a folder with labels.csv + texts/ e.g. %s" +"The backend rejected the session saved on this machine — it expired or was revoked. Signing in again." "The column holding the duration / time-to-event. e.g. time, tenure_days" "The dataset's catalog metadata is kept as a record on tracebloc, marked unavailable — never removed." "The ingestion hasn't started yet (usually a slow image pull or a busy cluster). It's queued to run once the cluster can schedule it — check on it with the command below." "The name you provided was only control characters — auto-naming this client instead." "The number of landmark points each sample is annotated with — dataset-specific. e.g. 17 for COCO human pose" +"The session saved on this machine expired at %s — signing in again." "The size your images already are, as WxH — tracebloc checks every image matches and never resizes. Press Enter to read it from your first image. e.g. 224x224" "The tracebloc CLI (your local data & config are kept — --keep-data)" "This CLI is out of date — update it: %s" @@ -475,6 +481,7 @@ screen. %s/%d are runtime placeholders. "authorized — confirming the token with the backend …" "auto-detect" "backend" +"backend %s — checking the session already on this machine …" "backend %s — requesting a device code …" "backfilling the cluster anchor onto the existing client: %w" "bucket bins the target before it leaves the cluster" From 2dfd636c84423979b15cbdac9012e783f6152e78 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 11 Sep 2026 13:22:17 +0500 Subject: [PATCH 2/4] chore(release): bump VERSION to 0.10.26 for the login short-circuit (cli#651) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit version-bump-gate failed on the previous head: VERSION still read 0.10.25, v0.10.25 is already released, and this PR changes a published file (internal/cli/auth.go, matching `internal/*`). The release train cuts the tag from this file and never bumps it, so leaving it stale doesn't fail here — it fails the next prod hop, days later, on somebody else (backend#1561). 0.10.26 is free: v0.10.25 is the highest released final version and no v0.10.26 tag exists. Patch, not minor — this ships one bug fix, and the new `login --force` flag is additive with no change to any existing invocation. (Open PR #657 also touches VERSION, but bumps 0.10.24 -> 0.10.25, which is already released — its gate is red for the same reason and it needs a rebase. It does not claim 0.10.26.) Co-Authored-By: Claude Opus 5 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index dbca4f35..61012ac6 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.25 +0.10.26 From 413fcbf111974f214cfb4d8743392aeea0b24e36 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 11 Sep 2026 15:12:52 +0500 Subject: [PATCH 3/4] fix(login): find raw-keyed profiles, and exit 130 on a cancelled probe (cli#651) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Bugbot findings on PR #658, both real. 1. storedSessionFor's second arm indexed cfg.Profiles with the ALREADY NORMALISED target env, so a live token written under a raw key such as `"Dev"` (config.migrateV1 stores a v1 `env` verbatim) went unseen the moment that profile stopped being the current one. `login --env dev` then ran a device flow and saved a SECOND profile under `"dev"`, stranding a perfectly good session beside it — on exactly the headless host this issue is about. The existing raw-key test could not see it: it keeps `"Dev"` CURRENT, which arm 1 catches before the map lookup is reached. The gap only opens after a `login --env` elsewhere has moved current_env. Lookup now folds the map's own KEYS (new profileKeyed). Exact match wins; the fold is a tie-break scanned in sorted order, so a config holding both `"Dev"` and `"dev"` cannot answer differently run to run on Go's randomised map iteration. The trim+lower-case is extracted from sessionEnv as normalizeEnv and shared, rather than hand-rolled a second time — a second copy is how the keys stop matching in the first place. 2. A cancelled context surfaces on the WhoAmI call as a plain error, so Ctrl-C during the new probe landed in the "couldn't check" arm: it printed "signing in again" and then failed RequestDeviceCode with exit 1, where every other interrupt in login exits 130 silently. Guarded on ctx.Err() before the classification — the same guard, for the same reason, as pollForToken's. Tests: the not-current raw-key case (asserts no flow, no duplicate profile, current_env set to the FOUND key), a determinism test running profileKeyed 50x over a config with three case variants, and a cancelled probe asserting exit 130, silence, and zero device codes. Co-Authored-By: Claude Opus 5 --- internal/cli/auth.go | 38 +++++++++++++ internal/cli/auth_test.go | 109 ++++++++++++++++++++++++++++++++++++++ internal/cli/client.go | 11 +++- 3 files changed, 157 insertions(+), 1 deletion(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 9eb381fe..4a348b53 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "os" + "sort" "time" "github.com/spf13/cobra" @@ -178,9 +179,36 @@ func storedSessionFor(cfg *config.Config, env string) (string, *config.Profile) if cfg.SignedIn() && sessionEnv(cfg) == env { return cfg.CurrentEnv, cfg.Current() } + return profileKeyed(cfg, env) +} + +// profileKeyed finds env's own profile by FOLDING the map's keys, not by +// indexing with the already-normalised target (Bugbot, PR #658). +// +// A plain `cfg.Profiles[env]` only matches a key that is already lower-cased, so +// a live token written under `"Dev"` went unseen the moment that profile stopped +// being the current one — and the flow that followed saved a SECOND profile under +// `"dev"`, leaving the original session stranded beside it on exactly the headless +// host cli#651 is about. Arm 1 of storedSessionFor hid this: it catches the raw +// key while it is current, so the gap only opens after a `login --env` elsewhere. +// +// Exact match wins, and the fold is a tie-break scanned in sorted order — with +// both `"Dev"` and `"dev"` on disk the answer must not depend on Go's randomised +// map iteration. +func profileKeyed(cfg *config.Config, env string) (string, *config.Profile) { if p := cfg.Profiles[env]; p != nil && p.Token != "" { return env, p } + keys := make([]string, 0, len(cfg.Profiles)) + for k := range cfg.Profiles { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + if p := cfg.Profiles[k]; p != nil && p.Token != "" && normalizeEnv(k) == env { + return k, p + } + } return "", nil } @@ -235,6 +263,16 @@ func reuseStoredSession(ctx context.Context, p *ui.Printer, cfg *config.Config, p.Detailf("backend %s — checking the session already on this machine …", client.BaseURL) id, err := client.WhoAmI(ctx) if err != nil { + // Ctrl-C landing during the probe is the OPERATOR, not an unverifiable + // session: fall through and we print "signing in again", then fail + // RequestDeviceCode with exit 1 — where every other interrupt in login exits + // 130 silently. Checked before the classification below because a cancelled + // context surfaces on the HTTP call as a plain error, which would otherwise + // land in the "couldn't check" arm (Bugbot, PR #658; same guard, same + // reason, as pollForToken's). + if ctx.Err() != nil { + return false, &exitError{code: exitInterrupted} + } // A 426 is the CLI being below the server's version floor, not a verdict on // the session — and a fresh device flow would hit the same floor. Surface the // upgrade instruction instead of burning a browser approval on it (the same diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 9f648e6c..61a311b9 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "io" "net" "net/http" "net/http/httptest" @@ -14,6 +15,7 @@ import ( "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/ui" ) // withTestBackend points the login command at an httptest server (via the @@ -1161,3 +1163,110 @@ func TestLogin_NoStoredSessionStillSignsIn(t *testing.T) { t.Errorf("%d /userinfo/ calls, want 1 (the post-flow confirmation only)", probes) } } + +// TestLogin_ReuseFindsARawKeyedProfileThatIsNotCurrent (Bugbot, PR #658) is the +// gap TestLogin_ReuseKeepsTheRawProfileKey could not see. That test keeps the +// `"Dev"` profile CURRENT, so arm 1 of storedSessionFor catches it and the map +// lookup is never exercised. Once a `login --env` elsewhere moves current_env, +// only arm 2 is left — and indexing the map with the already-normalised target +// misses `"Dev"` entirely, starting a flow and saving a second `"dev"` profile +// beside a perfectly good session. +func TestLogin_ReuseFindsARawKeyedProfileThatIsNotCurrent(t *testing.T) { + codes := loginBackend(t, func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Authorization"); got != "Bearer dev_tok" { + t.Errorf("probed with %q, want the `Dev` profile's token", got) + } + okUserinfo(w, r) + }) + // Signed in to prod; the dev session exists under a v1-migrated raw key. + if err := (&config.Config{CurrentEnv: "prod", Profiles: map[string]*config.Profile{ + "prod": {Token: "prod_tok", Email: "ds@co"}, + "Dev": {Token: "dev_tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + if _, err := runCmd(t, "login", "--env", "dev"); err != nil { + t.Fatalf("login --env dev over a `Dev`-keyed session: %v", err) + } + if *codes != 0 { + t.Errorf("requested %d device codes; the `Dev` session must be found and reused", *codes) + } + cfg, _ := config.Load() + if cfg.CurrentEnv != "Dev" { + t.Errorf("current_env = %q, want the key the profile was FOUND under", cfg.CurrentEnv) + } + if _, dup := cfg.Profiles["dev"]; dup { + t.Errorf("a duplicate lower-cased profile was minted: %v", cfg.Profiles) + } + if p := cfg.Profiles["Dev"]; p == nil || p.Token != "dev_tok" { + t.Errorf("the `Dev` profile lost its token: %+v", cfg.Profiles) + } +} + +// TestProfileKeyed_ExactMatchWinsAndFoldIsDeterministic: with both `"dev"` and +// `"Dev"` on disk the answer must not ride on Go's randomised map iteration. +// Exact match wins; the fold is only a tie-break, scanned in sorted order. +func TestProfileKeyed_ExactMatchWinsAndFoldIsDeterministic(t *testing.T) { + cfg := &config.Config{Profiles: map[string]*config.Profile{ + "Dev": {Token: "raw_tok"}, + "dev": {Token: "exact_tok"}, + "DEV": {Token: "shouty_tok"}, + }} + for i := 0; i < 50; i++ { + key, prof := profileKeyed(cfg, "dev") + if key != "dev" || prof.Token != "exact_tok" { + t.Fatalf("iteration %d: got (%q, %q), want the exact `dev` match", i, key, prof.Token) + } + } + // With no exact key, the sorted fold must still answer the same way every time. + delete(cfg.Profiles, "dev") + for i := 0; i < 50; i++ { + key, _ := profileKeyed(cfg, "dev") + if key != "DEV" { // "DEV" sorts before "Dev" + t.Fatalf("iteration %d: fold returned %q, want a stable sorted pick", i, key) + } + } + // A profile with no token is not a session. + cfg.Profiles = map[string]*config.Profile{"Dev": {Email: "ds@co"}} + if key, prof := profileKeyed(cfg, "dev"); prof != nil { + t.Errorf("a tokenless profile matched: (%q, %+v)", key, prof) + } +} + +// TestLogin_CancelDuringTheProbeExits130 (Bugbot, PR #658): Ctrl-C landing on the +// new session probe is the operator, not an unverifiable session. Falling through +// printed "signing in again" and then failed RequestDeviceCode with exit 1, where +// every other interrupt in login exits 130 silently. +func TestLogin_CancelDuringTheProbeExits130(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + codes := 0 + ctx, cancel := context.WithCancel(context.Background()) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/userinfo/": + cancel() // the operator hits Ctrl-C mid-probe + <-r.Context().Done() + case "/device/code": + codes++ + _, _ = w.Write([]byte(`{"device_code":"dc","user_code":"X","verification_uri":"https://x/a","expires_in":600,"interval":5}`)) + } + })) + t.Cleanup(srv.Close) + orig := newAPIClient + newAPIClient = func(string) *api.Client { return &api.Client{BaseURL: srv.URL, HTTP: srv.Client()} } + t.Cleanup(func() { newAPIClient = orig }) + + saveSignedIn(t, "live_tok") // CurrentEnv=dev + err := runLogin(ctx, ui.New(io.Discard, ui.WithColor(false)), "dev", false) + + if got := ExitCodeFromError(err); got != exitInterrupted { + t.Fatalf("exit code = %d, want %d (a cancelled probe is an interrupt)", got, exitInterrupted) + } + if !IsSilentError(err) { + t.Errorf("an interrupt must exit quietly, got: %v", err) + } + if codes != 0 { + t.Errorf("requested %d device codes; Ctrl-C must not start a flow", codes) + } +} diff --git a/internal/cli/client.go b/internal/cli/client.go index d69dbf55..63cb335d 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -164,12 +164,21 @@ func clientPrompter() prompter { // 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 e := strings.ToLower(strings.TrimSpace(cfg.CurrentEnv)); e != "" { + if e := normalizeEnv(cfg.CurrentEnv); e != "" { return e } return api.ResolveEnv("") } +// normalizeEnv is the trim+lower-case sessionEnv applies, as a pure function, so +// the one other place that has to COMPARE a raw stored env string — the profile +// lookup in auth.go, which reads the Profiles map's own keys — folds it exactly +// the same way. Two hand-rolled copies of this is how a `"Dev"` key stops +// matching a `dev` target. +func normalizeEnv(env string) string { + return strings.ToLower(strings.TrimSpace(env)) +} + // knownSessionEnv resolves the session env like sessionEnv and then REJECTS an // unrecognised value, instead of letting api.BaseURL fall it back to prod. Every // path that attaches the stored TOKEN — authedClient, logout's server-side revoke, From e27b36b1f66060d1c005743cc7b96fb6c8a85d73 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 11 Sep 2026 15:17:24 +0500 Subject: [PATCH 4/4] refactor(login): one shared WhoAmI classifier for both session probes (cli#651) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on PR #658: reuseStoredSession's 426 / 401-403 / everything-else classification of a WhoAmI failure duplicated the identical block in runAuthCheck a few dozen lines down. The duplication is one this PR introduced, so it is fixed here rather than left for a follow-up — "fix the class, not the instance". classifyWhoAmIError returns a named whoAmIVerdict (whoAmIUnverified / whoAmIRejected / whoAmIUpgradeRequired) plus the *api.UpgradeRequiredError, so the caller surfaces the server's own version floor rather than a paraphrase. The three arms are deliberately not collapsible: only whoAmIRejected is a statement about the credential. A 5xx folded into it would tell someone to re-authenticate during an outage, and a 426 folded into it would send them to a browser step that cannot lift a version floor. The COPY stays at the call sites. The two commands answer different questions — "should I start a device flow?" vs "what is this exit code?" — and say so in different words; only the classification is shared. No user-facing string changed, and the goldens confirm it (regenerated, no diff). Test: a table over 401/403/426/500/404/429, a transport error, a cancelled context, and wrapped 401/426 — wrapped because both call sites receive the error through the api client's own fmt.Errorf wrapping, so matching the concrete type alone would silently demote every real verdict to "unverified". Co-Authored-By: Claude Opus 5 --- internal/cli/auth.go | 74 ++++++++++++++++++++++++++++----------- internal/cli/auth_test.go | 45 ++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 21 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 4a348b53..9f6c068c 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -236,6 +236,48 @@ func sessionExpired(prof *config.Profile) (bool, string) { return true, t.Format(time.RFC3339) } +// whoAmIVerdict is what a FAILED WhoAmI says about the stored session. The three +// arms are deliberately not collapsible: only one of them is a statement about +// the credential. +type whoAmIVerdict int + +const ( + // whoAmIUnverified — no verdict was ever reached: DNS, a refused connection, + // a 5xx. NOT evidence the session is bad, and must never be reported as if it + // were: telling someone to re-authenticate during an outage sends them to a + // browser step that cannot help. + whoAmIUnverified whoAmIVerdict = iota + // whoAmIRejected — the backend refused the credential (401/403). The one arm + // where signing in again is the answer. + whoAmIRejected + // whoAmIUpgradeRequired — a 426: this CLI is below the server's version floor. + // Says nothing about the session, and no amount of re-authenticating fixes it. + whoAmIUpgradeRequired +) + +// classifyWhoAmIError turns a failed WhoAmI into that verdict, returning the +// *api.UpgradeRequiredError alongside it so the caller can surface the server's +// own version floor rather than a paraphrase. +// +// Shared because both places that probe a stored session — login's short-circuit +// and `auth status --check` — have to draw the SAME three-way distinction, and +// two hand-written copies of it drift the first time a fourth case appears +// (review on PR #658). The copy stays at the call sites: the two commands answer +// different questions ("should I start a flow?" vs "what is this exit code?") and +// say so in different words. +func classifyWhoAmIError(err error) (whoAmIVerdict, *api.UpgradeRequiredError) { + var ue *api.UpgradeRequiredError + if errors.As(err, &ue) { + return whoAmIUpgradeRequired, ue + } + var ae *api.APIError + if errors.As(err, &ae) && + (ae.StatusCode == http.StatusUnauthorized || ae.StatusCode == http.StatusForbidden) { + return whoAmIRejected, nil + } + return whoAmIUnverified, nil +} + // reuseStoredSession is login's "you are already signed in" short-circuit // (cli#651). It reports whether login is DONE: true means the machine holds a // session for env that the backend just accepted, and there is nothing to sign @@ -273,23 +315,17 @@ func reuseStoredSession(ctx context.Context, p *ui.Printer, cfg *config.Config, if ctx.Err() != nil { return false, &exitError{code: exitInterrupted} } - // A 426 is the CLI being below the server's version floor, not a verdict on - // the session — and a fresh device flow would hit the same floor. Surface the - // upgrade instruction instead of burning a browser approval on it (the same - // call `auth status --check` makes). - var ue *api.UpgradeRequiredError - if errors.As(err, &ue) { + switch verdict, ue := classifyWhoAmIError(err); verdict { + case whoAmIUpgradeRequired: + // Not a verdict on the session, and a fresh device flow would hit the same + // floor — surface the upgrade instruction instead of burning a browser + // approval on it. return false, &exitError{code: exitFailure, err: ue} - } - // Only a 401/403 is the backend REJECTING the credential. Anything else — - // DNS, a refused connection, a 5xx — means we couldn't verify, which is not - // the same thing and must not be reported as if the session were bad. - var ae *api.APIError - if errors.As(err, &ae) && (ae.StatusCode == http.StatusUnauthorized || ae.StatusCode == http.StatusForbidden) { + case whoAmIRejected: p.Hintf("The backend rejected the session saved on this machine — it expired or was revoked. Signing in again.") - return false, nil + default: // whoAmIUnverified + p.Hintf("Couldn't check the session saved on this machine (%v) — signing in again.", err) } - p.Hintf("Couldn't check the session saved on this machine (%v) — signing in again.", err) return false, nil } @@ -699,16 +735,12 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { // A 426 means the CLI is too old, not that the session is invalid — surface // the upgrade instruction (non-silent, so it shows even without --verbose) // instead of the "re-login" advice, which wouldn't help. - var ue *api.UpgradeRequiredError - if errors.As(err, &ue) { + verdict, ue := classifyWhoAmIError(err) + if verdict == whoAmIUpgradeRequired { return &exitError{code: exitFailure, err: ue} } if p.Verbose() { - // Only a 401/403 is genuinely a rejected token (where re-login helps); a - // network/DNS/5xx failure means we couldn't verify, not that the session - // is invalid — don't send the user to re-login for an outage. - var apiErr *api.APIError - if errors.As(err, &apiErr) && (apiErr.StatusCode == http.StatusUnauthorized || apiErr.StatusCode == http.StatusForbidden) { + if verdict == whoAmIRejected { p.Hintf("Signed-in token was rejected by the backend — run `tracebloc login`.") } else { p.Hintf("Couldn't verify your session with the backend (%v).", err) diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 61a311b9..f2cffd70 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -1270,3 +1270,48 @@ func TestLogin_CancelDuringTheProbeExits130(t *testing.T) { t.Errorf("requested %d device codes; Ctrl-C must not start a flow", codes) } } + +// TestClassifyWhoAmIError pins the three-way distinction both session probes +// share (review on PR #658). The arms are not interchangeable: only +// whoAmIRejected is a statement about the credential, so a 5xx landing in it +// would send someone to re-authenticate during an outage, and a 426 landing +// there would send them to a browser step that cannot lift a version floor. +func TestClassifyWhoAmIError(t *testing.T) { + for _, tc := range []struct { + name string + err error + want whoAmIVerdict + wantMin string // the server's floor, when the verdict carries one + }{ + {"401 is a rejection", &api.APIError{StatusCode: http.StatusUnauthorized}, whoAmIRejected, ""}, + {"403 is a rejection", &api.APIError{StatusCode: http.StatusForbidden}, whoAmIRejected, ""}, + {"426 is a version floor", &api.UpgradeRequiredError{MinVersion: "1.2.3"}, whoAmIUpgradeRequired, "1.2.3"}, + {"500 is not a verdict", &api.APIError{StatusCode: http.StatusInternalServerError}, whoAmIUnverified, ""}, + {"404 is not a rejection", &api.APIError{StatusCode: http.StatusNotFound}, whoAmIUnverified, ""}, + {"429 is not a rejection", &api.APIError{StatusCode: http.StatusTooManyRequests}, whoAmIUnverified, ""}, + {"a transport error is not a verdict", errors.New("dial tcp: no such host"), whoAmIUnverified, ""}, + {"a cancelled context is not a rejection", context.Canceled, whoAmIUnverified, ""}, + // Wrapped, because both call sites get their error back through the api + // client's own fmt.Errorf wrapping — matching on the concrete type only + // would silently demote every real verdict to "unverified". + {"wrapped 401 still a rejection", fmt.Errorf("confirming: %w", + &api.APIError{StatusCode: http.StatusUnauthorized}), whoAmIRejected, ""}, + {"wrapped 426 still a version floor", fmt.Errorf("confirming: %w", + &api.UpgradeRequiredError{MinVersion: "9.9.9"}), whoAmIUpgradeRequired, "9.9.9"}, + } { + t.Run(tc.name, func(t *testing.T) { + got, ue := classifyWhoAmIError(tc.err) + if got != tc.want { + t.Errorf("verdict = %d, want %d", got, tc.want) + } + switch { + case tc.wantMin != "": + if ue == nil || ue.MinVersion != tc.wantMin { + t.Errorf("upgrade error = %+v, want MinVersion %q", ue, tc.wantMin) + } + case ue != nil: + t.Errorf("upgrade error = %+v, want nil", ue) + } + }) + } +}