Skip to content

fix(cli#128): client-lifecycle follow-ups from live testing (§7.3) - #133

Closed
saadqbal wants to merge 5 commits into
developfrom
feat/cli-128-followups
Closed

fix(cli#128): client-lifecycle follow-ups from live testing (§7.3)#133
saadqbal wants to merge 5 commits into
developfrom
feat/cli-128-followups

Conversation

@saadqbal

@saadqbalsaadqbal commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #128 / #130, from testing the CLI on a real EC2 box. These three fixes were pushed to the #130 branch after it was squash-merged, so they never landed in develop — this PR brings them in.

Fixes

  1. client use accepts the slug or the id. RFC-0001 §7.1/D3 makes the namespace slug the client handle; the shipped command only took the numeric id. Now tracebloc client use tracebloc-amazon works as well as client use 1039 (non-breaking).
  2. cluster info binds to the active client's namespace (§7.3). It was diagnosing the kubeconfig's current-context namespace, so a signed-in user with a selected client got the wrong cluster. It now defaults to the active client's namespace (reusing bindActiveClientNamespace) unless --namespace/--context is given.
  3. Bare tracebloc home screen lists the sign-in + client commands. It hand-listed only the data/cluster/ingest verbs, hiding login and client create/list/use — so a new user couldn't discover how to sign in or pick a client. Grouped into "Sign in & pick a client" / "Work with data" and added a pointer to tracebloc --help.
  4. client list backfills the active client's namespace cache. A config written before the namespace cache existed (or by an older binary) has active_client_id but no cached namespace, so the §7.3 binding no-op'd until an explicit re-use. list already fetches every client, so it now backfills the cache with no extra request — binding then works without a manual re-select.

Verified live

On an EC2 box (x86_64, k3d): confirmed the home screen, client list state column, client use <slug>, the namespace backfill on an old config, and cluster info / data list binding to the selected client (Datasets in tracebloc-amazon (6)).

Tests

Extended TestClientUse (slug + id + cache), added TestBackfillActiveClientCache. Full go build / vet / gofmt / go test ./... green.

🤖 Generated with Claude Code


Note

Low Risk
CLI UX and kubeconfig defaulting only; behavior stays backward compatible for numeric client use and explicit --namespace/--context.

Overview
Brings in live-testing follow-ups for RFC-0001 §7.3 client targeting: tracebloc client use now resolves the active client by namespace slug (preferred) or numeric id, with slug winning on numeric collisions, and prints non-fatal connectivity hints (local cluster probe for online-here vs elsewhere, plus pending/offline).

client listbackfills missing ActiveClientNamespace / name on the signed-in profile from the list response so cluster info and data binding work without a manual re-use. cluster info defaults namespace via bindActiveClientNamespace (same as data commands) and routes genuine “no release” failures through explain() for the “runs on another machine” message; clustertarget drops the noParentReleaseError wrapper and gates that rewrite on cluster.ErrNoParentRelease.

The bare tracebloc home screen now surfaces login / client create / list / use / auth status and clarifies that cluster info checks the active client’s cluster.

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

saadqbaland others added 3 commits July 6, 2026 16:41
…(§7.3)
Testing on a real box surfaced two §7.3 gaps:
- `client use` only accepted the numeric id, but RFC-0001 §7.1/D3 makes the
namespace slug the handle. Accept either the slug (e.g. tracebloc-amazon)
or the id — non-breaking, and it matches what `client list` shows.
- `cluster info` ignored the active client and diagnosed the kubeconfig's
current-context namespace. Default it to the active client's namespace
(bindActiveClientNamespace) when the user gave neither --namespace nor
--context, so it diagnoses the selected client's cluster.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The bare `tracebloc` home screen hand-listed only the data / cluster / ingest
verbs, hiding the whole login + client lifecycle (login, client create/list/
use, auth status) — so a new user couldn't discover how to sign in or select
a client. Add those, group the screen into "Sign in & pick a client" vs
"Work with data", and point at `tracebloc --help` for the full command list.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t list`
A config written before the namespace cache existed has active_client_id set
but no cached namespace, so the §7.3 binding no-ops (cluster info / data
commands fall back to the current-context namespace) until an explicit
re-`use`. `client list` already fetches every client, so backfill the active
client's namespace + name from that list when the cache is empty — no extra
request, and binding then works without a manual re-select. Best-effort:
no-op when already cached or the active id isn't in the account.
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.)

@cursorcursorBot 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 2 potential issues.

Fix All in Cursor

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

Reviewed by Cursor Bugbot for commit 764c512. Configure here.

Comment threadinternal/cli/cluster.go
Comment threadinternal/cli/client.go Outdated
@saadqbalsaadqbal self-assigned this Jul 6, 2026
…ision (Bugbot)
Two Bugbot findings on #133:
- cluster info discarded the binding and returned the raw "no release" error
on exit 4, instead of the §7.3 "active client runs on another machine"
guidance the data commands give. Route its discovery error through
binding.explain. To make explain callable from both paths, gate it on the
cluster.ErrNoParentRelease sentinel (errors.Is) rather than the internal
noParentReleaseError wrapper — which is now removed, simplifying
resolveClusterTarget back to a single exit-4 return.
- `client use <handle>` matched slug OR id in one pass, so a numeric handle
that is one client's all-numeric slug and another's id resolved to whichever
the API listed first. Match slug first (unique per account), fall back to id
only if no slug matched — deterministic, slug wins.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@aptraceblocaptracebloc 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, low-risk follow-up — backward-compatible, well-tested, and it tidies #130's abstraction rather than piling on.

Summary

Four live-testing fixes to #128/#130:

  1. client use accepts slug or id — slug wins over id, and correctly independent of API list order (a slug match returns immediately; an id match is only remembered), so an all-numeric slug can't collide with another client's id. Namespaces are unique per account → unambiguous.
  2. cluster info binds to the active client's namespace via the same bindActiveClientNamespace + explain as the data commands (unless --namespace/--context).
  3. Home screen groups "Sign in & pick a client" / "Work with data" so login + client … are discoverable.
  4. client list backfills the namespace cache from the list it already fetched (no extra request), so older configs get §7.3 binding without a manual re-use.

Nice catch in passing

clustertarget.godrops the bespoke noParentReleaseError wrapper and keys explain directly on errors.Is(err, cluster.ErrNoParentRelease). The sentinel survives through the exitErrorfmt.Errorf("%w …") chain, so errors.Is reaches it; PVC-missing / RBAC / ambiguous-multi-release all lack the sentinel and pass through unchanged. Fewer types, same correctness — a genuine simplification.

One FYI (benign, not a change)

client list now has a config-write side effect (the backfill mutates + Saves the profile). It's one-time (no-op once cached), best-effort (ignores Save errors), and reuses the fetched list — fine, just slightly surprising for a read command.

Tests: TestClientUse (slug/id/cache), TestBackfillActiveClientCache, and clustertarget_test for the sentinel-keyed explain.

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

…nt use` (§7.3)
`client use` set the pointer and reported a bare "success" even for a client
that isn't connected to any cluster yet, or that runs on a different machine —
leaving the user to discover the problem via a later, confusing data-command
failure. Now it reports the client's state non-fatally (selection still
succeeds, per §7.3):
- online + reachable here → "Connected on this machine — data commands will
target namespace X."
- online + not reachable → "runs on another machine — data commands here
can't reach its cluster."
- pending → "isn't connected to a cluster yet — install it
on the target machine first."
- offline → "currently offline — data commands will fail
until it's back."
Online-here vs online-elsewhere uses a bounded (3s), best-effort local cluster
probe (stubbable var); the other states come from the backend status `client
list` already returns. This is the §7.3 "soft warning on client use" the RFC
left optional.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
saadqbal added a commit that referenced this pull request Jul 6, 2026
…el (from #133)
Salvage the two pieces of #133 that survive the "use removed / list hidden"
direction (the rest of #133 was `client use` UX, now moot):
- `cluster info` defaults its namespace to the active client's (§7.3) when the
user gave neither --namespace nor --context, and routes its no-release error
through binding.explain — so it diagnoses the selected client's cluster (set
by `client create`), matching the data commands.
- Gate explain on the cluster.ErrNoParentRelease sentinel instead of the
internal noParentReleaseError wrapper (removed) — simpler, and makes explain
callable from both the data commands and cluster info.
#133 is superseded by this direction (PR #136) and closed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal

Copy link
Copy Markdown
CollaboratorAuthor

Superseded by #136 and the direction decided since: client use is removed and client list is hidden (one machine owns one client), so this PR's headline work — slug/id use, the connection-state feedback on use, the slug-beats-id fix, and the home-screen "pick a client" block — is moot.

The two pieces that survive that direction have been folded into #136 (commit 6c85662):

  • cluster info binds to the active client's namespace (§7.3);
  • the explain-on-cluster.ErrNoParentRelease cleanup (dropping the noParentReleaseError wrapper).

Closing to avoid two PRs contending over the same files and shipping-then-removing client use. Not deleting the branch, in case we want the connection-state UX back if the direction changes.

@saadqbalsaadqbal closed this Jul 6, 2026
saadqbal added a commit that referenced this pull request Jul 7, 2026
…lete + nodeboot
develop's #142 landed the §7.3 namespace-discovery + cluster-info binding (the
pieces this branch had folded from #133), so those are dropped as redundant.
What remains unique to this prototype (RFC-0001 §15, cli#136) is re-applied
cleanly on top of #142:
- `client use` removed; `client list` hidden (installer #303 pre-flight only);
`client delete` added — deprovision + helm uninstall + local cluster delete.
- internal/nodeboot (TeardownCluster + UninstallChart) + api.DeleteClient.
- RFC-0001 §15 amendment (PROPOSED).
Rebuilt via reset-to-develop + re-apply to avoid a semantic mis-merge (the
auto-merge had silently reverted the surface to develop's create/list/use).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for freeto 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.

4 participants

@saadqbal@LukasWodka@saqlainsyed007@aptracebloc