Skip to content

feat(cli#128): selected-vs-connected — bind data commands to the active client (§7.3)#130

Merged
saadqbal merged 4 commits into
developfrom
feat/cli-128-selected-vs-connected
Jul 6, 2026
Merged

feat(cli#128): selected-vs-connected — bind data commands to the active client (§7.3)#130
saadqbal merged 4 commits into
developfrom
feat/cli-128-selected-vs-connected

Conversation

@saadqbal

@saadqbal saadqbal commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Closes #128. Implements RFC-0001 §7.3 ("'selected' is not 'connected'") — client use set only a local pointer while the data commands ignored it and targeted whatever the current kubeconfig pointed at, risking a silent wrong-target on multi-cluster / remote hosts.

Three logical commits (each builds + tests on its own):

  1. refactor — extract resolveClusterTarget, collapsing the duplicated Load → NewClientset → DiscoverParentRelease (→ DiscoverSharedPVC) block + its exit-code contract (3 kubeconfig/clientset, 4 missing release/PVC) out of data ingest/list/delete. This is the resolveClusterTarget task from CLI quick wins: cluster-connect + kubeconfig-flag helpers, rename sweep, pinned lint + deadcode gate #127, landed here as the base (the rest of CLI quick wins: cluster-connect + kubeconfig-flag helpers, rename sweep, pinned lint + deadcode gate #127 stays a separate quick-wins PR, different epic). cluster doctor deliberately excluded (different 2/3 exit contract).
  2. bind data commands (§7.3) — cache the active client's namespace + name in the env profile at client use/create (setActiveClient), so data commands bind without a backend round-trip (they run cluster-local, maybe offline). data ingest/list/delete default -n to it when the user gave neither --namespace nor --context; no active client → unchanged current-context behavior (backward compatible). A "no release here" miss on a bound namespace is rewritten to the §7.3 "active client runs on another machine" guidance (typed noParentReleaseError); a PVC-missing failure passes through.
  3. client list state column (§7.3) — add online/offline/pending from the backend EdgeDevice.status code (the "connected somewhere" signal, C.4) beside the (active — this machine) marker, plus a hint separating selected (local pointer) from connected (backend heartbeat). Plain words, not emoji glyphs (§12 mojibake watch-item).

Design decisions

  • Namespace source: cached at use/create (local) rather than a per-run backend lookup — keeps data commands cluster-local/offline-capable. Adds active_client_namespace + active_client_name to the config Profile (both omitempty → v1/v2 configs unaffected).
  • "Connected" = backend heartbeat only (status), not per-row local cluster probing (that's the slow path flagged in §12); local reachability stays a per-active-client concern.

Not in scope

  • Optional soft reachability probe on client use (issue marks it optional; would add a cluster dial to an intentionally cluster-free command) — left as a follow-up.

Verification

go build ./..., go vet ./..., gofmt, and the full go test ./... suite are green; new tests in clustertarget_test.go cover the state-label mapping, binding (defaults / explicit-override no-op / no-active-client no-op), explain (rewrite vs pass-through), and the setActiveClient cache.

🤖 Generated with Claude Code


Note

Medium Risk
Data commands may target a different namespace when an active client is set, which changes default cluster targeting for multi-cluster setups; behavior is backward compatible without a cached namespace.

Overview
Implements RFC-0001 §7.3 so data ingest, data list, and data delete no longer ignore the machine's active client and silently target whatever the kubeconfig's current context implies.

Active client binding: client create / client use now call setActiveClient, which persists active_client_namespace and active_client_name on the env profile (alongside active_client_id). When neither --namespace nor --context is set, data commands default the target namespace from that cache—no backend call—while explicit flags still win. No cached active client keeps the old current-context behavior.

Cluster resolution: Shared resolveClusterTarget replaces duplicated kubeconfig → clientset → parent release (→ PVC) logic and preserves exit codes 3/4. DiscoverParentRelease exposes ErrNoParentRelease so a true "no release in namespace" can be distinguished from RBAC or ambiguous multi-release errors; bound-namespace misses get a "active client runs on another machine" message via activeClientBinding.explain (PVC failures are not rewritten).

client list: Rows show backend state= (online / offline / pending as plain text), (active — this machine) on the selected id, and a short hint separating local selected from backend connected heartbeat.

Reviewed by Cursor Bugbot for commit e798dc8. Bugbot is set up for automated code reviews on this repo. Configure here.

saadqbal and others added 3 commits July 6, 2026 13:06
…i#127)

data ingest / list / delete each repeated the same
Load -> NewClientset -> DiscoverParentRelease (-> DiscoverSharedPVC)
sequence and its exit-code contract (3 for kubeconfig/clientset, 4 for a
missing release/PVC). Collapse it into one helper in clustertarget.go.

Behavior-preserving; `cluster doctor` is deliberately not a caller (it has
a different 2/3-escalation exit contract). This is the resolveClusterTarget
task from cli#127, landed here as the base for the cli#128 §7.3 binding.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
RFC-0001 §7.3: "selected" (a local pointer) was not "connected" (a
reachable cluster). `client use` set only a pointer, and the data commands
ignored it — they targeted whatever the current kubeconfig context pointed
at, risking a silent wrong-target on a multi-cluster / remote host.

- Cache the active client's namespace + display name in the env profile at
  `client use` / `create` time (new setActiveClient), so the data commands
  can bind without a backend round-trip (they run cluster-local, maybe
  offline).
- data ingest/list/delete default -n to the active client's namespace when
  the user gave neither --namespace nor --context (bindActiveClientNamespace).
  No active client → unchanged current-context behavior (backward compatible).
- A "no release in namespace" miss on a bound namespace is rewritten to the
  §7.3 "active client runs on another machine" guidance (typed
  noParentReleaseError), while a PVC-missing failure passes through.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`client list` marked only the active (selected) client and showed no
connection state, so a stale pointer to a client whose cluster is gone was
invisible. Add a state column (online/offline/pending) sourced from the
backend EdgeDevice.status code — the "connected somewhere" signal per
RFC-0001 C.4 — alongside the existing "(active — this machine)" marker, and
a hint separating selected (local pointer) from connected (backend
heartbeat).

Plain words, not flag/emoji glyphs, to avoid the CI/Windows-console
mojibake called out as a §12 watch-item.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
Contributor

👋 Heads-up — Code review queue is at 31 / 30

Above the WIP limit. The team convention is to review existing PRs before opening new work.

Open PRs currently in Code review (oldest first):

Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix All in Cursor

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

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 758af8d. Configure here.

Comment thread internal/cli/clustertarget.go Outdated
@saadqbal saadqbal self-assigned this Jul 6, 2026
…re" (Bugbot)

The §7.3 binding wrapped every DiscoverParentRelease failure in
noParentReleaseError, so `explain` rewrote API/RBAC list errors and
ambiguous multiple-release matches as "the active client runs on another
machine" — hiding the real diagnostic.

Add a cluster.ErrNoParentRelease sentinel on the true not-found case and
gate the wrapping (and thus the rewrite) on errors.Is; other discovery
failures keep their own message and exit 4. Tests assert the sentinel
matches not-found and not the multiple-release case.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@aptracebloc aptracebloc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Clean implementation of RFC-0001 §7.3 — the refactor is a solid DRY win and the binding is careful. A couple of non-blocking notes below.

Summary

  • resolveClusterTarget centralizes the Load → NewClientset → DiscoverParentRelease (→ PVC) block + the 3/4 exit contract out of the three data commands (cluster doctor correctly excluded — different 2/3 contract).
  • §7.3 binding: client use/create cache the active client's namespace+name; data commands default -n to it only when neither --namespace nor --context was given. A "no release here" miss on a bound namespace is rewritten to the "runs on another machine" guidance.
  • client list shows state= (backend heartbeat) + (active — this machine) + a selected-vs-connected hint.

What stands out

  • Precise error plumbing: the new cluster.ErrNoParentRelease sentinel + errors.Is/errors.As mean only the genuine "namespace has no release" binding-miss gets rewritten — RBAC / ambiguous-multi-release / PVC-missing errors pass through untouched. Not a blanket rewrite.
  • Backward-compatible + non-presumptuous: no active client / no cache / explicit --namespace|--context → unchanged current-context behavior; explicit flags always win; new config fields are omitempty (v1/v2 configs unaffected).
  • Offline-correct: namespace cached at use/create rather than a per-run backend lookup — and namespace is effectively immutable per client, so staleness isn't a concern.
  • Good coverage of the binding (default / explicit-override no-op / no-active no-op), the explain rewrite-vs-passthrough, the sentinel, and the state-label mapping.

Non-blocking notes

  1. Deliberate default-targeting change: once a user has run client use/create, a bare data … now targets the cached namespace, not the kubeconfig's current namespace. That's the §7.3 intent and it's mitigated (explicit flags override, clear message) — just flagging it changes behavior for already-migrated users.
  2. Binding is namespace-string-based, not identity-based — it defaults the namespace but not the cluster/context, so a wrong cluster is detected via the discovery miss + "runs elsewhere" message rather than prevented. Narrow gap: a different cluster hosting a release under the same namespace name wouldn't be caught (silent right-namespace-wrong-cluster). Almost certainly fine given slug-derived namespaces — worth a one-line confirmation, not a change.

— drafted with Claude (Opus 4.8), sent by @aptracebloc

@saadqbal saadqbal merged commit 66b69b9 into develop Jul 6, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants