diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 6b3306c..9e6d70c 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -73,6 +73,27 @@ Two things make this repo unusual and should shape every finding: (`internal/api/client.go`, `nextPath`). Where "empty" and "unknown" are different answers, prefer a three-valued return (`internal/cluster/discover.go:302`). +- **"Couldn't confirm" used as "confirmed" — in a BRANCH, not just a return type.** The rule + above is about the value a function hands back; this is about the `if` that consumes it, and + it is the defect this repo produces most often. cli#515 shipped it three times in one PR, + each in a different file, each after the previous one was fixed: a failed cluster scan + reported as "no client is running here"; an empty `cluster_id` on a legacy client read as + "runs elsewhere" (`ProvisionedClient.ClusterID` is documented empty on not-yet-backfilled + records); and `reachStateOf(x) != ReachNoEnv` used to mean "an environment is here", when + `ReachState` also has `ReachUnreachable` and `ReachError`. Two concrete shapes to flag: + - **A negated comparison against ONE member of a multi-valued enum.** `!= ReachNoEnv`, + `!= StatusFail` and friends silently include every member added later. Compare against the + member you actually require (`== ReachOK`), and derive the test's input domain from the + enum's declared surface — mutation coverage cannot see a vocabulary gap. + - **A lenient "not found" default reused where the question is "may I believe this?"** + `reachStateOf` returns `ReachOK` for an ABSENT check, which is right for a verdict roll-up + and wrong for authorising a claim — hence the separate `reachConfirmedOK` + (`internal/cli/doctor.go`). The same default is rarely correct for both. + + The customer-visible cost is never a wrong log line: on #515 each instance ended in advice to + run `client create` on a cluster nothing was confirmed on, where it MINTS rather than adopts — + i.e. the guidance manufactured the orphaned phantom of `backend#970`. + - **A cross-repo contract change that only lands on one side.** `scripts/.data-ingestors-ref`, `scripts/.client-ref` and `scripts/.backend-ref` pin upstream refs deliberately so an unrelated upstream commit can't red every open PR. Flag a hand-edit to a generated artifact diff --git a/STYLE.md b/STYLE.md index fcbde6b..7ba0644 100644 --- a/STYLE.md +++ b/STYLE.md @@ -55,10 +55,31 @@ flow) use one uniform rhythm so every question reads the same: - **A result that belongs to an answer attaches to it with no blank** — e.g. the `✔ Found a CSV table …` sniff echo sits directly under the path answer. -So: `header → blank → [supporting text → blank] → ? prompt`. The prompt line is -answer-only (`? train`); the question lives in the header (the prompter runs -`bare`), never repeated on the `?` line. Keep it uniform — don't hand-tune the -spacing of individual questions. +So: `header → blank → [supporting text → blank] → ? prompt`. Keep it uniform — +don't hand-tune the spacing of individual questions. + +## Guided-prompt labels + +The `?` line carries a **short noun label**, not the question: `? Path: ~/mydata`, +`? Task: tabular_classification`. The question lives in the header and is never +repeated on the `?` line; the label says what you are typing into. + +- The label is the **shortest noun phrase that names the answer**, with a trailing + colon — `Split:`, `Name:`, `Path:`, `Task:`, `Data type:`, `Label:` / `Target:`, + `Keypoints:`, `Resolution:`, `Column types:`, `Label policy:`, `Time column:`. +- **A prompt never goes label-less.** The guided flow used to blank survey's + `Message` (a `bare` mode), which rendered a lone `?` — and once answers were + pre-filled, `? [~/mydata]`: a question mark, a bracket and a path, with no verb + (cli#504). `internal/cli/interactive_test.go` asserts the property — non-empty, + ends in `:`, no `?`, within a length budget — against whatever the real flow + asks, so a new question is covered without editing the test. +- **A confirm is the exception: it carries the whole question** (`? Proceed with + the ingest? (y/N)`). A y/N prompt has no header of its own, and the + overwrite-replace confirm fires with nothing printed before it — a noun there + would name the object and hide the stakes. +- Flows with no step headers of their own (`client create`, `delete`, + `resources set`) pass the whole question as the label. The register follows the + header, not the prompter. ## Terminology diff --git a/VERSION b/VERSION index 1a46c7f..f314d02 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.8 +0.10.9 diff --git a/docs/cli-navigation.md b/docs/cli-navigation.md index f8ce400..cba1423 100644 --- a/docs/cli-navigation.md +++ b/docs/cli-navigation.md @@ -22,7 +22,7 @@ flowchart TD ACCT --> logout["logout"] ACCT --> authst["auth status"] ACCT --> clis["client status"] - ACCT -.-> clcreate["client create"]:::hidden + ACCT --> clcreate["client create — point this machine at its client"] ACCT -.-> cllist["client list"]:::hidden ENVC --> di["data ingest"] @@ -172,7 +172,7 @@ flowchart TD - **not signed in / token 401·403** → `login` - **426 upgrade-required** → upgrade the CLI - **kubeconfig (exit 3)** → fix `--kubeconfig`/`--context`, then `doctor` -- **no client / environment (exit 4)** → run the installer (or `--namespace`); triage with `doctor` +- **no client / environment (exit 4)** → the error now says what IS on the reached cluster before advising (cli#515): one client on a local cluster → `client create` repoints this machine (it adopts, no new credential); a client on a remote/shared cluster → `--namespace ` only; nothing there → run the installer. Triage with `doctor` - **no token (exit 5)** → grant RBAC; diagnose with `cluster info` / `doctor` - **destination exists (exit 6)** → `--overwrite`, a different `--name`, or `data delete` first - **staging partial (exit 7)** → `data delete` then re-ingest diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 6982161..c4237eb 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "os" "time" "github.com/spf13/cobra" @@ -130,6 +131,128 @@ func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { return nil } +// pollDisposition is what the poll loop does with a failed PollToken call. +type pollDisposition int + +const ( + // pollStop — the sign-in cannot succeed. Report it and exit. + pollStop pollDisposition = iota + // pollAgain — an expected non-answer (not approved yet). Poll again unchanged. + pollAgain + // pollSlower — the server asked us to back off (RFC 8628 §3.5). + pollSlower + // pollRetry — an infrastructure failure, not a verdict on the sign-in. Poll + // again, but count it: a backend that never answers must say so eventually. + pollRetry +) + +// maxPollFailures bounds how many CONSECUTIVE pollRetry outcomes the loop rides +// out before giving up and naming the last one. At the RFC's 5-second floor +// that is a minute of unbroken failure — long enough to cross a wifi handover, +// a DNS blip, or a backend restart; short enough that a genuinely unreachable +// backend reports itself instead of silently burning the code's whole window. +// The code's own expiry still bounds the loop; the counter only makes the +// give-up message honest when the network, not the human, is at fault. +const maxPollFailures = 12 + +// classifyPollError decides whether a PollToken failure ends the sign-in or is +// worth another poll inside the code's remaining window (cli#517). +// +// The default used to be "stop", which made one DNS hiccup fatal to an +// installer run that had already built a cluster. The default is now "retry", +// so every genuinely terminal state is enumerated HERE rather than being the +// leftover case — a refusal must never become an infinite loop: +// +// - the four RFC 8628 §3.5 sentinels are terminal or not by the spec; +// - a 426 means this CLI is below the server's version floor — polling can't +// make it newer; +// - an *APIError carries a server VERDICT: 5xx / 408 / 429 are the server or a +// proxy failing temporarily, every other status is a refusal we must respect; +// - a cancelled context is the operator, not a blip; +// - everything left never reached a server verdict at all — DNS, refused +// connection, TLS, a truncated read, a body we couldn't decode — and is the +// class this function exists to keep alive. +func classifyPollError(err error) pollDisposition { + switch { + case errors.Is(err, api.ErrAuthorizationPending): + return pollAgain + case errors.Is(err, api.ErrSlowDown): + return pollSlower + case errors.Is(err, api.ErrExpiredToken), errors.Is(err, api.ErrAccessDenied): + return pollStop + case errors.Is(err, context.Canceled): + return pollStop + } + var ue *api.UpgradeRequiredError + if errors.As(err, &ue) { + return pollStop + } + var ae *api.APIError + if errors.As(err, &ae) { + switch { + case ae.StatusCode >= 500, + ae.StatusCode == http.StatusRequestTimeout, + ae.StatusCode == http.StatusTooManyRequests: + return pollRetry + default: + return pollStop + } + } + return pollRetry +} + +// withSignInAdvice appends the command that starts a fresh sign-in — or leaves +// the error exactly as it is when the installer is driving us (cli#517). +// "`tracebloc login`" is right when a human typed it and WRONG under the +// installer, where a bare login leaves the client mint and the Helm install +// undone; the installer prints its own, correct next step, and two contradicting +// instructions are worse than one. The installer announces itself with +// TRACEBLOC_INSTALLER. +// +// The advice is appended by wrapping rather than composed into each message, so +// every sentence stays a literal argument of an errors.New / fmt.Errorf call — +// which is what keeps this copy visible to the copy catalog's AST harvest. +func withSignInAdvice(err error) error { + if os.Getenv("TRACEBLOC_INSTALLER") != "" { + return err + } + return fmt.Errorf("%w. Run `tracebloc login` to start a new one", err) +} + +// signInWindow renders the code's advertised lifetime as a clause for the +// expiry copy, or "" when the server didn't say. Named in the message because +// without it a ten-minute timeout reads as an instant failure to anyone who +// stepped away — which is exactly how cli#517 was first reported. Derived from +// expires_in rather than hardcoded, so the sentence cannot outlive a change to +// the backend's DEVICE_CODE_TTL. +func signInWindow(expiresIn int) string { + if expiresIn <= 0 { + return "" + } + d := time.Duration(expiresIn) * time.Second + human := d.Round(time.Second).String() + switch { + case d == time.Minute: + human = "1 minute" + case d%time.Minute == 0: + human = fmt.Sprintf("%d minutes", int(d/time.Minute)) + } + return fmt.Sprintf(" — sign-in codes are valid for %s", human) +} + +// terminalSignInError renders the user-facing copy for a poll outcome the loop +// must stop on. An error we have no bespoke copy for is surfaced verbatim: a +// vague "sign-in failed" would hide the only diagnostic we have. +func terminalSignInError(err error, window string) error { + switch { + case errors.Is(err, api.ErrExpiredToken): + return withSignInAdvice(fmt.Errorf("the sign-in code expired%s", window)) + case errors.Is(err, api.ErrAccessDenied): + return withSignInAdvice(errors.New("sign-in was denied in the browser")) + } + return err +} + // pollForToken runs the RFC 8628 device-token poll loop behind a live wait // spinner, returning the issued token or an *exitError. The spinner is cleared // on every return path (deferred Stop), so the caller prints the ✔ / error line @@ -143,13 +266,18 @@ func pollForToken(ctx context.Context, p *ui.Printer, client *api.Client, dc *ap if dc.ExpiresIn > 0 { deadline = time.Now().Add(time.Duration(dc.ExpiresIn) * time.Second) } + window := signInWindow(dc.ExpiresIn) sp := p.Spinner("Waiting for your browser…", "Ctrl-C to cancel") defer sp.Stop() + // Consecutive infrastructure failures. Reset by any answer from the server — + // a blip mid-way through a long wait must not accumulate toward the cap. + var failures int for { if !deadline.IsZero() && time.Now().After(deadline) { - return "", &exitError{code: exitFailure, err: errors.New("login timed out — re-run `tracebloc login`")} + return "", &exitError{code: exitFailure, err: withSignInAdvice( + fmt.Errorf("the sign-in code expired before it was approved%s", window))} } select { case <-ctx.Done(): @@ -158,21 +286,35 @@ func pollForToken(ctx context.Context, p *ui.Printer, client *api.Client, dc *ap } tok, err := client.PollToken(ctx, dc.DeviceCode) - switch { - case err == nil: + if err == nil { return tok, nil - case errors.Is(err, api.ErrAuthorizationPending): - // not approved yet — keep polling - case errors.Is(err, api.ErrSlowDown): + } + // Ctrl-C landing DURING the request surfaces as a cancelled context on the + // HTTP call, not on the select above — exit quietly there too, rather than + // reporting the operator's own interrupt as a sign-in failure. + if ctx.Err() != nil { + return "", &exitError{code: exitInterrupted} + } + switch classifyPollError(err) { + case pollAgain: + failures = 0 // not approved yet — keep polling + case pollSlower: // RFC 8628 §3.5: on slow_down the client MUST increase the poll // interval by 5 seconds for this and all subsequent polls. + failures = 0 interval += 5 - case errors.Is(err, api.ErrExpiredToken): - return "", &exitError{code: exitFailure, err: errors.New("the sign-in code expired — re-run `tracebloc login`")} - case errors.Is(err, api.ErrAccessDenied): - return "", &exitError{code: exitFailure, err: errors.New("sign-in was denied in the browser")} - default: - return "", &exitError{code: exitFailure, err: err} + case pollRetry: + failures++ + if failures >= maxPollFailures { + // One literal, not a concatenation: the copy catalog's AST harvest + // only sees whole string literals, so a "+"-joined message is copy + // nothing inventories. + return "", &exitError{code: exitFailure, err: fmt.Errorf( + "couldn't reach the backend to finish signing in — %d attempts failed in a row (check your network / HTTPS_PROXY): %w", + failures, err)} + } + default: // pollStop + return "", &exitError{code: exitFailure, err: terminalSignInError(err, window)} } } } diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 6c46a38..ff8e7ed 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -2,6 +2,10 @@ package cli import ( "bytes" + "context" + "errors" + "fmt" + "net" "net/http" "net/http/httptest" "strings" @@ -166,6 +170,342 @@ func TestLogin_Denied(t *testing.T) { } } +// ── cli#517: which poll failures end the sign-in, and which are worth another try ── + +// deviceCodeBody is the /device/code reply the poll tests share: a ten-minute +// window (so the expiry copy has a duration to name) and the RFC's 5s interval. +const deviceCodeBody = `{"device_code":"dc","user_code":"X","verification_uri":"https://x/activate","expires_in":600,"interval":5}` + +// pollBackend serves /device/code + /userinfo/ and hands every /device/token +// poll to tokenH, which sees the 1-based poll number. Returns a pointer to the +// live poll count so a test can assert the loop STOPPED (or kept going). +func pollBackend(t *testing.T, tokenH func(w http.ResponseWriter, poll int)) *int { + t.Helper() + polls := 0 + withTestBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/device/code": + _, _ = w.Write([]byte(deviceCodeBody)) + case "/device/token": + polls++ + tokenH(w, polls) + case "/userinfo/": + _, _ = w.Write([]byte(`{"email":"ds@tracebloc.io"}`)) + default: + t.Errorf("unexpected request path %s", r.URL.Path) + } + }) + return &polls +} + +// TestClassifyPollError_Table pins the retry classification directly, on the +// production function the loop calls (never a re-implementation of it). The +// inputs are written down independently of the matcher: each is the error a +// specific real-world failure produces, not a value read back off the rule. +func TestClassifyPollError_Table(t *testing.T) { + cases := []struct { + name string + err error + want pollDisposition + }{ + // The RFC 8628 §3.5 sentinels, bare and wrapped — PollToken returns them + // bare today, but a future wrapper must not silently reclassify them. + {"pending", api.ErrAuthorizationPending, pollAgain}, + {"pending wrapped", fmt.Errorf("poll: %w", api.ErrAuthorizationPending), pollAgain}, + {"slow_down", api.ErrSlowDown, pollSlower}, + {"slow_down wrapped", fmt.Errorf("poll: %w", api.ErrSlowDown), pollSlower}, + {"expired_token", api.ErrExpiredToken, pollStop}, + {"expired_token wrapped", fmt.Errorf("poll: %w", api.ErrExpiredToken), pollStop}, + {"access_denied", api.ErrAccessDenied, pollStop}, + {"access_denied wrapped", fmt.Errorf("poll: %w", api.ErrAccessDenied), pollStop}, + + // A server verdict we must respect: retrying an identical request cannot + // change any of these, and looping on one would hang the installer. + {"400 unrecognized", &api.APIError{StatusCode: 400, Body: `{"error":"invalid_grant"}`}, pollStop}, + {"401", &api.APIError{StatusCode: 401}, pollStop}, + {"403", &api.APIError{StatusCode: 403}, pollStop}, + {"404 no such endpoint", &api.APIError{StatusCode: 404}, pollStop}, + {"426 upgrade required", &api.UpgradeRequiredError{MinVersion: "1.2.3"}, pollStop}, + {"426 wrapped", fmt.Errorf("poll: %w", &api.UpgradeRequiredError{}), pollStop}, + {"operator cancelled", context.Canceled, pollStop}, + {"operator cancelled wrapped", fmt.Errorf("POST /device/token: %w", context.Canceled), pollStop}, + + // Temporary: the server (or a proxy in front of it) is failing, and the + // human at the browser has done nothing wrong. + {"500", &api.APIError{StatusCode: 500}, pollRetry}, + {"502 proxy", &api.APIError{StatusCode: 502}, pollRetry}, + {"503 deploying", &api.APIError{StatusCode: 503}, pollRetry}, + {"504 gateway timeout", &api.APIError{StatusCode: 504}, pollRetry}, + {"408 request timeout", &api.APIError{StatusCode: 408}, pollRetry}, + {"429 rate limited", &api.APIError{StatusCode: 429}, pollRetry}, + + // Never reached a verdict at all — the class cli#517 exists to keep alive. + {"dns failure", fmt.Errorf("POST /device/token: %w", &net.OpError{ + Op: "dial", Net: "tcp", Err: &net.DNSError{Err: "no such host", Name: "api.tracebloc.io"}}), pollRetry}, + {"connection refused", fmt.Errorf("POST /device/token: %w", + &net.OpError{Op: "dial", Net: "tcp", Err: errors.New("connect: connection refused")}), pollRetry}, + {"http client timeout", fmt.Errorf("POST /device/token: %w", context.DeadlineExceeded), pollRetry}, + {"undecodable body", errors.New(`device-token success response missing token (got "")`), pollRetry}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := classifyPollError(tc.err); got != tc.want { + t.Errorf("classifyPollError(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +// TestClassifyPollError_CoversPollTokenVocabulary derives the input domain from +// the PRODUCER instead of restating it: every error code RFC 8628 §3.5 lets the +// device-token endpoint return is driven through the real api.PollToken, and the +// error it actually produces is classified. A code the client stops mapping (or +// starts mapping differently) shows up here as a changed disposition — which a +// hand-written list of sentinels could not see. +func TestClassifyPollError_CoversPollTokenVocabulary(t *testing.T) { + want := map[string]pollDisposition{ + "authorization_pending": pollAgain, + "slow_down": pollSlower, + "expired_token": pollStop, + "access_denied": pollStop, + // Not in §3.5's happy vocabulary, but §3.5 defers to RFC 6749 §5.2 for + // the rest; those are refusals of the request, not transient conditions. + "invalid_request": pollStop, + "invalid_grant": pollStop, + } + for code, wantDisp := range want { + t.Run(code, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"` + code + `"}`)) + })) + t.Cleanup(srv.Close) + c := &api.Client{BaseURL: srv.URL, HTTP: srv.Client()} + _, err := c.PollToken(context.Background(), "dc") + if err == nil { + t.Fatalf("PollToken returned no error for %q", code) + } + if got := classifyPollError(err); got != wantDisp { + t.Errorf("%q → %v, want %v (err=%v)", code, got, wantDisp, err) + } + }) + } +} + +// TestLogin_TransientFailureRetriesWithinWindow is the headline cli#517 fix: a +// backend blip mid-poll used to abort the whole sign-in (and with it an +// installer run that had already built a cluster). It must be ridden out. +func TestLogin_TransientFailureRetriesWithinWindow(t *testing.T) { + polls := pollBackend(t, func(w http.ResponseWriter, poll int) { + switch { + case poll <= 3: // three 503s in a row — a backend restart + w.WriteHeader(http.StatusServiceUnavailable) + default: + _, _ = w.Write([]byte(`{"token":"cat_ok"}`)) + } + }) + if _, err := runCmd(t, "login"); err != nil { + t.Fatalf("a transient backend failure must not abort sign-in, got: %v", err) + } + if *polls != 4 { + t.Errorf("polls = %d, want 4 (three 503s ridden out, then the token)", *polls) + } + cfg, _ := config.Load() + if cfg.Current().Token != "cat_ok" { + t.Errorf("stored token = %q, want cat_ok", cfg.Current().Token) + } +} + +// TestLogin_TerminalErrorStopsImmediately is the other half of the same +// contract: making transient errors retryable must NOT turn a refusal into a +// loop. A 400 the client can't map to a §3.5 sentinel is a refusal — one poll, +// then out. +func TestLogin_TerminalErrorStopsImmediately(t *testing.T) { + polls := pollBackend(t, func(w http.ResponseWriter, _ int) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"invalid_grant"}`)) + }) + _, err := runCmd(t, "login") + if err == nil { + t.Fatal("a refused device code must fail the sign-in") + } + if *polls != 1 { + t.Errorf("polls = %d, want 1 — a refusal must not be retried", *polls) + } + if !strings.Contains(err.Error(), "invalid_grant") { + t.Errorf("the refusal must be surfaced verbatim, got: %v", err) + } +} + +// TestLogin_AccessDeniedStopsImmediately: the user said no in the browser. +// Polling past that would ignore an explicit refusal. +func TestLogin_AccessDeniedStopsImmediately(t *testing.T) { + polls := pollBackend(t, func(w http.ResponseWriter, _ int) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"access_denied"}`)) + }) + if _, err := runCmd(t, "login"); err == nil { + t.Fatal("a denied sign-in must fail") + } + if *polls != 1 { + t.Errorf("polls = %d, want 1 — an explicit denial must not be retried", *polls) + } +} + +// TestLogin_TransientFailuresGiveUpAtTheCap: retrying is bounded. A backend +// that never answers must report ITSELF as the cause, not burn the code's whole +// window and then blame the user for being slow. +func TestLogin_TransientFailuresGiveUpAtTheCap(t *testing.T) { + polls := pollBackend(t, func(w http.ResponseWriter, _ int) { + w.WriteHeader(http.StatusServiceUnavailable) + }) + _, err := runCmd(t, "login") + if err == nil { + t.Fatal("an unreachable backend must eventually fail the sign-in") + } + // Written down independently of the constant: a cap of 1 would satisfy the + // equality below while restoring exactly the cli#517 behaviour this fixes — + // one failure, no retry. The loop must ride out at least a few. + if *polls < 5 { + t.Errorf("polls = %d — a transient failure must be retried several times before giving up", *polls) + } + if *polls != maxPollFailures { + t.Errorf("polls = %d, want %d (the consecutive-failure cap)", *polls, maxPollFailures) + } + if !strings.Contains(err.Error(), "couldn't reach the backend") { + t.Errorf("the give-up message must name the network as the cause, got: %v", err) + } +} + +// TestLogin_TransientFailureStreakResets: the cap counts CONSECUTIVE failures. +// A blip every few polls during a long human-paced wait must not accumulate +// into a give-up — that would re-introduce cli#517 on a flaky link. +func TestLogin_TransientFailureStreakResets(t *testing.T) { + polls := pollBackend(t, func(w http.ResponseWriter, poll int) { + switch { + case poll >= 3*maxPollFailures: + _, _ = w.Write([]byte(`{"token":"cat_ok"}`)) + case poll%2 == 0: // every other poll fails — never maxPollFailures in a row + w.WriteHeader(http.StatusBadGateway) + default: + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"authorization_pending"}`)) + } + }) + if _, err := runCmd(t, "login"); err != nil { + t.Fatalf("an intermittent failure must not exhaust the cap, got: %v", err) + } + if *polls != 3*maxPollFailures { + t.Errorf("polls = %d, want %d", *polls, 3*maxPollFailures) + } +} + +// TestLogin_ExpiredNamesTheWindow (cli#517 §4): "the sign-in code expired" with +// no duration reads as an instant failure to a user who stepped away. The +// message must name the window the server advertised. +func TestLogin_ExpiredNamesTheWindow(t *testing.T) { + pollBackend(t, func(w http.ResponseWriter, _ int) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"expired_token"}`)) + }) + _, err := runCmd(t, "login") + if err == nil { + t.Fatal("an expired code must fail the sign-in") + } + if !strings.Contains(err.Error(), "10 minutes") { + t.Errorf("the expiry message must name the 600s window as 10 minutes, got: %v", err) + } +} + +// TestSignInWindow renders expires_in as the clause the copy carries. Derived +// from the server's value, never hardcoded, so it can't outlive a TTL change. +func TestSignInWindow(t *testing.T) { + cases := []struct { + expiresIn int + want string + }{ + {600, " — sign-in codes are valid for 10 minutes"}, + {60, " — sign-in codes are valid for 1 minute"}, + {300, " — sign-in codes are valid for 5 minutes"}, + {90, " — sign-in codes are valid for 1m30s"}, + {0, ""}, // server didn't say — say nothing rather than guess + {-1, ""}, // ditto + } + for _, tc := range cases { + if got := signInWindow(tc.expiresIn); got != tc.want { + t.Errorf("signInWindow(%d) = %q, want %q", tc.expiresIn, got, tc.want) + } + } +} + +// TestCopyCatalogSeesTheSignInStrings guards the guard: the copy catalog +// harvests string LITERALS passed to errors.New / fmt.Errorf / the Printer, so +// composing a message inside a helper (and handing the helper a variable) drops +// it silently out of the catalog — the completeness backstop goes on passing +// while the copy it exists to inventory is invisible. These sentences must stay +// literal arguments; assembling them here proves they still are. +func TestCopyCatalogSeesTheSignInStrings(t *testing.T) { + catalog := strings.Join(harvestMessages(t), "\n") + for _, want := range []string{ + "the sign-in code expired%s", + "the sign-in code expired before it was approved%s", + "sign-in was denied in the browser", + "%w. Run `tracebloc login` to start a new one", + "— sign-in codes are valid for %s", // the harvest trims leading space + "couldn't reach the backend to finish signing in", + } { + if !strings.Contains(catalog, want) { + t.Errorf("copy %q is not reachable by the catalog harvest — keep it a literal argument "+ + "of errors.New/fmt.Errorf, not a variable built inside a helper", want) + } + } +} + +// TestSignInAdvice_ContradictsNobody (cli#517 §3): standalone, the CLI names the +// command that starts a fresh sign-in. Under the installer it must NOT — a bare +// `tracebloc login` there leaves the client mint and the Helm install undone, +// and the installer prints its own, correct next step a line later. +func TestSignInAdvice_ContradictsNobody(t *testing.T) { + t.Run("standalone names the command", func(t *testing.T) { + t.Setenv("TRACEBLOC_INSTALLER", "") + err := terminalSignInError(api.ErrExpiredToken, " — sign-in codes are valid for 10 minutes") + if !strings.Contains(err.Error(), "tracebloc login") { + t.Errorf("a hand-run login should say how to retry, got: %v", err) + } + }) + t.Run("under the installer names nothing", func(t *testing.T) { + t.Setenv("TRACEBLOC_INSTALLER", "1") + err := terminalSignInError(api.ErrExpiredToken, " — sign-in codes are valid for 10 minutes") + if strings.Contains(err.Error(), "tracebloc login") { + t.Errorf("under the installer `tracebloc login` is wrong advice, got: %v", err) + } + if !strings.Contains(err.Error(), "expired") || !strings.Contains(err.Error(), "10 minutes") { + t.Errorf("suppressing the advice must not suppress the FACT, got: %v", err) + } + }) +} + +// TestLogin_InstallerContextSuppressesTheCliAdvice pins the same rule through +// the command, not just the helper — the env var has to reach the message a +// user actually sees. +func TestLogin_InstallerContextSuppressesTheCliAdvice(t *testing.T) { + pollBackend(t, func(w http.ResponseWriter, _ int) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"expired_token"}`)) + }) + t.Setenv("TRACEBLOC_INSTALLER", "1") + _, err := runCmd(t, "login") + if err == nil { + t.Fatal("an expired code must fail the sign-in") + } + if strings.Contains(err.Error(), "tracebloc login") { + t.Errorf("under the installer the CLI must not tell the user to re-run login, got: %v", err) + } + if !strings.Contains(err.Error(), "10 minutes") { + t.Errorf("the window must still be named under the installer, got: %v", err) + } +} + func TestLogout(t *testing.T) { // logout now revokes server-side (cli#112) — route it at a stub, not prod. var revoked bool diff --git a/internal/cli/client.go b/internal/cli/client.go index cd662a4..d7246ee 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -39,8 +39,9 @@ var readInClusterClient = cluster.DiscoverInClusterClient // The single-machine CLI (RFC-0001 §7.10) owns exactly one client, so there is // nothing to *select*: `client use` is withdrawn, and `client list` is hidden // (kept callable for the installer's one-client-per-machine pre-flight, off the -// user-facing surface). `create` provisions this machine's client; offboarding -// is the top-level `tracebloc delete`. +// user-facing surface). `create` points this machine at its client — adopting +// the one already on the cluster, which is the supported repoint (#515) — +// and offboarding is the top-level `tracebloc delete`. func newClientCmd() *cobra.Command { cmd := &cobra.Command{ Use: "client", @@ -63,15 +64,31 @@ func newClientCreateCmd() *cobra.Command { var yes bool cmd := &cobra.Command{ Use: "create", - Short: "Provision a tracebloc client for this machine (auto-named; no flags required)", - // HIDDEN: provisioning is the installer's job — provision.sh calls this with - // zero flags (cli#137). It stays fully callable (including `--help`, so the - // installer's capability probe still works), but is kept off the user-facing - // surface: a human running `client create` STANDALONE mints a client the - // installer never deploys — an orphaned "phantom" (backend#970). Mirrors the - // hidden `list`; leaves `tracebloc client` showing only the user-useful `status`. - Hidden: true, - Args: cobra.NoArgs, + Short: "Point this machine at its tracebloc client — adopts the one already on this cluster", + Long: `Point this machine at its tracebloc client. + +Keyed on the cluster your kubeconfig reaches: if a tracebloc client already runs +there, this ADOPTS it — no prompt, no new credential, no duplicate — which is how +you repoint a machine whose active client went stale. On a cluster that runs no +client yet it provisions a new one, and asks first. + +Provisioning a brand-new machine is normally the installer's job — it calls this +for you, with no flags.`, + // WAS HIDDEN (backend#970), and the reason still stands: a human running + // this STANDALONE on a cluster with no client mints one the installer never + // deploys — an orphaned "phantom". Hiding it was never what prevented that, + // though; the mint-path guards below are, and they are untouched: + // • on a TTY, the review + `Provision this client?` confirm (which a + // re-run on an already-registered cluster never reaches — it adopts + // before the prompt, so the repoint stays zero-friction); + // • off a TTY, a hard refusal without --yes/--credential-file, so a pipe + // or CI can never mint silently. + // What hiding DID cost is #515: the §7.3 "your active client runs on another + // machine" error had no supported way back, because the one command that + // repoints a machine was unlisted. Advice pointing at a hidden command is + // not advice, so it is listed now — described by what it does for a user + // (adopt/repoint) rather than by the installer's use of it. + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { return runClientCreate(cmd.Context(), printerFor(cmd), clientPrompter(), clientCreateOpts{name: name, location: location, kubeconfigPath: kubeconfigPath, contextOverride: contextOverride, credentialFile: credentialFile, yes: yes}) @@ -747,21 +764,106 @@ func runClientList(ctx context.Context, p *ui.Printer) error { } p.Section("Clients in your account") active := cfg.Current().ActiveClientID + // #515: "active" is a LOCAL POINTER, and this listing used to render it as + // "(active — this machine)" — a claim about location it never checked. When + // the pointer is stale that label sits next to a client provably not on the + // cluster this machine reaches, which is the state the whole ticket is about. + // Read the cluster anchor (kube-system UID, the §7.2 identity `client create` + // keys on) and mark residency separately from selection. A failed read is + // three-valued on purpose: unknown is not "elsewhere", so the marker then + // claims nothing about where anything runs. + clusterID, cidErr := readClusterID(ctx, cluster.KubeconfigOptions{}) + hereKnown := cidErr == nil && clusterID != "" + activeElsewhere, anyHere := false, false for _, c := range clients { - marker := "" - if strconv.Itoa(c.ID) == active { - marker = " (active — this machine)" + isActive := strconv.Itoa(c.ID) == active + res := residencyOf(hereKnown, clusterID, c.ClusterID) + if isActive && res == resElsewhere { + activeElsewhere = true } - p.Field(strconv.Itoa(c.ID)+marker, + if res == resHere { + anyHere = true + } + p.Field(strconv.Itoa(c.ID)+clientListMarker(isActive, res), fmt.Sprintf("%s state=%s namespace=%s location=%s", c.Name, clientStateLabel(c.Status), c.Namespace, c.Location)) } // §7.3: separate "selected" (this machine's local pointer) from "connected" // (the backend's last-heartbeat state) so a stale pointer is visible. p.Hintf("\"active\" is this machine's selected client; state is its last reported status to tracebloc.") + switch { + case activeElsewhere && anyHere: + // The exact state #515 describes, and the one supported way out of it: + // re-running create on a cluster that already hosts a client adopts it — + // no prompt, no new credential (§7.2). anyHere is what earns the phrase + // "the client that IS there": without a row we KNOW is on this cluster, + // `client create` would fall through to the mint path and produce the + // phantom backend#970 is about (Bugbot). + p.Hintf("Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create", launcher()) + case activeElsewhere: + // The pointer is provably wrong, but no listed client is provably here — + // either none is, or the ones that might be carry no anchor to prove it + // (resUnknown). Both are "we can't name a target", so name none: send + // them to the command whose whole job is to say what's on this cluster + // rather than advertise a repoint that may have nothing to adopt. + p.Hintf("Your active client is not on the cluster your kubeconfig reaches, and no client here is confirmed. Check your kubeconfig context, then run: %s doctor", launcher()) + } return nil } +// residency answers "does this client run on the cluster the kubeconfig +// reaches" in THREE values, because two of them are absences and an absence is +// never a "no" (Bugbot, #515). +type residency int + +const ( + // resUnknown: we cannot tell. Either the local cluster anchor was + // unreadable (no kubeconfig, unreachable API server, RBAC on kube-system), + // or the CLIENT carries no anchor — `ProvisionedClient.ClusterID` is empty + // on legacy / not-yet-backfilled records (api/client.go), and a record that + // never learned where it lives is not a record that lives elsewhere. + resUnknown residency = iota + resHere + resElsewhere +) + +// residencyOf compares the local cluster anchor with a client's, keeping both +// missing-anchor cases at resUnknown. Collapsing either into "elsewhere" would +// print "NOT on the cluster your kubeconfig reaches" — and the repoint hint — +// next to a legacy client that may be running on this very machine. +func residencyOf(hereKnown bool, localAnchor, clientAnchor string) residency { + if !hereKnown || clientAnchor == "" { + return resUnknown + } + if clientAnchor == localAnchor { + return resHere + } + return resElsewhere +} + +// clientListMarker renders one row's suffix in `client list`, keeping SELECTION +// (this machine's local pointer) and RESIDENCY (where the client actually runs) +// as two separate facts (#515). Under resUnknown the marker degrades to bare +// "(active)" — which says only what the local config actually knows — and +// claims nothing about location in either direction. +func clientListMarker(isActive bool, res residency) string { + switch { + case res == resUnknown: + if isActive { + return " (active)" + } + return "" + case isActive && res == resHere: + return " (active — on this cluster)" + case isActive: + return " (active — NOT on the cluster your kubeconfig reaches)" + case res == resHere: + return " (on this cluster)" + default: + return "" + } +} + // setActiveClient points this env's profile at c, caching its namespace and // display name alongside the id so the data commands can bind to the active // client's cluster (§7.3) without a backend round-trip. Callers Save() after. diff --git a/internal/cli/client_test.go b/internal/cli/client_test.go index fab9bd4..d771124 100644 --- a/internal/cli/client_test.go +++ b/internal/cli/client_test.go @@ -1470,19 +1470,18 @@ func TestClientStatus_WaitCtrlCIsSilent(t *testing.T) { } func TestClientSubcommandVisibility(t *testing.T) { - // `create` and `list` are installer-internal — Hidden so a user isn't invited to - // run them (a standalone `tracebloc client create` mints a client the installer - // never deploys, i.e. an orphaned phantom, backend#970). `status` stays - // user-visible. Hidden != disabled: all remain runnable (the installer still - // invokes create/list). + // `create` is VISIBLE since #515: it is the supported way to repoint a machine + // whose active client went stale, and the §7.3 error now names it — advice + // pointing at a hidden command is not advice. `list` stays installer-internal. + // `status` stays user-visible. Hidden != disabled: all remain runnable. hidden := map[string]bool{} runnable := map[string]bool{} for _, c := range newClientCmd().Commands() { hidden[c.Name()] = c.Hidden runnable[c.Name()] = c.RunE != nil } - if !hidden["create"] { - t.Error("client create must be Hidden (installer-internal; standalone mints a phantom)") + if hidden["create"] { + t.Error("client create must be visible — the #515 repoint advice names it") } if !hidden["list"] { t.Error("client list must stay Hidden") @@ -1491,7 +1490,248 @@ func TestClientSubcommandVisibility(t *testing.T) { t.Error("client status must stay user-visible") } if !runnable["create"] { - t.Error("hidden create must still be runnable (the installer invokes it)") + t.Error("create must still be runnable (the installer invokes it)") + } +} + +// Unhiding `create` must not reopen backend#970: hiding it was never what +// stopped a standalone run from minting a phantom — these two guards are, and +// they have to survive the visibility change. Off a TTY (pr == nil) with no +// --yes and no --credential-file, a fresh mint is REFUSED; nothing is posted. +func TestClientCreate_UnhiddenStillRefusesSilentMint(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + posted = true + } + _, _ = w.Write([]byte(`[]`)) // no clients on the account → a mint, not an adopt + }) + signInAs(t, "Lab", "lab@example.com") + var out bytes.Buffer + err := runClientCreate(context.Background(), ui.New(&out), nil, clientCreateOpts{}) + if err == nil { + t.Fatal("a non-interactive bare `client create` must refuse to mint") + } + if !strings.Contains(err.Error(), "refusing to provision non-interactively") { + t.Errorf("want the pipe refusal, got: %v", err) + } + if posted { + t.Error("nothing may be provisioned by a refused run") + } +} + +// The TTY half of the same guard: on a terminal a fresh mint asks first, and a +// "no" provisions nothing. (The repoint itself never reaches this prompt — an +// already-registered cluster adopts before it, covered by the adopt tests.) +func TestClientCreate_UnhiddenStillPromptsBeforeMinting(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + posted = true + } + _, _ = w.Write([]byte(`[]`)) + }) + signInAs(t, "Lab", "lab@example.com") + no := false + var out bytes.Buffer + if err := runClientCreate(context.Background(), ui.New(&out), &fakePrompter{confirm: &no}, clientCreateOpts{}); err != nil { + t.Fatalf("declining is a clean exit, got: %v", err) + } + if posted { + t.Error("a declined confirm must provision nothing") + } +} + +// #515: `client list` used to label the active pointer "(active — this machine)" +// without ever checking where that client runs, so a stale pointer read as +// confirmation. Selection and residency are now two separate facts, keyed on the +// cluster anchor (§7.2). +func TestClientListMarker(t *testing.T) { + cases := []struct { + name string + isActive bool + res residency + want string + }{ + {"residency unknown, active → claims only selection", true, resUnknown, " (active)"}, + {"residency unknown, other → no claim", false, resUnknown, ""}, + {"active and here", true, resHere, " (active — on this cluster)"}, + {"active but elsewhere", true, resElsewhere, " (active — NOT on the cluster your kubeconfig reaches)"}, + {"here but not selected", false, resHere, " (on this cluster)"}, + {"elsewhere and not selected", false, resElsewhere, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := clientListMarker(c.isActive, c.res); got != c.want { + t.Errorf("clientListMarker(%v,%v) = %q, want %q", c.isActive, c.res, got, c.want) + } + }) + } + // The specific claim #515 calls out: unknown residency must never let a row + // assert — or deny — that it is here. + for _, isActive := range []bool{true, false} { + if strings.Contains(clientListMarker(isActive, resUnknown), "this cluster") { + t.Errorf("isActive=%v: unknown residency must claim nothing about location", isActive) + } + } +} + +// Bugbot (#515): residency has to stay THREE-valued on both sides of the +// comparison. An unreadable LOCAL anchor was already handled; a client whose OWN +// anchor is empty — legacy / not-yet-backfilled, per api.ProvisionedClient — +// was being forced to "elsewhere", which told the owner of a perfectly local +// legacy client that it is not on this cluster. +func TestResidencyOf(t *testing.T) { + cases := []struct { + name string + hereKnown bool + local, clnt string + want residency + }{ + {"local anchor unreadable", false, "", "uid-A", resUnknown}, + {"client anchor empty (legacy record)", true, "uid-A", "", resUnknown}, + {"both unknown", false, "", "", resUnknown}, + {"anchors match", true, "uid-A", "uid-A", resHere}, + {"anchors differ", true, "uid-A", "uid-B", resElsewhere}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := residencyOf(c.hereKnown, c.local, c.clnt); got != c.want { + t.Errorf("residencyOf(%v,%q,%q) = %v, want %v", c.hereKnown, c.local, c.clnt, got, c.want) + } + }) + } +} + +// End to end: a legacy ACTIVE client with no anchor, on a machine whose cluster +// anchor reads fine, must not be accused of running elsewhere — and must not +// trigger the repoint hint, which would be advice to fix a non-problem. +func TestClientList_LegacyClientWithNoAnchorIsNotAccused(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"first_name":"legacy","namespace":"legacy-ns"}]`)) // no cluster_id + }) + stubClusterID(t, "uid-HERE", nil) + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if strings.Contains(got, "NOT on the cluster") { + t.Errorf("a client with no anchor has UNKNOWN residency, not elsewhere:\n%s", got) + } + if strings.Contains(got, "client create") { + t.Errorf("no mismatch is known, so the repoint must not be advised:\n%s", got) + } + if !strings.Contains(got, "1 (active)") { + t.Errorf("want the bare selection marker:\n%s", got) + } +} + +// End-to-end: with the anchor readable, the row that matches the LOCAL cluster +// is marked as such — even when the pointer names a different one — and the +// listing names the repoint. +func TestClientList_MarksTheClientOnThisCluster(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"first_name":"stale","namespace":"stale-ns","cluster_id":"uid-OTHER"},` + + `{"id":2,"first_name":"here","namespace":"lukas-02","cluster_id":"uid-HERE"}]`)) + }) + stubClusterID(t, "uid-HERE", nil) + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" // the pointer names the client that is NOT here + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if !strings.Contains(got, "1 (active — NOT on the cluster your kubeconfig reaches)") { + t.Errorf("the stale active pointer must be marked as not here:\n%s", got) + } + if !strings.Contains(got, "2 (on this cluster)") { + t.Errorf("the client that IS here must be marked:\n%s", got) + } + if !strings.Contains(got, "client create") { + t.Errorf("a mismatch must name the repoint:\n%s", got) + } +} + +// Bugbot (#515): the repoint hint says "the client that IS there", which is only +// true if some row is provably here. With the active client elsewhere and NOTHING +// confirmed on this cluster, `client create` would fall through to the MINT path +// and produce exactly the phantom backend#970 exists to prevent — so the advice +// must not be given. +func TestClientList_MismatchWithNoLocalClient_DoesNotPushCreate(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"first_name":"stale","namespace":"stale-ns","cluster_id":"uid-OTHER"},` + + `{"id":2,"first_name":"third","namespace":"third-ns","cluster_id":"uid-THIRD"}]`)) + }) + stubClusterID(t, "uid-HERE", nil) // this cluster hosts NEITHER + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if strings.Contains(got, "client create") { + t.Errorf("no client is confirmed here — advising the repoint would push a MINT:\n%s", got) + } + if !strings.Contains(got, "no client here is confirmed") { + t.Errorf("the mismatch is still real and must be reported, just without a target:\n%s", got) + } + // The mismatch itself must still be visible on the row. + if !strings.Contains(got, "NOT on the cluster your kubeconfig reaches") { + t.Errorf("the stale active pointer must still be marked:\n%s", got) + } +} + +// The unreadable-anchor path end to end: no cluster reachable ⇒ no row claims a +// location, and the mismatch hint stays silent (we cannot know there is one). +func TestClientList_UnreadableAnchorClaimsNoLocation(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { // stubs readClusterID to an error + _, _ = w.Write([]byte(`[{"id":1,"first_name":"stale","namespace":"stale-ns","cluster_id":"uid-OTHER"}]`)) + }) + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if !strings.Contains(got, "1 (active)") { + t.Errorf("want the bare selection marker when the anchor is unreadable:\n%s", got) + } + if strings.Contains(got, "this cluster") || strings.Contains(got, "kubeconfig reaches") { + t.Errorf("an unreadable anchor must claim nothing about location:\n%s", got) } } diff --git a/internal/cli/cluster.go b/internal/cli/cluster.go index 6d331a9..b75b8e2 100644 --- a/internal/cli/cluster.go +++ b/internal/cli/cluster.go @@ -164,7 +164,10 @@ func runClusterInfo( // installed on this cluster". A binding miss gets the §7.3 // "runs elsewhere" explanation, same as the data commands. if errors.Is(err, cluster.ErrNoParentRelease) { - return binding.explain(&exitError{code: exitNoWorkspace, err: &noParentReleaseError{err}}) + return binding.explain(ctx, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{ + err: err, + probe: &clusterProbe{cs: cs, serverURL: resolved.ServerURL}, + }}) } return &exitError{code: exitNoWorkspace, err: err} } diff --git a/internal/cli/clustertarget.go b/internal/cli/clustertarget.go index b760867..1ebcb68 100644 --- a/internal/cli/clustertarget.go +++ b/internal/cli/clustertarget.go @@ -5,11 +5,13 @@ import ( "errors" "fmt" "strings" + "time" "k8s.io/client-go/kubernetes" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/installer" "github.com/tracebloc/cli/internal/ui" ) @@ -19,11 +21,36 @@ import ( // release, an API/RBAC list failure, or an ambiguous multiple-release match. // §7.3 uses it to turn an active-client binding miss into a clear "runs on // another machine" message; the other failures keep their own diagnostics. -type noParentReleaseError struct{ err error } +// +// probe carries the read-only handles explain needs to say what IS on the +// reached cluster before advising (#515). It is attached wherever the error is +// built — both sites already hold a clientset and the resolved server URL — and +// travels ON the error rather than through the call signature so a caller can +// never hand explain a clientset for a DIFFERENT cluster than the one that +// missed. A nil probe (a synthesised error, a resolveClusterTargetFn test +// double) means "we could not look", and explain then claims nothing. +type noParentReleaseError struct { + err error + probe *clusterProbe +} func (e *noParentReleaseError) Error() string { return e.err.Error() } func (e *noParentReleaseError) Unwrap() error { return e.err } +// clusterProbe is the pair explain needs to diagnose before advising: a +// clientset for the cluster the kubeconfig actually reached, and that cluster's +// server URL (which isLocalServerURL judges). +type clusterProbe struct { + cs kubernetes.Interface + serverURL string +} + +// explainScanTimeout bounds the naming-only cluster scan explain runs on the +// §7.3 error path. The scan only makes the message better, so it must never +// make the failure slower than the failure itself: past this, explain falls +// back to the message it would have printed without looking. +const explainScanTimeout = 5 * time.Second + // loadClusterFn / newClientsetFn are the kubeconfig-load + clientset-build // seams every command that reaches a cluster goes through — resolveClusterTarget // (data ingest/list/delete), runClusterInfo, and runClusterDoctor. Production @@ -87,7 +114,10 @@ func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.Kubec // "runs elsewhere" rewrite; an API/RBAC list failure or an // ambiguous multiple-release match keeps its own message. if errors.Is(err, cluster.ErrNoParentRelease) { - return nil, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{err}} + return nil, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{ + err: err, + probe: &clusterProbe{cs: cs, serverURL: resolved.ServerURL}, + }} } return nil, &exitError{code: exitNoWorkspace, err: err} } @@ -214,7 +244,18 @@ func (b activeClientBinding) allowScan() bool { return !b.applied && !b.explicit // came from the active-client binding: the cluster the kubeconfig reaches // doesn't host that client. Non-binding errors (and PVC-missing, where the // release *was* found) pass through unchanged. -func (b activeClientBinding) explain(err error) error { +// +// DIAGNOSE BEFORE ADVISING (#515). The shipped §7.3 sentence named no way back: +// it offered --namespace without ever saying WHICH namespace, so a user on a +// healthy local install had no supported recovery. explain now spends one +// naming-only cluster scan — the same read discoverRelease already spends +// purely to write a better message — and says what is actually here. +// +// This changes what the CLI SAYS, never what it TARGETS: allowScan() stays +// false, so a binding miss still never silently retargets to some other +// machine's client (§7.5). The scan's result reaches the user as text they must +// act on, which is the whole difference. +func (b activeClientBinding) explain(ctx context.Context, err error) error { if !b.applied { return err } @@ -226,8 +267,88 @@ func (b activeClientBinding) explain(err error) error { if handle == "" { handle = b.namespace } - return &exitError{code: exitNoWorkspace, err: fmt.Errorf( - "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; "+ - "run this command there, or override with --namespace/--context", - handle, b.namespace)} + // errors.New, not %w: the rewrite deliberately REPLACES the discovery error + // rather than wrapping it, so the §7.3 guidance is the whole message and the + // raw "no release in namespace X" doesn't trail it. That was already true of + // the fmt.Errorf this replaced — the exit code (exitNoWorkspace, on the + // *exitError above) is the machine-readable contract here, not the chain. + // Wrapping would also make the result re-match errors.As(*noParentReleaseError) + // and so re-explainable, which nothing wants. + return &exitError{code: exitNoWorkspace, + err: errors.New(repointMessage(handle, b.namespace, surveyCluster(ctx, npr.probe)))} +} + +// clientSurvey is what explain managed to learn about the reached cluster +// before advising. +// +// looked distinguishes "we scanned and the cluster hosts none" from "we could +// not scan at all" (no probe on the error, or the cluster-wide list failed — +// RBAC, a timeout, an unreachable API server). Collapsing the two would let an +// absence of evidence print as evidence of absence: the CLI would tell a user +// with a perfectly healthy client that nothing is running here. When looked is +// false the message says nothing about the cluster's contents at all. +type clientSurvey struct { + looked bool + namespaces []string + local bool // the kubeconfig's server is THIS machine (isLocalServerURL) +} + +// surveyCluster runs cluster.FindClientNamespaces FOR NAMING ONLY — nothing in +// this path changes the namespace anything targets. A nil probe (synthesised +// error / test double) or a failed scan both return a survey that looked at +// nothing, so explain falls back to the message it printed before #515. +func surveyCluster(ctx context.Context, probe *clusterProbe) clientSurvey { + if probe == nil || probe.cs == nil { + return clientSurvey{} + } + ctx, cancel := context.WithTimeout(ctx, explainScanTimeout) + defer cancel() + found, err := cluster.FindClientNamespaces(ctx, probe.cs) + if err != nil { + return clientSurvey{} + } + return clientSurvey{looked: true, namespaces: found, local: isLocalServerURL(probe.serverURL)} +} + +// repointMessage is the §7.3 error text, branched on what surveyCluster found. +// Pure (no I/O) so every branch is unit-testable as text. +// +// - exactly one client on a LOCAL cluster — a cluster that IS this machine — +// name it and offer the repoint. `client create` re-run on a cluster that +// already hosts a client adopts it: no prompt, no new credential (§7.2). +// - any client on a remote/shared cluster (or several anywhere) — name the +// namespaces and offer ONLY --namespace. Never `client create` here: that +// is the §7.5 boundary, and on a shared cluster the client we found may well +// be a colleague's. +// - none, scan clean — say so, and point at the installer, which is then the +// correct advice rather than a guess. +// - could not look — the pre-#515 sentence, unchanged. We make no claim. +// +// Each branch is ONE format literal rather than a concatenation, so the whole +// sentence lands in the copy catalog (zz-all-strings harvests literal arguments; +// a `+`-joined message is only ever half-visible there) and can be reviewed as +// the user reads it. +func repointMessage(handle, boundNS string, s clientSurvey) string { + switch { + case !s.looked: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context", + handle, boundNS) + case len(s.namespaces) == 0: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context.\n\nNo tracebloc client is running on this cluster either — if this machine should have one, set one up: %s", + handle, boundNS, installer.Cmd) + case len(s.namespaces) == 1 && s.local: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client IS running on this machine, in namespace %q.\n Point this machine at it: %s client create\n (this cluster already runs a client, so it adopts it — no new credential)\n Or target it just this once: --namespace %s", + handle, boundNS, s.namespaces[0], launcher(), s.namespaces[0]) + case len(s.namespaces) == 1: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client is running on this cluster, in namespace %q.\n Target it just this once: --namespace %s", + handle, boundNS, s.namespaces[0], s.namespaces[0]) + default: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\ntracebloc clients are running on this cluster, in namespaces: %s.\n Target one just this once: --namespace %s", + handle, boundNS, strings.Join(s.namespaces, ", "), s.namespaces[0]) + } } diff --git a/internal/cli/clustertarget_test.go b/internal/cli/clustertarget_test.go index e9e394d..1772496 100644 --- a/internal/cli/clustertarget_test.go +++ b/internal/cli/clustertarget_test.go @@ -18,6 +18,7 @@ import ( "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/installer" "github.com/tracebloc/cli/internal/ui" ) @@ -162,12 +163,13 @@ func TestBindActiveClientNamespace_NoActiveClient(t *testing.T) { } func TestActiveClientBinding_Explain(t *testing.T) { - noRelease := &exitError{code: 4, err: &noParentReleaseError{errors.New("no release")}} + noRelease := &exitError{code: 4, err: &noParentReleaseError{err: errors.New("no release")}} pvcMissing := &exitError{code: 4, err: errors.New("shared PVC not bound")} + ctx := context.Background() // Applied + "no release here" → rewritten to the §7.3 guidance. bound := activeClientBinding{applied: true, name: "gpu-box-01", namespace: "gpu-box-01"} - got := bound.explain(noRelease) + got := bound.explain(ctx, noRelease) if got == noRelease { t.Fatal("expected a rewritten error") } @@ -180,16 +182,181 @@ func TestActiveClientBinding_Explain(t *testing.T) { } // Applied but a PVC failure (release WAS found) → pass through untouched. - if bound.explain(pvcMissing) != pvcMissing { + if bound.explain(ctx, pvcMissing) != pvcMissing { t.Error("PVC-missing error should not be rewritten") } // Not applied → always pass through. - if (activeClientBinding{}).explain(noRelease) != noRelease { + if (activeClientBinding{}).explain(ctx, noRelease) != noRelease { t.Error("unbound explain should pass the error through") } } +// #515 — the three branches of the §7.3 message, as text. A binding miss used to +// name --namespace without ever saying WHICH namespace; each branch below is the +// answer explain now derives from what is actually on the reached cluster. +// +// repointMessage is pure, so this pins the exact wording; the surveyCluster +// tests below pin that the survey fed to it is honest. +func TestRepointMessage_Branches(t *testing.T) { + const handle, boundNS = "gpu-box-01", "gpu-box-01" + lead := `active client "gpu-box-01" runs on another machine — namespace "gpu-box-01" isn't on the cluster your kubeconfig points at` + + t.Run("one client on a local cluster offers the repoint", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{looked: true, namespaces: []string{"lukas-02"}, local: true}) + for _, want := range []string{ + lead, + `A tracebloc client IS running on this machine, in namespace "lukas-02".`, + "client create", + "no new credential", + "--namespace lukas-02", + } { + if !strings.Contains(got, want) { + t.Errorf("missing %q:\n%s", want, got) + } + } + }) + + t.Run("remote cluster never suggests client create", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{looked: true, namespaces: []string{"colleague-07"}, local: false}) + if !strings.Contains(got, "colleague-07") || !strings.Contains(got, "--namespace colleague-07") { + t.Errorf("remote branch must name the namespace and offer --namespace:\n%s", got) + } + // The §7.5 boundary: on a shared cluster the client we found may be + // someone else's, so the repoint must NOT be advertised. + if strings.Contains(got, "client create") { + t.Errorf("remote/shared cluster must never suggest `client create`:\n%s", got) + } + }) + + t.Run("several clients name them all and offer only --namespace", func(t *testing.T) { + // Local or not: with more than one client here, "point this machine at + // it" has no unambiguous "it" — so this stays the --namespace branch even + // on a local cluster. + for _, local := range []bool{true, false} { + got := repointMessage(handle, boundNS, clientSurvey{looked: true, namespaces: []string{"alpha", "beta"}, local: local}) + if !strings.Contains(got, "alpha, beta") || !strings.Contains(got, "--namespace alpha") { + t.Errorf("local=%v: multi branch should list both and offer --namespace:\n%s", local, got) + } + if strings.Contains(got, "client create") { + t.Errorf("local=%v: ambiguous multi-client must not suggest `client create`:\n%s", local, got) + } + } + }) + + t.Run("clean scan finding nothing points at the installer", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{looked: true}) + for _, want := range []string{ + lead, + "--namespace/--context", + "No tracebloc client is running on this cluster either", + installer.Cmd, + } { + if !strings.Contains(got, want) { + t.Errorf("missing %q:\n%s", want, got) + } + } + }) + + t.Run("could not look claims nothing", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{}) + want := lead + "; run this command there, or override with --namespace/--context" + if got != want { + t.Errorf("unlooked message must stay the pre-#515 sentence exactly\n got: %q\nwant: %q", got, want) + } + // An absence of evidence must never print as evidence of absence. + if strings.Contains(got, "No tracebloc client is running") { + t.Errorf("a failed/absent scan must not claim the cluster is empty:\n%s", got) + } + }) +} + +// surveyCluster is the only thing standing between the message and a false +// claim, so each way of "we could not look" has to come back as looked=false. +func TestSurveyCluster_FailsClosed(t *testing.T) { + t.Run("nil probe", func(t *testing.T) { + if s := surveyCluster(context.Background(), nil); s.looked { + t.Errorf("a nil probe must not report as looked: %+v", s) + } + }) + + t.Run("nil clientset", func(t *testing.T) { + if s := surveyCluster(context.Background(), &clusterProbe{serverURL: "https://127.0.0.1:6550"}); s.looked { + t.Errorf("a probe with no clientset must not report as looked: %+v", s) + } + }) + + t.Run("scan forbidden", func(t *testing.T) { + cs := fake.NewSimpleClientset() + cs.PrependReactor("list", "deployments", func(ktesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("forbidden: cannot list deployments at the cluster scope") + }) + s := surveyCluster(context.Background(), &clusterProbe{cs: cs, serverURL: "https://127.0.0.1:6550"}) + if s.looked { + t.Errorf("an RBAC-refused scan must not report as looked: %+v", s) + } + }) + + t.Run("clean empty scan looked and found nothing", func(t *testing.T) { + s := surveyCluster(context.Background(), &clusterProbe{cs: fake.NewSimpleClientset(), serverURL: "https://127.0.0.1:6550"}) + if !s.looked || len(s.namespaces) != 0 { + t.Errorf("a clean empty scan is looked-with-nothing: %+v", s) + } + if !s.local { + t.Error("a loopback server URL must survey as local") + } + }) + + t.Run("finds the client and judges locality", func(t *testing.T) { + cs := fake.NewSimpleClientset(jmDep("lukas-02")) + s := surveyCluster(context.Background(), &clusterProbe{cs: cs, serverURL: "https://k8s.corp.example:6443"}) + if !s.looked || len(s.namespaces) != 1 || s.namespaces[0] != "lukas-02" { + t.Errorf("survey = %+v, want the one namespace", s) + } + if s.local { + t.Error("a corporate API server must not survey as local") + } + }) +} + +// End-to-end through the real resolve path: a binding miss on a LOCAL cluster +// that hosts the client elsewhere must NAME it — and must still not target it. +// This is the pairing that matters (§7.5): the namespace appears in the message +// and nowhere else. +func TestExplain_BindingMiss_NamesTheLocalClientWithoutRetargeting(t *testing.T) { + cs := fake.NewSimpleClientset(jmDep("lukas-02")) + origLoad, origCS := loadClusterFn, newClientsetFn + t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + return &cluster.ResolvedConfig{ + Namespace: o.Namespace, + ServerURL: "https://127.0.0.1:6550", + RestConfig: &rest.Config{}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { return cs, nil } + + binding := activeClientBinding{applied: true, name: "gpu-box-01", namespace: "stale-ns"} + target, err := resolveClusterTarget(context.Background(), nil, + cluster.KubeconfigOptions{Namespace: "stale-ns"}, binding, false, false) + if err == nil { + t.Fatal("a binding miss must still fail — this changes the message, not the target") + } + if target != nil { + t.Fatalf("no target may be resolved from a binding miss, got %+v", target) + } + got := binding.explain(context.Background(), err) + if !strings.Contains(got.Error(), "lukas-02") { + t.Errorf("the message must name the client that IS here:\n%s", got) + } + if !strings.Contains(got.Error(), "client create") { + t.Errorf("a single client on a local cluster must be offered the repoint:\n%s", got) + } + if ExitCodeFromError(got) != 4 { + t.Errorf("exit code = %d, want 4", ExitCodeFromError(got)) + } +} + // jmDep builds a chart-labeled jobs-manager Deployment in the given namespace, // for the fallback-scan tests (mirrors the cluster package's fixture). func jmDep(namespace string) *appsv1.Deployment { diff --git a/internal/cli/copy_catalog_test.go b/internal/cli/copy_catalog_test.go index 5826d4e..40405a9 100644 --- a/internal/cli/copy_catalog_test.go +++ b/internal/cli/copy_catalog_test.go @@ -140,29 +140,29 @@ func TestCopyCatalog(t *testing.T) { imgDir := imageDirLayout(t) txtDir := textDirLayout(t) tabularIngest := driveIngest(tabDir, "~/data/patients", map[string]string{ - "Do you want to ingest training or test data?": "train", - "Please name the dataset.": "hospital_train", - "Where is your data?": tabDir, - "Which task?": "tabular_classification", - "Which column holds the label?": "churned", + "Split:": "train", + "Name:": "hospital_train", + "Path:": tabDir, + "Task:": "tabular_classification", + "Label:": "churned", }) imageIngest := driveIngest(imgDir, "~/data/xray", map[string]string{ - "Do you want to ingest training or test data?": "train", - "Please name the dataset.": "xray_train", - "Where is your data?": imgDir, - "Which task?": "image_classification", - "Which column holds the label?": "label", - "Image resolution": "224x224", + "Split:": "train", + "Name:": "xray_train", + "Path:": imgDir, + "Task:": "image_classification", + "Label:": "label", + "Resolution:": "224x224", }) // Text family: text_classification shows the label question; the picker lists // every text task + blurb. (Self-supervised text — masked/causal LM, seq2seq // — skips the label step; that path is covered by the backstop.) textIngest := driveIngest(txtDir, "~/data/reviews", map[string]string{ - "Do you want to ingest training or test data?": "train", - "Please name the dataset.": "reviews_train", - "Where is your data?": txtDir, - "Which task?": "text_classification", - "Which column holds the label?": "label", + "Split:": "train", + "Name:": "reviews_train", + "Path:": txtDir, + "Task:": "text_classification", + "Label:": "label", }) // execIngest renders the run that follows the confirm — the three steps and // the final summary. printLocalSummary + submit.RenderSummary are the REAL @@ -211,7 +211,7 @@ func TestCopyCatalog(t *testing.T) { } dataIngestFile := doc( "tb data ingest — stage a dataset into your secure environment", - "What you see when you run `tb data ingest` with no flags: a short intro, a\nfour-step guided setup (intent, name, path, task) then the task-specific\nquestions, and — after you confirm — the run itself. The setup is\ndriven through the real flow for one task in each family (tabular, image, text)\nso the task-specific questions are visible; each core question prints as a\n`Step N of 4 · …` header, the task-specific ones (the label column, and extras\nlike resolution or schema) as their own header, the\nsupporting line beneath it, and the `?` line shows your answer. The run (shown\nonce, for tabular) is the three steps + the final summary as the CLI renders\nthem. Values passed as flags (--as, --task, a path, …) pre-fill the matching\nquestions rather than skipping them — guided mode always asks. The\nother tasks' extra questions (keypoints, label policy, time column),\nself-supervised text (which skips the label question), and the failure-summary\nwordings are in zz-all-strings.golden. The raw ingestor stream the CLI streams\nthrough (MySQL waits, the 📊 banner, per-validator lines) is the engine's own\nstdout — not CLI copy — so it isn't shown. (`tb ingest` is a hidden deprecated\nalias; `push` is a deprecated alias of the verb.)", + "What you see when you run `tb data ingest` with no flags: a short intro, a\nfour-step guided setup (intent, name, path, task) then the task-specific\nquestions, and — after you confirm — the run itself. The setup is\ndriven through the real flow for one task in each family (tabular, image, text)\nso the task-specific questions are visible; each core question prints as a\n`Step N of 4 · …` header, the task-specific ones (the label column, and extras\nlike resolution or schema) as their own header, the\nsupporting line beneath it, and the `?` line carries a short noun label\n(`Path:`, `Task:`) plus your answer. The run (shown\nonce, for tabular) is the three steps + the final summary as the CLI renders\nthem. Values passed as flags (--as, --task, a path, …) pre-fill the matching\nquestions rather than skipping them — guided mode always asks. The\nother tasks' extra questions (keypoints, label policy, time column),\nself-supervised text (which skips the label question), and the failure-summary\nwordings are in zz-all-strings.golden. The raw ingestor stream the CLI streams\nthrough (MySQL waits, the 📊 banner, per-validator lines) is the engine's own\nstdout — not CLI copy — so it isn't shown. (`tb ingest` is a hidden deprecated\nalias; `push` is a deprecated alias of the verb.)", []run{ {"tb data ingest # guided · tabular classification", tabularIngest}, {"tb data ingest # guided · image classification", imageIngest}, @@ -476,8 +476,8 @@ func quoteAll(in []string) []string { return out } -// catalogPrompter is the prompter seam's catalog double: it prints each question -// the way the terminal shows it ("? ") and returns a scripted +// catalogPrompter is the prompter seam's catalog double: it prints each prompt +// the way the terminal shows it ("?