From fef1dd3cd655105e8d9adc4f141acd9993c6f21d Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 08:37:39 +0200 Subject: [PATCH 01/15] =?UTF-8?q?feat(doctor):=20plain-language=20redesign?= =?UTF-8?q?=20=E2=80=94=20connected=20+=20ready,=20with=20concrete=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebuilds `tb doctor` around the one question a workspace owner has: is my secure environment connected to tracebloc and ready to run training — and if not, exactly what to do. The 9 Kubernetes-flavoured checks (release/chart/PVC/ pods/pull-secrets/proxy/Service-Bus/kubectl) collapse into two plain lines, and everything technical moves behind --verbose. Default view: Signed in as lukas@tracebloc.io Secure environment "lukas-test" Connected to tracebloc Ready to run training - No banner, no kubeconfig block, no numbers, no Kubernetes vocabulary. - Each health line expands on failure to the specific plain problem + ONE concrete action (a command, never a kubectl): stopped env -> "start Docker Desktop"; expired -> "tb login"; no env -> the one-line installer; results can't flow / component down -> email support with `tb doctor --diagnose`. - Not-connected degrades readiness honestly to "can't check" (the #354 reachability gate, folded in here). - Remedies resolve `tb` vs `tracebloc` like the home screen. - `tb doctor --verbose` keeps the full technical breakdown (kubeconfig + every granular check) for support. - `tb doctor --diagnose` now owns the redacted support bundle (moved off install-k8s.sh, which the user may not have on disk). summarizeDoctor (the roll-up) is unit-tested for every failure bucket; the render + verdict are covered via the doctorRunFn seam; no k8s vocabulary is allowed to leak into the default view (asserted). Supersedes #354 (doctor cascade) and #351 (requests-proxy wording) — this rewrites the same output and folds in their fixes. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 404 ++++++++++++++++------------ internal/cli/doctor_cluster_test.go | 142 ++++++---- internal/cli/doctor_test.go | 235 ++++++++-------- internal/cli/home_test.go | 4 +- internal/doctor/doctor.go | 4 +- 5 files changed, 447 insertions(+), 342 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 16c0482d..4fcbf25f 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -1,9 +1,13 @@ package cli import ( + "bytes" "context" "errors" + "fmt" "net/http" + "os" + "time" "github.com/spf13/cobra" @@ -14,67 +18,59 @@ import ( "github.com/tracebloc/cli/internal/ui" ) -// doctorRunFn is a test seam over doctor.Run (the cluster-side health sweep). -// Production runs the real checks; tests inject a fixed []doctor.Result so -// runClusterDoctor's render loop (the ✓/⚠/✖ switch + remedy hints) and its -// overall-verdict switch can be exercised with a controlled OK/Warn/Fail mix, -// without standing up a fake cluster that happens to produce that exact mix -// (the per-check logic has its own tests in internal/doctor). +// installCmd is the one-line installer we point people at when there's no +// secure environment on this machine, or a component needs reinstalling. Kept in +// one place so every remedy says the same thing. +const installCmd = "bash <(curl -fsSL https://tracebloc.io/i.sh)" + +// doctorRunFn is a test seam over doctor.Run (the cluster-side probe). Tests +// inject a fixed []doctor.Result so the roll-up + render can be exercised with a +// controlled mix without standing up a fake cluster. var doctorRunFn = doctor.Run -// newDoctorCmd builds the `doctor` command. The SAME command is registered two -// ways: as the top-level `tracebloc doctor` (visible — the home screen and its -// env-status lines point here), and, hidden, as `tracebloc cluster doctor` (its -// original path, kept working so existing docs, muscle memory, and scripts don't -// break). Pass hidden=true for the cluster-subtree alias. Both entry points -// share one RunE (runClusterDoctor), so there is a single diagnostic code path. -// -// It answers "is this running cluster healthy enough to run an experiment, and -// if not, what do I fix?" — a read-only, post-install health sweep with remedies -// (epic client-runtime#116, WS3). The three kubeconfig flags match `cluster -// info` exactly so muscle memory carries over; all are zero-value-safe. +// newDoctorCmd builds `doctor`, registered as top-level `tracebloc doctor` and +// (hidden) `tracebloc cluster doctor`. It answers, in plain language: is my +// secure environment connected to tracebloc and ready to run training — and if +// not, exactly what do I do. The Kubernetes detail lives behind --verbose; +// --diagnose writes a redacted bundle for support. func newDoctorCmd(hidden bool) *cobra.Command { var ( kubeconfigPath string contextOverride string nsOverride string + diagnose bool ) cmd := &cobra.Command{ Use: "doctor", Hidden: hidden, - Short: "Diagnose a running tracebloc client cluster (✔/⚠/✖ health checks + remedies)", - Long: `Runs a read-only health sweep over the tracebloc client release in the -configured cluster + namespace and prints a ✔/⚠/✖ line per check with a -remedy for anything that isn't green: - - • Cluster reachable — the API answers and the client chart is installed - • Pod health — nothing crash-looping or stuck Pending - • Dataset volume — the shared PVC exists and is Bound - • Proxy configuration — the in-cluster requests/egress proxy wiring - • Backend egress — the tracebloc backend is reachable (from this machine) - • Service Bus egress — the requests-proxy that brokers experiment egress is Ready + Short: "Check your secure environment is connected and ready to run training", + Long: `Checks, in plain terms, whether your secure environment is connected to +tracebloc and ready to run training — and if something's wrong, exactly what to +do about it. -For a full redacted support bundle to send to tracebloc, use the installer's -` + "`./install-k8s.sh --diagnose`" + ` instead. + --verbose the full technical breakdown (for support) + --diagnose write a redacted support bundle to email to tracebloc Exit codes: - 0 all checks passed (or warnings only) - 2 one or more checks failed - 3 kubeconfig could not be loaded`, + 0 healthy + 2 a problem was found + 3 couldn't read your local config`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { return runClusterDoctor( cmd.Context(), printerFor(cmd), - kubeconfigPath, contextOverride, nsOverride, + kubeconfigPath, contextOverride, nsOverride, diagnose, ) }, } addKubeconfigFlags(cmd, &kubeconfigPath, &contextOverride, kubeconfigFlagUsage, contextFlagUsage) addNamespaceFlag(cmd, &nsOverride, - "namespace where your tracebloc client is installed (default: your active client's namespace, else the context's)") + "namespace where your secure environment is installed (default: your active client's)") + cmd.Flags().BoolVar(&diagnose, "diagnose", false, + "write a redacted support bundle for tracebloc support and exit") return cmd } @@ -83,167 +79,237 @@ func runClusterDoctor( ctx context.Context, p *ui.Printer, kubeconfigPath, contextOverride, nsOverride string, + diagnose bool, ) error { - p.Banner("tracebloc", "doctor") - - // Auth / config checks run FIRST and don't need a cluster — so `doctor` can - // diagnose a failed provision (bad/expired token, wrong env, no active - // client) even before any cluster is reachable (RFC-0001 §8.5). - authStatus := runAuthChecks(ctx, p) + // 1. Who you are — a local config read (instant), so we can answer even + // before any cluster is reachable (RFC-0001 §8.5). + cfg, err := config.Load() + if err != nil { + p.Errorf("Couldn't read your tracebloc config — run `%s login` to recreate it.", launcher()) + return &exitError{code: exitLocalEnv, err: nil} + } + if !cfg.SignedIn() { + p.Errorf("Not signed in — run `%s login`.", launcher()) + return &exitError{code: exitChecksFailed, err: nil} + } + if email := cfg.Current().Email; email != "" { + p.Para("Signed in as " + email) + } else { + p.Para("Signed in") + } - // Target the active client's namespace exactly like `cluster info`, the data - // commands, and the home screen: bind opts.Namespace to the cached active - // client when the user overrode neither --namespace nor --context. Without - // this, doctor checked only the kubeconfig default namespace — so a typical - // install whose client lives in its slug namespace could show one state on the - // home screen and a conflicting "no client here" from the on-screen `doctor` - // hint (Bugbot / review). - opts := cluster.KubeconfigOptions{ - Path: kubeconfigPath, - Context: contextOverride, - Namespace: nsOverride, + // 2. Is the session still good with tracebloc? A 401 (expired/revoked) or a + // 426 (CLI too old) is a hard stop with a one-command fix. A network error + // is NOT fatal — it folds into the Connected line below. + tokenReachable := true + apiClient := newAPIClient(cfg.CurrentEnv) + apiClient.Token = cfg.Current().Token + if _, werr := apiClient.WhoAmI(ctx); werr != nil { + var ae *api.APIError + var ue *api.UpgradeRequiredError + switch { + case errors.As(werr, &ae) && ae.StatusCode == http.StatusUnauthorized: + p.Newline() + p.Errorf("Your session expired — run `%s login`.", launcher()) + return &exitError{code: exitChecksFailed, err: nil} + case errors.As(werr, &ue): + p.Newline() + p.Errorf("This CLI is out of date — update it: %s", installCmd) + return &exitError{code: exitChecksFailed, err: nil} + default: + tokenReachable = false // can't reach tracebloc from here — Connected will say so + } } - bindActiveClientNamespace(&opts) // side-effect: defaults opts.Namespace to the active client's - // Through the loadClusterFn/newClientsetFn seams (like cluster info and the - // data commands) so the cluster half — the Checks render loop and the verdict - // switch — is testable with a fake clientset instead of only a real kubeconfig. + + // 3. Find the secure environment (local kubeconfig read). + opts := cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride, Namespace: nsOverride} + bindActiveClientNamespace(&opts) resolved, err := loadClusterFn(opts) if err != nil { - // 3 = kubeconfig file/parse problem (same class as cluster info). The - // auth section above already ran; if IT also failed, escalate to 2 so - // automation doesn't read a real auth failure as a kubeconfig-only one. - p.Section("Cluster") - p.Errorf("Kubeconfig — couldn't load it: %v", err) - p.Hintf(" point --kubeconfig / --context at your cluster, or fix ~/.kube/config") - return &exitError{code: kubeconfigExitCode(authStatus), err: nil} + p.Newline() + p.Errorf("No secure environment on this machine yet.") + p.Hintf(" Set one up: %s", installCmd) + return &exitError{code: exitLocalEnv, err: nil} } - cs, err := newClientsetFn(resolved) if err != nil { - p.Section("Cluster") - p.Errorf("Kubeconfig — %v", err) - return &exitError{code: kubeconfigExitCode(authStatus), err: nil} + p.Newline() + p.Errorf("Couldn't connect to your secure environment — check your kubeconfig/context.") + return &exitError{code: exitLocalEnv, err: nil} } + p.Para(fmt.Sprintf("Secure environment %q", envDisplayName(resolved))) - p.Section("Kubeconfig") - p.Field("context", resolved.Context) - p.Field("server", resolved.ServerURL) - p.Field("namespace", resolved.Namespace) + // 4. Probe the cluster (the granular checks stay for --verbose + --diagnose). + results := doctorRunFn(ctx, cs, doctor.Options{Namespace: resolved.Namespace, ServerURL: resolved.ServerURL}) - results := doctorRunFn(ctx, cs, doctor.Options{ - Namespace: resolved.Namespace, - ServerURL: resolved.ServerURL, - }) + // --diagnose: write the full technical detail to a file for support, then stop. + if diagnose { + return writeDiagnoseBundle(p, resolved, results) + } - p.Section("Checks") - for _, r := range results { - switch r.Status { - case doctor.StatusOK: - p.Successf("%s — %s", r.Name, r.Detail) - case doctor.StatusWarn: - p.Warnf("%s — %s", r.Name, r.Detail) - if r.Remedy != "" { - p.Hintf(" %s", r.Remedy) - } - case doctor.StatusFail: - p.Errorf("%s — %s", r.Name, r.Detail) - if r.Remedy != "" { - p.Hintf(" %s", r.Remedy) - } - case doctor.StatusUnknown: - // No signal: a neutral · line, no ✖/⚠ and no remedy — the one honest - // "Cluster reachable" ✖ above already carries the cause and the fix. - p.Infof("%s — %s", r.Name, r.Detail) - } + // 5. Roll the granular checks up into two plain lines the owner can act on. + p.Newline() + connected, ready := summarizeDoctor(results, tokenReachable) + renderHealth(p, connected) + renderHealth(p, ready) + + if p.Verbose() { + renderDoctorDetails(p, resolved, results) } + // 6. Verdict + exit code (unchanged: 0 healthy, 2 a problem). p.Newline() - // Overall verdict folds in the auth section, so an auth ✖/⚠ counts even when - // the cluster itself is healthy. - switch worseStatus(authStatus, doctor.Worst(results)) { - case doctor.StatusFail: - p.Errorf("Problems found — fix the ✖ items above.") - p.Hintf("For deeper triage, send tracebloc a support bundle: ./install-k8s.sh --diagnose") - // Silent (err == nil): the per-check lines above already explained it, - // so main() shouldn't print a redundant "Error:" line. + if worseStatus(connected.status, ready.status) == doctor.StatusFail { + p.Hintf("Still stuck? Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher()) return &exitError{code: exitChecksFailed, err: nil} - case doctor.StatusWarn: - p.Warnf("Completed with warnings — review the ⚠ items above.") - return nil - default: - p.Successf("All checks passed — auth and cluster look healthy.") - return nil } + p.Successf("Everything looks good — you're ready to run training.") + return nil } -// runAuthChecks reports on the CLI's own auth/config state (~/.tracebloc): are -// we signed in, to which env, is an active client selected, and does the backend -// still accept the token. It's the half of `cluster doctor` that diagnoses a -// failed *provision* rather than a sick cluster (RFC-0001 §8.5). Returns the -// worst status seen so the caller can fold it into the overall verdict. -func runAuthChecks(ctx context.Context, p *ui.Printer) doctor.Status { - p.Section("Auth & config") +// healthLine is one rolled-up, user-facing health signal: a status, the plain +// line to show, and (on failure) one concrete action. +type healthLine struct { + status doctor.Status + text string + remedy string +} - cfg, err := config.Load() - if err != nil { - p.Errorf("Config — couldn't read the CLI config: %v", err) - p.Hintf(" check ~/.tracebloc/config.json, or run `tracebloc login` to recreate it") - return doctor.StatusFail +// summarizeDoctor collapses the granular checks into the two lines the owner +// reads: "Connected to tracebloc" and "Ready to run training". Each expands to +// the specific plain-language problem + fix on failure; the Kubernetes detail +// stays in --verbose. When the environment is unreachable, readiness can't be +// assessed, so it degrades honestly to "can't check" (never a false ✔). +func summarizeDoctor(results []doctor.Result, tokenReachable bool) (connected, ready healthLine) { + by := make(map[string]doctor.Result, len(results)) + for _, r := range results { + by[r.Name] = r } - if !cfg.SignedIn() { - p.Errorf("Sign-in — not signed in") - p.Hintf(" run `tracebloc login` (add --env dev|stg|prod for a non-prod backend)") - return doctor.StatusFail + reach := by["Cluster reachable"] + + switch { + case reach.Status == doctor.StatusFail: + // checkReachable already produced an endpoint-aware, plain remedy + // (start Docker for a local cluster / check it's running for a remote). + connected = healthLine{doctor.StatusFail, "Not connected — your secure environment isn't answering.", reach.Remedy} + case !tokenReachable || by["Backend egress (from this machine)"].Status == doctor.StatusFail: + connected = healthLine{doctor.StatusFail, + "Not connected — can't reach tracebloc from here.", + fmt.Sprintf("Check your network / HTTP(S)_PROXY, then run `%s doctor` again.", launcher())} + case by["Service Bus egress (requests-proxy)"].Status == doctor.StatusFail: + connected = healthLine{doctor.StatusFail, + "Training results can't reach tracebloc — experiments will stall.", + fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} + default: + connected = healthLine{doctor.StatusOK, "Connected to tracebloc", ""} } - env := cfg.CurrentEnv - prof := cfg.Current() - if prof.Email != "" { - p.Successf("Sign-in — signed in to %s as %s", env, prof.Email) - } else { - p.Successf("Sign-in — signed in to %s", env) + // Readiness is meaningless if we can't even reach the environment. + if reach.Status == doctor.StatusFail { + ready = healthLine{doctor.StatusUnknown, "Ready to run training — can't check until it's connected", ""} + return connected, ready + } + switch { + case by["Pod health"].Status == doctor.StatusFail: + ready = healthLine{doctor.StatusFail, + "Not ready — part of your secure environment isn't running.", + fmt.Sprintf("Reinstall with `%s`, or email support@tracebloc.io with `%s doctor --diagnose`.", installCmd, launcher())} + case by["Dataset volume (PVC)"].Status == doctor.StatusFail: + ready = healthLine{doctor.StatusFail, + "Not ready — dataset storage isn't available.", + fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} + case by["Node capacity"].Status == doctor.StatusFail: + ready = healthLine{doctor.StatusFail, + "Not ready — not enough free compute to start a training.", + "Free some up, or raise the machine's allocation in Docker Desktop → Resources."} + default: + ready = healthLine{doctor.StatusOK, "Ready to run training", ""} } + return connected, ready +} - worst := doctor.StatusOK - if prof.ActiveClientID == "" { - p.Warnf("Active client — none selected for %s", env) - p.Hintf(" run `tracebloc client create` (or re-run the installer) to set the client this machine enrolls as") - worst = doctor.StatusWarn - } else { - p.Successf("Active client — %s", prof.ActiveClientID) +// renderHealth prints one rolled-up line: ✔ for OK, ✖ + remedy for a problem, +// and a neutral · "can't check" for StatusUnknown (no false green, no alarm). +func renderHealth(p *ui.Printer, h healthLine) { + switch h.status { + case doctor.StatusOK: + p.Successf("%s", h.text) + case doctor.StatusUnknown: + p.Infof("%s", h.text) + default: + p.Errorf("%s", h.text) + if h.remedy != "" { + p.Hintf(" %s", h.remedy) + } } +} - // Live token check. Best-effort: an explicit 401 is a failure (expired / - // revoked → must re-login); a network/proxy error is only a warning, since - // we can't conclude the token itself is bad. - p.Detailf("verifying the token against %s …", api.BaseURL(env)) - client := newAPIClient(env) - client.Token = prof.Token - if _, werr := client.WhoAmI(ctx); werr != nil { - var ae *api.APIError - var ue *api.UpgradeRequiredError - switch { - case errors.As(werr, &ae) && ae.StatusCode == http.StatusUnauthorized: - p.Errorf("Backend auth — %s rejected the token (401)", api.BaseURL(env)) - p.Hintf(" your session expired or was revoked — run `tracebloc login`") - return doctor.StatusFail - case errors.As(werr, &ue): - // 426: the server enforces a newer CLI. That's a hard, actionable - // failure ("upgrade"), not a transient "couldn't verify" warning. - p.Errorf("Backend auth — this CLI is too old for %s (HTTP 426)", api.BaseURL(env)) - p.Hintf(" %s", ue.Error()) - return doctor.StatusFail - default: - p.Warnf("Backend auth — couldn't verify the token: %v", werr) - p.Hintf(" the backend may be unreachable from here — check your network / HTTP(S)_PROXY") - return worseStatus(worst, doctor.StatusWarn) +// renderDoctorDetails is the --verbose (and --diagnose) technical breakdown: +// kubeconfig + every granular check. This is the only place Kubernetes +// vocabulary appears. +func renderDoctorDetails(p *ui.Printer, resolved *cluster.ResolvedConfig, results []doctor.Result) { + p.Newline() + p.Section("Details (for support)") + p.Field("context", resolved.Context) + p.Field("server", resolved.ServerURL) + p.Field("namespace", resolved.Namespace) + for _, r := range results { + mark := "·" + switch r.Status { + case doctor.StatusOK: + mark = "OK " + case doctor.StatusWarn: + mark = "WARN" + case doctor.StatusFail: + mark = "FAIL" } + p.Detailf("%s %s — %s", mark, r.Name, r.Detail) + if r.Remedy != "" { + p.Detailf(" %s", r.Remedy) + } + } +} + +// writeDiagnoseBundle owns the support bundle (moved out of install-k8s.sh, which +// the user may not have on disk). It writes the redacted technical breakdown to a +// file the owner emails to support. +func writeDiagnoseBundle(p *ui.Printer, resolved *cluster.ResolvedConfig, results []doctor.Result) error { + var buf bytes.Buffer + fmt.Fprintf(&buf, "tracebloc doctor — support bundle (%s)\n", time.Now().Format(time.RFC3339)) + bp := ui.New(&buf, ui.WithColor(false), ui.WithVerbose(true)) + renderDoctorDetails(bp, resolved, results) + + name := fmt.Sprintf("tracebloc-doctor-%s.txt", time.Now().Format("20060102-150405")) + if werr := os.WriteFile(name, buf.Bytes(), 0o600); werr != nil { + p.Errorf("Couldn't write the support bundle: %v", werr) + return &exitError{code: exitLocalEnv, err: nil} } - p.Successf("Backend auth — token valid at %s", api.BaseURL(env)) - return worst + p.Successf("Wrote a support bundle to ./%s", name) + p.Hintf(" Email it to support@tracebloc.io.") + return nil +} + +// launcher resolves the command name to print in remedies: "tb" on a real +// install (the alias is beside the CLI), else the invoked name — same rule the +// home screen uses, so copy-paste always works. +func launcher() string { + if tbAliasAvailable() { + return binTB + } + return invokedName() +} + +// envDisplayName is the secure environment's user-facing handle — the namespace +// slug (RFC-0001 §7: the slug IS the handle). +func envDisplayName(r *cluster.ResolvedConfig) string { + if r != nil && r.Namespace != "" { + return r.Namespace + } + return "your secure environment" } // worseStatus returns the more severe of two doctor statuses (Fail > Warn > OK). +// StatusUnknown carries no signal, so it never worsens the verdict. func worseStatus(a, b doctor.Status) doctor.Status { if a == doctor.StatusFail || b == doctor.StatusFail { return doctor.StatusFail @@ -253,13 +319,3 @@ func worseStatus(a, b doctor.Status) doctor.Status { } return doctor.StatusOK } - -// kubeconfigExitCode is 3 ("kubeconfig could not be loaded") unless the auth -// section also failed — then it escalates to 2 ("a check failed"), so a bad -// token isn't masked behind a kubeconfig-only exit code (Bugbot). -func kubeconfigExitCode(authStatus doctor.Status) int { - if authStatus == doctor.StatusFail { - return exitChecksFailed - } - return exitLocalEnv -} diff --git a/internal/cli/doctor_cluster_test.go b/internal/cli/doctor_cluster_test.go index 931e9656..e60fdfe3 100644 --- a/internal/cli/doctor_cluster_test.go +++ b/internal/cli/doctor_cluster_test.go @@ -17,9 +17,9 @@ import ( "github.com/tracebloc/cli/internal/ui" ) -// withDoctorRun seams doctorRunFn to return a fixed set of check results, so -// runClusterDoctor's render loop + verdict switch can be exercised with a -// controlled OK/Warn/Fail mix (the per-check logic is tested in internal/doctor). +// withDoctorRun seams doctorRunFn to return a fixed set of granular check +// results, so runClusterDoctor's roll-up + verdict can be exercised with a +// controlled mix (the per-check logic is tested in internal/doctor). func withDoctorRun(t *testing.T, results []doctor.Result) { t.Helper() orig := doctorRunFn @@ -29,13 +29,9 @@ func withDoctorRun(t *testing.T, results []doctor.Result) { } } -// runDoctorClusterHalf drives runClusterDoctor with the auth half stubbed HEALTHY -// (signed in + active client + a 200 WhoAmI), so the overall verdict reflects the -// cluster checks rather than the auth section, and the cluster reached through the -// seams with doctor.Run returning `results`. Before this PR the cluster half — the -// ✓/⚠/✖ render loop and the verdict switch — was reachable only via a real -// kubeconfig pointing at an unroutable server (dial failure), never with a -// controlled result mix. +// runDoctorClusterHalf drives runClusterDoctor with the identity half stubbed +// HEALTHY (signed in + a 200 WhoAmI) and the cluster reached through the seams +// with doctor.Run returning `results`, into a non-verbose printer. func runDoctorClusterHalf(t *testing.T, results []doctor.Result) (string, error) { t.Helper() t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) @@ -45,80 +41,116 @@ func runDoctorClusterHalf(t *testing.T, results []doctor.Result) (string, error) t.Fatal(err) } stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // WhoAmI ok → auth OK + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // WhoAmI ok → session healthy }) withClusterSeams(t, fake.NewSimpleClientset()) // cs only flows to doctorRunFn, which ignores it withDoctorRun(t, results) var buf bytes.Buffer - err := runClusterDoctor(context.Background(), ui.New(&buf, ui.WithColor(false)), "", "", "") + err := runClusterDoctor(context.Background(), ui.New(&buf, ui.WithColor(false)), "", "", "", false) return buf.String(), err } -// All three render arms fire (✓ detail, ⚠ + remedy hint, ✖ + remedy hint) and a -// single failing check drives the overall verdict to Fail → exit 2 with a silent -// error (the per-check lines already explained it) + the support-bundle hint. -func TestRunClusterDoctor_RendersAllStatusesAndFails(t *testing.T) { +// All healthy → the two plain lines + the "ready to run training" verdict, exit 0. +// The resolved namespace is printed as the secure-environment name (the seam +// resolves it to "default"). +func TestRunClusterDoctor_AllHealthy(t *testing.T) { out, err := runDoctorClusterHalf(t, []doctor.Result{ - {Name: "Cluster reachable", Status: doctor.StatusOK, Detail: "api answered"}, - {Name: "Pod health", Status: doctor.StatusWarn, Detail: "1 pod restarted", Remedy: "check pod logs"}, - {Name: "Dataset volume", Status: doctor.StatusFail, Detail: "PVC not bound", Remedy: "provision the shared PVC"}, + {Name: "Cluster reachable", Status: doctor.StatusOK, Detail: "reachable"}, + {Name: "Backend egress (from this machine)", Status: doctor.StatusOK, Detail: "reachable"}, + }) + if err != nil { + t.Fatalf("all healthy → want nil error (exit 0), got %v", err) + } + for _, want := range []string{ + "Signed in as a@b.io", + `Secure environment "default"`, + "Connected to tracebloc", + "Ready to run training", + "Everything looks good", + } { + if !strings.Contains(out, want) { + t.Errorf("healthy view missing %q:\n%s", want, out) + } + } + // No Kubernetes vocabulary leaks into the default view. + for _, banned := range []string{"Kubeconfig", "context", "PVC", "kubectl", "requests-proxy", "Pending"} { + if strings.Contains(out, banned) { + t.Errorf("leaked k8s term %q in default view:\n%s", banned, out) + } + } +} + +// A failing cluster check rolls up into a plain ✖ line + concrete remedy, drives +// the verdict to Fail (exit 2, silent error), and points at the support bundle. +func TestRunClusterDoctor_FailRollsUp(t *testing.T) { + out, err := runDoctorClusterHalf(t, []doctor.Result{ + {Name: "Cluster reachable", Status: doctor.StatusOK, Detail: "reachable"}, + {Name: "Dataset volume (PVC)", Status: doctor.StatusFail, Detail: "PVC not bound"}, }) var ee *exitError if !errors.As(err, &ee) || ee.Code() != 2 { t.Fatalf("a failing check → want exit 2, got %v", err) } if ee.err != nil { - t.Errorf("the Fail verdict must be a silent exitError (per-check lines already printed), got err=%v", ee.err) + t.Errorf("Fail verdict must be a silent exitError (lines already printed), got err=%v", ee.err) } - for _, want := range []string{ - "Cluster reachable", "api answered", // OK arm - "Pod health", "1 pod restarted", "check pod logs", // Warn arm + remedy hint - "Dataset volume", "PVC not bound", "provision the shared PVC", // Fail arm + remedy hint - "Problems found", "support bundle", // Fail verdict + its hint - } { + for _, want := range []string{"Not ready", "dataset storage", "--diagnose"} { if !strings.Contains(out, want) { - t.Errorf("output missing %q:\n%s", want, out) + t.Errorf("failed view missing %q:\n%s", want, out) } } + // The raw k8s detail ("PVC not bound") must NOT appear in the default view. + if strings.Contains(out, "PVC not bound") { + t.Errorf("raw k8s detail leaked into the default view:\n%s", out) + } } -// Warnings but no failure → overall Warn → exit 0 (nil error) with the -// "completed with warnings" verdict. -func TestRunClusterDoctor_WarnVerdict(t *testing.T) { +// A Warn-level granular check (e.g. a proxy note) is not user-actionable and must +// NOT fail the verdict — it degrades to --verbose. Default view stays healthy. +func TestRunClusterDoctor_WarnsDontFail(t *testing.T) { out, err := runDoctorClusterHalf(t, []doctor.Result{ - {Name: "Cluster reachable", Status: doctor.StatusOK, Detail: "ok"}, - {Name: "Proxy configuration", Status: doctor.StatusWarn, Detail: "not fully wired", Remedy: "set the proxy env"}, + {Name: "Cluster reachable", Status: doctor.StatusOK, Detail: "reachable"}, + {Name: "Proxy configuration", Status: doctor.StatusWarn, Detail: "not fully wired"}, }) if err != nil { t.Fatalf("warnings-only → want nil error (exit 0), got %v", err) } - if !strings.Contains(out, "Completed with warnings") { - t.Errorf("want the warnings verdict:\n%s", out) + if !strings.Contains(out, "Everything looks good") { + t.Errorf("want the healthy verdict (warns are verbose-only):\n%s", out) } } -// All checks pass (and auth is healthy) → overall OK → exit 0 with the all-healthy -// verdict. Also pins that the resolved namespace is printed in the Kubeconfig -// section (the seam resolves it to "default"). -func TestRunClusterDoctor_AllHealthy(t *testing.T) { - out, err := runDoctorClusterHalf(t, []doctor.Result{ - {Name: "Cluster reachable", Status: doctor.StatusOK, Detail: "api answered, client installed"}, - {Name: "Backend egress", Status: doctor.StatusOK, Detail: "reachable"}, - }) - if err != nil { - t.Fatalf("all healthy → want nil error (exit 0), got %v", err) +// --verbose surfaces the Kubernetes detail (kubeconfig + granular checks) under +// the plain summary — the one place that vocabulary is allowed. +func TestRunClusterDoctor_VerboseShowsDetails(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ + "dev": {Token: "x", Email: "a@b.io", ActiveClientID: "5"}, + }}).Save(); err != nil { + t.Fatal(err) } - if !strings.Contains(out, "All checks passed") { - t.Errorf("want the all-healthy verdict:\n%s", out) + stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) + }) + withClusterSeams(t, fake.NewSimpleClientset()) + withDoctorRun(t, []doctor.Result{ + {Name: "Cluster reachable", Status: doctor.StatusOK, Detail: "reachable"}, + {Name: "Service Bus egress (requests-proxy)", Status: doctor.StatusOK, Detail: "ready"}, + }) + var buf bytes.Buffer + if err := runClusterDoctor(context.Background(), ui.New(&buf, ui.WithColor(false), ui.WithVerbose(true)), "", "", "", false); err != nil { + t.Fatalf("verbose healthy → want nil error, got %v", err) } - if !strings.Contains(out, "default") { - t.Errorf("want the resolved namespace printed in the Kubeconfig section:\n%s", out) + for _, want := range []string{"Details (for support)", "Service Bus egress (requests-proxy)"} { + if !strings.Contains(buf.String(), want) { + t.Errorf("verbose view missing %q:\n%s", want, buf.String()) + } } } // The clientset-build arm: loadClusterFn succeeds but newClientsetFn fails. With -// the auth half healthy, this keeps the documented exit-3 contract (a kubeconfig- -// class failure) and surfaces the underlying error. +// the session healthy, this is a local-environment problem (exit 3) framed +// plainly (no raw Kubernetes error in the default view). func TestRunClusterDoctor_ClientsetError(t *testing.T) { t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ @@ -127,7 +159,7 @@ func TestRunClusterDoctor_ClientsetError(t *testing.T) { t.Fatal(err) } stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // auth OK + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) }) origLoad, origCS := loadClusterFn, newClientsetFn t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) @@ -138,12 +170,12 @@ func TestRunClusterDoctor_ClientsetError(t *testing.T) { return nil, errors.New("bad rest config") } var buf bytes.Buffer - err := runClusterDoctor(context.Background(), ui.New(&buf, ui.WithColor(false)), "", "", "") + err := runClusterDoctor(context.Background(), ui.New(&buf, ui.WithColor(false)), "", "", "", false) var ee *exitError if !errors.As(err, &ee) || ee.Code() != 3 { - t.Fatalf("clientset build error + auth-OK → want exit 3, got %v", err) + t.Fatalf("clientset build error + healthy session → want exit 3, got %v", err) } - if !strings.Contains(buf.String(), "bad rest config") { - t.Errorf("want the clientset error surfaced in the Cluster section:\n%s", buf.String()) + if !strings.Contains(buf.String(), "Couldn't connect to your secure environment") { + t.Errorf("want the plain connect-failure message:\n%s", buf.String()) } } diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 880ec93a..3ca4cecf 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -28,149 +28,94 @@ func stubBackend(t *testing.T, h http.HandlerFunc) { t.Cleanup(func() { newAPIClient = orig }) } -// cli#101: `cluster doctor` auth/config/token checks (RFC-0001 §8.5). These pin -// runAuthChecks — the half of doctor that diagnoses a failed *provision*. - -func TestRunAuthChecks_NotSignedIn(t *testing.T) { - t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) - var out bytes.Buffer - if st := runAuthChecks(context.Background(), ui.New(&out)); st != doctor.StatusFail { - t.Errorf("not signed in → want Fail, got %v", st) - } - if !strings.Contains(out.String(), "Auth & config") || !strings.Contains(out.String(), "not signed in") { - t.Errorf("missing auth section / not-signed-in line:\n%s", out.String()) - } -} - -func TestRunAuthChecks_TokenValid(t *testing.T) { +// signedInConfig writes a signed-in config with an active client for the tests +// that need to get past the identity gate. +func signedInConfig(t *testing.T) { + t.Helper() t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ "dev": {Token: "x", Email: "a@b.io", ActiveClientID: "5"}, }}).Save(); err != nil { t.Fatal(err) } - stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) - }) +} + +// ── Identity / session gate (runs before any cluster I/O) ── + +func TestDoctor_NotSignedIn(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) var out bytes.Buffer - if st := runAuthChecks(context.Background(), ui.New(&out)); st != doctor.StatusOK { - t.Errorf("valid token + active client → want OK, got %v;\n%s", st, out.String()) + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("not signed in → want exit 2, got %v", err) } - if !strings.Contains(out.String(), "token valid") { - t.Errorf("missing token-valid line:\n%s", out.String()) + if !strings.Contains(out.String(), "Not signed in") || !strings.Contains(out.String(), "login") { + t.Errorf("want a plain 'Not signed in — run ... login', got:\n%s", out.String()) } } -func TestRunAuthChecks_TokenRejected401(t *testing.T) { - t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) - if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ - "dev": {Token: "x", ActiveClientID: "5"}, - }}).Save(); err != nil { - t.Fatal(err) - } +func TestDoctor_SessionExpired401(t *testing.T) { + signedInConfig(t) stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusUnauthorized) _, _ = w.Write([]byte(`{"detail":"Invalid token."}`)) }) var out bytes.Buffer - if st := runAuthChecks(context.Background(), ui.New(&out)); st != doctor.StatusFail { - t.Errorf("token rejected (401) → want Fail, got %v", st) + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("401 → want exit 2, got %v", err) } - if !strings.Contains(out.String(), "rejected the token (401)") { - t.Errorf("missing 401 line:\n%s", out.String()) + if !strings.Contains(out.String(), "session expired") || !strings.Contains(out.String(), "login") { + t.Errorf("want 'Your session expired — run ... login', got:\n%s", out.String()) } } -func TestRunAuthChecks_NoActiveClientWarns(t *testing.T) { - t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) - if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ - "dev": {Token: "x"}, // signed in, but no active client selected - }}).Save(); err != nil { - t.Fatal(err) - } +func TestDoctor_OutOfDate426(t *testing.T) { + signedInConfig(t) stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) + w.WriteHeader(http.StatusUpgradeRequired) // 426 + _, _ = w.Write([]byte(`{"error":"upgrade_required","min_version":"0.9.0"}`)) }) var out bytes.Buffer - if st := runAuthChecks(context.Background(), ui.New(&out)); st != doctor.StatusWarn { - t.Errorf("valid token but no active client → want Warn, got %v;\n%s", st, out.String()) - } - if !strings.Contains(out.String(), "Active client — none") { - t.Errorf("missing no-active-client warning:\n%s", out.String()) - } -} - -// TestClusterDoctor_KubeconfigFailEscalatesWhenAuthFails pins the Bugbot fix: a -// kubeconfig load failure normally exits 3, but if the auth section ALSO failed -// (here: not signed in) it escalates to 2 so a bad token isn't masked as a -// kubeconfig-only problem. -func TestClusterDoctor_KubeconfigFailEscalatesWhenAuthFails(t *testing.T) { - t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) // not signed in → auth Fail - var out bytes.Buffer - err := runClusterDoctor(context.Background(), ui.New(&out), "/nonexistent-kubeconfig-xyz", "", "") + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) var ee *exitError if !errors.As(err, &ee) || ee.Code() != 2 { - t.Fatalf("kubeconfig-fail + auth-fail → want exit 2, got %v", err) + t.Fatalf("426 → want exit 2, got %v", err) + } + if !strings.Contains(out.String(), "out of date") || !strings.Contains(out.String(), "tracebloc.io/i.sh") { + t.Errorf("want 'This CLI is out of date — update it: ', got:\n%s", out.String()) } } -// TestClusterDoctor_KubeconfigFailStays3WhenAuthOK: with auth healthy, a -// kubeconfig failure keeps the documented exit-3 contract. -func TestClusterDoctor_KubeconfigFailStays3WhenAuthOK(t *testing.T) { - t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) - if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ - "dev": {Token: "x", ActiveClientID: "5"}, - }}).Save(); err != nil { - t.Fatal(err) - } +// A bad kubeconfig, with auth healthy, is a local-environment problem (exit 3) +// framed as "no secure environment here yet" — not a Kubernetes error dump. +func TestDoctor_KubeconfigFailIsLocalEnv(t *testing.T) { + signedInConfig(t) stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // WhoAmI ok → auth OK + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // WhoAmI ok }) var out bytes.Buffer - err := runClusterDoctor(context.Background(), ui.New(&out), "/nonexistent-kubeconfig-xyz", "", "") + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "/nonexistent-kubeconfig-xyz", "", "", false) var ee *exitError if !errors.As(err, &ee) || ee.Code() != 3 { - t.Fatalf("kubeconfig-fail + auth-OK → want exit 3 (contract), got %v", err) - } -} - -// TestRunAuthChecks_426IsHardFailure pins the Bugbot fix: a 426 (server enforces -// a newer CLI) from the live token check is a hard "upgrade" failure, not a -// transient "couldn't verify — check your network" warning. -func TestRunAuthChecks_426IsHardFailure(t *testing.T) { - t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) - if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ - "dev": {Token: "x", ActiveClientID: "5"}, - }}).Save(); err != nil { - t.Fatal(err) + t.Fatalf("kubeconfig-fail + auth-OK → want exit 3, got %v", err) } - stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusUpgradeRequired) // 426 - _, _ = w.Write([]byte(`{"error":"upgrade_required","min_version":"0.9.0"}`)) - }) - var out bytes.Buffer - if st := runAuthChecks(context.Background(), ui.New(&out)); st != doctor.StatusFail { - t.Errorf("426 from the token check → want Fail (not a transient Warn), got %v", st) - } - if !strings.Contains(out.String(), "too old") || !strings.Contains(out.String(), "426") { - t.Errorf("426 should report a clear 'too old / upgrade' failure, got:\n%s", out.String()) + if !strings.Contains(out.String(), "No secure environment") { + t.Errorf("want the plain no-environment message, got:\n%s", out.String()) } } -// TestClusterDoctor_BindsActiveClientNamespace pins the review fix: with no -// --namespace/--context, doctor must target the active client's cached namespace -// (like `cluster info` and the home screen), not the kubeconfig default — else -// the home screen and doctor can disagree about the same install. The server is -// unroutable, so discovery falls back to the bound namespace; we assert doctor -// reports THAT namespace. Mutation-proven: drop bindActiveClientNamespace and the -// namespace becomes the kubeconfig default, failing this assertion. -func TestClusterDoctor_BindsActiveClientNamespace(t *testing.T) { +// With no --namespace/--context, doctor targets the active client's cached +// namespace (like `cluster info` + the home screen) and prints it as the +// secure-environment name. Mutation-proven: drop bindActiveClientNamespace and +// the printed name becomes the kubeconfig default. +func TestDoctor_BindsActiveClientNamespace(t *testing.T) { writeActiveClientConfig(t, "munich-radiology", "Munich Radiology") stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // WhoAmI ok → auth healthy + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // WhoAmI ok }) - // Valid kubeconfig at an unroutable TEST-NET address: loads fine, so the - // namespace resolves from the binding; the later cluster dial just fails. const kubeconfig = `apiVersion: v1 kind: Config clusters: @@ -188,14 +133,86 @@ users: if err := os.WriteFile(kc, []byte(kubeconfig), 0o600); err != nil { t.Fatal(err) } - // Bound the context so doctor.Run's dial to the unroutable server can't hang - // the test (and CI) — we only assert the namespace resolved from the binding, - // which is printed before any cluster I/O. - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() var out bytes.Buffer - _ = runClusterDoctor(ctx, ui.New(&out), kc, "", "") // no ns/context override + _ = runClusterDoctor(ctx, ui.New(&out, ui.WithColor(false)), kc, "", "", false) if !strings.Contains(out.String(), "munich-radiology") { - t.Fatalf("doctor should target the active client's namespace (munich-radiology), got:\n%s", out.String()) + t.Fatalf("doctor should name the active client's environment (munich-radiology), got:\n%s", out.String()) + } +} + +// ── summarizeDoctor: the roll-up of the granular checks into two plain lines ── + +func TestSummarizeDoctor(t *testing.T) { + res := func(name string, s doctor.Status) doctor.Result { return doctor.Result{Name: name, Status: s} } + allOK := []doctor.Result{ + res("Cluster reachable", doctor.StatusOK), + res("Backend egress (from this machine)", doctor.StatusOK), + res("Service Bus egress (requests-proxy)", doctor.StatusOK), + res("Pod health", doctor.StatusOK), + res("Dataset volume (PVC)", doctor.StatusOK), + res("Node capacity", doctor.StatusOK), + } + with := func(base []doctor.Result, name string, s doctor.Status) []doctor.Result { + out := make([]doctor.Result, len(base)) + copy(out, base) + for i := range out { + if out[i].Name == name { + out[i].Status = s + } + } + return out } + + t.Run("all healthy → both OK", func(t *testing.T) { + c, r := summarizeDoctor(allOK, true) + if c.status != doctor.StatusOK || r.status != doctor.StatusOK { + t.Fatalf("want both OK, got connected=%v ready=%v", c.status, r.status) + } + if c.text != "Connected to tracebloc" || r.text != "Ready to run training" { + t.Errorf("unexpected healthy text: %q / %q", c.text, r.text) + } + }) + + t.Run("unreachable → connected Fail, ready can't-check", func(t *testing.T) { + c, r := summarizeDoctor(with(allOK, "Cluster reachable", doctor.StatusFail), true) + if c.status != doctor.StatusFail { + t.Errorf("connected should Fail when unreachable, got %v", c.status) + } + if r.status != doctor.StatusUnknown { + t.Errorf("ready should be Unknown (can't check) when unreachable, got %v", r.status) + } + if !strings.Contains(r.text, "can't check") { + t.Errorf("ready text should say can't check, got %q", r.text) + } + }) + + t.Run("token unreachable → connected Fail", func(t *testing.T) { + c, _ := summarizeDoctor(allOK, false) + if c.status != doctor.StatusFail || !strings.Contains(c.text, "can't reach tracebloc") { + t.Errorf("token-unreachable → want connected Fail 'can't reach tracebloc', got %v %q", c.status, c.text) + } + }) + + t.Run("results egress down → connected Fail (experiments stall)", func(t *testing.T) { + c, _ := summarizeDoctor(with(allOK, "Service Bus egress (requests-proxy)", doctor.StatusFail), true) + if c.status != doctor.StatusFail || !strings.Contains(c.text, "results can't reach") { + t.Errorf("want connected Fail on results-egress down, got %v %q", c.status, c.text) + } + }) + + t.Run("no compute → ready Fail", func(t *testing.T) { + _, r := summarizeDoctor(with(allOK, "Node capacity", doctor.StatusFail), true) + if r.status != doctor.StatusFail || !strings.Contains(r.remedy, "Docker Desktop") { + t.Errorf("want ready Fail with a raise-allocation remedy, got %v remedy=%q", r.status, r.remedy) + } + }) + + t.Run("component down → ready Fail (reinstall/support)", func(t *testing.T) { + _, r := summarizeDoctor(with(allOK, "Pod health", doctor.StatusFail), true) + if r.status != doctor.StatusFail || !strings.Contains(r.remedy, "tracebloc.io/i.sh") { + t.Errorf("want ready Fail with a reinstall remedy, got %v remedy=%q", r.status, r.remedy) + } + }) } diff --git a/internal/cli/home_test.go b/internal/cli/home_test.go index 9cf2228e..f5949f82 100644 --- a/internal/cli/home_test.go +++ b/internal/cli/home_test.go @@ -891,8 +891,8 @@ func TestDoctor_TopLevelSharesClusterDoctor(t *testing.T) { t.Errorf("`doctor` exit = %d, want 2 (auth fail + kubeconfig fail escalates)", topErr.Code()) } for label, out := range map[string]string{"doctor": topOut, "cluster doctor": clOut} { - if !strings.Contains(out, "Auth & config") || !strings.Contains(out, "not signed in") { - t.Errorf("%s output missing the shared diagnostic (auth section + not-signed-in):\n%s", label, out) + if !strings.Contains(out, "Not signed in") { + t.Errorf("%s output missing the shared diagnostic (not-signed-in):\n%s", label, out) } } } diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 25e15677..64f3372b 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -191,9 +191,9 @@ func checkReachable(release *cluster.ParentRelease, err error, ns, serverURL str if serverURL != "" { detail = fmt.Sprintf("the cluster API server at %s isn't answering — is the cluster running?", serverURL) } - remedy := "Check the cluster is running and that your kubeconfig/context points at it." + remedy := "Check your secure environment is running." if isLoopback(serverURL) { - remedy = "This is a local cluster — start Docker Desktop (or your container runtime), then `k3d cluster start` (or your cluster's start command)." + remedy = "Start Docker Desktop (`open -a Docker`) — your secure environment restarts with it — then run this again." } return Result{Name: name, Status: StatusFail, Detail: detail, Remedy: remedy} } From c4ee89034a3e1e59be45c6e72ccc4c564db5eaf8 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 08:56:12 +0200 Subject: [PATCH 02/15] fix(doctor): classify reachability + count image-pull in readiness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the Lint failure and both Bugbot findings on #365. - Deadcode (Lint): the redesign derives the exit code in the cli layer from the two rolled-up health lines (worseStatus), so doctor.Worst is no longer reachable from the binary. Delete it (the allowlist's own guidance is "prefer deleting"); the two Run() tests that used it as an overall-verdict assertion keep a small test-local worstStatus helper. - Bugbot HIGH — no-chart misclassified as offline: a reachable cluster with no tracebloc installed was reported as "isn't answering" and handed the kubectl remedy, and readiness never appeared. checkReachable now tags the "Cluster reachable" result with a ReachState (Unreachable / NoEnv / Error); summarizeDoctor words each failure from that class — NoEnv -> "No secure environment installed here" + the one-line installer (never a kubectl), Error -> support, Unreachable (and any unclassified fail) -> "isn't answering". Legacy hand-built results default safely to Unreachable. - Bugbot MEDIUM — image-pull skipped in readiness: a failing "Image pull secret" check now rolls up to "Not ready — the training images can't be pulled" instead of a false Ready + exit 0. summarizeDoctor gains table cases for the no-env and image-pull buckets (and a guard that the no-env remedy never leaks kubectl). gofmt/vet/lint/deadcode/ file-budget green; doctor + cli packages pass. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 27 +++++++++++++--- internal/cli/doctor_test.go | 37 +++++++++++++++++++++ internal/doctor/doctor.go | 59 +++++++++++++++++++++------------- internal/doctor/doctor_test.go | 24 ++++++++------ 4 files changed, 110 insertions(+), 37 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 4fcbf25f..bd7355d4 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -189,9 +189,24 @@ func summarizeDoctor(results []doctor.Result, tokenReachable bool) (connected, r switch { case reach.Status == doctor.StatusFail: - // checkReachable already produced an endpoint-aware, plain remedy - // (start Docker for a local cluster / check it's running for a remote). - connected = healthLine{doctor.StatusFail, "Not connected — your secure environment isn't answering.", reach.Remedy} + // A failed reachability check has three very different fixes; word each + // from the classification checkReachable attached, never a kubectl. An + // unclassified fail (e.g. a hand-built result) defaults to "isn't + // answering" — the safe, pre-existing interpretation. + switch reach.Reach { + case doctor.ReachNoEnv: + connected = healthLine{doctor.StatusFail, + "No secure environment installed here.", + "Set one up: " + installCmd} + case doctor.ReachError: + connected = healthLine{doctor.StatusFail, + "Not connected — couldn't read your secure environment.", + fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} + default: // ReachUnreachable + connected = healthLine{doctor.StatusFail, + "Not connected — your secure environment isn't answering.", + reach.Remedy} + } case !tokenReachable || by["Backend egress (from this machine)"].Status == doctor.StatusFail: connected = healthLine{doctor.StatusFail, "Not connected — can't reach tracebloc from here.", @@ -206,7 +221,7 @@ func summarizeDoctor(results []doctor.Result, tokenReachable bool) (connected, r // Readiness is meaningless if we can't even reach the environment. if reach.Status == doctor.StatusFail { - ready = healthLine{doctor.StatusUnknown, "Ready to run training — can't check until it's connected", ""} + ready = healthLine{doctor.StatusUnknown, "Ready to run training — can't check yet", ""} return connected, ready } switch { @@ -214,6 +229,10 @@ func summarizeDoctor(results []doctor.Result, tokenReachable bool) (connected, r ready = healthLine{doctor.StatusFail, "Not ready — part of your secure environment isn't running.", fmt.Sprintf("Reinstall with `%s`, or email support@tracebloc.io with `%s doctor --diagnose`.", installCmd, launcher())} + case by["Image pull secret"].Status == doctor.StatusFail: + ready = healthLine{doctor.StatusFail, + "Not ready — the training images can't be pulled.", + fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} case by["Dataset volume (PVC)"].Status == doctor.StatusFail: ready = healthLine{doctor.StatusFail, "Not ready — dataset storage isn't available.", diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 3ca4cecf..5260fd68 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -215,4 +215,41 @@ func TestSummarizeDoctor(t *testing.T) { t.Errorf("want ready Fail with a reinstall remedy, got %v remedy=%q", r.status, r.remedy) } }) + + // A reachable cluster with no tracebloc installed must NOT be reported as + // "isn't answering" with a kubectl remedy — it's a reinstall (Bugbot #365). + t.Run("no environment installed → connected Fail (installer, not kubectl)", func(t *testing.T) { + noEnv := with(allOK, "Cluster reachable", doctor.StatusFail) + for i := range noEnv { + if noEnv[i].Name == "Cluster reachable" { + noEnv[i].Reach = doctor.ReachNoEnv + } + } + c, r := summarizeDoctor(noEnv, true) + if c.status != doctor.StatusFail || !strings.Contains(c.text, "No secure environment") { + t.Errorf("no-env → want connected Fail 'No secure environment', got %v %q", c.status, c.text) + } + if !strings.Contains(c.remedy, "tracebloc.io/i.sh") { + t.Errorf("no-env remedy should be the installer, got %q", c.remedy) + } + if strings.Contains(c.remedy, "kubectl") { + t.Errorf("no-env remedy must not leak kubectl, got %q", c.remedy) + } + if r.status != doctor.StatusUnknown { + t.Errorf("no-env → ready should be can't-check (Unknown), got %v", r.status) + } + }) + + // A failing image-pull check means training images can't be fetched — that + // is not-ready, and must not be silently dropped from the rollup (Bugbot #365). + t.Run("images can't be pulled → ready Fail", func(t *testing.T) { + withPull := append(append([]doctor.Result{}, allOK...), res("Image pull secret", doctor.StatusFail)) + _, r := summarizeDoctor(withPull, true) + if r.status != doctor.StatusFail || !strings.Contains(r.text, "images can't be pulled") { + t.Errorf("image-pull down → want ready Fail 'images can't be pulled', got %v %q", r.status, r.text) + } + if !strings.Contains(r.remedy, "--diagnose") { + t.Errorf("image-pull remedy should point at --diagnose, got %q", r.remedy) + } + }) } diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 64f3372b..329b8ce2 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -33,7 +33,7 @@ import ( ) // Status is a single check's severity. Ordered so the numerically-greatest -// status is the worst — see Worst. +// status is the worst; the cli layer rolls the checks up into the verdict. type Status int const ( @@ -44,9 +44,10 @@ const ( // StatusUnknown marks a check that could not run because a prerequisite was // unavailable — today, the cluster API being unreachable. It carries NO signal: -// Worst() ignores it, so it never affects the overall verdict or exit code. It -// exists so a single root cause (a stopped cluster) renders as one honest ✖ plus -// neutral "couldn't check" lines, instead of every check inventing a false cause. +// the verdict rollup ignores it, so it never affects the overall result or exit +// code. It exists so a single root cause (a stopped cluster) renders as one +// honest ✖ plus neutral "couldn't check" lines, instead of every check inventing +// a false cause. const StatusUnknown Status = -1 func (s Status) String() string { @@ -71,23 +72,27 @@ type Result struct { Status Status Detail string Remedy string -} -// Worst returns the most severe status across results (StatusOK for none). -// The doctor command maps a non-OK worst to a non-zero exit code. -func Worst(results []Result) Status { - worst := StatusOK - for _, r := range results { - if r.Status == StatusUnknown { - continue // no signal — a "couldn't check" never sets the verdict - } - if r.Status > worst { - worst = r.Status - } - } - return worst + // Reach classifies WHY the "Cluster reachable" check landed where it did, so + // the cli summary can word the remedy correctly — an unreachable API (stopped + // container runtime / network) needs a different fix than a reachable cluster + // with no tracebloc installed. Zero (ReachOK) on every other check. + Reach ReachState } +// ReachState classifies the "Cluster reachable" outcome so the cli summary can +// word its remedy correctly (see Result.Reach). A stopped API, a reachable +// cluster with no tracebloc installed, and an RBAC/other error each need a +// different plain-language fix — never the same "isn't answering" line. +type ReachState int + +const ( + ReachOK ReachState = iota // API answered and the tracebloc chart is here + ReachUnreachable // API never answered (container runtime/network down) + ReachNoEnv // API answered, but no tracebloc secure environment here + ReachError // API answered with some other error (RBAC/NotFound/…) +) + // Conservative tunables, kept as package consts to avoid false positives on a // busy cluster. If a check ever needs runtime tuning, thread it through Options // (like HTTPProbe) rather than making these package vars. @@ -127,8 +132,8 @@ type Options struct { // Run executes every check in display order and returns their results. It // never returns an error: an unreachable cluster or a failing probe is a -// Result, not a Go error — the command renders all of them and derives the -// exit code from Worst. +// Result, not a Go error — the cli layer rolls them up into the two health +// lines the owner reads and derives the exit code from that verdict. func Run(ctx context.Context, cs kubernetes.Interface, opts Options) []Result { if opts.HTTPProbe == nil { opts.HTTPProbe = httpProbe @@ -195,18 +200,26 @@ func checkReachable(release *cluster.ParentRelease, err error, ns, serverURL str if isLoopback(serverURL) { remedy = "Start Docker Desktop (`open -a Docker`) — your secure environment restarts with it — then run this again." } - return Result{Name: name, Status: StatusFail, Detail: detail, Remedy: remedy} + return Result{Name: name, Status: StatusFail, Detail: detail, Remedy: remedy, Reach: ReachUnreachable} } // API answered but no chart here (ErrNoParentRelease) or another error. // The discovery error's remediation tail points at doctor — which is // what's running. Strip it so doctor never tells the user to run doctor. // (Must match the exact suffix cluster.discover appends.) detail := strings.TrimSuffix(strings.TrimSpace(err.Error()), "Diagnose with `tracebloc doctor`.") + // The API answered: a missing chart means "no environment installed here" + // (fix: reinstall), anything else is an RBAC/NotFound-class error (fix: + // support). The cli summary words each differently — never a kubectl. + reach := ReachError + if errors.Is(err, cluster.ErrNoParentRelease) { + reach = ReachNoEnv + } return Result{ Name: name, Status: StatusFail, Detail: strings.TrimSpace(detail), Remedy: "Check your kubeconfig/context and that the tracebloc client chart is installed here: kubectl get deploy -n " + ns, + Reach: reach, } } return Result{ @@ -277,8 +290,8 @@ func isLoopback(serverURL string) bool { } // unknownCheck is the placeholder for a cluster check that could not run because -// the API is unreachable — no signal, so Worst() ignores it and the verdict -// comes from "Cluster reachable" alone. +// the API is unreachable — no signal, so the verdict rollup ignores it and the +// verdict comes from "Cluster reachable" alone. func unknownCheck(name string) Result { return Result{ Name: name, diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index b1f6b34b..de4c2066 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -157,14 +157,18 @@ func initCrashPod(name string) *corev1.Pod { } } -func TestWorst(t *testing.T) { - if got := Worst(nil); got != StatusOK { - t.Fatalf("Worst(nil) = %v, want ok", got) - } - rs := []Result{{Status: StatusOK}, {Status: StatusFail}, {Status: StatusWarn}} - if got := Worst(rs); got != StatusFail { - t.Fatalf("Worst = %v, want fail", got) +// worstStatus mirrors the overall-verdict rollup for the Run() tests below: the +// most severe status across results, StatusUnknown ("couldn't check") ignored. +// Production derives the exit code in the cli layer from the two rolled-up health +// lines; this keeps the package tests' verdict assertions concise. +func worstStatus(results []Result) Status { + worst := StatusOK + for _, r := range results { + if r.Status != StatusUnknown && r.Status > worst { + worst = r.Status + } } + return worst } func TestCheckReachable(t *testing.T) { @@ -243,8 +247,8 @@ func TestRun_UnreachableCascade(t *testing.T) { if r := byName["Backend egress (from this machine)"]; r.Status != StatusOK { t.Errorf("Backend egress = %v, want ok (probed from this machine, independent of the cluster API)", r.Status) } - if w := Worst(results); w != StatusFail { - t.Fatalf("Worst = %v, want fail (verdict from the one real ✖, StatusUnknown ignored)", w) + if w := worstStatus(results); w != StatusFail { + t.Fatalf("worst = %v, want fail (verdict from the one real ✖, StatusUnknown ignored)", w) } } @@ -491,7 +495,7 @@ func TestRun_HealthyCluster(t *testing.T) { if len(results) != 9 { t.Fatalf("want 9 checks, got %d", len(results)) } - if w := Worst(results); w != StatusOK { + if w := worstStatus(results); w != StatusOK { for _, r := range results { t.Logf("%-32s %-4s %s", r.Name, r.Status, r.Detail) } From 5e424bfc04f3222c3b6388644bc035a6b87ebece Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 09:05:05 +0200 Subject: [PATCH 03/15] fix(doctor): treat 0.0.0.0 / host.docker.internal as a local cluster MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot MEDIUM on #365: isLoopback only matched localhost / 127.0.0.0/8 / ::1, so a stopped k3d cluster whose kubeconfig advertises 0.0.0.0 (the host k3d writes when the installer pins no explicit --api-port host — the bash install-k8s.sh path, unlike the Windows .ps1 which pins 127.0.0.1:6550) fell through to the generic "check your secure environment is running" line instead of the redesign's main local-failure remedy, "Start Docker Desktop". isLoopback now also treats the unspecified/wildcard bind addresses (0.0.0.0, ::) and Docker Desktop's host alias (host.docker.internal) as local. None of these is ever a genuinely-remote endpoint, so there's no false-positive risk. Tests: a dedicated TestIsLoopback table (local vs remote hosts) and a checkReachable assertion that a stopped 0.0.0.0 endpoint gets the start-Docker remedy + ReachUnreachable. Also locks in the doctor-side reach classification that feeds the summary rollup — ReachNoEnv for a missing chart, ReachError otherwise. Co-Authored-By: Claude Opus 4.8 --- internal/doctor/doctor.go | 19 ++++++++----- internal/doctor/doctor_test.go | 51 ++++++++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 329b8ce2..ed134196 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -268,9 +268,14 @@ func isUnreachable(err error) bool { return false } -// isLoopback reports whether serverURL points at the local machine (127.0.0.1 / -// localhost / ::1) — a k3d/kind/Docker-Desktop cluster, whose "isn't answering" -// almost always means the container runtime or the cluster is simply stopped. +// isLoopback reports whether serverURL points at the local machine — a +// k3d/kind/Docker-Desktop cluster, whose "isn't answering" almost always means +// the container runtime or the cluster is simply stopped, so the remedy is +// "start Docker Desktop". It covers the loopback addresses (127.0.0.0/8, ::1, +// localhost), the unspecified/wildcard bind addresses k3d writes into a +// kubeconfig when no explicit host is pinned (0.0.0.0, ::), and Docker Desktop's +// host alias (host.docker.internal). No genuinely-remote cluster is ever reached +// through any of these, so treating them as local carries no false-positive risk. func isLoopback(serverURL string) bool { if serverURL == "" { return false @@ -279,12 +284,12 @@ func isLoopback(serverURL string) bool { if err != nil { return false } - host := u.Hostname() - if host == "localhost" { + switch u.Hostname() { + case "localhost", "host.docker.internal": return true } - if ip := net.ParseIP(host); ip != nil { - return ip.IsLoopback() + if ip := net.ParseIP(u.Hostname()); ip != nil { + return ip.IsLoopback() || ip.IsUnspecified() } return false } diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index de4c2066..be6ee50e 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -172,9 +172,15 @@ func worstStatus(results []Result) Status { } func TestCheckReachable(t *testing.T) { - // A non-transport error (e.g. no chart / RBAC) keeps the kubeconfig+chart remedy. - if r := checkReachable(nil, errors.New("boom"), ns, ""); r.Status != StatusFail { - t.Fatalf("error => %v, want fail", r.Status) + // A non-transport, non-chart error (e.g. RBAC) keeps the kubeconfig+chart + // remedy and classifies as ReachError. + if r := checkReachable(nil, errors.New("boom"), ns, ""); r.Status != StatusFail || r.Reach != ReachError { + t.Fatalf("other error => status %v reach %v, want fail/ReachError", r.Status, r.Reach) + } + // No chart installed (the API answered) classifies as ReachNoEnv so the + // summary points at the installer, never a kubectl (Bugbot #365). + if r := checkReachable(nil, cluster.ErrNoParentRelease, ns, ""); r.Reach != ReachNoEnv { + t.Fatalf("no-chart => reach %v, want ReachNoEnv", r.Reach) } rel := &cluster.ParentRelease{ReleaseName: "tb", ChartVersion: "1.3.5", AppVersion: "1.3.5"} r := checkReachable(rel, nil, ns, "") @@ -194,6 +200,45 @@ func TestCheckReachable(t *testing.T) { if !strings.Contains(tr.Remedy, "start") || strings.Contains(tr.Remedy, "kubectl get deploy") { t.Fatalf("transport remedy = %q, want a start-the-cluster hint, not the chart remedy", tr.Remedy) } + + // A stopped k3d cluster advertised as 0.0.0.0 (the kubeconfig host when the + // installer pins no explicit --api-port host) must still get the start-Docker + // remedy, not the generic "check it's running" line (Bugbot #365). + wc := checkReachable(nil, errors.New(`Get "https://0.0.0.0:6550/api": dial tcp 0.0.0.0:6550: connect: connection refused`), ns, "https://0.0.0.0:6550") + if wc.Reach != ReachUnreachable { + t.Fatalf("0.0.0.0 transport => reach %v, want ReachUnreachable", wc.Reach) + } + if !strings.Contains(wc.Remedy, "Docker Desktop") { + t.Fatalf("0.0.0.0 remedy = %q, want the start-Docker-Desktop hint", wc.Remedy) + } +} + +// TestIsLoopback covers every kubeconfig host that means "the cluster is local, +// so the fix is start Docker Desktop": the loopback addresses, the wildcard bind +// addresses k3d writes when no host is pinned (0.0.0.0, ::), and Docker Desktop's +// host alias — but never a genuinely-remote endpoint (Bugbot #365). +func TestIsLoopback(t *testing.T) { + for _, s := range []string{ + "https://127.0.0.1:6550", + "https://localhost:6550", + "https://[::1]:6550", + "https://0.0.0.0:6550", + "https://[::]:6550", + "https://host.docker.internal:6550", + } { + if !isLoopback(s) { + t.Errorf("isLoopback(%q) = false, want true (local cluster)", s) + } + } + for _, s := range []string{ + "https://api.k8s.example.com:6443", + "https://10.1.2.3:6443", + "", + } { + if isLoopback(s) { + t.Errorf("isLoopback(%q) = true, want false (not a local cluster)", s) + } + } } // TestRun_UnreachableCascade mimics the reported failure: the cluster API is From bdf0962a621f4c98c6bd2d09d930e1d4bb438e41 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 09:15:32 +0200 Subject: [PATCH 04/15] fix(doctor): don't blame the network for a server error; gate readiness on connected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Bugbot findings on #365: - WhoAmI misclassification: every non-401/426 error set tokenReachable=false, so a backend that ANSWERED with an error (5xx/403/decode) was rolled up as "can't reach tracebloc from here" with a network/proxy remedy — contradicting a green Backend-egress check and exiting 2 for a tracebloc-side problem. Split the WhoAmI outcome into a tokenState: tokenUnreachable (genuine transport failure -> network remedy) vs tokenServerErr (the backend answered, just not 200 -> "tracebloc didn't confirm your session (server error)", retry / email support, never a proxy remedy). - False readiness check: readiness only degraded to "can't check" when Cluster reachable failed, so other Connected failures (Service Bus down, backend/token unreachable, server error) still allowed a green "Ready to run training" next to a Connected ✖ — even though training can't complete while disconnected. Readiness now degrades whenever Connected is not OK. Tests: TestSummarizeDoctor gains a tokenServerErr case (asserts support remedy, not a network/proxy one) and a "disconnected but cluster healthy" case (ready must not be a green check under a Connected ✖). All 8 existing call sites move from the bool to the tokenState enum. gofmt/vet/lint/deadcode green; doctor + cli packages pass. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 46 ++++++++++++++++++++++++++--------- internal/cli/doctor_test.go | 48 ++++++++++++++++++++++++++++++------- 2 files changed, 75 insertions(+), 19 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index bd7355d4..7b7dae3c 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -99,9 +99,12 @@ func runClusterDoctor( } // 2. Is the session still good with tracebloc? A 401 (expired/revoked) or a - // 426 (CLI too old) is a hard stop with a one-command fix. A network error - // is NOT fatal — it folds into the Connected line below. - tokenReachable := true + // 426 (CLI too old) is a hard stop with a one-command fix. Anything else + // folds into the Connected line below — but a backend that ANSWERED with + // an error (5xx/403/decode) is a tracebloc-side problem, distinct from a + // network failure to reach it at all. Conflating the two would blame the + // user's network (and hand them a proxy remedy) for tracebloc's own error. + tok := tokenOK apiClient := newAPIClient(cfg.CurrentEnv) apiClient.Token = cfg.Current().Token if _, werr := apiClient.WhoAmI(ctx); werr != nil { @@ -116,8 +119,10 @@ func runClusterDoctor( p.Newline() p.Errorf("This CLI is out of date — update it: %s", installCmd) return &exitError{code: exitChecksFailed, err: nil} + case errors.As(werr, &ae): + tok = tokenServerErr // tracebloc answered, just not with 200 default: - tokenReachable = false // can't reach tracebloc from here — Connected will say so + tok = tokenUnreachable // couldn't reach tracebloc at all (network/proxy) } } @@ -149,7 +154,7 @@ func runClusterDoctor( // 5. Roll the granular checks up into two plain lines the owner can act on. p.Newline() - connected, ready := summarizeDoctor(results, tokenReachable) + connected, ready := summarizeDoctor(results, tok) renderHealth(p, connected) renderHealth(p, ready) @@ -175,12 +180,25 @@ type healthLine struct { remedy string } +// tokenState classifies the WhoAmI probe for the Connected rollup: the session +// confirmed (tokenOK), the backend unreachable from this machine (tokenUnreachable +// — network/proxy), or the backend reachable but answering with an error +// (tokenServerErr — 5xx/403/decode, a tracebloc-side problem, not the network). +type tokenState int + +const ( + tokenOK tokenState = iota + tokenUnreachable + tokenServerErr +) + // summarizeDoctor collapses the granular checks into the two lines the owner // reads: "Connected to tracebloc" and "Ready to run training". Each expands to // the specific plain-language problem + fix on failure; the Kubernetes detail -// stays in --verbose. When the environment is unreachable, readiness can't be -// assessed, so it degrades honestly to "can't check" (never a false ✔). -func summarizeDoctor(results []doctor.Result, tokenReachable bool) (connected, ready healthLine) { +// stays in --verbose. When the owner isn't connected — for ANY reason — a +// healthy local cluster still can't run training, so readiness degrades honestly +// to "can't check" rather than showing a reassuring ✔ next to a Connected ✖. +func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready healthLine) { by := make(map[string]doctor.Result, len(results)) for _, r := range results { by[r.Name] = r @@ -207,10 +225,14 @@ func summarizeDoctor(results []doctor.Result, tokenReachable bool) (connected, r "Not connected — your secure environment isn't answering.", reach.Remedy} } - case !tokenReachable || by["Backend egress (from this machine)"].Status == doctor.StatusFail: + case tok == tokenUnreachable || by["Backend egress (from this machine)"].Status == doctor.StatusFail: connected = healthLine{doctor.StatusFail, "Not connected — can't reach tracebloc from here.", fmt.Sprintf("Check your network / HTTP(S)_PROXY, then run `%s doctor` again.", launcher())} + case tok == tokenServerErr: + connected = healthLine{doctor.StatusFail, + "Not connected — tracebloc didn't confirm your session (server error).", + fmt.Sprintf("Try again shortly; if it persists, email support@tracebloc.io with `%s doctor --diagnose`.", launcher())} case by["Service Bus egress (requests-proxy)"].Status == doctor.StatusFail: connected = healthLine{doctor.StatusFail, "Training results can't reach tracebloc — experiments will stall.", @@ -219,8 +241,10 @@ func summarizeDoctor(results []doctor.Result, tokenReachable bool) (connected, r connected = healthLine{doctor.StatusOK, "Connected to tracebloc", ""} } - // Readiness is meaningless if we can't even reach the environment. - if reach.Status == doctor.StatusFail { + // Readiness only means something once we're connected: a healthy local + // cluster still can't run training while it can't reach tracebloc. If + // Connected is anything but OK, degrade honestly — never a green ✔ under a ✖. + if connected.status != doctor.StatusOK { ready = healthLine{doctor.StatusUnknown, "Ready to run training — can't check yet", ""} return connected, ready } diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 5260fd68..e16fc8a7 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -166,7 +166,7 @@ func TestSummarizeDoctor(t *testing.T) { } t.Run("all healthy → both OK", func(t *testing.T) { - c, r := summarizeDoctor(allOK, true) + c, r := summarizeDoctor(allOK, tokenOK) if c.status != doctor.StatusOK || r.status != doctor.StatusOK { t.Fatalf("want both OK, got connected=%v ready=%v", c.status, r.status) } @@ -176,7 +176,7 @@ func TestSummarizeDoctor(t *testing.T) { }) t.Run("unreachable → connected Fail, ready can't-check", func(t *testing.T) { - c, r := summarizeDoctor(with(allOK, "Cluster reachable", doctor.StatusFail), true) + c, r := summarizeDoctor(with(allOK, "Cluster reachable", doctor.StatusFail), tokenOK) if c.status != doctor.StatusFail { t.Errorf("connected should Fail when unreachable, got %v", c.status) } @@ -189,28 +189,28 @@ func TestSummarizeDoctor(t *testing.T) { }) t.Run("token unreachable → connected Fail", func(t *testing.T) { - c, _ := summarizeDoctor(allOK, false) + c, _ := summarizeDoctor(allOK, tokenUnreachable) if c.status != doctor.StatusFail || !strings.Contains(c.text, "can't reach tracebloc") { t.Errorf("token-unreachable → want connected Fail 'can't reach tracebloc', got %v %q", c.status, c.text) } }) t.Run("results egress down → connected Fail (experiments stall)", func(t *testing.T) { - c, _ := summarizeDoctor(with(allOK, "Service Bus egress (requests-proxy)", doctor.StatusFail), true) + c, _ := summarizeDoctor(with(allOK, "Service Bus egress (requests-proxy)", doctor.StatusFail), tokenOK) if c.status != doctor.StatusFail || !strings.Contains(c.text, "results can't reach") { t.Errorf("want connected Fail on results-egress down, got %v %q", c.status, c.text) } }) t.Run("no compute → ready Fail", func(t *testing.T) { - _, r := summarizeDoctor(with(allOK, "Node capacity", doctor.StatusFail), true) + _, r := summarizeDoctor(with(allOK, "Node capacity", doctor.StatusFail), tokenOK) if r.status != doctor.StatusFail || !strings.Contains(r.remedy, "Docker Desktop") { t.Errorf("want ready Fail with a raise-allocation remedy, got %v remedy=%q", r.status, r.remedy) } }) t.Run("component down → ready Fail (reinstall/support)", func(t *testing.T) { - _, r := summarizeDoctor(with(allOK, "Pod health", doctor.StatusFail), true) + _, r := summarizeDoctor(with(allOK, "Pod health", doctor.StatusFail), tokenOK) if r.status != doctor.StatusFail || !strings.Contains(r.remedy, "tracebloc.io/i.sh") { t.Errorf("want ready Fail with a reinstall remedy, got %v remedy=%q", r.status, r.remedy) } @@ -225,7 +225,7 @@ func TestSummarizeDoctor(t *testing.T) { noEnv[i].Reach = doctor.ReachNoEnv } } - c, r := summarizeDoctor(noEnv, true) + c, r := summarizeDoctor(noEnv, tokenOK) if c.status != doctor.StatusFail || !strings.Contains(c.text, "No secure environment") { t.Errorf("no-env → want connected Fail 'No secure environment', got %v %q", c.status, c.text) } @@ -244,7 +244,7 @@ func TestSummarizeDoctor(t *testing.T) { // is not-ready, and must not be silently dropped from the rollup (Bugbot #365). t.Run("images can't be pulled → ready Fail", func(t *testing.T) { withPull := append(append([]doctor.Result{}, allOK...), res("Image pull secret", doctor.StatusFail)) - _, r := summarizeDoctor(withPull, true) + _, r := summarizeDoctor(withPull, tokenOK) if r.status != doctor.StatusFail || !strings.Contains(r.text, "images can't be pulled") { t.Errorf("image-pull down → want ready Fail 'images can't be pulled', got %v %q", r.status, r.text) } @@ -252,4 +252,36 @@ func TestSummarizeDoctor(t *testing.T) { t.Errorf("image-pull remedy should point at --diagnose, got %q", r.remedy) } }) + + // A backend that ANSWERED with an error (5xx/403/decode) is a tracebloc-side + // problem — it must not be blamed on the user's network with a proxy remedy + // (Bugbot #365). + t.Run("backend answered with an error → connected Fail (support, not network)", func(t *testing.T) { + c, r := summarizeDoctor(allOK, tokenServerErr) + if c.status != doctor.StatusFail || !strings.Contains(c.text, "server error") { + t.Errorf("server-err → want connected Fail 'server error', got %v %q", c.status, c.text) + } + if strings.Contains(c.remedy, "PROXY") || strings.Contains(c.remedy, "network") { + t.Errorf("server-err remedy must not blame the network, got %q", c.remedy) + } + if !strings.Contains(c.remedy, "--diagnose") { + t.Errorf("server-err remedy should point at support/--diagnose, got %q", c.remedy) + } + if r.status != doctor.StatusUnknown { + t.Errorf("server-err → ready should be can't-check (Unknown), got %v", r.status) + } + }) + + // Disconnected but the local cluster is healthy: Ready must NOT show a green + // ✔ next to a Connected ✖ — training can't complete while disconnected + // (Bugbot #365). + t.Run("disconnected but cluster healthy → ready not a false check", func(t *testing.T) { + c, r := summarizeDoctor(with(allOK, "Service Bus egress (requests-proxy)", doctor.StatusFail), tokenOK) + if c.status != doctor.StatusFail { + t.Fatalf("precondition: want connected Fail (service bus down), got %v", c.status) + } + if r.status == doctor.StatusOK { + t.Errorf("ready must not be a green check while disconnected, got OK %q", r.text) + } + }) } From 066270cf9a554552ea7e4999fe46323b9f2b3b2a Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 09:29:01 +0200 Subject: [PATCH 05/15] fix(doctor): stop early returns from dropping detected signals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Bugbot findings on #365, both the same anti-pattern — an early return throwing away something doctor already learned: - --diagnose skipped the roll-up: it returned right after writing the bundle, so the file recorded only the raw k8s checks (never the WhoAmI/session outcome or the Connected/Ready verdict), and it always exited 0 even when checks failed — while the remedies tell owners to email that very bundle for session/server errors. --diagnose is now additive: it runs summarizeDoctor, writes session + verdict + detail into the bundle, and falls through to the real verdict/exit code (2 when something's wrong, not a misleading 0). - Kubeconfig/clientset failure hid a session fault: after WhoAmI set tokenUnreachable/tokenServerErr, a failed local-env read returned with only "no / can't connect to secure environment", steering owners to reinstall while a live backend/session problem went unmentioned. Both no-env branches now surface the session fault first via noteSessionProblem (a no-op when the session is fine; 401/426 remain hard stops upstream). Adds tokenLabel (bundle session line) + a TestRunClusterDoctor_DiagnoseBundle that asserts the bundle records session/connected/ready and that a failing check still exits 2 under --diagnose. gofmt/vet/lint/deadcode green; doctor + cli packages pass. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 72 +++++++++++++++++++++++------ internal/cli/doctor_cluster_test.go | 66 ++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 14 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 7b7dae3c..684fcfa1 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -126,12 +126,17 @@ func runClusterDoctor( } } - // 3. Find the secure environment (local kubeconfig read). + // 3. Find the secure environment (local kubeconfig read). If it isn't here, + // that's the headline — but surface a session/backend fault we already + // found first, so a reinstall is never recommended while a live session + // problem goes unexplained. (A 401/426 is a hard stop earlier; only the + // soft tokenUnreachable/tokenServerErr states reach here.) opts := cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride, Namespace: nsOverride} bindActiveClientNamespace(&opts) resolved, err := loadClusterFn(opts) if err != nil { p.Newline() + noteSessionProblem(p, tok) p.Errorf("No secure environment on this machine yet.") p.Hintf(" Set one up: %s", installCmd) return &exitError{code: exitLocalEnv, err: nil} @@ -139,33 +144,40 @@ func runClusterDoctor( cs, err := newClientsetFn(resolved) if err != nil { p.Newline() + noteSessionProblem(p, tok) p.Errorf("Couldn't connect to your secure environment — check your kubeconfig/context.") return &exitError{code: exitLocalEnv, err: nil} } p.Para(fmt.Sprintf("Secure environment %q", envDisplayName(resolved))) - // 4. Probe the cluster (the granular checks stay for --verbose + --diagnose). + // 4. Probe the cluster and roll the granular checks up into the two plain + // lines the owner acts on (the granular results stay for --verbose/--diagnose). results := doctorRunFn(ctx, cs, doctor.Options{Namespace: resolved.Namespace, ServerURL: resolved.ServerURL}) + connected, ready := summarizeDoctor(results, tok) - // --diagnose: write the full technical detail to a file for support, then stop. - if diagnose { - return writeDiagnoseBundle(p, resolved, results) - } - - // 5. Roll the granular checks up into two plain lines the owner can act on. p.Newline() - connected, ready := summarizeDoctor(results, tok) renderHealth(p, connected) renderHealth(p, ready) - if p.Verbose() { renderDoctorDetails(p, resolved, results) } - // 6. Verdict + exit code (unchanged: 0 healthy, 2 a problem). + // 5. --diagnose: write the full bundle (session + verdict + technical detail) + // for support. It's additive — the run still ends on the real verdict and + // exit code below, so `--diagnose` never masks a failing check as a 0. + if diagnose { + p.Newline() + if werr := writeDiagnoseBundle(p, resolved, results, tok, connected, ready); werr != nil { + return werr + } + } + + // 6. Verdict + exit code (0 healthy, 2 a problem). p.Newline() if worseStatus(connected.status, ready.status) == doctor.StatusFail { - p.Hintf("Still stuck? Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher()) + if !diagnose { // they just wrote a bundle — don't send them to write it again + p.Hintf("Still stuck? Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher()) + } return &exitError{code: exitChecksFailed, err: nil} } p.Successf("Everything looks good — you're ready to run training.") @@ -192,6 +204,33 @@ const ( tokenServerErr ) +// noteSessionProblem prints a soft WhoAmI/session fault (unreachable backend or a +// server-side error) as a standalone ✖ + fix. Used on the no-local-environment +// path so a fault detected before the kubeconfig read isn't silently dropped when +// that read fails. It's a no-op when the session is fine (tokenOK). +func noteSessionProblem(p *ui.Printer, tok tokenState) { + switch tok { + case tokenUnreachable: + p.Errorf("Can't reach tracebloc from here.") + p.Hintf(" Check your network / HTTP(S)_PROXY, then run `%s doctor` again.", launcher()) + case tokenServerErr: + p.Errorf("tracebloc didn't confirm your session (server error).") + p.Hintf(" Try again shortly; if it persists, email support@tracebloc.io with `%s doctor --diagnose`.", launcher()) + } +} + +// tokenLabel is the one-line session status recorded in the support bundle. +func tokenLabel(tok tokenState) string { + switch tok { + case tokenUnreachable: + return "backend unreachable from this machine (network/proxy)" + case tokenServerErr: + return "backend answered with an error (5xx/403/decode)" + default: + return "confirmed" + } +} + // summarizeDoctor collapses the granular checks into the two lines the owner // reads: "Connected to tracebloc" and "Ready to run training". Each expands to // the specific plain-language problem + fix on failure; the Kubernetes detail @@ -316,10 +355,15 @@ func renderDoctorDetails(p *ui.Printer, resolved *cluster.ResolvedConfig, result // writeDiagnoseBundle owns the support bundle (moved out of install-k8s.sh, which // the user may not have on disk). It writes the redacted technical breakdown to a // file the owner emails to support. -func writeDiagnoseBundle(p *ui.Printer, resolved *cluster.ResolvedConfig, results []doctor.Result) error { +func writeDiagnoseBundle(p *ui.Printer, resolved *cluster.ResolvedConfig, results []doctor.Result, tok tokenState, connected, ready healthLine) error { var buf bytes.Buffer - fmt.Fprintf(&buf, "tracebloc doctor — support bundle (%s)\n", time.Now().Format(time.RFC3339)) + fmt.Fprintf(&buf, "tracebloc doctor — support bundle (%s)\n\n", time.Now().Format(time.RFC3339)) bp := ui.New(&buf, ui.WithColor(false), ui.WithVerbose(true)) + // Session + rolled-up verdict first — the state support reads before the raw + // Kubernetes detail, and the reason each remedy sends them here. + bp.Detailf("session: %s", tokenLabel(tok)) + bp.Detailf("connected: %s — %s", connected.status, connected.text) + bp.Detailf("ready: %s — %s", ready.status, ready.text) renderDoctorDetails(bp, resolved, results) name := fmt.Sprintf("tracebloc-doctor-%s.txt", time.Now().Format("20060102-150405")) diff --git a/internal/cli/doctor_cluster_test.go b/internal/cli/doctor_cluster_test.go index e60fdfe3..4a3077cb 100644 --- a/internal/cli/doctor_cluster_test.go +++ b/internal/cli/doctor_cluster_test.go @@ -5,6 +5,8 @@ import ( "context" "errors" "net/http" + "os" + "path/filepath" "strings" "testing" @@ -179,3 +181,67 @@ func TestRunClusterDoctor_ClientsetError(t *testing.T) { t.Errorf("want the plain connect-failure message:\n%s", buf.String()) } } + +// --diagnose writes a support bundle that records the session + rolled-up verdict +// (not just k8s detail) and still exits on the REAL verdict — never a misleading +// 0 when checks failed (Bugbot #365). +func TestRunClusterDoctor_DiagnoseBundle(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ + "dev": {Token: "x", Email: "a@b.io", ActiveClientID: "5"}, + }}).Save(); err != nil { + t.Fatal(err) + } + stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // session healthy + }) + withClusterSeams(t, fake.NewSimpleClientset()) + // A failing check → the verdict is a problem, so exit must be 2, not a bare 0. + withDoctorRun(t, []doctor.Result{ + {Name: "Cluster reachable", Status: doctor.StatusOK}, + {Name: "Pod health", Status: doctor.StatusFail, Detail: "a pod is CrashLoopBackOff"}, + }) + + // writeDiagnoseBundle writes into the CWD — run it inside a temp dir. + tmp := t.TempDir() + orig, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(tmp); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(orig) }) + + var buf bytes.Buffer + err = runClusterDoctor(context.Background(), ui.New(&buf, ui.WithColor(false)), "", "", "", true) + + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("diagnose with a failing check → want exit 2 (real verdict), got %v", err) + } + if !strings.Contains(buf.String(), "Wrote a support bundle") { + t.Errorf("want a 'wrote bundle' confirmation, got:\n%s", buf.String()) + } + + // The bundle exists and records the session + verdict, not just the k8s detail. + entries, _ := os.ReadDir(tmp) + var bundle string + for _, e := range entries { + if strings.HasPrefix(e.Name(), "tracebloc-doctor-") { + bundle = filepath.Join(tmp, e.Name()) + } + } + if bundle == "" { + t.Fatalf("no support bundle written to %s (entries: %v)", tmp, entries) + } + b, err := os.ReadFile(bundle) + if err != nil { + t.Fatal(err) + } + for _, want := range []string{"session:", "connected:", "ready:", "Pod health"} { + if !strings.Contains(string(b), want) { + t.Errorf("bundle missing %q; got:\n%s", want, string(b)) + } + } +} From 3a099230c4275feca068770a1fbbeb9120b0a12d Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 10:16:48 +0200 Subject: [PATCH 06/15] fix(doctor): don't let the indicative backend-egress probe contradict WhoAmI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot on #365: the Connected roll-up ORed tokenUnreachable with a failing "Backend egress (from this machine)" check, so after a successful WhoAmI a backend-egress probe miss (a different CLIENT_ENV host, a transient miss) still rendered "Not connected — can't reach tracebloc from here" + a check-your-network remedy, contradicting the session probe that had just reached the backend. WhoAmI is the definitive from-this-machine reachability+auth signal; the backend-egress check is explicitly indicative-not-definitive (it probes the cluster's configured host from here, not the cluster's real egress path). So it no longer feeds the Connected headline — it stays a --verbose/--diagnose diagnostic. Connected now keys on tok (+ reachability + Service Bus egress). Test: a backend-egress miss with a healthy session keeps Connected OK and never prints the network remedy. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 8 +++++++- internal/cli/doctor_test.go | 13 +++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 684fcfa1..28f795fc 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -264,7 +264,13 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready "Not connected — your secure environment isn't answering.", reach.Remedy} } - case tok == tokenUnreachable || by["Backend egress (from this machine)"].Status == doctor.StatusFail: + case tok == tokenUnreachable: + // WhoAmI is the definitive "can this machine reach tracebloc" signal, so it + // alone drives this line. The "Backend egress (from this machine)" check is + // explicitly indicative-not-definitive (it probes the cluster's configured + // host from here, not the cluster's real egress path), so it must NOT flip + // Connected to a network error after WhoAmI already succeeded — that would + // contradict the successful session probe. It stays a --verbose diagnostic. connected = healthLine{doctor.StatusFail, "Not connected — can't reach tracebloc from here.", fmt.Sprintf("Check your network / HTTP(S)_PROXY, then run `%s doctor` again.", launcher())} diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index e16fc8a7..39ae8fe6 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -284,4 +284,17 @@ func TestSummarizeDoctor(t *testing.T) { t.Errorf("ready must not be a green check while disconnected, got OK %q", r.text) } }) + + // The "Backend egress (from this machine)" probe is indicative-not-definitive; + // a miss must NOT contradict a successful WhoAmI by claiming the network is + // down. With a healthy session it stays a --verbose diagnostic (Bugbot #365). + t.Run("backend-egress miss + healthy session → connected stays OK", func(t *testing.T) { + c, _ := summarizeDoctor(with(allOK, "Backend egress (from this machine)", doctor.StatusFail), tokenOK) + if c.status != doctor.StatusOK { + t.Errorf("indicative backend-egress miss + healthy WhoAmI → want connected OK, got %v %q", c.status, c.text) + } + if strings.Contains(c.text, "can't reach tracebloc from here") { + t.Errorf("must not blame the network after a successful WhoAmI, got %q", c.text) + } + }) } From 8a7fb72a5c45ccf1e2adc49993cf9a8ac278ce6c Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 10:28:04 +0200 Subject: [PATCH 07/15] fix(doctor): --diagnose always leaves a bundle; never masks the verdict exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Bugbot findings on #365, one root cause — early returns bypassing the inline --diagnose block: - Diagnose was only honored after the full probe, so every early exit (soft session fault, no environment, clientset error) wrote no bundle — the exact states whose remedies tell the user to run `doctor --diagnose`. - If checks failed (exit 2) but the bundle write errored, the run returned the write failure (exit 3), masking the health verdict. Replace the inline write with a deferred writer registered right after the sign-in gate: it fires on every exit path (so --diagnose always leaves a bundle once authenticated — "not signed in" is still answered by login, not a bundle), and via the named return it only sets the exit code when nothing worse already did, so a bundle hiccup never masks a Fail verdict. writeDiagnoseBundle records partial state (session + an "exited before probe" note) when the roll-up didn't run and skips the k8s section when there's no resolved environment. Tests: --diagnose on a clientset-fail early exit still writes a bundle and preserves exit 3. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 59 +++++++++++++++++++---------- internal/cli/doctor_cluster_test.go | 58 ++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 19 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 28f795fc..4d968ca9 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -80,7 +80,7 @@ func runClusterDoctor( p *ui.Printer, kubeconfigPath, contextOverride, nsOverride string, diagnose bool, -) error { +) (rerr error) { // 1. Who you are — a local config read (instant), so we can answer even // before any cluster is reachable (RFC-0001 §8.5). cfg, err := config.Load() @@ -98,13 +98,34 @@ func runClusterDoctor( p.Para("Signed in") } + // Keep whatever we learn from here on, so `--diagnose` can always leave a + // bundle — even on an early exit (a soft session fault, no environment, a + // clientset error), which is exactly when a remedy tells the user to run it. + // Registered after the sign-in gate on purpose: --diagnose needs an + // authenticated context, and "not signed in" is answered by `login`, not a + // bundle. The deferred write only sets the exit code when nothing worse did, + // so a bundle hiccup never masks a real Fail verdict. + var ( + tok = tokenOK + resolved *cluster.ResolvedConfig + results []doctor.Result + connected, ready healthLine + ) + if diagnose { + defer func() { + p.Newline() + if werr := writeDiagnoseBundle(p, resolved, results, tok, connected, ready); werr != nil && rerr == nil { + rerr = werr + } + }() + } + // 2. Is the session still good with tracebloc? A 401 (expired/revoked) or a // 426 (CLI too old) is a hard stop with a one-command fix. Anything else // folds into the Connected line below — but a backend that ANSWERED with // an error (5xx/403/decode) is a tracebloc-side problem, distinct from a // network failure to reach it at all. Conflating the two would blame the // user's network (and hand them a proxy remedy) for tracebloc's own error. - tok := tokenOK apiClient := newAPIClient(cfg.CurrentEnv) apiClient.Token = cfg.Current().Token if _, werr := apiClient.WhoAmI(ctx); werr != nil { @@ -133,7 +154,7 @@ func runClusterDoctor( // soft tokenUnreachable/tokenServerErr states reach here.) opts := cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride, Namespace: nsOverride} bindActiveClientNamespace(&opts) - resolved, err := loadClusterFn(opts) + resolved, err = loadClusterFn(opts) if err != nil { p.Newline() noteSessionProblem(p, tok) @@ -152,8 +173,8 @@ func runClusterDoctor( // 4. Probe the cluster and roll the granular checks up into the two plain // lines the owner acts on (the granular results stay for --verbose/--diagnose). - results := doctorRunFn(ctx, cs, doctor.Options{Namespace: resolved.Namespace, ServerURL: resolved.ServerURL}) - connected, ready := summarizeDoctor(results, tok) + results = doctorRunFn(ctx, cs, doctor.Options{Namespace: resolved.Namespace, ServerURL: resolved.ServerURL}) + connected, ready = summarizeDoctor(results, tok) p.Newline() renderHealth(p, connected) @@ -161,16 +182,8 @@ func runClusterDoctor( if p.Verbose() { renderDoctorDetails(p, resolved, results) } - - // 5. --diagnose: write the full bundle (session + verdict + technical detail) - // for support. It's additive — the run still ends on the real verdict and - // exit code below, so `--diagnose` never masks a failing check as a 0. - if diagnose { - p.Newline() - if werr := writeDiagnoseBundle(p, resolved, results, tok, connected, ready); werr != nil { - return werr - } - } + // --diagnose writes the support bundle via the deferred writer registered + // above, so it fires on this path and on every early exit alike. // 6. Verdict + exit code (0 healthy, 2 a problem). p.Newline() @@ -366,11 +379,19 @@ func writeDiagnoseBundle(p *ui.Printer, resolved *cluster.ResolvedConfig, result fmt.Fprintf(&buf, "tracebloc doctor — support bundle (%s)\n\n", time.Now().Format(time.RFC3339)) bp := ui.New(&buf, ui.WithColor(false), ui.WithVerbose(true)) // Session + rolled-up verdict first — the state support reads before the raw - // Kubernetes detail, and the reason each remedy sends them here. + // Kubernetes detail, and the reason each remedy sends them here. On an early + // exit (no environment, clientset error) the roll-up never ran, so record the + // session state and note the short-circuit instead of empty verdict lines. bp.Detailf("session: %s", tokenLabel(tok)) - bp.Detailf("connected: %s — %s", connected.status, connected.text) - bp.Detailf("ready: %s — %s", ready.status, ready.text) - renderDoctorDetails(bp, resolved, results) + if connected.text != "" { + bp.Detailf("connected: %s — %s", connected.status, connected.text) + bp.Detailf("ready: %s — %s", ready.status, ready.text) + } else { + bp.Detailf("outcome: exited before the environment could be probed") + } + if resolved != nil { + renderDoctorDetails(bp, resolved, results) + } name := fmt.Sprintf("tracebloc-doctor-%s.txt", time.Now().Format("20060102-150405")) if werr := os.WriteFile(name, buf.Bytes(), 0o600); werr != nil { diff --git a/internal/cli/doctor_cluster_test.go b/internal/cli/doctor_cluster_test.go index 4a3077cb..6c729e5d 100644 --- a/internal/cli/doctor_cluster_test.go +++ b/internal/cli/doctor_cluster_test.go @@ -245,3 +245,61 @@ func TestRunClusterDoctor_DiagnoseBundle(t *testing.T) { } } } + +// --diagnose must still leave a bundle when the run exits EARLY (here a clientset +// failure) — that's exactly when a remedy tells the user to run it. The early-exit +// code (3) is preserved: the bundle write doesn't mask it (Bugbot #365). +func TestRunClusterDoctor_DiagnoseOnEarlyExit(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ + "dev": {Token: "x", Email: "a@b.io", ActiveClientID: "5"}, + }}).Save(); err != nil { + t.Fatal(err) + } + stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // session healthy + }) + origLoad, origCS := loadClusterFn, newClientsetFn + t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) + loadClusterFn = func(cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + return &cluster.ResolvedConfig{Namespace: "default", Context: "test-ctx"}, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { + return nil, errors.New("bad rest config") + } + + tmp := t.TempDir() + orig, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(tmp); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(orig) }) + + var buf bytes.Buffer + err = runClusterDoctor(context.Background(), ui.New(&buf, ui.WithColor(false)), "", "", "", true) + + // The early-exit code survives the bundle write (not masked to 0 or the write code). + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 3 { + t.Fatalf("clientset-fail + --diagnose → want exit 3 preserved, got %v", err) + } + if !strings.Contains(buf.String(), "Couldn't connect to your secure environment") { + t.Errorf("want the connect-failure message, got:\n%s", buf.String()) + } + if !strings.Contains(buf.String(), "Wrote a support bundle") { + t.Errorf("--diagnose on an early exit should still write a bundle, got:\n%s", buf.String()) + } + entries, _ := os.ReadDir(tmp) + found := false + for _, e := range entries { + if strings.HasPrefix(e.Name(), "tracebloc-doctor-") { + found = true + } + } + if !found { + t.Errorf("no support bundle written on early exit (entries: %v)", entries) + } +} From e17bd91b23fd47d1074cdc156489d9b7537202b5 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 10:35:31 +0200 Subject: [PATCH 08/15] fix(doctor): no --diagnose bundle claiming a confirmed session on 401/426 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot on #365: the deferred bundle writer was registered before the WhoAmI 401/426 hard stops, which return without updating tok. A bundle written on an expired (401) or upgrade-required (426) session therefore recorded "session: confirmed" — misleading triage. Move the defer registration to after the session probe: 401/426 return first (their fix is login/update, not a bundle), and every path past there has an accurate tok, so --diagnose still leaves a truthful bundle on the soft-fault / no-environment / clientset-error exits. Test: a 401 + --diagnose exits 2 and writes no bundle file. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 35 ++++++++++++++++++++--------------- internal/cli/doctor_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 15 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 4d968ca9..e5d1851a 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -98,27 +98,14 @@ func runClusterDoctor( p.Para("Signed in") } - // Keep whatever we learn from here on, so `--diagnose` can always leave a - // bundle — even on an early exit (a soft session fault, no environment, a - // clientset error), which is exactly when a remedy tells the user to run it. - // Registered after the sign-in gate on purpose: --diagnose needs an - // authenticated context, and "not signed in" is answered by `login`, not a - // bundle. The deferred write only sets the exit code when nothing worse did, - // so a bundle hiccup never masks a real Fail verdict. + // State we accumulate so `--diagnose` can leave a bundle from wherever we + // exit. tok is declared here because the session probe below sets it. var ( tok = tokenOK resolved *cluster.ResolvedConfig results []doctor.Result connected, ready healthLine ) - if diagnose { - defer func() { - p.Newline() - if werr := writeDiagnoseBundle(p, resolved, results, tok, connected, ready); werr != nil && rerr == nil { - rerr = werr - } - }() - } // 2. Is the session still good with tracebloc? A 401 (expired/revoked) or a // 426 (CLI too old) is a hard stop with a one-command fix. Anything else @@ -147,6 +134,24 @@ func runClusterDoctor( } } + // Register the --diagnose bundle writer now that the session outcome is known. + // Placed AFTER the 401/426 hard stops (which return above): a bundle is + // pointless for those one-command fixes (login / update), and — crucially — + // writing one there would record "session: confirmed" for an expired or + // upgrade-required session, misleading triage. For every path past here tok is + // accurate, so `--diagnose` always leaves a truthful bundle (even on the + // no-environment / clientset-error exits, exactly where a remedy sends the + // user). The write only sets the exit code when nothing worse already did, so + // a bundle hiccup never masks a real Fail verdict. + if diagnose { + defer func() { + p.Newline() + if werr := writeDiagnoseBundle(p, resolved, results, tok, connected, ready); werr != nil && rerr == nil { + rerr = werr + } + }() + } + // 3. Find the secure environment (local kubeconfig read). If it isn't here, // that's the headline — but surface a session/backend fault we already // found first, so a reinstall is never recommended while a live session diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 39ae8fe6..3f08acd8 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -89,6 +89,43 @@ func TestDoctor_OutOfDate426(t *testing.T) { } } +// A 401 hard stop with --diagnose must NOT write a bundle: the fix is `login` +// (no bundle needed), and one written here would falsely record "session: +// confirmed" for an expired session. The defer is registered after the session +// probe precisely so 401/426 return first (Bugbot #365). +func TestDoctor_DiagnoseNotWrittenOnExpiredSession(t *testing.T) { + signedInConfig(t) + stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"detail":"Invalid token."}`)) + }) + tmp := t.TempDir() + orig, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(tmp); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(orig) }) + + var out bytes.Buffer + err = runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", true) + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("401 + --diagnose → want exit 2, got %v", err) + } + if strings.Contains(out.String(), "Wrote a support bundle") { + t.Errorf("must not write a bundle on an expired session, got:\n%s", out.String()) + } + entries, _ := os.ReadDir(tmp) + for _, e := range entries { + if strings.HasPrefix(e.Name(), "tracebloc-doctor-") { + t.Errorf("a bundle file was written on 401 (%s) — should be none", e.Name()) + } + } +} + // A bad kubeconfig, with auth healthy, is a local-environment problem (exit 3) // framed as "no secure environment here yet" — not a Kubernetes error dump. func TestDoctor_KubeconfigFailIsLocalEnv(t *testing.T) { From eac3ebe517cbb6b98b1e2a53e5c250a4c13541b9 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 10:44:02 +0200 Subject: [PATCH 09/15] fix(doctor): a session fault exits 2 even when local config also fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot on #365: on the no-environment / clientset-error early exits, a Fail-level session state (tokenUnreachable / tokenServerErr) that noteSessionProblem had just printed was still returned as exitLocalEnv (3) — while the same session states exit 2 (exitChecksFailed) on the full probe path. A script keying on "exit 2 = a problem was found" would miss the session fault whenever local config also failed. earlyExitCode(tok) now returns exit 2 when a session/connectivity fault was detected, else the local-env code (3), so the exit code is consistent across paths. Test: a transport-error session (tokenUnreachable) + no environment exits 2 and surfaces both the session fault and the no-environment line. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 17 +++++++++++++++-- internal/cli/doctor_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index e5d1851a..c4b3d8b4 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -165,14 +165,14 @@ func runClusterDoctor( noteSessionProblem(p, tok) p.Errorf("No secure environment on this machine yet.") p.Hintf(" Set one up: %s", installCmd) - return &exitError{code: exitLocalEnv, err: nil} + return &exitError{code: earlyExitCode(tok), err: nil} } cs, err := newClientsetFn(resolved) if err != nil { p.Newline() noteSessionProblem(p, tok) p.Errorf("Couldn't connect to your secure environment — check your kubeconfig/context.") - return &exitError{code: exitLocalEnv, err: nil} + return &exitError{code: earlyExitCode(tok), err: nil} } p.Para(fmt.Sprintf("Secure environment %q", envDisplayName(resolved))) @@ -237,6 +237,19 @@ func noteSessionProblem(p *ui.Printer, tok tokenState) { } } +// earlyExitCode picks the exit code for a local-environment early exit (no +// kubeconfig / clientset error). A connectivity or session fault we already +// detected (tok != tokenOK) is a checks failure and takes precedence over the +// local-env code, so those states exit 2 here just as they do on the full probe +// path — a script keying on "exit 2 = a problem was found" never misses a +// session fault just because local config also failed. +func earlyExitCode(tok tokenState) int { + if tok != tokenOK { + return exitChecksFailed + } + return exitLocalEnv +} + // tokenLabel is the one-line session status recorded in the support bundle. func tokenLabel(tok tokenState) string { switch tok { diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 3f08acd8..a279c696 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -13,6 +13,7 @@ import ( "time" "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" "github.com/tracebloc/cli/internal/doctor" "github.com/tracebloc/cli/internal/ui" @@ -126,6 +127,40 @@ func TestDoctor_DiagnoseNotWrittenOnExpiredSession(t *testing.T) { } } +// A session fault (here a transport error → tokenUnreachable) that coincides with +// a local-env failure must surface as exit 2 ("a problem was found"), matching the +// full-probe path — not be masked as the local-env exit 3 (Bugbot #365). +func TestDoctor_SessionFaultDominatesEarlyExit(t *testing.T) { + signedInConfig(t) + // Point the API client at a closed port so WhoAmI is a transport error → + // tokenUnreachable (not an APIError). + origAPI := newAPIClient + t.Cleanup(func() { newAPIClient = origAPI }) + newAPIClient = func(string) *api.Client { + return &api.Client{BaseURL: "http://127.0.0.1:1", HTTP: &http.Client{Timeout: 2 * time.Second}} + } + // No environment on this machine. + origLoad := loadClusterFn + t.Cleanup(func() { loadClusterFn = origLoad }) + loadClusterFn = func(cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + return nil, errors.New("no kubeconfig here") + } + + var out bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("session fault + no-env → want exit 2 (problem found), got %v", err) + } + if !strings.Contains(out.String(), "No secure environment") { + t.Errorf("want the no-environment line, got:\n%s", out.String()) + } + if !strings.Contains(out.String(), "Can't reach tracebloc from here") && + !strings.Contains(out.String(), "didn't confirm your session") { + t.Errorf("want the session fault surfaced alongside no-environment, got:\n%s", out.String()) + } +} + // A bad kubeconfig, with auth healthy, is a local-environment problem (exit 3) // framed as "no secure environment here yet" — not a Kubernetes error dump. func TestDoctor_KubeconfigFailIsLocalEnv(t *testing.T) { From 85b1aea4d6815f7b3ba62acddcb678be77856096 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 10:53:51 +0200 Subject: [PATCH 10/15] fix(doctor): don't name a secure environment that isn't installed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot on #365: the identity header printed `Secure environment ""` from the resolved kubeconfig namespace before the probe. When the cluster is reachable but no tracebloc chart is installed (ReachNoEnv), the Connected line then says none is installed here — so the default view asserted the environment both exists and does not. Move the header to after the roll-up and print it only when an environment is actually installed (reach state != ReachNoEnv). Nothing prints between it and "Signed in as …", so the two context lines still read as a pair. Every other reach state (running, stopped, RBAC) means an environment exists, so it's still named. Test: a ReachNoEnv run shows "No secure environment installed here", never names one, and exits 2. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 25 +++++++++++++++++++++++-- internal/cli/doctor_cluster_test.go | 19 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index c4b3d8b4..9cfa6a4b 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -174,13 +174,22 @@ func runClusterDoctor( p.Errorf("Couldn't connect to your secure environment — check your kubeconfig/context.") return &exitError{code: earlyExitCode(tok), err: nil} } - p.Para(fmt.Sprintf("Secure environment %q", envDisplayName(resolved))) - // 4. Probe the cluster and roll the granular checks up into the two plain // lines the owner acts on (the granular results stay for --verbose/--diagnose). results = doctorRunFn(ctx, cs, doctor.Options{Namespace: resolved.Namespace, ServerURL: resolved.ServerURL}) connected, ready = summarizeDoctor(results, tok) + // Name the secure environment only when one is actually installed here. A + // reachable cluster with no tracebloc chart (ReachNoEnv) is "no environment", + // and the Connected line below says so — naming it too would assert it both + // exists and doesn't. Every other reach state (running, stopped, RBAC) means + // an environment IS installed, so it's named. Printed here (not before the + // probe) so we know which; nothing prints between it and "Signed in" above, so + // the two context lines still read as a pair. + if reachStateOf(results) != doctor.ReachNoEnv { + p.Para(fmt.Sprintf("Secure environment %q", envDisplayName(resolved))) + } + p.Newline() renderHealth(p, connected) renderHealth(p, ready) @@ -440,6 +449,18 @@ func envDisplayName(r *cluster.ResolvedConfig) string { return "your secure environment" } +// reachStateOf returns the "Cluster reachable" check's classification, so the +// caller can tell a reachable-but-uninstalled cluster (ReachNoEnv) apart from one +// that simply isn't answering. ReachOK when the check is absent. +func reachStateOf(results []doctor.Result) doctor.ReachState { + for _, r := range results { + if r.Name == "Cluster reachable" { + return r.Reach + } + } + return doctor.ReachOK +} + // worseStatus returns the more severe of two doctor statuses (Fail > Warn > OK). // StatusUnknown carries no signal, so it never worsens the verdict. func worseStatus(a, b doctor.Status) doctor.Status { diff --git a/internal/cli/doctor_cluster_test.go b/internal/cli/doctor_cluster_test.go index 6c729e5d..129254d6 100644 --- a/internal/cli/doctor_cluster_test.go +++ b/internal/cli/doctor_cluster_test.go @@ -303,3 +303,22 @@ func TestRunClusterDoctor_DiagnoseOnEarlyExit(t *testing.T) { t.Errorf("no support bundle written on early exit (entries: %v)", entries) } } + +// A reachable cluster with no tracebloc chart (ReachNoEnv) must not print +// `Secure environment ""` — the Connected line says none is installed, so +// naming one would assert it both exists and doesn't (Bugbot #365). +func TestRunClusterDoctor_NoEnvSuppressesEnvHeader(t *testing.T) { + out, err := runDoctorClusterHalf(t, []doctor.Result{ + {Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv}, + }) + if !strings.Contains(out, "No secure environment installed here") { + t.Errorf("want the no-environment Connected line, got:\n%s", out) + } + if strings.Contains(out, `Secure environment "`) { + t.Errorf("must not name a secure environment when none is installed, got:\n%s", out) + } + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("ReachNoEnv → want exit 2, got %v", err) + } +} From 2064b626b670024f860c7d3559d143f0df7937a1 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 11:02:50 +0200 Subject: [PATCH 11/15] fix(doctor): unify the three "no environment" exits; surface session faults on all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot on #365: a session fault (tokenUnreachable / tokenServerErr) was hidden on the reachable-but-no-chart (ReachNoEnv) path, because that path flowed through summarizeDoctor whose Connected line ignores tok — unlike the missing-kubeconfig and clientset-error exits, which surface it via noteSessionProblem. Route ReachNoEnv through the same short-circuit as the other two "no environment" states: noteSessionProblem (surfaces any session fault) + "No secure environment on this machine yet" + the installer + earlyExitCode (a session fault dominates the exit). Remove the now-redundant ReachNoEnv case from summarizeDoctor — one source of truth. This also preserves the earlier fix (never name an environment that isn't installed): the short-circuit returns before the identity header. Tests: ReachNoEnv with a healthy session exits 3 and names no environment; with a session fault it surfaces the fault and exits 2. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 41 +++++++++++++------------ internal/cli/doctor_cluster_test.go | 47 +++++++++++++++++++++++++++-- internal/cli/doctor_test.go | 22 -------------- 3 files changed, 65 insertions(+), 45 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 9cfa6a4b..6c77b912 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -174,22 +174,27 @@ func runClusterDoctor( p.Errorf("Couldn't connect to your secure environment — check your kubeconfig/context.") return &exitError{code: earlyExitCode(tok), err: nil} } - // 4. Probe the cluster and roll the granular checks up into the two plain - // lines the owner acts on (the granular results stay for --verbose/--diagnose). + // 4. Probe the cluster. results = doctorRunFn(ctx, cs, doctor.Options{Namespace: resolved.Namespace, ServerURL: resolved.ServerURL}) - connected, ready = summarizeDoctor(results, tok) - // Name the secure environment only when one is actually installed here. A - // reachable cluster with no tracebloc chart (ReachNoEnv) is "no environment", - // and the Connected line below says so — naming it too would assert it both - // exists and doesn't. Every other reach state (running, stopped, RBAC) means - // an environment IS installed, so it's named. Printed here (not before the - // probe) so we know which; nothing prints between it and "Signed in" above, so - // the two context lines still read as a pair. - if reachStateOf(results) != doctor.ReachNoEnv { - p.Para(fmt.Sprintf("Secure environment %q", envDisplayName(resolved))) + // A reachable cluster with no tracebloc chart installed is the same "no secure + // environment here" state as a missing kubeconfig — route it through the same + // message (which also surfaces any session fault) rather than naming an + // environment that isn't installed. This unifies all three no-environment + // exits: missing kubeconfig, clientset error, and no chart. + if reachStateOf(results) == doctor.ReachNoEnv { + p.Newline() + noteSessionProblem(p, tok) + p.Errorf("No secure environment on this machine yet.") + p.Hintf(" Set one up: %s", installCmd) + return &exitError{code: earlyExitCode(tok), err: nil} } + // An environment is installed here — name it (nothing prints between this and + // "Signed in" above, so the two context lines read as a pair), then roll up. + p.Para(fmt.Sprintf("Secure environment %q", envDisplayName(resolved))) + connected, ready = summarizeDoctor(results, tok) + p.Newline() renderHealth(p, connected) renderHealth(p, ready) @@ -286,15 +291,11 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready switch { case reach.Status == doctor.StatusFail: - // A failed reachability check has three very different fixes; word each - // from the classification checkReachable attached, never a kubectl. An - // unclassified fail (e.g. a hand-built result) defaults to "isn't - // answering" — the safe, pre-existing interpretation. + // ReachNoEnv (reachable, no chart) is short-circuited in runClusterDoctor, + // so it never reaches here. The two remaining fails are worded from the + // classification, never a kubectl; an unclassified fail (e.g. a hand-built + // result) defaults to "isn't answering" — the safe interpretation. switch reach.Reach { - case doctor.ReachNoEnv: - connected = healthLine{doctor.StatusFail, - "No secure environment installed here.", - "Set one up: " + installCmd} case doctor.ReachError: connected = healthLine{doctor.StatusFail, "Not connected — couldn't read your secure environment.", diff --git a/internal/cli/doctor_cluster_test.go b/internal/cli/doctor_cluster_test.go index 129254d6..19384982 100644 --- a/internal/cli/doctor_cluster_test.go +++ b/internal/cli/doctor_cluster_test.go @@ -9,10 +9,12 @@ import ( "path/filepath" "strings" "testing" + "time" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" + "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" "github.com/tracebloc/cli/internal/doctor" @@ -311,14 +313,53 @@ func TestRunClusterDoctor_NoEnvSuppressesEnvHeader(t *testing.T) { out, err := runDoctorClusterHalf(t, []doctor.Result{ {Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv}, }) - if !strings.Contains(out, "No secure environment installed here") { - t.Errorf("want the no-environment Connected line, got:\n%s", out) + if !strings.Contains(out, "No secure environment on this machine yet") { + t.Errorf("want the unified no-environment message, got:\n%s", out) } if strings.Contains(out, `Secure environment "`) { t.Errorf("must not name a secure environment when none is installed, got:\n%s", out) } + if strings.Contains(out, "kubectl") { + t.Errorf("no-env output must not leak kubectl, got:\n%s", out) + } + // Healthy session → a local-env setup problem (exit 3), consistent with a + // missing kubeconfig. + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 3 { + t.Fatalf("ReachNoEnv (healthy session) → want exit 3, got %v", err) + } +} + +// A session fault alongside no-environment must be surfaced (not hidden) and must +// dominate the exit code (2), matching the other no-env exits (Bugbot #365). +func TestRunClusterDoctor_NoEnvSurfacesSessionFault(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ + "dev": {Token: "x", Email: "a@b.io", ActiveClientID: "5"}, + }}).Save(); err != nil { + t.Fatal(err) + } + // WhoAmI at a closed port → transport error → tokenUnreachable. + origAPI := newAPIClient + t.Cleanup(func() { newAPIClient = origAPI }) + newAPIClient = func(string) *api.Client { + return &api.Client{BaseURL: "http://127.0.0.1:1", HTTP: &http.Client{Timeout: 2 * time.Second}} + } + withClusterSeams(t, fake.NewSimpleClientset()) + withDoctorRun(t, []doctor.Result{ + {Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv}, + }) + + var buf bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&buf, ui.WithColor(false)), "", "", "", false) + if !strings.Contains(buf.String(), "Can't reach tracebloc from here") { + t.Errorf("session fault must be surfaced on no-env, got:\n%s", buf.String()) + } + if !strings.Contains(buf.String(), "No secure environment on this machine yet") { + t.Errorf("want the no-environment message too, got:\n%s", buf.String()) + } var ee *exitError if !errors.As(err, &ee) || ee.Code() != 2 { - t.Fatalf("ReachNoEnv → want exit 2, got %v", err) + t.Fatalf("session fault + no-env → want exit 2 (fault dominates), got %v", err) } } diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index a279c696..317150a3 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -290,28 +290,6 @@ func TestSummarizeDoctor(t *testing.T) { // A reachable cluster with no tracebloc installed must NOT be reported as // "isn't answering" with a kubectl remedy — it's a reinstall (Bugbot #365). - t.Run("no environment installed → connected Fail (installer, not kubectl)", func(t *testing.T) { - noEnv := with(allOK, "Cluster reachable", doctor.StatusFail) - for i := range noEnv { - if noEnv[i].Name == "Cluster reachable" { - noEnv[i].Reach = doctor.ReachNoEnv - } - } - c, r := summarizeDoctor(noEnv, tokenOK) - if c.status != doctor.StatusFail || !strings.Contains(c.text, "No secure environment") { - t.Errorf("no-env → want connected Fail 'No secure environment', got %v %q", c.status, c.text) - } - if !strings.Contains(c.remedy, "tracebloc.io/i.sh") { - t.Errorf("no-env remedy should be the installer, got %q", c.remedy) - } - if strings.Contains(c.remedy, "kubectl") { - t.Errorf("no-env remedy must not leak kubectl, got %q", c.remedy) - } - if r.status != doctor.StatusUnknown { - t.Errorf("no-env → ready should be can't-check (Unknown), got %v", r.status) - } - }) - // A failing image-pull check means training images can't be fetched — that // is not-ready, and must not be silently dropped from the rollup (Bugbot #365). t.Run("images can't be pulled → ready Fail", func(t *testing.T) { From 4aef9f5f6b850ee1fa8c3d7c44150c56ab14382d Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 11:13:11 +0200 Subject: [PATCH 12/15] fix(doctor): --verbose details + honest bundle outcome on early exits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Bugbot findings on #365, both from the no-environment short-circuits returning before the main render: - --verbose (Medium): renderDoctorDetails only ran on the full path, so `tb doctor --verbose` printed no Details on the ReachNoEnv / clientset failure paths — exactly where support needs the kubeconfig + granular output. Extracted renderDetailsIfVerbose and call it at every exit that has a resolved config. - --diagnose bundle (Low): on ReachNoEnv the roll-up never ran, so the bundle recorded "exited before the environment could be probed" even though the checks WERE collected and written below. The outcome line is now three-way: verdict when summarized, "no roll-up verdict (granular checks below)" when probed but not summarized, and "before the cluster was probed" only when truly not probed. Test: --verbose on a ReachNoEnv run prints the Details section with the checks. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 28 ++++++++++++++++++++++------ internal/cli/doctor_cluster_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 6c77b912..59836811 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -172,6 +172,7 @@ func runClusterDoctor( p.Newline() noteSessionProblem(p, tok) p.Errorf("Couldn't connect to your secure environment — check your kubeconfig/context.") + renderDetailsIfVerbose(p, resolved, results) return &exitError{code: earlyExitCode(tok), err: nil} } // 4. Probe the cluster. @@ -187,6 +188,7 @@ func runClusterDoctor( noteSessionProblem(p, tok) p.Errorf("No secure environment on this machine yet.") p.Hintf(" Set one up: %s", installCmd) + renderDetailsIfVerbose(p, resolved, results) return &exitError{code: earlyExitCode(tok), err: nil} } @@ -198,9 +200,7 @@ func runClusterDoctor( p.Newline() renderHealth(p, connected) renderHealth(p, ready) - if p.Verbose() { - renderDoctorDetails(p, resolved, results) - } + renderDetailsIfVerbose(p, resolved, results) // --diagnose writes the support bundle via the deferred writer registered // above, so it fires on this path and on every early exit alike. @@ -376,6 +376,17 @@ func renderHealth(p *ui.Printer, h healthLine) { // renderDoctorDetails is the --verbose (and --diagnose) technical breakdown: // kubeconfig + every granular check. This is the only place Kubernetes // vocabulary appears. +// renderDetailsIfVerbose prints the --verbose technical breakdown when the flag +// is set and a cluster config was resolved. Called at every exit that has a +// resolved config — including the no-environment / clientset-error early exits — +// so `tb doctor --verbose` still yields the support-facing detail on the failure +// paths that most need it, not only on the healthy full run. +func renderDetailsIfVerbose(p *ui.Printer, resolved *cluster.ResolvedConfig, results []doctor.Result) { + if p.Verbose() && resolved != nil { + renderDoctorDetails(p, resolved, results) + } +} + func renderDoctorDetails(p *ui.Printer, resolved *cluster.ResolvedConfig, results []doctor.Result) { p.Newline() p.Section("Details (for support)") @@ -411,11 +422,16 @@ func writeDiagnoseBundle(p *ui.Printer, resolved *cluster.ResolvedConfig, result // exit (no environment, clientset error) the roll-up never ran, so record the // session state and note the short-circuit instead of empty verdict lines. bp.Detailf("session: %s", tokenLabel(tok)) - if connected.text != "" { + switch { + case connected.text != "": bp.Detailf("connected: %s — %s", connected.status, connected.text) bp.Detailf("ready: %s — %s", ready.status, ready.text) - } else { - bp.Detailf("outcome: exited before the environment could be probed") + case len(results) > 0: + // Probed, but exited before the two-line roll-up (e.g. no chart installed). + // The granular checks are written below, so don't claim we never probed. + bp.Detailf("outcome: early exit — no roll-up verdict (granular checks below)") + default: + bp.Detailf("outcome: early exit before the cluster was probed") } if resolved != nil { renderDoctorDetails(bp, resolved, results) diff --git a/internal/cli/doctor_cluster_test.go b/internal/cli/doctor_cluster_test.go index 19384982..d6d108cc 100644 --- a/internal/cli/doctor_cluster_test.go +++ b/internal/cli/doctor_cluster_test.go @@ -363,3 +363,31 @@ func TestRunClusterDoctor_NoEnvSurfacesSessionFault(t *testing.T) { t.Fatalf("session fault + no-env → want exit 2 (fault dominates), got %v", err) } } + +// --verbose must still print the support-facing Details on failure paths that +// have a resolved config (here ReachNoEnv), not only on a healthy full run — the +// early exit returns before the main render (Bugbot #365). +func TestRunClusterDoctor_VerboseDetailsOnNoEnv(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ + "dev": {Token: "x", Email: "a@b.io", ActiveClientID: "5"}, + }}).Save(); err != nil { + t.Fatal(err) + } + stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) // session healthy + }) + withClusterSeams(t, fake.NewSimpleClientset()) + withDoctorRun(t, []doctor.Result{ + {Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv, Detail: "no tracebloc client found"}, + }) + + var buf bytes.Buffer + _ = runClusterDoctor(context.Background(), ui.New(&buf, ui.WithColor(false), ui.WithVerbose(true)), "", "", "", false) + if !strings.Contains(buf.String(), "Details (for support)") { + t.Errorf("--verbose on ReachNoEnv should still print the Details section, got:\n%s", buf.String()) + } + if !strings.Contains(buf.String(), "Cluster reachable") { + t.Errorf("--verbose Details should include the granular checks, got:\n%s", buf.String()) + } +} From de9f908f4937712781225288b8cb848a71151c1e Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 11:24:55 +0200 Subject: [PATCH 13/15] fix(doctor): stuck-Pending pods roll up to Not-ready, not a false green MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit summarizeDoctor only degraded the "Ready to run training" verdict on Pod-health StatusFail (crash-loop). Pods stuck Pending past the grace window surface as StatusWarn, so an environment where training can't schedule still rolled up to ✔ "Ready to run training" and the "Everything looks good" verdict. Treat Pod-health Warn as not-ready (training genuinely can't schedule) with a compute / image-pull remedy, and cover it with a regression test. Bugbot (PR #365): "Warn pod states claim ready". Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 9 +++++++++ internal/cli/doctor_test.go | 13 +++++++++++++ 2 files changed, 22 insertions(+) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 59836811..8c57d699 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -339,6 +339,15 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready ready = healthLine{doctor.StatusFail, "Not ready — part of your secure environment isn't running.", fmt.Sprintf("Reinstall with `%s`, or email support@tracebloc.io with `%s doctor --diagnose`.", installCmd, launcher())} + case by["Pod health"].Status == doctor.StatusWarn: + // Pods stuck Pending past the grace window (unschedulable / image can't + // pull) mean training can't actually schedule — so this is NOT ready, even + // though the granular Pod-health check rates it a softer ⚠. Without this, + // a stuck-pending environment rolled up to ✔ "Ready to run training" and + // the "Everything looks good" verdict (Bugbot). + ready = healthLine{doctor.StatusFail, + "Not ready — part of your secure environment can't start yet.", + fmt.Sprintf("Some pods are stuck starting — usually not enough free compute, or a training image that can't be pulled. Free some up in Docker Desktop → Resources, then re-run `%s doctor`; if it persists, email support@tracebloc.io with `%s doctor --diagnose`.", launcher(), launcher())} case by["Image pull secret"].Status == doctor.StatusFail: ready = healthLine{doctor.StatusFail, "Not ready — the training images can't be pulled.", diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 317150a3..2cca916f 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -288,6 +288,19 @@ func TestSummarizeDoctor(t *testing.T) { } }) + // Pods stuck Pending past the grace window surface as Pod-health StatusWarn + // (not Fail). Training still can't schedule, so the rollup must NOT report ✔ + // "Ready to run training" — that false green was the Bugbot finding. + t.Run("pods stuck pending (warn) → ready Fail, not a false green", func(t *testing.T) { + _, r := summarizeDoctor(with(allOK, "Pod health", doctor.StatusWarn), tokenOK) + if r.status == doctor.StatusOK { + t.Fatalf("stuck-pending pods must not roll up to Ready, got %v %q", r.status, r.text) + } + if !strings.Contains(r.text, "Not ready") { + t.Errorf("want a Not-ready readiness line for stuck-pending pods, got %q", r.text) + } + }) + // A reachable cluster with no tracebloc installed must NOT be reported as // "isn't answering" with a kubectl remedy — it's a reinstall (Bugbot #365). // A failing image-pull check means training images can't be fetched — that From b08b970f4f51cd4a864dbcdcd614f6e8847c13da Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 11:32:02 +0200 Subject: [PATCH 14/15] fix(doctor): distinguish the two Pod-health warn sources in the readiness rollup checkPods returns StatusWarn for BOTH stuck-Pending pods AND a failure to list pods (e.g. RBAC). The previous fix gave the stuck-pending / Docker-Desktop remedy to both, misdiagnosing the read-failure case. Split them: "could not list pods" now rolls up to an honest can't-check (StatusUnknown, no compute remedy), while stuck-Pending stays not-ready with the compute/image-pull remedy. Both covered. Bugbot (PR #365): "Pod warn misclassified as pending". Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 9 +++++++++ internal/cli/doctor_test.go | 37 +++++++++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 8c57d699..a15a1da1 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "os" + "strings" "time" "github.com/spf13/cobra" @@ -339,6 +340,14 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready ready = healthLine{doctor.StatusFail, "Not ready — part of your secure environment isn't running.", fmt.Sprintf("Reinstall with `%s`, or email support@tracebloc.io with `%s doctor --diagnose`.", installCmd, launcher())} + case by["Pod health"].Status == doctor.StatusWarn && strings.HasPrefix(by["Pod health"].Detail, "could not list pods"): + // checkPods returns StatusWarn for TWO different situations: pods stuck + // Pending (below) AND a failure to list pods at all (e.g. RBAC, doctor.go + // checkPods). For the latter we simply can't tell whether training can run, + // so report an honest can't-check — never the stuck-pending/compute remedy, + // which would misdiagnose a permissions problem (Bugbot). + ready = healthLine{doctor.StatusUnknown, + "Ready to run training — couldn't check your workloads (run with --verbose)", ""} case by["Pod health"].Status == doctor.StatusWarn: // Pods stuck Pending past the grace window (unschedulable / image can't // pull) mean training can't actually schedule — so this is NOT ready, even diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 2cca916f..2fd218c2 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -288,19 +288,44 @@ func TestSummarizeDoctor(t *testing.T) { } }) - // Pods stuck Pending past the grace window surface as Pod-health StatusWarn - // (not Fail). Training still can't schedule, so the rollup must NOT report ✔ - // "Ready to run training" — that false green was the Bugbot finding. + // Pod health has TWO StatusWarn sources (checkPods): pods stuck Pending, and a + // failure to list pods at all (e.g. RBAC). They must roll up differently. + warnPods := func(detail string) []doctor.Result { + out := with(allOK, "Pod health", doctor.StatusWarn) + for i := range out { + if out[i].Name == "Pod health" { + out[i].Detail = detail + } + } + return out + } + + // Pods stuck Pending past the grace window: training can't schedule, so the + // rollup must NOT report ✔ "Ready to run training" — that false green was the + // original Bugbot finding. t.Run("pods stuck pending (warn) → ready Fail, not a false green", func(t *testing.T) { - _, r := summarizeDoctor(with(allOK, "Pod health", doctor.StatusWarn), tokenOK) - if r.status == doctor.StatusOK { - t.Fatalf("stuck-pending pods must not roll up to Ready, got %v %q", r.status, r.text) + _, r := summarizeDoctor(warnPods("Pending > 5m0s: [trainer-x]"), tokenOK) + if r.status != doctor.StatusFail { + t.Fatalf("stuck-pending pods must roll up to not-ready, got %v %q", r.status, r.text) } if !strings.Contains(r.text, "Not ready") { t.Errorf("want a Not-ready readiness line for stuck-pending pods, got %q", r.text) } }) + // The other Pod-health Warn source, "could not list pods" (RBAC), is a + // can't-check — it must NOT get the stuck-pending/compute (Docker Desktop) + // remedy (Bugbot follow-up). + t.Run("pod-health warn = could not list pods (RBAC) → can't-check, not stuck-pending", func(t *testing.T) { + _, r := summarizeDoctor(warnPods("could not list pods: pods is forbidden"), tokenOK) + if r.status == doctor.StatusFail { + t.Errorf("a can't-list-pods warn must not be a hard not-ready, got %v %q", r.status, r.text) + } + if strings.Contains(r.remedy, "Docker Desktop") { + t.Errorf("must not give the stuck-pending/compute remedy for a read failure, got remedy=%q", r.remedy) + } + }) + // A reachable cluster with no tracebloc installed must NOT be reported as // "isn't answering" with a kubectl remedy — it's a reinstall (Bugbot #365). // A failing image-pull check means training images can't be fetched — that From 81b1b282f56d216ad971d5d106779017ae7a42bc Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 11:39:07 +0200 Subject: [PATCH 15/15] fix(doctor): don't claim "everything looks good" when readiness is unverified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RBAC / can't-list-pods case rolls readiness up to StatusUnknown ("couldn't check your workloads"), but worseStatus treats Unknown as non-worsening, so the closing verdict still printed "Everything looks good — you're ready to run training" (exit 0), contradicting that line. Extract the decision into doctorVerdict: "everything looks good" now requires BOTH connected and ready to be genuinely StatusOK; a can't-check reports an honest partial result instead (still exit 0, since nothing failed). Unit-tested. Bugbot (PR #365): "Can't-check still claims ready". Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 28 +++++++++++++++++++++++++--- internal/cli/doctor_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index a15a1da1..61bd0b00 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -205,15 +205,23 @@ func runClusterDoctor( // --diagnose writes the support bundle via the deferred writer registered // above, so it fires on this path and on every early exit alike. - // 6. Verdict + exit code (0 healthy, 2 a problem). + // 6. Verdict + exit code (0 healthy/partial, 2 a problem). p.Newline() - if worseStatus(connected.status, ready.status) == doctor.StatusFail { + fail, allGood := doctorVerdict(connected.status, ready.status) + switch { + case fail: if !diagnose { // they just wrote a bundle — don't send them to write it again p.Hintf("Still stuck? Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher()) } return &exitError{code: exitChecksFailed, err: nil} + case allGood: + p.Successf("Everything looks good — you're ready to run training.") + default: + // Connected and nothing failed, but a check couldn't complete (e.g. pod + // health unreadable — RBAC → ready is StatusUnknown). Don't overclaim + // "everything looks good"; say so honestly. Not a failure → exit 0 (Bugbot). + p.Infof("No problems found, but some checks couldn't finish — re-run with --verbose for detail.") } - p.Successf("Everything looks good — you're ready to run training.") return nil } @@ -507,3 +515,17 @@ func worseStatus(a, b doctor.Status) doctor.Status { } return doctor.StatusOK } + +// doctorVerdict decides the closing line from the two rolled-up health lines: +// fail (a real problem → exit 2), or allGood (BOTH genuinely OK → "everything +// looks good"). The key subtlety: allGood requires both to be StatusOK, NOT +// merely "not Fail" — a readiness we couldn't determine (StatusUnknown, e.g. a +// pod-list RBAC failure) must not be reported as good, even though worseStatus +// treats Unknown as non-worsening (Bugbot). When neither holds, the caller +// reports a partial "couldn't finish some checks" result (still exit 0). +func doctorVerdict(connected, ready doctor.Status) (fail, allGood bool) { + if worseStatus(connected, ready) == doctor.StatusFail { + return true, false + } + return false, connected == doctor.StatusOK && ready == doctor.StatusOK +} diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 2fd218c2..2ef1917f 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -386,3 +386,34 @@ func TestSummarizeDoctor(t *testing.T) { } }) } + +// ── doctorVerdict: the closing "everything looks good" / problem / partial call ── + +func TestDoctorVerdict(t *testing.T) { + ok, warn, fail, unknown := doctor.StatusOK, doctor.StatusWarn, doctor.StatusFail, doctor.StatusUnknown + cases := []struct { + name string + connected, ready doctor.Status + wantFail bool + wantAllGood bool + }{ + {"both OK → everything good", ok, ok, false, true}, + {"ready Fail → problem", ok, fail, true, false}, + {"connected Fail → problem", fail, ok, true, false}, + // The Bugbot case: connected but readiness couldn't be checked (RBAC → + // Unknown). Not a hard failure, but NOT "everything looks good". + {"connected + ready can't-check → neither", ok, unknown, false, false}, + // Not-connected already Fails via connected, regardless of ready=Unknown. + {"disconnected + ready unknown → problem", fail, unknown, true, false}, + {"a warn that isn't Fail → not everything-good", ok, warn, false, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + gotFail, gotAllGood := doctorVerdict(tc.connected, tc.ready) + if gotFail != tc.wantFail || gotAllGood != tc.wantAllGood { + t.Errorf("doctorVerdict(%v,%v) = fail=%v allGood=%v, want fail=%v allGood=%v", + tc.connected, tc.ready, gotFail, gotAllGood, tc.wantFail, tc.wantAllGood) + } + }) + } +}