From b05266baef2b8e80918f896a05c35e707734cca5 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 12:23:50 +0200 Subject: [PATCH 01/11] fix(cluster): say which namespace, instead of offering --namespace blindly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The §7.3 binding-miss error is RFC-0001's sentence with the remedy deleted: it names --namespace but never says WHICH namespace, and nothing else in the CLI will tell you. On a laptop with a healthy local install that left no supported way back. explain now diagnoses before advising. A binding-applied noParentReleaseError carries the clientset and server URL of the cluster that actually missed, and explain spends one naming-only cluster.FindClientNamespaces — the same read discoverRelease already spends purely to write a better message — then branches on isLocalServerURL: one client + local server URL name it, offer `client create` (a re-run on a cluster that already hosts a client adopts it, so the repoint mints nothing) client(s) on a remote cluster name the namespaces, offer ONLY --namespace; never `client create` there, because the client we found may be a colleague's (§7.5) none, scan clean today's text plus "No tracebloc client is running on this cluster either", which is when the installer is the right advice could not look today's text, byte for byte The last branch is the point of the three-valued clientSurvey: a nil probe or a failed scan is an absence of evidence, and printing it as "nothing is running here" would tell a user with a working client the opposite of the truth. allowScan() is untouched and still false for an applied binding: this changes what the CLI says, never what it targets. TestActiveClientBinding_AllowScan and TestDiscoverRelease_NoScanWhenExplicit pass unmodified, and TestExplain_BindingMiss_NamesTheLocalClientWithoutRetargeting pins both halves at once — the namespace appears in the message and nowhere else. The probe travels on the error rather than through explain's signature so a caller cannot hand it a clientset for a different cluster than the one that missed; six of the seven call sites never held one anyway (resolveClusterTarget builds it internally and returns nil on the error path). Refs cli#515 Co-Authored-By: Claude Opus 5 --- internal/cli/cluster.go | 5 +- internal/cli/clustertarget.go | 128 ++++++++++++++++++-- internal/cli/clustertarget_test.go | 175 +++++++++++++++++++++++++++- internal/cli/data_delete.go | 2 +- internal/cli/data_ingest_cluster.go | 2 +- internal/cli/data_list.go | 2 +- internal/cli/resources.go | 2 +- internal/cli/resources_set.go | 2 +- internal/cli/seal.go | 2 +- 9 files changed, 302 insertions(+), 18 deletions(-) diff --git a/internal/cli/cluster.go b/internal/cli/cluster.go index 6d331a9..b75b8e2 100644 --- a/internal/cli/cluster.go +++ b/internal/cli/cluster.go @@ -164,7 +164,10 @@ func runClusterInfo( // installed on this cluster". A binding miss gets the §7.3 // "runs elsewhere" explanation, same as the data commands. if errors.Is(err, cluster.ErrNoParentRelease) { - return binding.explain(&exitError{code: exitNoWorkspace, err: &noParentReleaseError{err}}) + return binding.explain(ctx, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{ + err: err, + probe: &clusterProbe{cs: cs, serverURL: resolved.ServerURL}, + }}) } return &exitError{code: exitNoWorkspace, err: err} } diff --git a/internal/cli/clustertarget.go b/internal/cli/clustertarget.go index b760867..57805c2 100644 --- a/internal/cli/clustertarget.go +++ b/internal/cli/clustertarget.go @@ -5,11 +5,13 @@ import ( "errors" "fmt" "strings" + "time" "k8s.io/client-go/kubernetes" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/installer" "github.com/tracebloc/cli/internal/ui" ) @@ -19,11 +21,36 @@ import ( // release, an API/RBAC list failure, or an ambiguous multiple-release match. // §7.3 uses it to turn an active-client binding miss into a clear "runs on // another machine" message; the other failures keep their own diagnostics. -type noParentReleaseError struct{ err error } +// +// probe carries the read-only handles explain needs to say what IS on the +// reached cluster before advising (#515). It is attached wherever the error is +// built — both sites already hold a clientset and the resolved server URL — and +// travels ON the error rather than through the call signature so a caller can +// never hand explain a clientset for a DIFFERENT cluster than the one that +// missed. A nil probe (a synthesised error, a resolveClusterTargetFn test +// double) means "we could not look", and explain then claims nothing. +type noParentReleaseError struct { + err error + probe *clusterProbe +} func (e *noParentReleaseError) Error() string { return e.err.Error() } func (e *noParentReleaseError) Unwrap() error { return e.err } +// clusterProbe is the pair explain needs to diagnose before advising: a +// clientset for the cluster the kubeconfig actually reached, and that cluster's +// server URL (which isLocalServerURL judges). +type clusterProbe struct { + cs kubernetes.Interface + serverURL string +} + +// explainScanTimeout bounds the naming-only cluster scan explain runs on the +// §7.3 error path. The scan only makes the message better, so it must never +// make the failure slower than the failure itself: past this, explain falls +// back to the message it would have printed without looking. +const explainScanTimeout = 5 * time.Second + // loadClusterFn / newClientsetFn are the kubeconfig-load + clientset-build // seams every command that reaches a cluster goes through — resolveClusterTarget // (data ingest/list/delete), runClusterInfo, and runClusterDoctor. Production @@ -87,7 +114,10 @@ func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.Kubec // "runs elsewhere" rewrite; an API/RBAC list failure or an // ambiguous multiple-release match keeps its own message. if errors.Is(err, cluster.ErrNoParentRelease) { - return nil, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{err}} + return nil, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{ + err: err, + probe: &clusterProbe{cs: cs, serverURL: resolved.ServerURL}, + }} } return nil, &exitError{code: exitNoWorkspace, err: err} } @@ -214,7 +244,18 @@ func (b activeClientBinding) allowScan() bool { return !b.applied && !b.explicit // came from the active-client binding: the cluster the kubeconfig reaches // doesn't host that client. Non-binding errors (and PVC-missing, where the // release *was* found) pass through unchanged. -func (b activeClientBinding) explain(err error) error { +// +// DIAGNOSE BEFORE ADVISING (#515). The shipped §7.3 sentence named no way back: +// it offered --namespace without ever saying WHICH namespace, so a user on a +// healthy local install had no supported recovery. explain now spends one +// naming-only cluster scan — the same read discoverRelease already spends +// purely to write a better message — and says what is actually here. +// +// This changes what the CLI SAYS, never what it TARGETS: allowScan() stays +// false, so a binding miss still never silently retargets to some other +// machine's client (§7.5). The scan's result reaches the user as text they must +// act on, which is the whole difference. +func (b activeClientBinding) explain(ctx context.Context, err error) error { if !b.applied { return err } @@ -226,8 +267,81 @@ func (b activeClientBinding) explain(err error) error { if handle == "" { handle = b.namespace } - return &exitError{code: exitNoWorkspace, err: fmt.Errorf( - "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; "+ - "run this command there, or override with --namespace/--context", - handle, b.namespace)} + return &exitError{code: exitNoWorkspace, + err: errors.New(repointMessage(handle, b.namespace, surveyCluster(ctx, npr.probe)))} +} + +// clientSurvey is what explain managed to learn about the reached cluster +// before advising. +// +// looked distinguishes "we scanned and the cluster hosts none" from "we could +// not scan at all" (no probe on the error, or the cluster-wide list failed — +// RBAC, a timeout, an unreachable API server). Collapsing the two would let an +// absence of evidence print as evidence of absence: the CLI would tell a user +// with a perfectly healthy client that nothing is running here. When looked is +// false the message says nothing about the cluster's contents at all. +type clientSurvey struct { + looked bool + namespaces []string + local bool // the kubeconfig's server is THIS machine (isLocalServerURL) +} + +// surveyCluster runs cluster.FindClientNamespaces FOR NAMING ONLY — nothing in +// this path changes the namespace anything targets. A nil probe (synthesised +// error / test double) or a failed scan both return a survey that looked at +// nothing, so explain falls back to the message it printed before #515. +func surveyCluster(ctx context.Context, probe *clusterProbe) clientSurvey { + if probe == nil || probe.cs == nil { + return clientSurvey{} + } + ctx, cancel := context.WithTimeout(ctx, explainScanTimeout) + defer cancel() + found, err := cluster.FindClientNamespaces(ctx, probe.cs) + if err != nil { + return clientSurvey{} + } + return clientSurvey{looked: true, namespaces: found, local: isLocalServerURL(probe.serverURL)} +} + +// repointMessage is the §7.3 error text, branched on what surveyCluster found. +// Pure (no I/O) so every branch is unit-testable as text. +// +// - exactly one client on a LOCAL cluster — a cluster that IS this machine — +// name it and offer the repoint. `client create` re-run on a cluster that +// already hosts a client adopts it: no prompt, no new credential (§7.2). +// - any client on a remote/shared cluster (or several anywhere) — name the +// namespaces and offer ONLY --namespace. Never `client create` here: that +// is the §7.5 boundary, and on a shared cluster the client we found may well +// be a colleague's. +// - none, scan clean — say so, and point at the installer, which is then the +// correct advice rather than a guess. +// - could not look — the pre-#515 sentence, unchanged. We make no claim. +// +// Each branch is ONE format literal rather than a concatenation, so the whole +// sentence lands in the copy catalog (zz-all-strings harvests literal arguments; +// a `+`-joined message is only ever half-visible there) and can be reviewed as +// the user reads it. +func repointMessage(handle, boundNS string, s clientSurvey) string { + switch { + case !s.looked: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context", + handle, boundNS) + case len(s.namespaces) == 0: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context.\n\nNo tracebloc client is running on this cluster either — if this machine should have one, set one up: %s", + handle, boundNS, installer.Cmd) + case len(s.namespaces) == 1 && s.local: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client IS running on this machine, in namespace %q.\n Point this machine at it: %s client create\n (this cluster already runs a client, so it adopts it — no new credential)\n Or target it just this once: --namespace %s", + handle, boundNS, s.namespaces[0], launcher(), s.namespaces[0]) + case len(s.namespaces) == 1: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client is running on this cluster, in namespace %q.\n Target it just this once: --namespace %s", + handle, boundNS, s.namespaces[0], s.namespaces[0]) + default: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\ntracebloc clients are running on this cluster, in namespaces: %s.\n Target one just this once: --namespace %s", + handle, boundNS, strings.Join(s.namespaces, ", "), s.namespaces[0]) + } } diff --git a/internal/cli/clustertarget_test.go b/internal/cli/clustertarget_test.go index e9e394d..1772496 100644 --- a/internal/cli/clustertarget_test.go +++ b/internal/cli/clustertarget_test.go @@ -18,6 +18,7 @@ import ( "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/installer" "github.com/tracebloc/cli/internal/ui" ) @@ -162,12 +163,13 @@ func TestBindActiveClientNamespace_NoActiveClient(t *testing.T) { } func TestActiveClientBinding_Explain(t *testing.T) { - noRelease := &exitError{code: 4, err: &noParentReleaseError{errors.New("no release")}} + noRelease := &exitError{code: 4, err: &noParentReleaseError{err: errors.New("no release")}} pvcMissing := &exitError{code: 4, err: errors.New("shared PVC not bound")} + ctx := context.Background() // Applied + "no release here" → rewritten to the §7.3 guidance. bound := activeClientBinding{applied: true, name: "gpu-box-01", namespace: "gpu-box-01"} - got := bound.explain(noRelease) + got := bound.explain(ctx, noRelease) if got == noRelease { t.Fatal("expected a rewritten error") } @@ -180,16 +182,181 @@ func TestActiveClientBinding_Explain(t *testing.T) { } // Applied but a PVC failure (release WAS found) → pass through untouched. - if bound.explain(pvcMissing) != pvcMissing { + if bound.explain(ctx, pvcMissing) != pvcMissing { t.Error("PVC-missing error should not be rewritten") } // Not applied → always pass through. - if (activeClientBinding{}).explain(noRelease) != noRelease { + if (activeClientBinding{}).explain(ctx, noRelease) != noRelease { t.Error("unbound explain should pass the error through") } } +// #515 — the three branches of the §7.3 message, as text. A binding miss used to +// name --namespace without ever saying WHICH namespace; each branch below is the +// answer explain now derives from what is actually on the reached cluster. +// +// repointMessage is pure, so this pins the exact wording; the surveyCluster +// tests below pin that the survey fed to it is honest. +func TestRepointMessage_Branches(t *testing.T) { + const handle, boundNS = "gpu-box-01", "gpu-box-01" + lead := `active client "gpu-box-01" runs on another machine — namespace "gpu-box-01" isn't on the cluster your kubeconfig points at` + + t.Run("one client on a local cluster offers the repoint", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{looked: true, namespaces: []string{"lukas-02"}, local: true}) + for _, want := range []string{ + lead, + `A tracebloc client IS running on this machine, in namespace "lukas-02".`, + "client create", + "no new credential", + "--namespace lukas-02", + } { + if !strings.Contains(got, want) { + t.Errorf("missing %q:\n%s", want, got) + } + } + }) + + t.Run("remote cluster never suggests client create", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{looked: true, namespaces: []string{"colleague-07"}, local: false}) + if !strings.Contains(got, "colleague-07") || !strings.Contains(got, "--namespace colleague-07") { + t.Errorf("remote branch must name the namespace and offer --namespace:\n%s", got) + } + // The §7.5 boundary: on a shared cluster the client we found may be + // someone else's, so the repoint must NOT be advertised. + if strings.Contains(got, "client create") { + t.Errorf("remote/shared cluster must never suggest `client create`:\n%s", got) + } + }) + + t.Run("several clients name them all and offer only --namespace", func(t *testing.T) { + // Local or not: with more than one client here, "point this machine at + // it" has no unambiguous "it" — so this stays the --namespace branch even + // on a local cluster. + for _, local := range []bool{true, false} { + got := repointMessage(handle, boundNS, clientSurvey{looked: true, namespaces: []string{"alpha", "beta"}, local: local}) + if !strings.Contains(got, "alpha, beta") || !strings.Contains(got, "--namespace alpha") { + t.Errorf("local=%v: multi branch should list both and offer --namespace:\n%s", local, got) + } + if strings.Contains(got, "client create") { + t.Errorf("local=%v: ambiguous multi-client must not suggest `client create`:\n%s", local, got) + } + } + }) + + t.Run("clean scan finding nothing points at the installer", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{looked: true}) + for _, want := range []string{ + lead, + "--namespace/--context", + "No tracebloc client is running on this cluster either", + installer.Cmd, + } { + if !strings.Contains(got, want) { + t.Errorf("missing %q:\n%s", want, got) + } + } + }) + + t.Run("could not look claims nothing", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{}) + want := lead + "; run this command there, or override with --namespace/--context" + if got != want { + t.Errorf("unlooked message must stay the pre-#515 sentence exactly\n got: %q\nwant: %q", got, want) + } + // An absence of evidence must never print as evidence of absence. + if strings.Contains(got, "No tracebloc client is running") { + t.Errorf("a failed/absent scan must not claim the cluster is empty:\n%s", got) + } + }) +} + +// surveyCluster is the only thing standing between the message and a false +// claim, so each way of "we could not look" has to come back as looked=false. +func TestSurveyCluster_FailsClosed(t *testing.T) { + t.Run("nil probe", func(t *testing.T) { + if s := surveyCluster(context.Background(), nil); s.looked { + t.Errorf("a nil probe must not report as looked: %+v", s) + } + }) + + t.Run("nil clientset", func(t *testing.T) { + if s := surveyCluster(context.Background(), &clusterProbe{serverURL: "https://127.0.0.1:6550"}); s.looked { + t.Errorf("a probe with no clientset must not report as looked: %+v", s) + } + }) + + t.Run("scan forbidden", func(t *testing.T) { + cs := fake.NewSimpleClientset() + cs.PrependReactor("list", "deployments", func(ktesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("forbidden: cannot list deployments at the cluster scope") + }) + s := surveyCluster(context.Background(), &clusterProbe{cs: cs, serverURL: "https://127.0.0.1:6550"}) + if s.looked { + t.Errorf("an RBAC-refused scan must not report as looked: %+v", s) + } + }) + + t.Run("clean empty scan looked and found nothing", func(t *testing.T) { + s := surveyCluster(context.Background(), &clusterProbe{cs: fake.NewSimpleClientset(), serverURL: "https://127.0.0.1:6550"}) + if !s.looked || len(s.namespaces) != 0 { + t.Errorf("a clean empty scan is looked-with-nothing: %+v", s) + } + if !s.local { + t.Error("a loopback server URL must survey as local") + } + }) + + t.Run("finds the client and judges locality", func(t *testing.T) { + cs := fake.NewSimpleClientset(jmDep("lukas-02")) + s := surveyCluster(context.Background(), &clusterProbe{cs: cs, serverURL: "https://k8s.corp.example:6443"}) + if !s.looked || len(s.namespaces) != 1 || s.namespaces[0] != "lukas-02" { + t.Errorf("survey = %+v, want the one namespace", s) + } + if s.local { + t.Error("a corporate API server must not survey as local") + } + }) +} + +// End-to-end through the real resolve path: a binding miss on a LOCAL cluster +// that hosts the client elsewhere must NAME it — and must still not target it. +// This is the pairing that matters (§7.5): the namespace appears in the message +// and nowhere else. +func TestExplain_BindingMiss_NamesTheLocalClientWithoutRetargeting(t *testing.T) { + cs := fake.NewSimpleClientset(jmDep("lukas-02")) + origLoad, origCS := loadClusterFn, newClientsetFn + t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + return &cluster.ResolvedConfig{ + Namespace: o.Namespace, + ServerURL: "https://127.0.0.1:6550", + RestConfig: &rest.Config{}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { return cs, nil } + + binding := activeClientBinding{applied: true, name: "gpu-box-01", namespace: "stale-ns"} + target, err := resolveClusterTarget(context.Background(), nil, + cluster.KubeconfigOptions{Namespace: "stale-ns"}, binding, false, false) + if err == nil { + t.Fatal("a binding miss must still fail — this changes the message, not the target") + } + if target != nil { + t.Fatalf("no target may be resolved from a binding miss, got %+v", target) + } + got := binding.explain(context.Background(), err) + if !strings.Contains(got.Error(), "lukas-02") { + t.Errorf("the message must name the client that IS here:\n%s", got) + } + if !strings.Contains(got.Error(), "client create") { + t.Errorf("a single client on a local cluster must be offered the repoint:\n%s", got) + } + if ExitCodeFromError(got) != 4 { + t.Errorf("exit code = %d, want 4", ExitCodeFromError(got)) + } +} + // jmDep builds a chart-labeled jobs-manager Deployment in the given namespace, // for the fallback-scan tests (mirrors the cluster package's fixture). func jmDep(namespace string) *appsv1.Deployment { diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index e90c9fc..98c84d2 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -175,7 +175,7 @@ undone — re-ingesting the data is the only way back.`) // mid-output blank between the warning and the note (§380). target, err := resolveClusterTargetFn(ctx, a.Printer, opts, binding, true, false) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } resolved, cs, release, pvc := target.Resolved, target.Clientset, target.Release, target.PVC diff --git a/internal/cli/data_ingest_cluster.go b/internal/cli/data_ingest_cluster.go index ffd17b4..a750d80 100644 --- a/internal/cli/data_ingest_cluster.go +++ b/internal/cli/data_ingest_cluster.go @@ -58,7 +58,7 @@ func connectIngestTarget(ctx context.Context, a *runDataIngestArgs) (target *clu // mid-output blank between "Connecting…" and the note (§380). target, err = resolveClusterTarget(ctx, a.Printer, opts, binding, true, false) if err != nil { - return nil, "", false, binding.explain(err) + return nil, "", false, binding.explain(ctx, err) } resolved, cs, release, pvc := target.Resolved, target.Clientset, target.Release, target.PVC // release.IngestorSAName is discovered from the ingestionAuthz ConfigMap by diff --git a/internal/cli/data_list.go b/internal/cli/data_list.go index 0196273..f89cbab 100644 --- a/internal/cli/data_list.go +++ b/internal/cli/data_list.go @@ -124,7 +124,7 @@ func runDataList(ctx context.Context, a runDataListArgs) (err error) { // multi-client redirect note is the opening line and self-leads its blank. target, err := resolveClusterTarget(ctx, p, opts, binding, false, true) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } resolved, cs, release := target.Resolved, target.Clientset, target.Release diff --git a/internal/cli/resources.go b/internal/cli/resources.go index b2ac85c..c636297 100644 --- a/internal/cli/resources.go +++ b/internal/cli/resources.go @@ -92,7 +92,7 @@ func runResourcesShow(ctx context.Context, p *ui.Printer, opts cluster.Kubeconfi // multi-client redirect note self-leads its one leading blank (§380). target, err := resolveClusterTargetFn(ctx, p, opts, binding, false, true) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } return renderResources(ctx, p, target) } diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index b9f5048..c9f5c27 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -145,7 +145,7 @@ func runResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, opts clust // and never regresses the #375 double-blank (§380). target, err := resolveClusterTarget(ctx, p, opts, binding, false, true) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } return applyResourcesSet(ctx, p, pr, target, opts, req) } diff --git a/internal/cli/seal.go b/internal/cli/seal.go index 04ec661..d2dca1f 100644 --- a/internal/cli/seal.go +++ b/internal/cli/seal.go @@ -78,7 +78,7 @@ func runSealCheck(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOpt // multi-client redirect note is the opening line and self-leads its blank. target, err := resolveClusterTargetFn(ctx, p, opts, binding, false, true) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } tt := helm.TestTarget{ Release: target.Release.ReleaseName, From 1843a112b47f5dd9bc86af659bffa32fa485fbe3 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 12:24:09 +0200 Subject: [PATCH 02/11] fix(doctor,home): a wrong pointer is not proof there is no environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #401 taught the home screen that an EMPTY active-client pointer says nothing about what runs on this machine. The wrong-pointer case was never covered, and it is the worse one: doctor binds the stale pointer, probes only that namespace, never scans, and prints "No secure environment on this machine yet" with the installer command underneath — over a perfectly healthy install. home has the same hole from the other side: its local-env fallback sat behind `if !binding.applied`, so a non-empty pointer skipped the #401 fix entirely. Both now route the miss through the same fallback: home the ErrNoParentRelease branch returns localEnvFallback(ctx) instead of a bare localNoRelease. Every failure inside the fallback degrades to localNoRelease, so this branch's old return value is still its floor. doctor on a ReachNoEnv result that a BINDING (not the user) aimed, re-probe the namespace the kubeconfig itself selects, via localEnvNamespace. The ownership gate is what makes this safe, and it is unchanged: both adopt only when isLocalServerURL says the kubeconfig's server is this machine — a cluster that is this machine by definition, so whatever runs there is this machine's environment. On a remote or shared cluster the honest no-environment answer stands, and a colleague's client is never greeted as yours (§7.5). No scan is spent either: the installer points the kubeconfig context at the client's namespace (client/scripts/lib/install-client-helm.sh runs `kubectl config set-context --current --namespace `), so reading the context is enough to find a healthy install that the pointer missed. doctor keeps the original results unless the re-probe actually finds an environment, so a genuinely bare machine still gets the installer advice and the --diagnose bundle still describes the namespace the user is configured for. An explicit --namespace is never second-guessed — no binding, no re-probe. Refs cli#515 Co-Authored-By: Claude Opus 5 --- internal/cli/doctor.go | 21 ++- internal/cli/doctor_pointer_test.go | 195 +++++++++++++++++++++++ internal/cli/home.go | 12 +- internal/cli/home_local_fallback.go | 46 +++++- internal/cli/home_local_fallback_test.go | 59 +++++++ 5 files changed, 326 insertions(+), 7 deletions(-) create mode 100644 internal/cli/doctor_pointer_test.go diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 3e22d3e..e381e0d 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -156,7 +156,7 @@ func runClusterDoctor( // 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) + binding := bindActiveClientNamespace(&opts) resolved, err = loadClusterFn(opts) if err != nil { p.Newline() @@ -176,6 +176,25 @@ func runClusterDoctor( // 4. Probe the cluster. results = doctorRunFn(ctx, cs, doctor.Options{Namespace: resolved.Namespace, ServerURL: resolved.ServerURL}) + // #515: a namespace we CHOSE for the user can be wrong, and a miss on it is + // not evidence that this machine has no environment — yet doctor's only + // reading of "no chart here" is "no secure environment on this machine yet", + // which then recommends reinstalling over a healthy install. Extend #401's + // local fallback to the wrong-pointer case: re-probe the namespace the + // KUBECONFIG selects, but only when the binding (not the user) picked the + // namespace that missed, and only on a LOCAL cluster — on a remote/shared one + // the ownership gate stands and we would risk naming a colleague's client. + // The retry is adopted only if it actually finds an environment, so a genuine + // no-environment machine keeps the original results (and the --diagnose + // bundle keeps describing the namespace the user is configured for). + if binding.applied && reachStateOf(results) == doctor.ReachNoEnv { + if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride}); ok && ns != resolved.Namespace { + if retry := doctorRunFn(ctx, cs, doctor.Options{Namespace: ns, ServerURL: resolved.ServerURL}); reachStateOf(retry) != doctor.ReachNoEnv { + resolved.Namespace, results = ns, retry + } + } + } + // 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 diff --git a/internal/cli/doctor_pointer_test.go b/internal/cli/doctor_pointer_test.go new file mode 100644 index 0000000..5a7a1cc --- /dev/null +++ b/internal/cli/doctor_pointer_test.go @@ -0,0 +1,195 @@ +package cli + +import ( + "bytes" + "context" + "net/http" + "strings" + "testing" + + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/rest" + + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/doctor" + "github.com/tracebloc/cli/internal/ui" +) + +// #515 — a WRONG active-client pointer must not read as "no environment". +// Split from doctor_test.go, which is already at its file budget. + +// stubDoctorForNamespace makes doctor's cluster I/O deterministic: the kubeconfig +// resolves to a caller-chosen server URL with the namespace opts asked for (or +// kubeconfigNS when nothing was pinned — exactly how cluster.Load layers an +// explicit namespace over the context's own), and the probe reports a healthy +// environment in envNS and ReachNoEnv anywhere else. It returns the list of +// namespaces the probe ran against, so a test can assert what was and wasn't +// re-probed rather than inferring it from the rendered text. +func stubDoctorForNamespace(t *testing.T, serverURL, kubeconfigNS, envNS string) *[]string { + t.Helper() + origLoad, origCS, origRun := loadClusterFn, newClientsetFn, doctorRunFn + t.Cleanup(func() { loadClusterFn, newClientsetFn, doctorRunFn = origLoad, origCS, origRun }) + + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + ns := o.Namespace + if ns == "" { + ns = kubeconfigNS + } + return &cluster.ResolvedConfig{ + Namespace: ns, Context: "test-ctx", ServerURL: serverURL, RestConfig: &rest.Config{}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { + return fake.NewSimpleClientset(), nil + } + var probed []string + doctorRunFn = func(_ context.Context, _ kubernetes.Interface, o doctor.Options) []doctor.Result { + probed = append(probed, o.Namespace) + if o.Namespace != envNS { + return []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv}} + } + return []doctor.Result{ + {Name: "Cluster reachable", Status: doctor.StatusOK, Reach: doctor.ReachOK}, + {Name: "Pod health", Status: doctor.StatusOK}, + } + } + return &probed +} + +// okWhoAmI stubs the session probe so these tests reach the cluster stage. +func okWhoAmI(t *testing.T) { + t.Helper() + stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) + }) +} + +// The field symptom: a healthy local k3d install whose active-client pointer +// names a namespace that isn't on this cluster. doctor bound the wrong pointer, +// probed only it, and told the user to reinstall over a working environment +// (#401 fixed only the EMPTY-pointer case). +func TestDoctor_WrongPointerOnLocalCluster_FindsTheEnvironment(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://127.0.0.1:6550", "lukas-02", "lukas-02") + + var out bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) + if err != nil { + t.Fatalf("a healthy local environment must not exit non-zero: %v", err) + } + if strings.Contains(out.String(), "No secure environment") { + t.Errorf("must not recommend a reinstall over a healthy install:\n%s", out.String()) + } + if !strings.Contains(out.String(), "lukas-02") { + t.Errorf("doctor should name the environment that IS here:\n%s", out.String()) + } + if len(*probed) != 2 || (*probed)[0] != "stale-ns" || (*probed)[1] != "lukas-02" { + t.Errorf("probe namespaces = %v, want [stale-ns lukas-02] (bound pointer first, then the kubeconfig's own)", *probed) + } +} + +// The ownership gate is what keeps the fallback honest, so it gets its own test: +// on a REMOTE/shared cluster a pointer miss stays "no secure environment" and the +// re-probe never runs — the client sitting in another namespace there may well be +// a colleague's (§7.5). +func TestDoctor_WrongPointerOnRemoteCluster_StaysGated(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://k8s.corp.example:6443", "colleague-07", "colleague-07") + + var out bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) + if err == nil { + t.Fatal("a pointer miss on a remote cluster is still a problem — want a non-zero exit") + } + if !strings.Contains(out.String(), "No secure environment") { + t.Errorf("remote cluster must keep the honest no-environment message:\n%s", out.String()) + } + if strings.Contains(out.String(), "colleague-07") { + t.Errorf("a remote cluster's other namespace must never be named as yours:\n%s", out.String()) + } + if len(*probed) != 1 { + t.Errorf("probe namespaces = %v, want exactly one (no re-probe off a remote cluster)", *probed) + } +} + +// A user who pinned --namespace themselves is never second-guessed: no binding +// was applied, so nothing re-probes and the miss stands as they asked for it. +func TestDoctor_ExplicitNamespaceMiss_IsNotReprobed(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://127.0.0.1:6550", "lukas-02", "lukas-02") + + var out bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "chosen-ns", false) + if err == nil { + t.Fatal("an explicit --namespace miss must still fail") + } + if len(*probed) != 1 || (*probed)[0] != "chosen-ns" { + t.Errorf("probe namespaces = %v, want only the namespace the user pinned", *probed) + } + if !strings.Contains(out.String(), "No secure environment") { + t.Errorf("want the no-environment message for an explicitly-pinned miss:\n%s", out.String()) + } +} + +// A machine that genuinely has nothing must keep the installer advice: the +// re-probe runs, finds no environment either, and the original results stand. +func TestDoctor_LocalClusterWithNothing_KeepsInstallerAdvice(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://127.0.0.1:6550", "default", "nowhere") + + var out bytes.Buffer + if err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false); err == nil { + t.Fatal("a machine with no environment must still exit non-zero") + } + if !strings.Contains(out.String(), "No secure environment") { + t.Errorf("want the no-environment message when the re-probe finds nothing too:\n%s", out.String()) + } + if len(*probed) != 2 { + t.Errorf("probe namespaces = %v, want the bound namespace and the kubeconfig's own", *probed) + } +} + +// localEnvNamespace is the doctor-side half of the #401 carve-out; its three +// refusals are what stop the re-probe from ever naming someone else's client. +func TestLocalEnvNamespace(t *testing.T) { + set := func(rc *cluster.ResolvedConfig, err error) { + t.Helper() + orig := loadClusterFn + t.Cleanup(func() { loadClusterFn = orig }) + loadClusterFn = func(cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { return rc, err } + } + + t.Run("local server with a namespace", func(t *testing.T) { + set(&cluster.ResolvedConfig{Namespace: "lukas-02", ServerURL: "https://127.0.0.1:6550"}, nil) + ns, ok := localEnvNamespace(cluster.KubeconfigOptions{}) + if !ok || ns != "lukas-02" { + t.Errorf("= %q,%v; want lukas-02,true", ns, ok) + } + }) + + t.Run("remote server is refused", func(t *testing.T) { + set(&cluster.ResolvedConfig{Namespace: "colleague-07", ServerURL: "https://k8s.corp.example:6443"}, nil) + if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{}); ok { + t.Errorf("= %q,%v; a remote cluster must be refused", ns, ok) + } + }) + + t.Run("empty namespace is refused", func(t *testing.T) { + set(&cluster.ResolvedConfig{Namespace: "", ServerURL: "https://127.0.0.1:6550"}, nil) + if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{}); ok { + t.Errorf("= %q,%v; an empty namespace is not a reading", ns, ok) + } + }) + + t.Run("load failure is refused", func(t *testing.T) { + set(nil, context.DeadlineExceeded) + if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{}); ok { + t.Errorf("= %q,%v; an unreadable kubeconfig is not a reading", ns, ok) + } + }) +} diff --git a/internal/cli/home.go b/internal/cli/home.go index c438ff8..7860ef0 100644 --- a/internal/cli/home.go +++ b/internal/cli/home.go @@ -511,8 +511,16 @@ func realProbeEnv(ctx context.Context) envProbe { if err != nil { if errors.Is(err, cluster.ErrNoParentRelease) { // Cluster reachable, but this release isn't in the resolved context. - // Provisioned ⇒ resolveHomeModel turns this into a named "offline". - return envProbe{local: localNoRelease} + // #515: a WRONG pointer is no more proof of "no environment" than the + // empty one #401 covered — the binding above overrode the kubeconfig's + // own namespace with a stale/foreign one, so this miss says nothing + // about what runs here. Re-ask through the same local-only fallback: + // it adopts a release ONLY when the kubeconfig's server is this + // machine, so the shared-cluster guarantee is untouched, and every + // other outcome is localNoRelease — exactly what this branch returned + // before. Provisioned ⇒ resolveHomeModel turns that into a named + // "offline". + return localEnvFallback(ctx) } // A list/RBAC/connect failure: we couldn't confirm what's here. Treat it // as unreachable (→ offline if provisioned, else no-env). diff --git a/internal/cli/home_local_fallback.go b/internal/cli/home_local_fallback.go index 8900154..916bbd6 100644 --- a/internal/cli/home_local_fallback.go +++ b/internal/cli/home_local_fallback.go @@ -15,10 +15,23 @@ import ( ) // localEnvFallback answers "is there a secure environment on THIS machine?" -// when the active-client pointer is empty — the state every pre-#388 Windows -// install is in permanently, because only `client create` writes the pointer -// and the Windows installer never ran it. Field case: `doctor` said "Ready to -// run training" while home said "No secure environment on this machine yet". +// when the active-client pointer cannot be trusted: +// +// - it is EMPTY (#401) — the state every pre-#388 Windows install is in +// permanently, because only `client create` writes the pointer and the +// Windows installer never ran it. Field case: `doctor` said "Ready to run +// training" while home said "No secure environment on this machine yet". +// - it is SET BUT WRONG (#515) — it names a namespace that isn't on the +// cluster this kubeconfig reaches (an orphaned record left by a cluster +// recreation, a pointer written on another machine). #401 covered only the +// empty case, so a wrong pointer went on recommending a reinstall over a +// healthy install. +// +// Both are the same question, and the answer must not come from the pointer: +// this reloads the kubeconfig with NO binding applied, so it probes the +// namespace the kubeconfig itself selects — which is the client's own namespace +// on any installer-provisioned machine (install-client-helm.sh runs +// `kubectl config set-context --current --namespace `). // // The ownership gate in realProbeEnv exists so a status screen never greets a // SHARED cluster's unrelated client as yours (§7.5). This fallback keeps that @@ -62,6 +75,31 @@ func localEnvFallback(ctx context.Context) envProbe { return ep } +// localEnvNamespace reports the namespace the KUBECONFIG itself selects, and +// whether that reading is usable — i.e. the kubeconfig loads and the cluster it +// reaches is LOCAL (the #401 carve-out: a cluster that is this machine by +// definition, so whatever tracebloc release runs there is this machine's). +// +// It is the doctor-side half of localEnvFallback (#515), for the one caller that +// already holds a clientset and only needs the namespace to re-probe. Like the +// fallback it applies NO active-client binding — that pointer is precisely the +// thing under suspicion — and it never scans: the installer points the +// kubeconfig context at the client's namespace +// (install-client-helm.sh: `kubectl config set-context --current --namespace`), +// so reading it is enough and no cluster-wide list is spent. On a remote or +// shared cluster it returns false, so the ownership gate holds exactly as it +// does on the home screen. +func localEnvNamespace(opts cluster.KubeconfigOptions) (string, bool) { + resolved, err := loadClusterFn(opts) + if err != nil || resolved == nil { + return "", false + } + if !isLocalServerURL(resolved.ServerURL) || resolved.Namespace == "" { + return "", false + } + return resolved.Namespace, true +} + // isLocalServerURL reports whether a kubeconfig server URL points at THIS // machine. Covers loopback names/addresses, the wildcard binds k3d writes when // no host is pinned, and Docker Desktop's host alias (the same signals diff --git a/internal/cli/home_local_fallback_test.go b/internal/cli/home_local_fallback_test.go index b8e3894..01a80ab 100644 --- a/internal/cli/home_local_fallback_test.go +++ b/internal/cli/home_local_fallback_test.go @@ -91,6 +91,65 @@ func TestLocalEnvFallback_NoKubeconfigIsNoRelease(t *testing.T) { } } +// #515: the home screen's hole was the mirror of doctor's. Its local-env +// fallback was reached only when the pointer was EMPTY (`if !binding.applied`), +// so a pointer that was set but WRONG skipped the #401 fix entirely and the +// screen said "No secure environment on this machine yet" over a live install. +func TestRealProbeEnv_WrongPointerOnLocalCluster_FallsBack(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") // binding APPLIED, and wrong + o := fallbackRelease("lukas-02") + cs := fake.NewClientset(o[0].(*appsv1.Deployment), o[1].(*corev1.Service)) + + origLoad, origCS := loadClusterFn, newClientsetFn + t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) + // Models cluster.Load: an explicit opts.Namespace wins, otherwise the + // context's own namespace — which the installer points at the client's + // namespace (install-client-helm.sh `kubectl config set-context --current + // --namespace`). So the binding sends the first probe to "stale-ns" and the + // unbound fallback reload lands on "lukas-02". + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + ns := o.Namespace + if ns == "" { + ns = "lukas-02" + } + return &cluster.ResolvedConfig{ + Namespace: ns, ServerURL: "https://127.0.0.1:6550", RestConfig: &rest.Config{}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { return cs, nil } + + ep := realProbeEnv(context.Background()) + if ep.local != localLive || ep.name != "tracebloc" { + t.Fatalf("=> %+v, want the live local environment despite the stale pointer", ep) + } +} + +// …and the ownership gate survives it: the same wrong pointer on a REMOTE +// cluster stays no-release, so a shared cluster's unrelated client is never +// greeted as yours (§7.5). +func TestRealProbeEnv_WrongPointerOnRemoteCluster_StaysGated(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + o := fallbackRelease("colleague-07") + cs := fake.NewClientset(o[0].(*appsv1.Deployment), o[1].(*corev1.Service)) + + origLoad, origCS := loadClusterFn, newClientsetFn + t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + ns := o.Namespace + if ns == "" { + ns = "colleague-07" + } + return &cluster.ResolvedConfig{ + Namespace: ns, ServerURL: "https://k8s.corp.example:6443", RestConfig: &rest.Config{}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { return cs, nil } + + if ep := realProbeEnv(context.Background()); ep.local != localNoRelease || ep.name != "" { + t.Fatalf("=> %+v, want localNoRelease (a shared cluster's client is not yours)", ep) + } +} + func TestIsLocalServerURL(t *testing.T) { local := []string{ "https://127.0.0.1:6550", From f4bdf9bfd04d1dd306bbffe14534ebd8a35c4bbc Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 12:24:29 +0200 Subject: [PATCH 03/11] feat(client): list `create`, and stop calling a stale pointer "this machine" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the diagnose-before-advising fix needs in order to reach anyone. `client create` is visible. It was Hidden because a human running it standalone on a cluster with no client mints one the installer never deploys — an orphaned phantom (backend#970). That risk is real and unchanged, but hiding the command was never what prevented it. The two guards that do are untouched, and now have tests naming them: • on a TTY, the review + "Provision this client?" confirm. A re-run on a cluster that already hosts a client never reaches it — adoption happens first — so the repoint stays prompt-free and mints nothing. • off a TTY, a hard refusal without --yes/--credential-file, so a pipe or CI can never mint silently. What hiding did cost is #515: the one command that repoints a machine was unlisted, so the error telling a user to repoint pointed at nothing they could find. Its Short/Long now describe what it does for a user (adopt/repoint) rather than the installer's use of it. `client list` stays hidden. `client list` marks residency, not just selection. It labelled the active pointer "(active — this machine)" without ever checking where that client runs — so in exactly the state this ticket is about, the listing sat there confirming a client provably not on this machine. Selection (the local pointer) and residency (does it run on the cluster the kubeconfig reaches, keyed on the §7.2 cluster anchor) are now two separate facts, and a mismatch names the repoint. An unreadable anchor is a third state, not a "no": with no kubeconfig or an unreachable API server, no row claims to be here and none is denied — the marker degrades to bare "(active)". The installer's #303 pre-flight is unaffected; the markers sit in the row label and the greppable `namespace=` field is untouched (client_list_contract_test.go still passes). Refs cli#515 Co-Authored-By: Claude Opus 5 --- internal/cli/client.go | 91 +++++++++++++++++---- internal/cli/client_test.go | 158 ++++++++++++++++++++++++++++++++++-- 2 files changed, 226 insertions(+), 23 deletions(-) diff --git a/internal/cli/client.go b/internal/cli/client.go index cd662a4..c06ffe8 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -39,8 +39,9 @@ var readInClusterClient = cluster.DiscoverInClusterClient // The single-machine CLI (RFC-0001 §7.10) owns exactly one client, so there is // nothing to *select*: `client use` is withdrawn, and `client list` is hidden // (kept callable for the installer's one-client-per-machine pre-flight, off the -// user-facing surface). `create` provisions this machine's client; offboarding -// is the top-level `tracebloc delete`. +// user-facing surface). `create` points this machine at its client — adopting +// the one already on the cluster, which is the supported repoint (#515) — +// and offboarding is the top-level `tracebloc delete`. func newClientCmd() *cobra.Command { cmd := &cobra.Command{ Use: "client", @@ -63,15 +64,31 @@ func newClientCreateCmd() *cobra.Command { var yes bool cmd := &cobra.Command{ Use: "create", - Short: "Provision a tracebloc client for this machine (auto-named; no flags required)", - // HIDDEN: provisioning is the installer's job — provision.sh calls this with - // zero flags (cli#137). It stays fully callable (including `--help`, so the - // installer's capability probe still works), but is kept off the user-facing - // surface: a human running `client create` STANDALONE mints a client the - // installer never deploys — an orphaned "phantom" (backend#970). Mirrors the - // hidden `list`; leaves `tracebloc client` showing only the user-useful `status`. - Hidden: true, - Args: cobra.NoArgs, + Short: "Point this machine at its tracebloc client — adopts the one already on this cluster", + Long: `Point this machine at its tracebloc client. + +Keyed on the cluster your kubeconfig reaches: if a tracebloc client already runs +there, this ADOPTS it — no prompt, no new credential, no duplicate — which is how +you repoint a machine whose active client went stale. On a cluster that runs no +client yet it provisions a new one, and asks first. + +Provisioning a brand-new machine is normally the installer's job — it calls this +for you, with no flags.`, + // WAS HIDDEN (backend#970), and the reason still stands: a human running + // this STANDALONE on a cluster with no client mints one the installer never + // deploys — an orphaned "phantom". Hiding it was never what prevented that, + // though; the mint-path guards below are, and they are untouched: + // • on a TTY, the review + `Provision this client?` confirm (which a + // re-run on an already-registered cluster never reaches — it adopts + // before the prompt, so the repoint stays zero-friction); + // • off a TTY, a hard refusal without --yes/--credential-file, so a pipe + // or CI can never mint silently. + // What hiding DID cost is #515: the §7.3 "your active client runs on another + // machine" error had no supported way back, because the one command that + // repoints a machine was unlisted. Advice pointing at a hidden command is + // not advice, so it is listed now — described by what it does for a user + // (adopt/repoint) rather than by the installer's use of it. + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { return runClientCreate(cmd.Context(), printerFor(cmd), clientPrompter(), clientCreateOpts{name: name, location: location, kubeconfigPath: kubeconfigPath, contextOverride: contextOverride, credentialFile: credentialFile, yes: yes}) @@ -747,21 +764,65 @@ func runClientList(ctx context.Context, p *ui.Printer) error { } p.Section("Clients in your account") active := cfg.Current().ActiveClientID + // #515: "active" is a LOCAL POINTER, and this listing used to render it as + // "(active — this machine)" — a claim about location it never checked. When + // the pointer is stale that label sits next to a client provably not on the + // cluster this machine reaches, which is the state the whole ticket is about. + // Read the cluster anchor (kube-system UID, the §7.2 identity `client create` + // keys on) and mark residency separately from selection. A failed read is + // three-valued on purpose: unknown is not "elsewhere", so the marker then + // claims nothing about where anything runs. + clusterID, cidErr := readClusterID(ctx, cluster.KubeconfigOptions{}) + hereKnown := cidErr == nil && clusterID != "" + mismatch := false for _, c := range clients { - marker := "" - if strconv.Itoa(c.ID) == active { - marker = " (active — this machine)" + isActive := strconv.Itoa(c.ID) == active + here := hereKnown && c.ClusterID != "" && c.ClusterID == clusterID + if isActive && hereKnown && !here { + mismatch = true } - p.Field(strconv.Itoa(c.ID)+marker, + p.Field(strconv.Itoa(c.ID)+clientListMarker(isActive, hereKnown, here), fmt.Sprintf("%s state=%s namespace=%s location=%s", c.Name, clientStateLabel(c.Status), c.Namespace, c.Location)) } // §7.3: separate "selected" (this machine's local pointer) from "connected" // (the backend's last-heartbeat state) so a stale pointer is visible. p.Hintf("\"active\" is this machine's selected client; state is its last reported status to tracebloc.") + if mismatch { + // The exact state #515 describes, and the one supported way out of it: + // re-running create on a cluster that already hosts a client adopts it — + // no prompt, no new credential (§7.2). + p.Hintf("Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create", launcher()) + } return nil } +// clientListMarker renders one row's suffix in `client list`, keeping SELECTION +// (this machine's local pointer) and RESIDENCY (does this client run on the +// cluster the kubeconfig reaches) as two separate facts (#515). +// +// hereKnown false means the cluster anchor could not be read — no kubeconfig, an +// unreachable API server, RBAC on kube-system. That is an absence of evidence, +// so no row may claim to be here and no row may be denied: the marker degrades +// to bare "(active)", which says only what the local config actually knows. +func clientListMarker(isActive, hereKnown, here bool) string { + switch { + case !hereKnown: + if isActive { + return " (active)" + } + return "" + case isActive && here: + return " (active — on this cluster)" + case isActive: + return " (active — NOT on the cluster your kubeconfig reaches)" + case here: + return " (on this cluster)" + default: + return "" + } +} + // setActiveClient points this env's profile at c, caching its namespace and // display name alongside the id so the data commands can bind to the active // client's cluster (§7.3) without a backend round-trip. Callers Save() after. diff --git a/internal/cli/client_test.go b/internal/cli/client_test.go index fab9bd4..d3698e3 100644 --- a/internal/cli/client_test.go +++ b/internal/cli/client_test.go @@ -1470,19 +1470,18 @@ func TestClientStatus_WaitCtrlCIsSilent(t *testing.T) { } func TestClientSubcommandVisibility(t *testing.T) { - // `create` and `list` are installer-internal — Hidden so a user isn't invited to - // run them (a standalone `tracebloc client create` mints a client the installer - // never deploys, i.e. an orphaned phantom, backend#970). `status` stays - // user-visible. Hidden != disabled: all remain runnable (the installer still - // invokes create/list). + // `create` is VISIBLE since #515: it is the supported way to repoint a machine + // whose active client went stale, and the §7.3 error now names it — advice + // pointing at a hidden command is not advice. `list` stays installer-internal. + // `status` stays user-visible. Hidden != disabled: all remain runnable. hidden := map[string]bool{} runnable := map[string]bool{} for _, c := range newClientCmd().Commands() { hidden[c.Name()] = c.Hidden runnable[c.Name()] = c.RunE != nil } - if !hidden["create"] { - t.Error("client create must be Hidden (installer-internal; standalone mints a phantom)") + if hidden["create"] { + t.Error("client create must be visible — the #515 repoint advice names it") } if !hidden["list"] { t.Error("client list must stay Hidden") @@ -1491,7 +1490,150 @@ func TestClientSubcommandVisibility(t *testing.T) { t.Error("client status must stay user-visible") } if !runnable["create"] { - t.Error("hidden create must still be runnable (the installer invokes it)") + t.Error("create must still be runnable (the installer invokes it)") + } +} + +// Unhiding `create` must not reopen backend#970: hiding it was never what +// stopped a standalone run from minting a phantom — these two guards are, and +// they have to survive the visibility change. Off a TTY (pr == nil) with no +// --yes and no --credential-file, a fresh mint is REFUSED; nothing is posted. +func TestClientCreate_UnhiddenStillRefusesSilentMint(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + posted = true + } + _, _ = w.Write([]byte(`[]`)) // no clients on the account → a mint, not an adopt + }) + signInAs(t, "Lab", "lab@example.com") + var out bytes.Buffer + err := runClientCreate(context.Background(), ui.New(&out), nil, clientCreateOpts{}) + if err == nil { + t.Fatal("a non-interactive bare `client create` must refuse to mint") + } + if !strings.Contains(err.Error(), "refusing to provision non-interactively") { + t.Errorf("want the pipe refusal, got: %v", err) + } + if posted { + t.Error("nothing may be provisioned by a refused run") + } +} + +// The TTY half of the same guard: on a terminal a fresh mint asks first, and a +// "no" provisions nothing. (The repoint itself never reaches this prompt — an +// already-registered cluster adopts before it, covered by the adopt tests.) +func TestClientCreate_UnhiddenStillPromptsBeforeMinting(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + posted = true + } + _, _ = w.Write([]byte(`[]`)) + }) + signInAs(t, "Lab", "lab@example.com") + no := false + var out bytes.Buffer + if err := runClientCreate(context.Background(), ui.New(&out), &fakePrompter{confirm: &no}, clientCreateOpts{}); err != nil { + t.Fatalf("declining is a clean exit, got: %v", err) + } + if posted { + t.Error("a declined confirm must provision nothing") + } +} + +// #515: `client list` used to label the active pointer "(active — this machine)" +// without ever checking where that client runs, so a stale pointer read as +// confirmation. Selection and residency are now two separate facts, keyed on the +// cluster anchor (§7.2). +func TestClientListMarker(t *testing.T) { + cases := []struct { + name string + isActive, known, here bool + want string + }{ + {"anchor unreadable, active → claims only selection", true, false, false, " (active)"}, + {"anchor unreadable, other → no claim", false, false, false, ""}, + {"active and here", true, true, true, " (active — on this cluster)"}, + {"active but elsewhere", true, true, false, " (active — NOT on the cluster your kubeconfig reaches)"}, + {"here but not selected", false, true, true, " (on this cluster)"}, + {"neither", false, true, false, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := clientListMarker(c.isActive, c.known, c.here); got != c.want { + t.Errorf("clientListMarker(%v,%v,%v) = %q, want %q", c.isActive, c.known, c.here, got, c.want) + } + }) + } + // The specific claim #515 calls out: an unreadable anchor must never let a + // row assert it is here. + for _, isActive := range []bool{true, false} { + if strings.Contains(clientListMarker(isActive, false, false), "this cluster") { + t.Errorf("isActive=%v: an unreadable cluster anchor must claim nothing about location", isActive) + } + } +} + +// End-to-end: with the anchor readable, the row that matches the LOCAL cluster +// is marked as such — even when the pointer names a different one — and the +// listing names the repoint. +func TestClientList_MarksTheClientOnThisCluster(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"first_name":"stale","namespace":"stale-ns","cluster_id":"uid-OTHER"},` + + `{"id":2,"first_name":"here","namespace":"lukas-02","cluster_id":"uid-HERE"}]`)) + }) + stubClusterID(t, "uid-HERE", nil) + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" // the pointer names the client that is NOT here + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if !strings.Contains(got, "1 (active — NOT on the cluster your kubeconfig reaches)") { + t.Errorf("the stale active pointer must be marked as not here:\n%s", got) + } + if !strings.Contains(got, "2 (on this cluster)") { + t.Errorf("the client that IS here must be marked:\n%s", got) + } + if !strings.Contains(got, "client create") { + t.Errorf("a mismatch must name the repoint:\n%s", got) + } +} + +// The unreadable-anchor path end to end: no cluster reachable ⇒ no row claims a +// location, and the mismatch hint stays silent (we cannot know there is one). +func TestClientList_UnreadableAnchorClaimsNoLocation(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { // stubs readClusterID to an error + _, _ = w.Write([]byte(`[{"id":1,"first_name":"stale","namespace":"stale-ns","cluster_id":"uid-OTHER"}]`)) + }) + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if !strings.Contains(got, "1 (active)") { + t.Errorf("want the bare selection marker when the anchor is unreadable:\n%s", got) + } + if strings.Contains(got, "this cluster") || strings.Contains(got, "kubeconfig reaches") { + t.Errorf("an unreadable anchor must claim nothing about location:\n%s", got) } } From 84a6ef604ba5cb87dd01134a8f42531f80d8f704 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 12:24:46 +0200 Subject: [PATCH 04/11] docs(cli): regenerate the copy catalog; `client create` is no longer hidden MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The golden diff is the whole user-visible change, reviewed line by line: 08-client.golden `create` now appears under Available Commands, with its new Short and the Long that explains adoption. zz-all-strings.golden the five §7.3 branches and the `client list` mismatch hint. Each branch of repointMessage is one format literal rather than a `+`-joined string, because the catalog's AST harvest only sees literal arguments — the message it replaced was invisible there for exactly that reason, and half a sentence in the completeness backstop is worse than none. cli-navigation.md carried two statements this change makes false: it drew `client create` as a hidden node, and its exit-4 remedy line said "run the installer (or --namespace)", which is now only one of three answers. Refs cli#515 Co-Authored-By: Claude Opus 5 --- docs/cli-navigation.md | 4 ++-- internal/cli/testdata/golden/08-client.golden | 11 ++++++++++- internal/cli/testdata/golden/zz-all-strings.golden | 6 ++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/docs/cli-navigation.md b/docs/cli-navigation.md index f8ce400..cba1423 100644 --- a/docs/cli-navigation.md +++ b/docs/cli-navigation.md @@ -22,7 +22,7 @@ flowchart TD ACCT --> logout["logout"] ACCT --> authst["auth status"] ACCT --> clis["client status"] - ACCT -.-> clcreate["client create"]:::hidden + ACCT --> clcreate["client create — point this machine at its client"] ACCT -.-> cllist["client list"]:::hidden ENVC --> di["data ingest"] @@ -172,7 +172,7 @@ flowchart TD - **not signed in / token 401·403** → `login` - **426 upgrade-required** → upgrade the CLI - **kubeconfig (exit 3)** → fix `--kubeconfig`/`--context`, then `doctor` -- **no client / environment (exit 4)** → run the installer (or `--namespace`); triage with `doctor` +- **no client / environment (exit 4)** → the error now says what IS on the reached cluster before advising (cli#515): one client on a local cluster → `client create` repoints this machine (it adopts, no new credential); a client on a remote/shared cluster → `--namespace ` only; nothing there → run the installer. Triage with `doctor` - **no token (exit 5)** → grant RBAC; diagnose with `cluster info` / `doctor` - **destination exists (exit 6)** → `--overwrite`, a different `--name`, or `data delete` first - **staging partial (exit 7)** → `data delete` then re-ingest diff --git a/internal/cli/testdata/golden/08-client.golden b/internal/cli/testdata/golden/08-client.golden index 8a94809..f8ff84e 100644 --- a/internal/cli/testdata/golden/08-client.golden +++ b/internal/cli/testdata/golden/08-client.golden @@ -57,6 +57,7 @@ Usage: tracebloc client [command] Available Commands: + create Point this machine at its tracebloc client — adopts the one already on this cluster status Show whether tracebloc can see this machine's client (online) Flags: @@ -69,7 +70,15 @@ Global Flags: Use "tracebloc client [command] --help" for more information about a command. $ tracebloc client create --help -Provision a tracebloc client for this machine (auto-named; no flags required) +Point this machine at its tracebloc client. + +Keyed on the cluster your kubeconfig reaches: if a tracebloc client already runs +there, this ADOPTS it — no prompt, no new credential, no duplicate — which is how +you repoint a machine whose active client went stale. On a cluster that runs no +client yet it provisions a new one, and asks first. + +Provisioning a brand-new machine is normally the installer's job — it calls this +for you, with no flags. Usage: tracebloc client create [flags] diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 3dec5c0..8e2b022 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -348,6 +348,7 @@ screen. %s/%d are runtime placeholders. "Wrote client id + namespace to %s (no new credential — the existing one stands)." "You don't have permission to %s in this account." "Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule." +"Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create" "Your data is registered as a dataset. View it at https://ai.tracebloc.io/metadata" "Your dataset records (marked unavailable, not deleted)" "Your files are copied securely into your secure environment's storage — set up and cleaned up for you." @@ -363,6 +364,11 @@ screen. %s/%d are runtime placeholders. "a training run needs at least %s — %s is too little." "account" "active client" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client IS running on this machine, in namespace %q.\n Point this machine at it: %s client create\n (this cluster already runs a client, so it adopts it — no new credential)\n Or target it just this once: --namespace %s" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client is running on this cluster, in namespace %q.\n Target it just this once: --namespace %s" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\ntracebloc clients are running on this cluster, in namespaces: %s.\n Target one just this once: --namespace %s" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context.\n\nNo tracebloc client is running on this cluster either — if this machine should have one, set one up: %s" "annotations" "app version" "authorized — confirming the token with the backend …" From 4139a74a85dd672c6e33b38ecafa1644004aaa55 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 12:28:14 +0200 Subject: [PATCH 05/11] chore(release): bump VERSION to 0.10.9 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.10.8 is already released and this PR changes published files under `internal/*`, so version-bump-gate (a required check) asks for the bump here rather than leaving it to fail the next prod hop on somebody else (backend#1561). The release train reads VERSION and cuts the tag from it. Patch, matching this repo's dominant pattern for user-facing copy and surface changes — say so on the PR if 0.11.0 is wanted for the `client create` unhide. Refs cli#515 Co-Authored-By: Claude Opus 5 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 1a46c7f..f314d02 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.8 +0.10.9 From 44e942c08e7ce7e6925ac14b1078e7d06ae13616 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 12:30:45 +0200 Subject: [PATCH 06/11] docs(cluster): say why explain replaces the error instead of wrapping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `%w` is the house convention (~325 sites), which makes a bare errors.New here read as an oversight. It is the same deliberate replacement the fmt.Errorf it replaced did: the §7.3 guidance is meant to BE the message, not to trail the raw "no release in namespace X". Wrapping would also make the result re-match errors.As(*noParentReleaseError) and so re-explainable. Recorded in place so a reviewer doesn't have to re-derive it. Refs cli#515 Co-Authored-By: Claude Opus 5 --- internal/cli/clustertarget.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/cli/clustertarget.go b/internal/cli/clustertarget.go index 57805c2..1ebcb68 100644 --- a/internal/cli/clustertarget.go +++ b/internal/cli/clustertarget.go @@ -267,6 +267,13 @@ func (b activeClientBinding) explain(ctx context.Context, err error) error { if handle == "" { handle = b.namespace } + // errors.New, not %w: the rewrite deliberately REPLACES the discovery error + // rather than wrapping it, so the §7.3 guidance is the whole message and the + // raw "no release in namespace X" doesn't trail it. That was already true of + // the fmt.Errorf this replaced — the exit code (exitNoWorkspace, on the + // *exitError above) is the machine-readable contract here, not the chain. + // Wrapping would also make the result re-match errors.As(*noParentReleaseError) + // and so re-explainable, which nothing wants. return &exitError{code: exitNoWorkspace, err: errors.New(repointMessage(handle, b.namespace, surveyCluster(ctx, npr.probe)))} } From 63364ff398e650cee27ff92c3ca7ce449f4bf318 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 12:43:52 +0200 Subject: [PATCH 07/11] fix(doctor,home,client): finding the environment is only half the story MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both Bugbot findings on this PR were right, and both are the same mistake: a two-valued answer where the honest answer has three values. HIGH — "stale pointer still blocks after fallback". Re-probing found the healthy environment and then said nothing about the pointer that missed. So doctor printed "Everything looks good — you're ready to run training" and exited 0 while `data`/`resources`/`seal` all still bind the stale namespace and exit 4; before this PR that state at least exited 3. doctor now names the stale pointer and the repoint, and exits 2 — the code it already uses for every actionable finding. A problem WAS found; it just isn't in the cluster. The home half was worse, and was newly introduced here: local liveness came from the fallback's client while the heartbeat is still looked up by the STALE client's id, so a colleague's machine being online could render this one green. envProbe carries pointerStale, and resolveHomeModel refuses both directions off it — a stale heartbeat can no longer green the screen, nor harden into "backend reports not online" for a client it isn't about. It drops to "running, couldn't confirm", which is exactly true. MEDIUM — "empty cluster ID marked absent". `client list` compared anchors as a boolean, so a client whose OWN anchor is empty — legacy / not-yet-backfilled, which api.ProvisionedClient documents — was reported as "NOT on the cluster your kubeconfig reaches", with the repoint hint, possibly while running on this very machine. Exactly the collapse this PR's cluster-anchor handling was careful to avoid, missed one level down. Residency is now a three-valued residencyOf(): either anchor missing is resUnknown, and unknown claims nothing either way. realProbeEnv moved to home_local_fallback.go: home.go went 13 lines over its file budget, and the probe is now mostly a decision about WHICH fallback to take, so it reads better beside them than beside the renderer. Six mutations, each with its anchor asserted and each reddening an assertion rather than the compiler: collapse the empty client anchor; let doctor green a stale pointer; drop doctor's stale-pointer note; stop marking the fallback's result stale; let a stale pointer render Online; let another client's not-online harden into a verdict. TestDoctor_HealthyPointer_StillGreen is the control — without it, "never says Everything looks good" would pass vacuously. Refs cli#515 Co-Authored-By: Claude Opus 5 --- internal/cli/client.go | 55 ++++++++--- internal/cli/client_test.go | 91 ++++++++++++++--- internal/cli/doctor.go | 25 ++++- internal/cli/doctor_pointer_test.go | 44 ++++++++- internal/cli/home.go | 97 +++---------------- internal/cli/home_local_fallback.go | 94 +++++++++++++++++- internal/cli/home_local_fallback_test.go | 60 ++++++++++++ .../cli/testdata/golden/zz-all-strings.golden | 3 + 8 files changed, 351 insertions(+), 118 deletions(-) diff --git a/internal/cli/client.go b/internal/cli/client.go index c06ffe8..832fc13 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -777,11 +777,11 @@ func runClientList(ctx context.Context, p *ui.Printer) error { mismatch := false for _, c := range clients { isActive := strconv.Itoa(c.ID) == active - here := hereKnown && c.ClusterID != "" && c.ClusterID == clusterID - if isActive && hereKnown && !here { + res := residencyOf(hereKnown, clusterID, c.ClusterID) + if isActive && res == resElsewhere { mismatch = true } - p.Field(strconv.Itoa(c.ID)+clientListMarker(isActive, hereKnown, here), + p.Field(strconv.Itoa(c.ID)+clientListMarker(isActive, res), fmt.Sprintf("%s state=%s namespace=%s location=%s", c.Name, clientStateLabel(c.Status), c.Namespace, c.Location)) } @@ -797,26 +797,53 @@ func runClientList(ctx context.Context, p *ui.Printer) error { return nil } +// residency answers "does this client run on the cluster the kubeconfig +// reaches" in THREE values, because two of them are absences and an absence is +// never a "no" (Bugbot, #515). +type residency int + +const ( + // resUnknown: we cannot tell. Either the local cluster anchor was + // unreadable (no kubeconfig, unreachable API server, RBAC on kube-system), + // or the CLIENT carries no anchor — `ProvisionedClient.ClusterID` is empty + // on legacy / not-yet-backfilled records (api/client.go), and a record that + // never learned where it lives is not a record that lives elsewhere. + resUnknown residency = iota + resHere + resElsewhere +) + +// residencyOf compares the local cluster anchor with a client's, keeping both +// missing-anchor cases at resUnknown. Collapsing either into "elsewhere" would +// print "NOT on the cluster your kubeconfig reaches" — and the repoint hint — +// next to a legacy client that may be running on this very machine. +func residencyOf(hereKnown bool, localAnchor, clientAnchor string) residency { + if !hereKnown || clientAnchor == "" { + return resUnknown + } + if clientAnchor == localAnchor { + return resHere + } + return resElsewhere +} + // clientListMarker renders one row's suffix in `client list`, keeping SELECTION -// (this machine's local pointer) and RESIDENCY (does this client run on the -// cluster the kubeconfig reaches) as two separate facts (#515). -// -// hereKnown false means the cluster anchor could not be read — no kubeconfig, an -// unreachable API server, RBAC on kube-system. That is an absence of evidence, -// so no row may claim to be here and no row may be denied: the marker degrades -// to bare "(active)", which says only what the local config actually knows. -func clientListMarker(isActive, hereKnown, here bool) string { +// (this machine's local pointer) and RESIDENCY (where the client actually runs) +// as two separate facts (#515). Under resUnknown the marker degrades to bare +// "(active)" — which says only what the local config actually knows — and +// claims nothing about location in either direction. +func clientListMarker(isActive bool, res residency) string { switch { - case !hereKnown: + case res == resUnknown: if isActive { return " (active)" } return "" - case isActive && here: + case isActive && res == resHere: return " (active — on this cluster)" case isActive: return " (active — NOT on the cluster your kubeconfig reaches)" - case here: + case res == resHere: return " (on this cluster)" default: return "" diff --git a/internal/cli/client_test.go b/internal/cli/client_test.go index d3698e3..76daefc 100644 --- a/internal/cli/client_test.go +++ b/internal/cli/client_test.go @@ -1548,33 +1548,94 @@ func TestClientCreate_UnhiddenStillPromptsBeforeMinting(t *testing.T) { // cluster anchor (§7.2). func TestClientListMarker(t *testing.T) { cases := []struct { - name string - isActive, known, here bool - want string + name string + isActive bool + res residency + want string }{ - {"anchor unreadable, active → claims only selection", true, false, false, " (active)"}, - {"anchor unreadable, other → no claim", false, false, false, ""}, - {"active and here", true, true, true, " (active — on this cluster)"}, - {"active but elsewhere", true, true, false, " (active — NOT on the cluster your kubeconfig reaches)"}, - {"here but not selected", false, true, true, " (on this cluster)"}, - {"neither", false, true, false, ""}, + {"residency unknown, active → claims only selection", true, resUnknown, " (active)"}, + {"residency unknown, other → no claim", false, resUnknown, ""}, + {"active and here", true, resHere, " (active — on this cluster)"}, + {"active but elsewhere", true, resElsewhere, " (active — NOT on the cluster your kubeconfig reaches)"}, + {"here but not selected", false, resHere, " (on this cluster)"}, + {"elsewhere and not selected", false, resElsewhere, ""}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - if got := clientListMarker(c.isActive, c.known, c.here); got != c.want { - t.Errorf("clientListMarker(%v,%v,%v) = %q, want %q", c.isActive, c.known, c.here, got, c.want) + if got := clientListMarker(c.isActive, c.res); got != c.want { + t.Errorf("clientListMarker(%v,%v) = %q, want %q", c.isActive, c.res, got, c.want) } }) } - // The specific claim #515 calls out: an unreadable anchor must never let a - // row assert it is here. + // The specific claim #515 calls out: unknown residency must never let a row + // assert — or deny — that it is here. for _, isActive := range []bool{true, false} { - if strings.Contains(clientListMarker(isActive, false, false), "this cluster") { - t.Errorf("isActive=%v: an unreadable cluster anchor must claim nothing about location", isActive) + if strings.Contains(clientListMarker(isActive, resUnknown), "this cluster") { + t.Errorf("isActive=%v: unknown residency must claim nothing about location", isActive) } } } +// Bugbot (#515): residency has to stay THREE-valued on both sides of the +// comparison. An unreadable LOCAL anchor was already handled; a client whose OWN +// anchor is empty — legacy / not-yet-backfilled, per api.ProvisionedClient — +// was being forced to "elsewhere", which told the owner of a perfectly local +// legacy client that it is not on this cluster. +func TestResidencyOf(t *testing.T) { + cases := []struct { + name string + hereKnown bool + local, clnt string + want residency + }{ + {"local anchor unreadable", false, "", "uid-A", resUnknown}, + {"client anchor empty (legacy record)", true, "uid-A", "", resUnknown}, + {"both unknown", false, "", "", resUnknown}, + {"anchors match", true, "uid-A", "uid-A", resHere}, + {"anchors differ", true, "uid-A", "uid-B", resElsewhere}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := residencyOf(c.hereKnown, c.local, c.clnt); got != c.want { + t.Errorf("residencyOf(%v,%q,%q) = %v, want %v", c.hereKnown, c.local, c.clnt, got, c.want) + } + }) + } +} + +// End to end: a legacy ACTIVE client with no anchor, on a machine whose cluster +// anchor reads fine, must not be accused of running elsewhere — and must not +// trigger the repoint hint, which would be advice to fix a non-problem. +func TestClientList_LegacyClientWithNoAnchorIsNotAccused(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"first_name":"legacy","namespace":"legacy-ns"}]`)) // no cluster_id + }) + stubClusterID(t, "uid-HERE", nil) + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if strings.Contains(got, "NOT on the cluster") { + t.Errorf("a client with no anchor has UNKNOWN residency, not elsewhere:\n%s", got) + } + if strings.Contains(got, "client create") { + t.Errorf("no mismatch is known, so the repoint must not be advised:\n%s", got) + } + if !strings.Contains(got, "1 (active)") { + t.Errorf("want the bare selection marker:\n%s", got) + } +} + // End-to-end: with the anchor readable, the row that matches the LOCAL cluster // is marked as such — even when the pointer names a different one — and the // listing names the repoint. diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index e381e0d..b8dad97 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -187,10 +187,15 @@ func runClusterDoctor( // The retry is adopted only if it actually finds an environment, so a genuine // no-environment machine keeps the original results (and the --diagnose // bundle keeps describing the namespace the user is configured for). + // pointerStale records that the re-probe SUCCEEDED — i.e. this machine has a + // healthy environment AND its active-client pointer is wrong. Both halves + // have to be said; see the verdict block below for why finding the + // environment is not on its own good news. + pointerStale := false if binding.applied && reachStateOf(results) == doctor.ReachNoEnv { if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride}); ok && ns != resolved.Namespace { if retry := doctorRunFn(ctx, cs, doctor.Options{Namespace: ns, ServerURL: resolved.ServerURL}); reachStateOf(retry) != doctor.ReachNoEnv { - resolved.Namespace, results = ns, retry + resolved.Namespace, results, pointerStale = ns, retry, true } } } @@ -212,6 +217,16 @@ func runClusterDoctor( // 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))) + if pointerStale { + // The environment above is healthy, but nothing else in the CLI will find + // it: `data ingest`, `data list`, `resources` and `seal` all bind the + // active-client pointer and will keep failing with exit 4 until it is + // repointed. Say so here rather than letting the health lines below imply + // the machine is usable. + p.Warnf("Your active client points at namespace %q, which isn't on this cluster — data commands will keep failing until you repoint.", binding.namespace) + p.Hintf(" Point this machine at the environment above: %s client create", launcher()) + p.Hintf(" (this cluster already runs a client, so it adopts it — no new credential)") + } connected, ready = summarizeDoctor(results, tok) p.Newline() @@ -225,6 +240,14 @@ func runClusterDoctor( p.Newline() fail, allGood := doctorVerdict(connected.status, ready.status) switch { + case pointerStale: + // A problem WAS found — it just isn't in the cluster. Exit 2 (the code + // doctor already uses for every actionable finding), never 0 with + // "you're ready to run training": the very next `data ingest` exits 4, + // and a doctor that greens that is reporting success it hasn't earned + // — the class BUGBOT.md flags first. The remedy is already printed + // above, so this doesn't also send them to write a support bundle. + return &exitError{code: exitChecksFailed, err: nil} 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()) diff --git a/internal/cli/doctor_pointer_test.go b/internal/cli/doctor_pointer_test.go index 5a7a1cc..c547fa3 100644 --- a/internal/cli/doctor_pointer_test.go +++ b/internal/cli/doctor_pointer_test.go @@ -3,6 +3,7 @@ package cli import ( "bytes" "context" + "errors" "net/http" "strings" "testing" @@ -76,9 +77,6 @@ func TestDoctor_WrongPointerOnLocalCluster_FindsTheEnvironment(t *testing.T) { var out bytes.Buffer err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) - if err != nil { - t.Fatalf("a healthy local environment must not exit non-zero: %v", err) - } if strings.Contains(out.String(), "No secure environment") { t.Errorf("must not recommend a reinstall over a healthy install:\n%s", out.String()) } @@ -88,6 +86,46 @@ func TestDoctor_WrongPointerOnLocalCluster_FindsTheEnvironment(t *testing.T) { if len(*probed) != 2 || (*probed)[0] != "stale-ns" || (*probed)[1] != "lukas-02" { t.Errorf("probe namespaces = %v, want [stale-ns lukas-02] (bound pointer first, then the kubeconfig's own)", *probed) } + + // Bugbot (#515): finding the environment is only HALF the story. The pointer + // is still stale, so `data ingest`/`resources`/`seal` keep exiting 4 — doctor + // must say so and must not green the machine. + if strings.Contains(out.String(), "Everything looks good") { + t.Errorf("a stale pointer means data commands still fail — this is not 'ready to run training':\n%s", out.String()) + } + if !strings.Contains(out.String(), "stale-ns") { + t.Errorf("doctor must name the stale pointer, not just the environment it found:\n%s", out.String()) + } + if !strings.Contains(out.String(), "client create") { + t.Errorf("doctor must name the repoint:\n%s", out.String()) + } + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("a stale pointer is a problem doctor found → want exit 2, got %v", err) + } +} + +// The other side of the same coin: with NO stale pointer, a healthy machine +// still gets its green line and exit 0. Without this, the assertion above could +// be satisfied by doctor never saying "Everything looks good" at all. +func TestDoctor_HealthyPointer_StillGreen(t *testing.T) { + writeActiveClientConfig(t, "lukas-02", "Lukas") // pointer matches reality + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://127.0.0.1:6550", "lukas-02", "lukas-02") + + var out bytes.Buffer + if err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false); err != nil { + t.Fatalf("a healthy machine with a correct pointer must exit 0, got %v", err) + } + if !strings.Contains(out.String(), "Everything looks good") { + t.Errorf("want the green verdict when nothing is stale:\n%s", out.String()) + } + if strings.Contains(out.String(), "client create") { + t.Errorf("no repoint advice when the pointer is correct:\n%s", out.String()) + } + if len(*probed) != 1 { + t.Errorf("probe namespaces = %v, want one (nothing to re-probe)", *probed) + } } // The ownership gate is what keeps the fallback honest, so it gets its own test: diff --git a/internal/cli/home.go b/internal/cli/home.go index 7860ef0..e62c3cf 100644 --- a/internal/cli/home.go +++ b/internal/cli/home.go @@ -30,7 +30,6 @@ package cli import ( "context" - "errors" "fmt" "math" "os" @@ -155,6 +154,13 @@ type envProbe struct { name string compute computeInfo hasCompute bool + // pointerStale: the environment above was found by the #515 local fallback + // AFTER the active-client pointer missed — so the release we can see running + // here is NOT the client the pointer names. The heartbeat is looked up by + // that pointer's client id, so it describes a different machine's client and + // must never be allowed to green this one (Bugbot: local liveness from one + // client + a heartbeat from another is an Online nobody earned). + pointerStale bool } // homeDeps are the detection seams. defaultHomeDeps wires the real @@ -291,11 +297,16 @@ func resolveHomeModel(ctx context.Context, d homeDeps) homeModel { // honest "· running" state, never a green Online — but the model records // WHICH kind of not-Online it is, so the running line can word a // backend-confirmed "not online" differently from a mere couldn't-confirm. - if beat == beatOnline { + // #515: with a stale pointer the heartbeat is about a DIFFERENT client + // than the release running here, so it carries no signal about this one + // in either direction — it can neither green it (a colleague's machine + // being online is not this one being online) nor red it. Drop to the + // honest "running, couldn't confirm" line, which is exactly true. + if beat == beatOnline && !env.pointerStale { m.state = homeOnline } else { m.state = homeRunning - m.confirmedNotOnline = beat == beatNotOnline + m.confirmedNotOnline = beat == beatNotOnline && !env.pointerStale } m.fullMenu = true case localDegraded: @@ -463,86 +474,6 @@ func realRememberedClient() (provisioned bool, name string) { return p.ActiveClientNamespace != "", name } -// realProbeEnv is the bounded cluster probe. It reuses the exact namespace -// binding + discovery the data/cluster commands use, so the home screen reports -// the very environment those commands would target. Best-effort throughout: any -// failure degrades to unreachable/no-release, never an error. -func realProbeEnv(ctx context.Context) envProbe { - ctx, cancel := context.WithTimeout(ctx, homeProbeTimeout) - defer cancel() - - // The name for a discovered release is set below; the unreachable / no-release - // returns leave it empty and let resolveHomeModel fill the remembered name, so - // the "provisioned ⇒ named offline" fallback lives in exactly one place. - opts := cluster.KubeconfigOptions{} - binding := bindActiveClientNamespace(&opts) - // OWNERSHIP GATE: no active-client binding ⇒ nothing was ever provisioned - // for this profile, so no release the kubeconfig can reach is honestly - // YOURS. Without the binding, discovery would fall back to the kubeconfig's - // default namespace and then the cluster-wide scan — either can surface an - // UNRELATED client (a shared cluster, a colleague's install), which this - // screen would then greet as "your secure environment". The data commands - // run that scan behind a visible retarget note and an explicit user action; - // a status screen has neither, and §7.5's rule (a miss must never silently - // retarget to some other client) applies doubly here. Report no-release — - // resolveHomeModel renders the honest no-env screen (or a named offline via - // the remembered-name fallback) — and skip the cluster I/O entirely, which - // also keeps the common unprovisioned re-entry instant. - if !binding.applied { - // #401: an empty pointer isn't proof of "no environment" — the Windows - // installer never writes it. localEnvFallback adopts a release only on - // a LOCAL (loopback/k3d) cluster, so the shared-cluster guarantee above - // is preserved; everything else still reads as no-release. - return localEnvFallback(ctx) - } - resolved, err := loadClusterFn(opts) - if err != nil { - return envProbe{local: localUnreachable} - } - // Bound every API call so an unreachable API server can't hang the home - // screen (mirrors cluster.ClusterID's time-boxed best-effort read). - resolved.RestConfig.Timeout = homeProbeTimeout - cs, err := newClientsetFn(resolved) - if err != nil { - return envProbe{local: localUnreachable} - } - - release, nsUsed, err := discoverRelease(ctx, nil, cs, resolved.Namespace, binding.allowScan(), false) - if err != nil { - if errors.Is(err, cluster.ErrNoParentRelease) { - // Cluster reachable, but this release isn't in the resolved context. - // #515: a WRONG pointer is no more proof of "no environment" than the - // empty one #401 covered — the binding above overrode the kubeconfig's - // own namespace with a stale/foreign one, so this miss says nothing - // about what runs here. Re-ask through the same local-only fallback: - // it adopts a release ONLY when the kubeconfig's server is this - // machine, so the shared-cluster guarantee is untouched, and every - // other outcome is localNoRelease — exactly what this branch returned - // before. Provisioned ⇒ resolveHomeModel turns that into a named - // "offline". - return localEnvFallback(ctx) - } - // A list/RBAC/connect failure: we couldn't confirm what's here. Treat it - // as unreachable (→ offline if provisioned, else no-env). - return envProbe{local: localUnreachable} - } - - ep := envProbe{name: release.ReleaseName} - if jobsManagerReady(ctx, cs, nsUsed, release) { - ep.local = localLive - } else { - ep.local = localDegraded - } - // Compute is only surfaced on the Online line, and only worth reading when the - // environment is actually up. - if ep.local == localLive { - if c, ok := machineCapacity(ctx, cs); ok { - ep.compute, ep.hasCompute = c, true - } - } - return ep -} - // realHeartbeat reports tracebloc's view of this machine's client — the honest // "is it heartbeating" signal. // diff --git a/internal/cli/home_local_fallback.go b/internal/cli/home_local_fallback.go index 916bbd6..abbd580 100644 --- a/internal/cli/home_local_fallback.go +++ b/internal/cli/home_local_fallback.go @@ -1,10 +1,15 @@ package cli -// Home-screen fallbacks for machines the provisioning pointer never reached -// (#401). Split from home.go to respect its file budget. +// The home screen's environment probe, and the fallbacks it leans on when the +// provisioning pointer can't be trusted — never reached this machine at all +// (#401) or names a namespace that isn't on the reached cluster (#515). Split +// from home.go to respect its file budget; realProbeEnv moved here under #515 +// because it is now mostly a decision about WHICH fallback to take, and reads +// better next to them than next to the renderer. import ( "context" + "errors" "net" "net/url" "os" @@ -154,3 +159,88 @@ func tbCmdAliasOurs(dir, exe string) bool { } return strings.Contains(strings.ToLower(string(b)), strings.ToLower(filepath.Clean(exe))) } + +// realProbeEnv is the bounded cluster probe. It reuses the exact namespace +// binding + discovery the data/cluster commands use, so the home screen reports +// the very environment those commands would target. Best-effort throughout: any +// failure degrades to unreachable/no-release, never an error. +func realProbeEnv(ctx context.Context) envProbe { + ctx, cancel := context.WithTimeout(ctx, homeProbeTimeout) + defer cancel() + + // The name for a discovered release is set below; the unreachable / no-release + // returns leave it empty and let resolveHomeModel fill the remembered name, so + // the "provisioned ⇒ named offline" fallback lives in exactly one place. + opts := cluster.KubeconfigOptions{} + binding := bindActiveClientNamespace(&opts) + // OWNERSHIP GATE: no active-client binding ⇒ nothing was ever provisioned + // for this profile, so no release the kubeconfig can reach is honestly + // YOURS. Without the binding, discovery would fall back to the kubeconfig's + // default namespace and then the cluster-wide scan — either can surface an + // UNRELATED client (a shared cluster, a colleague's install), which this + // screen would then greet as "your secure environment". The data commands + // run that scan behind a visible retarget note and an explicit user action; + // a status screen has neither, and §7.5's rule (a miss must never silently + // retarget to some other client) applies doubly here. Report no-release — + // resolveHomeModel renders the honest no-env screen (or a named offline via + // the remembered-name fallback) — and skip the cluster I/O entirely, which + // also keeps the common unprovisioned re-entry instant. + if !binding.applied { + // #401: an empty pointer isn't proof of "no environment" — the Windows + // installer never writes it. localEnvFallback adopts a release only on + // a LOCAL (loopback/k3d) cluster, so the shared-cluster guarantee above + // is preserved; everything else still reads as no-release. + return localEnvFallback(ctx) + } + resolved, err := loadClusterFn(opts) + if err != nil { + return envProbe{local: localUnreachable} + } + // Bound every API call so an unreachable API server can't hang the home + // screen (mirrors cluster.ClusterID's time-boxed best-effort read). + resolved.RestConfig.Timeout = homeProbeTimeout + cs, err := newClientsetFn(resolved) + if err != nil { + return envProbe{local: localUnreachable} + } + + release, nsUsed, err := discoverRelease(ctx, nil, cs, resolved.Namespace, binding.allowScan(), false) + if err != nil { + if errors.Is(err, cluster.ErrNoParentRelease) { + // Cluster reachable, but this release isn't in the resolved context. + // #515: a WRONG pointer is no more proof of "no environment" than the + // empty one #401 covered — the binding above overrode the kubeconfig's + // own namespace with a stale/foreign one, so this miss says nothing + // about what runs here. Re-ask through the same local-only fallback: + // it adopts a release ONLY when the kubeconfig's server is this + // machine, so the shared-cluster guarantee is untouched, and every + // other outcome is localNoRelease — exactly what this branch returned + // before. Provisioned ⇒ resolveHomeModel turns that into a named + // "offline". + ep := localEnvFallback(ctx) + // Mark it: the release the fallback found is not the client the + // pointer names, so the heartbeat keyed on that pointer describes + // someone else. resolveHomeModel refuses to render Online off this. + ep.pointerStale = ep.local != localNoRelease + return ep + } + // A list/RBAC/connect failure: we couldn't confirm what's here. Treat it + // as unreachable (→ offline if provisioned, else no-env). + return envProbe{local: localUnreachable} + } + + ep := envProbe{name: release.ReleaseName} + if jobsManagerReady(ctx, cs, nsUsed, release) { + ep.local = localLive + } else { + ep.local = localDegraded + } + // Compute is only surfaced on the Online line, and only worth reading when the + // environment is actually up. + if ep.local == localLive { + if c, ok := machineCapacity(ctx, cs); ok { + ep.compute, ep.hasCompute = c, true + } + } + return ep +} diff --git a/internal/cli/home_local_fallback_test.go b/internal/cli/home_local_fallback_test.go index 01a80ab..f3256d3 100644 --- a/internal/cli/home_local_fallback_test.go +++ b/internal/cli/home_local_fallback_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "testing" + "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -122,6 +123,65 @@ func TestRealProbeEnv_WrongPointerOnLocalCluster_FallsBack(t *testing.T) { if ep.local != localLive || ep.name != "tracebloc" { t.Fatalf("=> %+v, want the live local environment despite the stale pointer", ep) } + // Bugbot (#515): the release we found is NOT the client the pointer names, so + // the heartbeat (looked up by that pointer's id) is about a different machine + // and must be barred from greening this one. + if !ep.pointerStale { + t.Error("a fallback that fired after a pointer MISS must mark the pointer stale") + } +} + +// The stale mark must actually change the verdict: local liveness from one +// client plus a beatOnline from another is an Online nobody earned. +func TestResolveHomeModel_StalePointerNeverRendersOnline(t *testing.T) { + base := func(stale bool) homeModel { + return resolveHomeModel(context.Background(), homeDeps{ + budget: 2 * time.Second, + invoked: func() string { return binTB }, + tbAvailable: func() bool { return true }, + hasResources: func() bool { return true }, + signIn: func() (bool, string, string) { return true, "a@b.io", "Lukas" }, + rememberedClient: func() (bool, string) { return true, "stale-01" }, + probeBeat: func(context.Context) heartbeatState { return beatOnline }, + probeEnv: func(context.Context) envProbe { + return envProbe{local: localLive, name: "tracebloc", pointerStale: stale} + }, + }) + } + + if m := base(false); m.state != homeOnline { + t.Fatalf("control: live + beatOnline + fresh pointer must be Online, got %v", m.state) + } + m := base(true) + if m.state == homeOnline { + t.Error("a stale pointer must never render Online — the heartbeat is another client's") + } + if m.state != homeRunning { + t.Errorf("want the honest running state, got %v", m.state) + } + if m.confirmedNotOnline { + t.Error("nor may another client's heartbeat be reported as THIS one being not-online") + } +} + +// …and a beatNotOnline off a stale pointer is equally uninformative: it must not +// harden into "backend reports not online" for a client it isn't about. +func TestResolveHomeModel_StalePointerNotOnlineIsNotConfirmed(t *testing.T) { + m := resolveHomeModel(context.Background(), homeDeps{ + budget: 2 * time.Second, + invoked: func() string { return binTB }, + tbAvailable: func() bool { return true }, + hasResources: func() bool { return true }, + signIn: func() (bool, string, string) { return true, "a@b.io", "Lukas" }, + rememberedClient: func() (bool, string) { return true, "stale-01" }, + probeBeat: func(context.Context) heartbeatState { return beatNotOnline }, + probeEnv: func(context.Context) envProbe { + return envProbe{local: localLive, name: "tracebloc", pointerStale: true} + }, + }) + if m.confirmedNotOnline { + t.Error("a stale pointer's heartbeat carries no signal in either direction") + } } // …and the ownership gate survives it: the same wrong pointer on a REMOTE diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 8e2b022..7f574e8 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -72,6 +72,7 @@ screen. %s/%d are runtime placeholders. "(remote tar stderr: %s)" "(scheduling: %s — %s)" "(table: %s)" +"(this cluster already runs a client, so it adopts it — no new credential)" ", %s=%s" "- %s, age %s%s" "-%02d" @@ -238,6 +239,7 @@ screen. %s/%d are runtime placeholders. "Pending > %s: %v" "Pick this dataset when you set it up." "Please name the dataset." +"Point this machine at the environment above: %s client create" "Preparing this host and granting %s container-runtime access — re-running the installer's prepare-host step (needs administrator rights once)." "Preparing this host — re-running the installer's prepare-host step (installs the container runtime and prerequisites; needs administrator rights once). Pass a researcher's username to also grant them access: tracebloc prepare-host " "Private image pulls will ImagePullBackOff. Reinstall the chart with valid registry credentials." @@ -349,6 +351,7 @@ screen. %s/%d are runtime placeholders. "You don't have permission to %s in this account." "Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule." "Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create" +"Your active client points at namespace %q, which isn't on this cluster — data commands will keep failing until you repoint." "Your data is registered as a dataset. View it at https://ai.tracebloc.io/metadata" "Your dataset records (marked unavailable, not deleted)" "Your files are copied securely into your secure environment's storage — set up and cleaned up for you." From 43a4b0729413fca5d210fb85264e4b18c13280a6 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 12:48:52 +0200 Subject: [PATCH 08/11] fix(doctor): put the stale-pointer finding IN the readiness line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rendering the previous commit showed the fix was half done: the closing verdict was honest, but the line above it still read ✔ Connected to tracebloc ✔ Ready to run training ⚠ Your active client points at namespace "stale-ns" … — a green tick and, directly beneath it, a warning saying the opposite. That is the same unearned success the finding was about, moved up the screen. `Ready to run training` is false whenever the pointer is stale, however green the cluster checks are, because every data command binds the pointer. So the readiness healthLine is replaced rather than accompanied, and it carries the remedy, so the finding and the fix read as one thing: ✔ Connected to tracebloc ✖ Not ready — your active client points at namespace "stale-ns", which isn't on this cluster, so data commands will keep failing until you repoint. Point this machine at the environment above: tracebloc client create (this cluster already runs a client, so it adopts it — no new credential) Phrased "Not ready — …" to match the three readiness failures already in the catalog. `--diagnose` records the replaced line, which is what triage needs. Exit stays 2 via the pointerStale branch, which skips the "email support" nudge a doctorVerdict fail would add — we just gave a precise one-command fix. Three more mutations: disable the replacement (green tick returns) → red; drop the remedy → red; stop naming the stale namespace → red. The already-added TestDoctor_HealthyPointer_StillGreen now also asserts the green tick IS present when nothing is stale, so "no green tick" can't pass by the line disappearing entirely. Refs cli#515 Co-Authored-By: Claude Opus 5 --- internal/cli/doctor.go | 25 ++++++++++++------- internal/cli/doctor_pointer_test.go | 12 +++++++++ .../cli/testdata/golden/zz-all-strings.golden | 5 ++-- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index b8dad97..ff51f18 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -217,17 +217,24 @@ func runClusterDoctor( // 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) if pointerStale { - // The environment above is healthy, but nothing else in the CLI will find - // it: `data ingest`, `data list`, `resources` and `seal` all bind the - // active-client pointer and will keep failing with exit 4 until it is - // repointed. Say so here rather than letting the health lines below imply - // the machine is usable. - p.Warnf("Your active client points at namespace %q, which isn't on this cluster — data commands will keep failing until you repoint.", binding.namespace) - p.Hintf(" Point this machine at the environment above: %s client create", launcher()) - p.Hintf(" (this cluster already runs a client, so it adopts it — no new credential)") + // The environment above is healthy — but `data ingest`, `data list`, + // `resources` and `seal` all bind the active-client POINTER, and that + // still misses, so they keep failing with exit 4. "Ready to run training" + // is therefore false no matter how green the cluster checks are. Replace + // the readiness line rather than printing a green tick with a warning + // beside it that contradicts it — and the replacement carries the remedy, + // so the finding and the fix read as one thing. The support bundle + // records this line too, which is what triage needs to see. + ready = healthLine{ + status: doctor.StatusFail, + text: fmt.Sprintf("Not ready — your active client points at namespace %q, which isn't on this cluster, so data commands will keep failing until you repoint.", + binding.namespace), + remedy: fmt.Sprintf("Point this machine at the environment above: %s client create (this cluster already runs a client, so it adopts it — no new credential)", + launcher()), + } } - connected, ready = summarizeDoctor(results, tok) p.Newline() renderHealth(p, connected) diff --git a/internal/cli/doctor_pointer_test.go b/internal/cli/doctor_pointer_test.go index c547fa3..174afaa 100644 --- a/internal/cli/doctor_pointer_test.go +++ b/internal/cli/doctor_pointer_test.go @@ -93,6 +93,15 @@ func TestDoctor_WrongPointerOnLocalCluster_FindsTheEnvironment(t *testing.T) { if strings.Contains(out.String(), "Everything looks good") { t.Errorf("a stale pointer means data commands still fail — this is not 'ready to run training':\n%s", out.String()) } + // …and the readiness LINE must carry it too: a green "✔ Ready to run + // training" beside a warning that contradicts it is the same unearned + // success, just moved up the screen. + if strings.Contains(out.String(), "✔ Ready to run training") { + t.Errorf("the readiness line must not tick green while the pointer is stale:\n%s", out.String()) + } + if !strings.Contains(out.String(), "Not ready") { + t.Errorf("want the readiness line to carry the finding:\n%s", out.String()) + } if !strings.Contains(out.String(), "stale-ns") { t.Errorf("doctor must name the stale pointer, not just the environment it found:\n%s", out.String()) } @@ -120,6 +129,9 @@ func TestDoctor_HealthyPointer_StillGreen(t *testing.T) { if !strings.Contains(out.String(), "Everything looks good") { t.Errorf("want the green verdict when nothing is stale:\n%s", out.String()) } + if !strings.Contains(out.String(), "✔ Ready to run training") { + t.Errorf("control: the readiness line must still tick green when nothing is stale:\n%s", out.String()) + } if strings.Contains(out.String(), "client create") { t.Errorf("no repoint advice when the pointer is correct:\n%s", out.String()) } diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 7f574e8..0f85545 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -72,7 +72,6 @@ screen. %s/%d are runtime placeholders. "(remote tar stderr: %s)" "(scheduling: %s — %s)" "(table: %s)" -"(this cluster already runs a client, so it adopts it — no new credential)" ", %s=%s" "- %s, age %s%s" "-%02d" @@ -226,6 +225,7 @@ screen. %s/%d are runtime placeholders. "Not ready — part of your secure environment can't start yet." "Not ready — part of your secure environment isn't running." "Not ready — the training images can't be pulled." +"Not ready — your active client points at namespace %q, which isn't on this cluster, so data commands will keep failing until you repoint." "Not signed in yet." "Not signed in — run `%s login`." "Not signed in. Run `tracebloc login`." @@ -239,7 +239,7 @@ screen. %s/%d are runtime placeholders. "Pending > %s: %v" "Pick this dataset when you set it up." "Please name the dataset." -"Point this machine at the environment above: %s client create" +"Point this machine at the environment above: %s client create (this cluster already runs a client, so it adopts it — no new credential)" "Preparing this host and granting %s container-runtime access — re-running the installer's prepare-host step (needs administrator rights once)." "Preparing this host — re-running the installer's prepare-host step (installs the container runtime and prerequisites; needs administrator rights once). Pass a researcher's username to also grant them access: tracebloc prepare-host " "Private image pulls will ImagePullBackOff. Reinstall the chart with valid registry credentials." @@ -351,7 +351,6 @@ screen. %s/%d are runtime placeholders. "You don't have permission to %s in this account." "Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule." "Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create" -"Your active client points at namespace %q, which isn't on this cluster — data commands will keep failing until you repoint." "Your data is registered as a dataset. View it at https://ai.tracebloc.io/metadata" "Your dataset records (marked unavailable, not deleted)" "Your files are copied securely into your secure environment's storage — set up and cleaned up for you." From 60c6d54970c8d0f961217c0c7ce565d923a475c4 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 12:55:53 +0200 Subject: [PATCH 09/11] fix(client,home): don't name a target we haven't confirmed exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot round 2, two Mediums, and the same class a third time: a sentence asserting something the code never established. `client list` set the mismatch hint from the ACTIVE row alone, so with the pointer elsewhere and nothing confirmed on this cluster it still said "point this machine at the client that IS there: … client create". There may be no client there — and on a cluster with none, `client create` falls through to the MINT path and produces the orphaned phantom backend#970 exists to prevent. So this PR's own advice could manufacture the bug the command was hidden for. The repoint is now offered only when some row is resHere — that is what earns the phrase "the client that IS there". Otherwise the mismatch is still reported, without a target: "no client here is confirmed. Check your kubeconfig context, then run: … doctor". Deliberately covers BOTH remaining cases, because they are equally unnameable — no client here at all, and clients that might be here but carry no anchor to prove it (resUnknown). The home screen had the label version of the same thing: with a stale pointer, `env.name` was still overridden by the remembered handle, so the client the pointer names was printed as the environment running here — a client that is by construction NOT what the fallback found, and a name contradicting doctor's for the identical state. The override is now skipped when the pointer is stale, so the screen falls through to the probe's own name for the release that is actually running. Three mutations: let the repoint hint fire without anyHere → red; source anyHere from the active row instead of residency → red; restore the unconditional remembered-name override → red. Both fixes carry a control assertion in the same test (the repoint IS offered when a row is here; the remembered name IS still preferred when the pointer is fresh), so neither can pass by the behaviour disappearing altogether. N10's first attempt left `anyHere` unused and reddened the compiler; rewritten to keep it used and re-run. Refs cli#515 Co-Authored-By: Claude Opus 5 --- internal/cli/client.go | 22 +++++++++-- internal/cli/client_test.go | 37 +++++++++++++++++++ internal/cli/home.go | 9 ++++- internal/cli/home_local_fallback_test.go | 16 ++++++++ .../cli/testdata/golden/zz-all-strings.golden | 1 + 5 files changed, 80 insertions(+), 5 deletions(-) diff --git a/internal/cli/client.go b/internal/cli/client.go index 832fc13..d7246ee 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -774,12 +774,15 @@ func runClientList(ctx context.Context, p *ui.Printer) error { // claims nothing about where anything runs. clusterID, cidErr := readClusterID(ctx, cluster.KubeconfigOptions{}) hereKnown := cidErr == nil && clusterID != "" - mismatch := false + activeElsewhere, anyHere := false, false for _, c := range clients { isActive := strconv.Itoa(c.ID) == active res := residencyOf(hereKnown, clusterID, c.ClusterID) if isActive && res == resElsewhere { - mismatch = true + activeElsewhere = true + } + if res == resHere { + anyHere = true } p.Field(strconv.Itoa(c.ID)+clientListMarker(isActive, res), fmt.Sprintf("%s state=%s namespace=%s location=%s", @@ -788,11 +791,22 @@ func runClientList(ctx context.Context, p *ui.Printer) error { // §7.3: separate "selected" (this machine's local pointer) from "connected" // (the backend's last-heartbeat state) so a stale pointer is visible. p.Hintf("\"active\" is this machine's selected client; state is its last reported status to tracebloc.") - if mismatch { + switch { + case activeElsewhere && anyHere: // The exact state #515 describes, and the one supported way out of it: // re-running create on a cluster that already hosts a client adopts it — - // no prompt, no new credential (§7.2). + // no prompt, no new credential (§7.2). anyHere is what earns the phrase + // "the client that IS there": without a row we KNOW is on this cluster, + // `client create` would fall through to the mint path and produce the + // phantom backend#970 is about (Bugbot). p.Hintf("Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create", launcher()) + case activeElsewhere: + // The pointer is provably wrong, but no listed client is provably here — + // either none is, or the ones that might be carry no anchor to prove it + // (resUnknown). Both are "we can't name a target", so name none: send + // them to the command whose whole job is to say what's on this cluster + // rather than advertise a repoint that may have nothing to adopt. + p.Hintf("Your active client is not on the cluster your kubeconfig reaches, and no client here is confirmed. Check your kubeconfig context, then run: %s doctor", launcher()) } return nil } diff --git a/internal/cli/client_test.go b/internal/cli/client_test.go index 76daefc..d771124 100644 --- a/internal/cli/client_test.go +++ b/internal/cli/client_test.go @@ -1670,6 +1670,43 @@ func TestClientList_MarksTheClientOnThisCluster(t *testing.T) { } } +// Bugbot (#515): the repoint hint says "the client that IS there", which is only +// true if some row is provably here. With the active client elsewhere and NOTHING +// confirmed on this cluster, `client create` would fall through to the MINT path +// and produce exactly the phantom backend#970 exists to prevent — so the advice +// must not be given. +func TestClientList_MismatchWithNoLocalClient_DoesNotPushCreate(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"first_name":"stale","namespace":"stale-ns","cluster_id":"uid-OTHER"},` + + `{"id":2,"first_name":"third","namespace":"third-ns","cluster_id":"uid-THIRD"}]`)) + }) + stubClusterID(t, "uid-HERE", nil) // this cluster hosts NEITHER + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if strings.Contains(got, "client create") { + t.Errorf("no client is confirmed here — advising the repoint would push a MINT:\n%s", got) + } + if !strings.Contains(got, "no client here is confirmed") { + t.Errorf("the mismatch is still real and must be reported, just without a target:\n%s", got) + } + // The mismatch itself must still be visible on the row. + if !strings.Contains(got, "NOT on the cluster your kubeconfig reaches") { + t.Errorf("the stale active pointer must still be marked:\n%s", got) + } +} + // The unreadable-anchor path end to end: no cluster reachable ⇒ no row claims a // location, and the mismatch hint stays silent (we cannot know there is one). func TestClientList_UnreadableAnchorClaimsNoLocation(t *testing.T) { diff --git a/internal/cli/home.go b/internal/cli/home.go index e62c3cf..7855db3 100644 --- a/internal/cli/home.go +++ b/internal/cli/home.go @@ -276,7 +276,14 @@ func resolveHomeModel(ctx context.Context, d homeDeps) homeModel { // a release present on a machine that never cached a client. This also keeps // the "provisioned ⇒ named offline" fallback (a degraded probe returns no // name) living in exactly one place. - if remembered != "" { + // + // EXCEPT when the pointer is stale (#515): the remembered name is the handle + // of the client the pointer names, and that client is NOT what the fallback + // found running here. Applying it would print another machine's handle as the + // environment on this one — a wrong label, and one that contradicts doctor's + // namespace-based name for the very same state (Bugbot). Fall through to the + // probe's own name, which at least describes what is actually running. + if remembered != "" && !env.pointerStale { env.name = remembered } diff --git a/internal/cli/home_local_fallback_test.go b/internal/cli/home_local_fallback_test.go index f3256d3..a194067 100644 --- a/internal/cli/home_local_fallback_test.go +++ b/internal/cli/home_local_fallback_test.go @@ -162,6 +162,22 @@ func TestResolveHomeModel_StalePointerNeverRendersOnline(t *testing.T) { if m.confirmedNotOnline { t.Error("nor may another client's heartbeat be reported as THIS one being not-online") } + // Bugbot (#515): the LABEL is a claim too. "stale-01" is the handle of the + // client the pointer names — not what the fallback found running here — so + // presenting it as this machine's environment is a wrong name, and one that + // contradicts doctor's namespace-based label for the same state. + if m.envName == "stale-01" { + t.Errorf("a stale pointer's client handle must not label the environment running here, got %q", m.envName) + } + if m.envName != "tracebloc" { + t.Errorf("want the probe's own name for the release that IS here, got %q", m.envName) + } + + // Control: with a fresh pointer the remembered handle is still preferred — + // otherwise "don't use the remembered name" could pass by never using it. + if fresh := base(false); fresh.envName != "stale-01" { + t.Errorf("a non-stale pointer must still prefer the remembered client name, got %q", fresh.envName) + } } // …and a beatNotOnline off a stale pointer is equally uninformative: it must not diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 0f85545..dbb06f0 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -350,6 +350,7 @@ screen. %s/%d are runtime placeholders. "Wrote client id + namespace to %s (no new credential — the existing one stands)." "You don't have permission to %s in this account." "Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule." +"Your active client is not on the cluster your kubeconfig reaches, and no client here is confirmed. Check your kubeconfig context, then run: %s doctor" "Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create" "Your data is registered as a dataset. View it at https://ai.tracebloc.io/metadata" "Your dataset records (marked unavailable, not deleted)" From c37086adaee11b404bca713b690e4767a4bf9684 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 13:02:35 +0200 Subject: [PATCH 10/11] fix(doctor): adopt the re-probe only on a CONFIRMED reachable cluster MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot round 3, High, and correct. The re-probe adopted on `reachStateOf(retry) != doctor.ReachNoEnv`, and ReachState has four members: ReachUnreachable and ReachError also satisfy that. Both mean "we could not tell". So a stale pointer plus RBAC on the context namespace, or a transient read failure, would have: • named an unconfirmed namespace as `Secure environment "…"`, • set pointerStale and printed "this cluster already runs a client, so it adopts it — no new credential", • and sent the user to `client create` on a cluster that may host nothing, where it does not adopt but MINTS — the backend#970 phantom. Which is the exact absence-as-presence collapse surveyCluster's `looked` and residencyOf's resUnknown exist to prevent, made twice more in the same PR. Adoption now requires a positive confirmation, via reachConfirmedOK(). It is deliberately NOT `reachStateOf(results) == ReachOK`: reachStateOf defaults to ReachOK when the check is ABSENT, which is the right lenient default on the main path and precisely the wrong one here, where the whole question is whether an unproven namespace may be believed. Absent, unreachable and errored all answer "could not tell", and none may authorize naming an environment or advising a repoint. The test derives its input domain from doctor.ReachState's declared surface — every non-OK member, plus the absent case — rather than picking the states that came to mind: mutation coverage cannot see a vocabulary gap, so a future member has to be added to the enum's own list to escape it. Three mutations: restore `!= ReachNoEnv` → three subtests red; make an absent check count as confirmed → red; let ReachError confirm → red. Refs cli#515 Co-Authored-By: Claude Opus 5 --- internal/cli/doctor.go | 31 ++++++++++- internal/cli/doctor_pointer_test.go | 86 +++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index ff51f18..cd3341a 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -194,7 +194,16 @@ func runClusterDoctor( pointerStale := false if binding.applied && reachStateOf(results) == doctor.ReachNoEnv { if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride}); ok && ns != resolved.Namespace { - if retry := doctorRunFn(ctx, cs, doctor.Options{Namespace: ns, ServerURL: resolved.ServerURL}); reachStateOf(retry) != doctor.ReachNoEnv { + // Adopt the re-probe ONLY on a positive confirmation that a client is + // there. `!= ReachNoEnv` was wrong (Bugbot): ReachUnreachable and + // ReachError also satisfy it, and both mean "we could not tell" — so a + // stale pointer plus RBAC or a transient read on the context namespace + // would have named an unconfirmed namespace as a secure environment and + // told the user this cluster already runs a client, pushing `client + // create` into a MINT. Same absence-as-presence collapse surveyCluster + // and residencyOf exist to avoid; an unconfirmed re-probe keeps the + // original results and the honest no-environment path. + if retry := doctorRunFn(ctx, cs, doctor.Options{Namespace: ns, ServerURL: resolved.ServerURL}); reachConfirmedOK(retry) { resolved.Namespace, results, pointerStale = ns, retry, true } } @@ -580,6 +589,26 @@ func reachStateOf(results []doctor.Result) doctor.ReachState { return doctor.ReachOK } +// reachConfirmedOK reports whether the "Cluster reachable" check RAN and came +// back ReachOK — a positive confirmation that a tracebloc client is in the probed +// namespace. +// +// Deliberately not `reachStateOf(results) == ReachOK`: reachStateOf defaults to +// ReachOK when the check is ABSENT, which is the right lenient default for the +// main path (an older probe set shouldn't block a verdict) and precisely the +// wrong one for #515's re-probe, where the whole question is whether we may +// believe an unproven namespace. Absent, unreachable and errored all answer +// "we could not tell", and none of them may authorize naming a secure +// environment or advising `client create`. +func reachConfirmedOK(results []doctor.Result) bool { + for _, r := range results { + if r.Name == "Cluster reachable" { + return r.Reach == doctor.ReachOK + } + } + return false +} + // 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_pointer_test.go b/internal/cli/doctor_pointer_test.go index 174afaa..d7a03b3 100644 --- a/internal/cli/doctor_pointer_test.go +++ b/internal/cli/doctor_pointer_test.go @@ -204,6 +204,92 @@ func TestDoctor_LocalClusterWithNothing_KeepsInstallerAdvice(t *testing.T) { } } +// Bugbot (#515): the re-probe used to adopt on anything that wasn't ReachNoEnv, +// which swept in ReachUnreachable and ReachError — both of which mean "we could +// not tell". Adopting either would name an unconfirmed namespace as a secure +// environment and tell the user this cluster already runs a client, pushing +// `client create` into a MINT on a cluster that may host nothing. +// +// The input domain is derived from doctor.ReachState's declared surface rather +// than hand-picked, plus the ABSENT case (reachStateOf's lenient default is +// exactly what must not apply here). Only ReachOK may adopt. +func TestDoctor_ReProbeAdoptsOnlyOnConfirmedReach(t *testing.T) { + // Every non-OK member of the enum, and the missing-check case. + cases := []struct { + name string + results []doctor.Result + }{ + {"unreachable", []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachUnreachable}}}, + {"error (RBAC/NotFound)", []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachError}}}, + {"no env", []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv}}}, + {"check absent entirely", []doctor.Result{{Name: "Pod health", Status: doctor.StatusOK}}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + + origLoad, origCS, origRun := loadClusterFn, newClientsetFn, doctorRunFn + t.Cleanup(func() { loadClusterFn, newClientsetFn, doctorRunFn = origLoad, origCS, origRun }) + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + ns := o.Namespace + if ns == "" { + ns = "unproven-ns" + } + return &cluster.ResolvedConfig{Namespace: ns, ServerURL: "https://127.0.0.1:6550", RestConfig: &rest.Config{}}, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { + return fake.NewSimpleClientset(), nil + } + doctorRunFn = func(_ context.Context, _ kubernetes.Interface, o doctor.Options) []doctor.Result { + if o.Namespace == "stale-ns" { // the bound pointer always misses + return []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv}} + } + return c.results // the re-probe's inconclusive answer + } + + var out bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) + got := out.String() + if strings.Contains(got, "unproven-ns") { + t.Errorf("an unconfirmed namespace must never be named as a secure environment:\n%s", got) + } + if strings.Contains(got, "client create") { + t.Errorf("advising the repoint here can push a MINT — the cluster was never confirmed to run a client:\n%s", got) + } + if !strings.Contains(got, "No secure environment") { + t.Errorf("an unconfirmed re-probe must keep the honest no-environment path:\n%s", got) + } + if err == nil { + t.Error("want a non-zero exit when nothing was confirmed") + } + }) + } +} + +// reachConfirmedOK is the guard above, tested directly against the whole +// declared enum so a future ReachState member can't quietly slip through the +// "could not tell" side. Mutation coverage cannot see a vocabulary gap. +func TestReachConfirmedOK(t *testing.T) { + res := func(r doctor.ReachState) []doctor.Result { + return []doctor.Result{{Name: "Cluster reachable", Reach: r}} + } + if !reachConfirmedOK(res(doctor.ReachOK)) { + t.Error("ReachOK is the one positive confirmation") + } + for _, r := range []doctor.ReachState{doctor.ReachUnreachable, doctor.ReachNoEnv, doctor.ReachError} { + if reachConfirmedOK(res(r)) { + t.Errorf("Reach %v must not count as confirmed", r) + } + } + if reachConfirmedOK([]doctor.Result{{Name: "Pod health"}}) { + t.Error("an ABSENT reachability check is 'could not tell', not OK — reachStateOf's lenient default must not leak in here") + } + if reachConfirmedOK(nil) { + t.Error("no results at all is not a confirmation") + } +} + // localEnvNamespace is the doctor-side half of the #401 carve-out; its three // refusals are what stop the re-probe from ever naming someone else's client. func TestLocalEnvNamespace(t *testing.T) { From aaa00f26cdc56d87f73b67777e155bbb17d227cd Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 17 Aug 2026 13:04:22 +0200 Subject: [PATCH 11/11] docs(bugbot): make the recurring finding on this PR a rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Org standard: "A finding that recurs across PRs becomes a rule: add it to .cursor/BUGBOT.md". This one recurred three times inside a SINGLE PR — a failed scan read as "nothing is here", an empty legacy cluster_id read as "runs elsewhere", and `!= ReachNoEnv` read as "an environment is here" — each found by Bugbot only after the previous was fixed. Three instances of one root cause is past the threshold. BUGBOT.md already had the neighbouring rule, but scoped to the value a function RETURNS ("prefer a three-valued return"). Every instance here got the return type right and then collapsed it at the `if` that consumed it, so the existing bullet didn't catch any of them. The new bullet is about the branch, and names the two concrete shapes rather than restating the principle: • a negated comparison against ONE member of a multi-valued enum, which silently absorbs every member added later — with the corollary that the test's input domain must come from the enum's declared surface, since mutation coverage cannot see a vocabulary gap; • a lenient "not found" default reused where the question is "may I believe this?" — reachStateOf returning ReachOK for an ABSENT check is right for a verdict roll-up and wrong for authorising a claim, which is why reachConfirmedOK exists beside it. It closes with the customer-visible cost, per this file's own Tone section: each instance ended in advice to run `client create` on a cluster nothing was confirmed on, where it mints instead of adopting — the guidance manufacturing the orphaned phantom backend#970 is about. Refs cli#515 Co-Authored-By: Claude Opus 5 --- .cursor/BUGBOT.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 6b3306c..9e6d70c 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -73,6 +73,27 @@ Two things make this repo unusual and should shape every finding: (`internal/api/client.go`, `nextPath`). Where "empty" and "unknown" are different answers, prefer a three-valued return (`internal/cluster/discover.go:302`). +- **"Couldn't confirm" used as "confirmed" — in a BRANCH, not just a return type.** The rule + above is about the value a function hands back; this is about the `if` that consumes it, and + it is the defect this repo produces most often. cli#515 shipped it three times in one PR, + each in a different file, each after the previous one was fixed: a failed cluster scan + reported as "no client is running here"; an empty `cluster_id` on a legacy client read as + "runs elsewhere" (`ProvisionedClient.ClusterID` is documented empty on not-yet-backfilled + records); and `reachStateOf(x) != ReachNoEnv` used to mean "an environment is here", when + `ReachState` also has `ReachUnreachable` and `ReachError`. Two concrete shapes to flag: + - **A negated comparison against ONE member of a multi-valued enum.** `!= ReachNoEnv`, + `!= StatusFail` and friends silently include every member added later. Compare against the + member you actually require (`== ReachOK`), and derive the test's input domain from the + enum's declared surface — mutation coverage cannot see a vocabulary gap. + - **A lenient "not found" default reused where the question is "may I believe this?"** + `reachStateOf` returns `ReachOK` for an ABSENT check, which is right for a verdict roll-up + and wrong for authorising a claim — hence the separate `reachConfirmedOK` + (`internal/cli/doctor.go`). The same default is rarely correct for both. + + The customer-visible cost is never a wrong log line: on #515 each instance ended in advice to + run `client create` on a cluster nothing was confirmed on, where it MINTS rather than adopts — + i.e. the guidance manufactured the orphaned phantom of `backend#970`. + - **A cross-repo contract change that only lands on one side.** `scripts/.data-ingestors-ref`, `scripts/.client-ref` and `scripts/.backend-ref` pin upstream refs deliberately so an unrelated upstream commit can't red every open PR. Flag a hand-edit to a generated artifact