Skip to content

feat(cli): auto-name clients <firstname>-NN + make --location optional (#137)#144

Merged
saadqbal merged 7 commits into
developfrom
feat/137-autoname-optional-location
Jul 7, 2026
Merged

feat(cli): auto-name clients <firstname>-NN + make --location optional (#137)#144
saadqbal merged 7 commits into
developfrom
feat/137-autoname-optional-location

Conversation

@saadqbal

@saadqbal saadqbal commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Closes #137. Part of the installer first-run UX v2 spec (umbrella backend#992; interim copy fixes shipped in client#307).

Makes client create need zero flags and zero prompts on the installer path.

What changed

Auto-name <firstname>-NN

  • first_name is parsed from GET /userinfo/ into api.Identity and persisted in the profile at login.
  • When --name / TRACEBLOC_CLIENT_NAME is absent, the name is auto-derived as <slug(first_name)>-NN (fallback: email local-part, then client). No name prompt, ever.
  • NN is the next free two-digit number across the account's existing client names and namespaces, so a second machine lands on lukas-02 rather than a slug -2 bump — and the derived name passes through slug.Derive unchanged (name = namespace by construction).

--location optional

  • No errMissingFlag("--location"), no prompt. CreateClientRequest.Locationjson:"location,omitempty": with no --location the CLI sends nothing and the backend records the client with no location (backend#993), not a silent default.
  • --location still honored verbatim. internal/geo stays in the tree for opt-in enrichment but no longer runs on the silent path; the now-dead detectZone seam and errMissingFlag/validateNonEmpty helpers are removed.

Noun pass — adopt message is now "This machine is already registered as client …" (machine = hardware, client = tracebloc entity). The vendor→collaborator sweep was already done in client#307; no user-facing vendor strings remain in the Go.

Docs — RFC-0001 Rev 8 amendment: §6.6/§6.7/§7.7 + the D1 row note the <firstname>-NN / optional-location deviation from the hostname-derived design (decided by Lukas 2026-07-06), keeping the original text as design-of-record.

Acceptance (all covered by tests)

  • client create --yes --credential-file f with no other flags, non-interactive → succeeds, auto-name minted, no location key in the request body.
  • Two machines, same account/first name → lukas-01, lukas-02.
  • Re-run on the same cluster still adopts (idempotency unchanged — R7 tests green).
  • --name/--location honored verbatim.

Local: go build, go vet, gofmt, errcheck, goimports, and the full go test ./... all clean.

Coordination notes for review

🤖 Generated with Claude Code


Note

Medium Risk
Changes provisioning defaults and API payloads (omitted location, new naming) and adds fail-closed paths around list failures and credential minting—behavioral risk for installer/CI, not auth/crypto.

Overview
Implements cli#137 so the installer can run client create with zero flags and zero prompts, while RFC-0001 documents the product shift in Rev 8.

Auto-name <firstname>-NN: Login persists first_name from GET /userinfo/ in the profile; when --name / TRACEBLOC_CLIENT_NAME are absent, the CLI picks the next free two-digit handle from the account’s existing clients (reuses the anchored client’s name on cluster re-run). Location is optional: CreateClientRequest.location uses omitempty so unset means no location key in JSON; internal/geo is deleted and name/location prompts are removed.

Safety: Login clears stale email/first name before WhoAmI; non-interactive mint requires --yes or --credential-file unless the cluster will adopt; auto-naming and that guard fail closed if the client list can’t be read (426 surfaces upgrade-required). Flags and env vars still override; resume commands pin the resolved auto-name.

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

…nal (#137)

Zero flags, zero prompts on the installer's `client create` path (installer
UX v2 spec):

- Auto-name: when neither --name nor TRACEBLOC_CLIENT_NAME is given, derive
  <slug(first_name)>-NN from the signed-in identity (fallback: email
  local-part, then "client"). NN is the next free two-digit number across the
  account's existing client names AND namespaces, so a second machine lands on
  lukas-02 rather than a slug -2 bump, and the name passes through slug.Derive
  unchanged (name = namespace by construction). No name prompt, ever.
  first_name is parsed from /userinfo/ into api.Identity and persisted in the
  profile at login.
- Optional location: no required-flag error, no prompt. CreateClientRequest
  .Location is now json:"location,omitempty", so with no --location the CLI
  sends nothing and the backend records the client with no location
  (backend#993) instead of a silent default. --name/--location still honored
  verbatim; internal/geo stays in the tree for opt-in enrichment but no longer
  runs on the silent path (the detectZone seam and the now-dead
  errMissingFlag/validateNonEmpty helpers are removed).
- Noun pass: the adopt message now says "This machine is already registered
  as client ..." (machine = hardware, client = tracebloc entity).
- Docs: RFC-0001 Rev 8 amendment — §6.6/§6.7/§7.7 + the D1 row note the
  <firstname>-NN and optional-location deviation from the hostname-derived
  design (decided by Lukas 2026-07-06), retaining the original as
  design-of-record.

Tests: auto-name (no-location, numbering, email fallback), flags-honored,
reworked interactive/cancel and resume-command tests to the zero-prompt flow.

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

@LukasWodka LukasWodka 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.

Deep review (multi-agent: 8 finder angles → independent verification per finding; two findings confirmed empirically against this branch). Overall: the core design is right and the test evolution is genuinely good — the acceptance tests check the raw JSON body for the absent location key and pin the auto-name into the failure-resume command. But the silent path has two real safety regressions worth fixing before merge; the rest is polish.

Findings, most severe first

1. internal/cli/client.go:256 — a bare non-interactive client create now provisions silently and prints the credential to stdout.
Deleting errMissingFlag removed the only guard that stopped a zero-flag, no---yes run from side-effecting. tracebloc client create | tee out.log → stdout isn't a TTY → prompter nil → review+confirm skipped → real client minted, machine credential lands in the log file. Before this PR: fail-fast with "required (non-interactive)" before any network write. Suggest: when pr == nil, require --yes (or --credential-file).

2. client.go:207 — a transient list failure deterministically mints a duplicate <base>-01.
ListClients failure is deliberately non-fatal, but auto-naming then numbers against an empty list. Since auto-naming is the default installer path, <base>-01 exists on virtually every account — so one 502 makes the duplicate the deterministic outcome, not a race; name and namespace collide (no server-side uniqueness), breaking D3's unique-handle contract, with the only trace in the install log file. The adopt pre-flight already fail-closes on listErr — this branch should too: refuse to auto-name (ask for --name), or fall back to a random -<hex4> suffix + a terminal warning.

3. internal/cli/auth.go:117 — a failed /userinfo/ during re-login leaves the previous user's identity in the profile.
Email/FirstName are written only on WhoAmI success and never cleared. Shared box: bob logs in while /userinfo/ 500s → profile keeps FirstName: Alice with bob's token → bob's client is durably named alice-01. Suggest: reset both fields before the best-effort fetch.

4. client.go:668–684 — the numbering bypasses the slug layer's 63-char cap, breaking the PR's own invariant.
fmt.Sprintf("%s-%02d", base, n) appends to a base already capped at 63, so a ≥61-char first_name yields a 66-char display name while slug.Derive truncates the namespace back — name ≠ namespace, and machine 2 then gets the exact slug -2 bump this PR exists to eliminate (verified empirically on this branch: base×63 → name …-01 (66 chars) vs namespace (63); machine 2 → display …-02 but namespace …-2). Deeper fix: move NN-numbering into the slug package next to Derive's existing collision logic — that makes the name==namespace invariant structural and collapses the two hand-rolled taken-sets (lines 678 vs 243) that already have subtly different membership rules.

5. client.go:63--help still promises the old contract.
The --location help says "prompted if omitted" and the command Short still advertises (--name, --location) — the surface says prompts exist while the behavior is silent omission.

6. client.go:201 — comments + RFC promise TRACEBLOC_CLIENT_NAME overrides, but no code reads it.
It works via the installer (provision.sh maps the env var to --name), but a direct CLI invocation silently ignores it. Either wire os.Getenv as a fallback or scope the claim to the installer path.

7. internal/geo/ is now a dead package (~265 non-test lines + CI-run tests) with a comment claiming it "stays available".
The detectZone seam was its last caller. Meanwhile --location is sent verbatim with zero validation — the zone table that could catch --location Germany at the boundary ships in the binary, unreachable. Delete the package (git remembers), or actually wire opt-in validation.

8. client.go:307 — the noun swap went the wrong direction.
"This machine is already registered as client…" — but the adopt is keyed on the cluster anchor (kube-system UID), and the CLI can run from a workstation against a remote cluster a colleague provisioned; that workstation was never registered. The original "This cluster…" was factually right (and the sibling hint at line 149 still says "on the same cluster it adopts").

Minor, below the cap: the prompt-era answers: map[string]string{} scaffolding left in client_test.go:683 / verbose_install_test.go:142 is dead weight now that create never calls Input; and the new signInAs test helper silently writes to the developer's real ~/.tracebloc if ever called before withClientBackend sets TRACEBLOC_CONFIG_DIR — worth a guard.

Findings 1–3 are cheap fixes; #4's slug-layer move is the one structural ask.

🤖 Review generated with Claude Code

From Lukas's deep review on PR #144. The zero-prompt design had removed
several guards; putting them back:

- Non-interactive mint now fails closed: a bare `client create` with no TTY,
  no --yes, and no --credential-file would have minted silently and printed
  the machine credential to stdout. Require --yes (consent) or
  --credential-file (keeps the secret off stdout); the installer passes both.
- Auto-naming fails closed on a client-list error instead of numbering against
  an empty list and minting a deterministic `<base>-01` duplicate (name AND
  namespace collide, no server-side uniqueness). An explicit --name still
  tolerates a list blip.
- login clears the profile's email/first_name before the best-effort WhoAmI,
  so a re-login as a different user on a shared box can't inherit the previous
  user's identity (and auto-name the new client after them) when WhoAmI fails.
- Auto-name caps <base>-NN at the 63-char DNS label so a long first_name keeps
  name == namespace instead of reintroducing the slug -2 bump.
- --help/Short no longer claim location is "prompted"; wire
  TRACEBLOC_CLIENT_NAME / TRACEBLOC_CLIENT_LOCATION as real flag defaults so
  the documented env-var override is true for a direct CLI call, not just via
  the installer.
- Revert the adopt message to "This cluster is already registered…": adopt is
  keyed on the cluster anchor and can run against a remote cluster, and the
  sibling resume/hint lines already say "cluster" (consistency + accuracy).

Test hygiene: drop the now-dead prompter answer maps; guard signInAs against
writing to a real ~/.tracebloc. New regression tests for each of the above.

Kept internal/geo (review #7): cli#137 explicitly parks it for opt-in
--location enrichment, and PR #95 (location validation) builds on it — deleting
now would churn a package that lands a caller shortly.

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

saadqbal commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Great review — 1/2/3 are real and worth catching. Pushed 010ddf7 fixing everything except #7:

  • 1/2 (the safety ones): non-interactive mint now fails closed unless --yes or --credential-file is given, and auto-naming refuses on a client-list error instead of numbering against an empty list into a <base>-01 duplicate. Installer passes both flags, so it's unaffected.
  • 3: login clears email/first_name before the best-effort WhoAmI, so a failed confirm can't leave the prior user's identity.
  • 4: capped <base>-NN at 63 so name == namespace holds for long names. Kept it in autoClientName rather than moving numbering into slug — same invariant, less churn; happy to do the slug-layer move as a follow-up if you'd prefer the structural version.
  • 5/6: help/Short no longer claim prompting; wired TRACEBLOC_CLIENT_NAME/_LOCATION as real flag defaults so the override is true for a direct CLI call, not just the installer.
  • 8: reverted to "This cluster…" (agreed — cluster-keyed + remote-capable, and the sibling hints say cluster).
  • minors: dropped the dead prompter maps + guarded signInAs.

On #7 I'd keep internal/geo: the ticket parks it for opt-in --location enrichment and #95 builds directly on it, so deleting now churns a package that gets a caller back shortly. Fine to leave it, or do you want the --location zone-validation wired here instead of in #95?

Each fix has a regression test; full suite green under -race.

Comment thread internal/cli/client.go
…arked (#137, review #7)

Per Lukas's review #7: the zero-prompt location decision removed detectZone,
geo's only caller, so the package (geo.go + regions.go + tests) was dead. Rather
than park it "for later" behind a comment, delete it — git remembers, and the
product direction is that location is optional and validated by the backend (a
bad --location surfaces as its real create error), not auto-detected or
CLI-validated. Updates the RFC Rev 8 callouts + the create comment to match.

Supersedes PR #95 (CLI-side --location zone validation), which is closed.

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

saadqbal commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Re #7 — went with deleting it (c1aaa68). internal/geo is gone (geo.go + regions.go + tests), RFC Rev 8 callouts updated to say auto-detect is removed rather than parked. The rationale: location is optional and the backend is the source of truth for zones, so there's no caller and no plan for one. Closed PR #95 + issue #94 (the CLI-side zone-validation work) as superseded by this direction.

Comment thread internal/cli/client.go
…collisions (#137)

- Auto-name list-failure path: a 426 (UpgradeRequiredError) now surfaces the
  upgrade message verbatim instead of the "couldn't reach the backend — retry"
  framing, which is wrong for a too-old CLI (retrying never helps). Non-426
  list errors keep the retry/pass-`--name` guidance. (Bugbot #144-A)
- autoClientName reserves each existing client's handle in BOTH raw and
  slugified form, so a legacy client stored as "Lukas 01" (blank namespace)
  still blocks the derived handle "lukas-01". (Bugbot #144-B)

Regression tests for both.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread internal/cli/client.go
… follow-up)

The non-interactive consent guard sat before CreateClient, which can return
HTTP 200 and adopt an already-anchored client without printing any credential.
Skip the guard when this cluster is already anchored to a client in the account
(willAdopt) — a non-interactive re-run then adopts as before instead of erroring
"refusing to provision". A genuine fresh mint still requires --yes or
--credential-file. Regression test added.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread internal/cli/client.go Outdated
#137, Bugbot follow-up)

On a re-run without --name against an already-anchored cluster, auto-naming
picked the next free handle (lukas-02) even though the backend adopts the
anchored client by cluster_id — so the review/confirm and POST body described a
client that was never created. Reuse the anchored client's existing name in that
case (new anchoredClient helper); fresh clusters still auto-name as before.
Regression test added.

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

@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.

Reviewed by Cursor Bugbot for commit c15d354. Configure here.

Comment thread internal/cli/client.go
…re (#137, Bugbot follow-up)

When the non-interactive consent guard trips because ListClients failed
(willAdopt unknown), the error now names the real cause — the client list
couldn't be read, so we can't tell a fresh mint from an idempotent adopt — and
notes that a retry once the backend is reachable adopts an existing client
without any flag. Still fail-closed (refusing beats an unconsented credential
print), just no longer implying --yes is the only path. Regression test added.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal saadqbal merged commit 0656719 into develop Jul 7, 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.

2 participants