fix(cli): diagnose before advising — a wrong active-client pointer has a way back (cli#515)

[LukasWodka]

Closes #515.

Summary

A wrong active-client pointer had no supported recovery, and the two commands a
stuck user reaches for next told them to reinstall over a healthy environment.
Three changes, in the order the issue lays them out.

1. Diagnose before advising (clustertarget.go)

The shipped §7.3 sentence is RFC-0001's with the third clause deleted: it names
--namespace but never says which namespace, and nothing else in the CLI will
tell you. explain now spends one naming-only cluster.FindClientNamespaces —
the same read discoverRelease already spends purely to write a better message —
and branches on the existing isLocalServerURL predicate.

What the user sees, in each branch:

One client + local server URL:

Error: active client "gpu-box-01" runs on another machine — namespace "gpu-box-01" isn't on the cluster your kubeconfig points at.

A tracebloc client IS running on this machine, in namespace "lukas-02".
  Point this machine at it:  tb client create
      (this cluster already runs a client, so it adopts it — no new credential)
  Or target it just this once:  --namespace lukas-02

Client(s) on a remote/shared cluster — --namespace only, never client create:

Error: active client "gpu-box-01" runs on another machine — namespace "gpu-box-01" isn't on the cluster your kubeconfig points at.

A tracebloc client is running on this cluster, in namespace "colleague-07".
  Target it just this once:  --namespace colleague-07

(several → in namespaces: alpha, beta. + Target one just this once: --namespace alpha. More than one client is the --namespace branch even on a local cluster — "point this machine at it" has no unambiguous "it".)

None found, scan clean:

Error: active client "gpu-box-01" runs on another machine — namespace "gpu-box-01" isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context.

No tracebloc client is running on this cluster either — if this machine should have one, set one up: (set -e; tmp="$(mktemp)"; …)

Could not look — no probe, or the cluster-wide list failed (RBAC, timeout,
unreachable API) — the pre-#515 sentence, byte for byte. That fourth branch is
the point of the three-valued clientSurvey: an absence of evidence must never
print as evidence of absence, or we'd 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 a new test pins both halves at once — the discovered
namespace appears in the message and nowhere else.

2. Extend #401's local fallback to binding.applied misses (doctor.go, home.go)

• 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 a new
  localEnvNamespace.

The ownership gate is unchanged and is what makes this safe: both adopt only when
isLocalServerURL says the server is this machine. On a remote/shared cluster the
honest no-environment answer stands (§7.5).

No scan is spent, and it still finds the install — the installer points the
kubeconfig context at the client's namespace
(client/scripts/lib/install-client-helm.sh:657, kubectl config set-context --current --namespace <ns>), so reading the context is enough. doctor keeps the
original results unless the re-probe actually finds something, so a bare machine
still gets the installer advice and --diagnose still describes the namespace the
user is configured for.

3. Unhide client create; client list marks the local cluster (client.go)

Hidden: true was set because a standalone run on a cluster with no client mints
a phantom (backend#970). That risk is real and unchanged — but hiding was never
what prevented it. The two guards that do are untouched, and now have tests
naming them: the TTY review + confirm, and the off-TTY refusal without
--yes/--credential-file. The repoint itself never reaches the prompt (adoption
happens first), so it stays prompt-free and mints nothing. client list stays hidden.

client list labelled the active pointer (active — this machine) without ever
checking where that client runs. Selection and residency are now separate facts,
keyed on the §7.2 cluster anchor, and a mismatch names the repoint. An unreadable
anchor is a third state: no row claims to be here and none is denied — the marker
degrades to bare (active). The installer's #303 pre-flight is unaffected (markers
live in the row label; the greppable namespace=<ns> field is untouched, and
client_list_contract_test.go still passes).

Not in scope

The two RFC-0001 §7.5 mitigations that were never implemented (drop the pointer on
account switch; cheap re-validation per command) would have prevented this state
rather than explained it, and the upstream cause — orphaned client records from
cluster recreation — is tracked separately. This PR makes the state recoverable.

One correction to the issue's design

The issue says all seven explain call sites already hold a clientset and server
URL. Six do not: resolveClusterTarget builds the clientset internally and returns
nil on its error path. So the probe travels on noParentReleaseError (attached
at both construction sites, which do hold both) rather than through explain's
signature — which also makes it impossible for a caller to hand explain a
clientset for a different cluster than the one that missed.

Test plan

make check and make check-all green; go test ./... green. Golden files
regenerated and the diff reviewed line by line (only create's help entry and the
new strings).

Mutation evidence — 15 mutations, each with the anchor asserted to have applied,
each reddening an assertion:

#	mutation	reddens
M1	surveyCluster always returns "didn't look"	TestExplain_BindingMiss_…
M2	a failed scan reports looked: true	TestSurveyCluster_FailsClosed/scan_forbidden
M3	remote cluster gets the client create branch	…/remote_cluster_never_suggests_client_create
M4	doctor never re-probes (the bug itself)	TestDoctor_WrongPointerOnLocalCluster_…, …LocalClusterWithNothing…
M5b	doctor re-probes an explicit --namespace	TestDoctor_ExplicitNamespaceMiss_IsNotReprobed
M6	localEnvNamespace drops the local-cluster gate	TestLocalEnvNamespace/remote_server_is_refused, TestDoctor_WrongPointerOnRemoteCluster_StaysGated
M7	home keeps the pre-#515 localNoRelease return	TestRealProbeEnv_WrongPointerOnLocalCluster_FallsBack
M8	localEnvFallback adopts a remote cluster	TestRealProbeEnv_WrongPointerOnRemoteCluster_StaysGated
M9	client create back to Hidden: true	TestClientSubcommandVisibility
M10b	the off-TTY mint refusal is dead (backend#970)	TestClientCreate_UnhiddenStillRefusesSilentMint
M11	an unreadable anchor claims "on this cluster"	TestClientListMarker, TestClientList_UnreadableAnchorClaimsNoLocation
M12	client list marks the active row regardless of residency (today's bug)	TestClientList_MarksTheClientOnThisCluster
M13b	the empty-scan branch stops saying the cluster is empty	…/clean_scan_finding_nothing_points_at_the_installer
M14b	the local branch names the stale namespace	…/one_client_on_a_local_cluster_offers_the_repoint
M15b	the multi-client branch offers the repoint	…/several_clients_name_them_all_and_offer_only_--namespace

Control: mutating allowScan() to return true for an applied binding reddens
TestActiveClientBinding_AllowScan/active-client_binding_applied and the new
no-retarget test — the rule this PR must not have changed is still guarded.

Three earlier attempts (M5, M10, M13, M14 first pass) reddened the compiler
(unused variable / unused import) or go vet's Printf check rather than an
assertion. Those prove nothing about coverage, so they were rewritten to keep every
identifier used and the verb count intact and re-run; the table lists only the
valid runs.

🤖 Generated with Claude Code

---

Note

Medium Risk
Touches exit-4 messaging, doctor exit 2 verdicts, and user-visible client create; behavior is heavily tested but wrong survey/re-probe logic could still mis-advise repoint vs mint on edge cases (RBAC, legacy empty cluster_id).

Overview
Fixes cli#515: when the local active-client pointer names a namespace that isn’t on the cluster the kubeconfig reaches, users previously got generic “runs elsewhere” / reinstall advice with no supported way to repoint. This PR makes that state recoverable without changing what commands target (allowScan() stays false on binding misses).

§7.3 errors now diagnose before advising (clustertarget.go): on active-client binding misses, explain runs a bounded naming-only scan (surveyCluster / repointMessage) and branches on what’s actually on the reached cluster—local single client → visible client create (adopt); remote/shared → --namespace only; empty cluster → installer; scan failed → pre-#515 message unchanged. A clusterProbe rides on noParentReleaseError so the scan uses the same cluster that missed.

client create is user-visible again with unchanged mint guards (TTY confirm / non-interactive --yes or --credential-file). client list separates selection vs residency via three-valued residencyOf (unknown anchor ≠ “elsewhere”) and only suggests client create when a client is confirmed on this cluster.

Doctor and home extend the #401 local-only fallback to wrong pointers: re-probe the kubeconfig’s namespace on loopback via localEnvNamespace, adopt only on reachConfirmedOK (not != ReachNoEnv). Stale pointer → exit 2, “Not ready” line, and client create remedy—not green “ready to train.” Home’s realProbeEnv uses the same fallback on pointer miss and ignores heartbeat when pointerStale.

Docs/navigation, golden strings, VERSION 0.10.9, and BUGBOT.md guidance on “couldn’t confirm” treated as confirmed are updated; broad test coverage pins message branches and phantom-mint prevention.

^{Reviewed by Cursor Bugbot for commit aaa00f2. Bugbot is set up for automated code reviews on this repo. Configure here.}

[LukasWodka]

Two notes for review, both about judgement calls rather than the code:

VERSION → 0.10.9. version-bump-gate is a required check and failed on the first push: v0.10.8 is already released and this PR touches internal/*. I picked a patch bump to match this repo's dominant pattern for user-facing copy and surface changes. If the client create unhide is felt to warrant 0.11.0, say so and I'll change the one line — this is the only part of the PR I don't have evidence for, only convention.

client list now spends a cluster-anchor read. Marking residency requires knowing which cluster the kubeconfig reaches, so runClientList calls readClusterID (bounded at 8s by clusterIDReadTimeout). That adds a read to the installer's #303 pre-flight (_account_owns_namespace), which runs when the cluster is already up, so it should be fast — and the greppable namespace=<ns> field is untouched either way, so a slow or failed read can only cost time, never correctness. Flagging it because it's the one place this PR adds I/O to a path the installer depends on.

On the added scan's cost in explain: it can't stall a failing command for long. We only reach it after DiscoverParentRelease already got an answer from the API server (that's what ErrNoParentRelease means), so the API is reachable by construction; the 5s explainScanTimeout is there for the pathological case, and a timeout degrades to the pre-#515 message rather than to a hang.

[LukasWodka]

Bugbot round 1 — 2 findings, both real, both fixed in 63364ff

Replies are on the threads; the short version, because both were the same mistake at two levels:

	finding	verdict
High	stale pointer still blocks after fallback	valid — and the doctor half was a regression introduced by this PR
Medium	empty cluster ID marked absent	valid — a two-valued compare where the honest answer has three values

High. Finding the environment was only half the job: doctor printed Everything looks good — you're ready to run training. and exited 0 while data/resources/seal still bind the stale namespace and exit 4. (Before this PR that state exited 3, so the PR had made it worse on this axis.) It now names the stale pointer and the repoint, and exits 2 — exitChecksFailed, the code doctor already uses for every actionable finding.

home was the sharper version and was genuinely new here: local liveness came from the fallback's client while realHeartbeat still keys on the stale ActiveClientID, so another machine's client being online could green this one. envProbe.pointerStale now blocks that in both directions — no Online, and no confirmedNotOnline either, since a heartbeat about a different client carries no signal about this one.

Medium. ProvisionedClient.ClusterID is empty on legacy / not-yet-backfilled records, so an empty client anchor means residency is unknown. I guarded the local anchor's absence and then compared the client's as a boolean, so a legacy active client was told it is NOT on the cluster your kubeconfig reaches — possibly while running on that very machine. Now a three-valued residencyOf() with both absences on the same side.

Deliberately not done: repointing automatically. Writing ActiveClient* from a status command is the silent retarget §7.5 forbids and this PR is scoped to avoid. The pointer stays wrong until the user runs client create — but every surface that meets the state now says so and names that command.

Also in this commit: realProbeEnv moved from home.go 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. home.go is 782/855; no behaviour change in the move (it's a cut-and-paste plus the errors import following it).

Six more mutations, same discipline as the table above — anchor asserted, build+vet required to still pass so the compiler can't stand in for an assertion, then RED:

#	mutation	reddens
N1	empty client anchor collapses to "elsewhere"	TestResidencyOf/client_anchor_empty_(legacy_record), TestClientList_LegacyClientWithNoAnchorIsNotAccused
N2	doctor greens a stale pointer	TestDoctor_WrongPointerOnLocalCluster_FindsTheEnvironment
N3	doctor drops the stale-pointer note	same
N4b	realProbeEnv stops marking the result stale	TestRealProbeEnv_WrongPointerOnLocalCluster_FallsBack
N5	a stale pointer may render Online again	TestResolveHomeModel_StalePointerNeverRendersOnline
N6	another client's not-online hardens into a verdict	TestResolveHomeModel_StalePointerNotOnlineIsNotConfirmed

TestDoctor_HealthyPointer_StillGreen is the control: without it, "never says Everything looks good" would pass vacuously if doctor stopped saying it at all. N4's first attempt came back INERT (I wrote the pre-file-move indentation into the pattern) — reported rather than silently counted, then re-run.

make check-all green; go test ./... green; goldens regenerated (three new doctor strings only).

[LukasWodka]

Bugbot round 2 — 2 more Mediums, both valid, both fixed in 60c6d54

Round 1's two threads were auto-resolved by Bugbot on the fix. These two are the same class a third time: a sentence asserting something the code never established.

client list hinted create with no local client — the sharpest finding on this PR, because the advice could have manufactured the bug the command was hidden for. mismatch came from the active row alone, so with the pointer elsewhere and nothing confirmed here we still said "point this machine at the client that IS there". On a cluster hosting no client, client create doesn't adopt — it mints, producing the orphaned phantom backend#970 is about. The repoint is now gated on some row being resHere; otherwise the mismatch is still reported, with no target and a pointer at doctor. That branch deliberately covers both remaining cases (none here / can't prove it, resUnknown) because they're equally unnameable.

Home labelled the environment with the stale client's handle — blocking the Online state while leaving the label wrong fixed only half of it. The remembered-name override is now skipped when pointerStale, so the screen names the release that's actually running rather than the client the pointer names.

Three more mutations (N10b, N11, N12), each with a control assertion in the same test so neither fix can pass by the behaviour vanishing:

#	mutation	reddens
N10b	repoint hint fires without anyHere	TestClientList_MismatchWithNoLocalClient_DoesNotPushCreate
N11	anyHere sourced from the active row, not residency	same
N12	unconditional remembered-name override restored	TestResolveHomeModel_StalePointerNeverRendersOnline

Controls: TestClientList_MarksTheClientOnThisCluster still asserts the repoint IS offered when a row is confirmed here; base(false) still asserts the remembered handle IS preferred when the pointer is fresh.

Running total: 27 mutations. Every one had its anchor asserted to have applied and was required to redden an assertion — six attempts that reddened the compiler or go vet instead (unused variable ×3, unused import, a dropped Printf verb) plus two INERT anchors were reported and rewritten rather than counted.

make check-all green, go test ./... green, one new golden string.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 60c6d54. Configure here.}

[LukasWodka]

Bugbot round 3 — 1 High, valid, fixed in c37086a

doctor.ReachState has four members and my re-probe guard checked one:

ReachOK   ReachUnreachable   ReachNoEnv   ReachError

!= ReachNoEnv therefore adopted on ReachUnreachable and ReachError too — both of which mean we could not tell. A stale pointer plus RBAC on the context namespace, or a transient read, would have named an unconfirmed namespace as a secure environment and sent the user to client create on a cluster that may host nothing — where it mints rather than adopts. Straight into backend#970, from advice this PR added.

That is the third instance of one mistake on this PR (surveyCluster.looked, residencyOf's resUnknown, and now this), which is worth saying plainly: the recurring defect here was treating "couldn't confirm" as "confirmed", and it kept reappearing in each new place a decision got made. All three now fail closed.

Fixed with reachConfirmedOK(), deliberately not reachStateOf(…) == ReachOK — reachStateOf defaults to ReachOK when the check is absent, the right lenient default on the main path and exactly the wrong one where the question is whether an unproven namespace may be believed.

The new test derives its input domain from the enum's declared surface (every non-OK member, plus absent) rather than the states that came to mind — per the house rule that mutation coverage can't see a vocabulary gap, which is precisely what this finding was.

#	mutation	reddens
N13	restore != ReachNoEnv	…/unreachable, …/error_(RBAC/NotFound), …/check_absent_entirely
N14	absent check counts as confirmed	…/check_absent_entirely, TestReachConfirmedOK
N15	ReachError counts as confirmed	…/error_(RBAC/NotFound), TestReachConfirmedOK

Control: TestDoctor_WrongPointerOnLocalCluster_FindsTheEnvironment still asserts the genuine ReachOK case does adopt, so "never adopt" can't pass vacuously.

Running total: 30 mutations, each anchor-asserted and each required to redden an assertion rather than the compiler or go vet. make check-all green; go test ./... green; no golden drift this round.

[LukasWodka]

Also added the recurring finding to .cursor/BUGBOT.md (aaa00f2), per the org standard "a finding that recurs across PRs becomes a rule" — three instances of one root cause inside a single PR is past the threshold.

The file already carried the neighbouring rule, but scoped to the value a function returns ("prefer a three-valued return"). All three instances here got the return type right and then collapsed it at the if that consumed it, so the existing bullet caught none of them. The new one is about the branch, and names the two shapes rather than restating the principle: a negated comparison against one member of a multi-valued enum (which silently absorbs every member added later), and a lenient "not found" default reused where the question is "may I believe this?". It closes with the customer-visible consequence, per the file's own Tone section.

[saqlainsyed007]

Approve — correctness review, focused on the three changes in #519.

What this does

Turns a wrong active-client pointer from a dead end into a recoverable state, without changing what any command targets:

1. explain diagnoses before advising — a naming-only surveyCluster feeds repointMessage, branched on what's actually on the reached cluster (local single → client create adopt; remote/multi → --namespace only; empty → installer; couldn't-look → the byte-for-byte pre-#515 sentence). allowScan() stays false, so this changes the message, never the target.
2. doctor/home extend #401's local fallback to a wrong (not just empty) pointer, gated on isLocalServerURL + binding.applied and adopted only on a positive reachConfirmedOK.
3. client create is unhidden (with its mint guards intact and now tested); client list splits selection from residency via a three-valued residencyOf.

Correctness verification

• All 5 Bugbot findings (3 rounds) are fixed in the head and I re-checked each against the current diff: reachConfirmedOK requires == ReachOK (not != ReachNoEnv, which would sweep in ReachUnreachable/ReachError); residencyOf keeps an empty client cluster_id at resUnknown; the client list repoint hint requires anyHere so it can't push a mint; home blocks the Online state, the stale display label, and confirmedNotOnline; and doctor's closing verdict fix (43a4b07) is present.
• The absence-vs-presence discipline is consistent across all three sites (clientSurvey.looked, residency, reachConfirmedOK) — no lenient default leaks into a claim-authorising branch.
• FindClientNamespaces sorts, so the multi-namespace message and the --namespace <first> suggestion are deterministic.
• localEnvFallback only ever returns localNoRelease/localLive/localDegraded, so pointerStale = ep.local != localNoRelease correctly excludes the no-release case.
• doctor's re-probe reuses the original clientset against the kubeconfig's own namespace; since localEnvNamespace reloads the same Path/Context, its isLocalServerURL gate applies to the same cluster the probe hits — the ownership guarantee holds.
• The --namespace/--context explicit-override paths stay un-re-probed (binding not applied), and the remote/shared gate is exercised on both doctor and home.

Non-blocking observations

• Test coverage is unusually strong — the mutation table plus per-branch text pins are exactly what this class of message-vs-target bug needs.
• CI is fully green (Test, Lint, golangci-lint, govulncheck, all builds, Bugbot).

No correctness issues found on my pass. Nice work on the three-valued discipline and the fail-closed survey.