Uh oh!
There was an error while loading. Please reload this page.
feat(cli): validate --location against vendored zone list (#94) - #95
Closed
LukasWodka wants to merge 4 commits into
Closed
feat(cli): validate --location against vendored zone list (#94)#95LukasWodka wants to merge 4 commits into
LukasWodka wants to merge 4 commits into
Conversation
RFC-0001 P1. Adds the `client` subtree the login flow hands off to: provision a tracebloc client for this machine, list the account's clients, and attach this machine to an existing one. - internal/slug: Go port of RFC-0001 Appendix B (backend common/utils/slug.py) — DNS-1123 slugify (NFKD via x/text) + collision suffix + empty-slug guard, kept in lock-step with the backend that validates the result. - internal/api: CreateClient / ListClients / ListClientAdmins against /edge-device/, Bearer-authed (backend#836). - internal/cli/client.go: create (--name / --location / --yes), list (ls), use <id>. The derived namespace is shown for confirmation; location is required (never silent-empty); a 403 surfaces the ask-an-admin path (backend#836); the generated machine credential is printed once. Location auto-detect (cloud-metadata / GeoIP suggested default) is a fast-follow — this PR takes --location or prompts for it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eractive tests Self-review follow-up on the client commands: - api: ListClients now follows DRF `next` to the end (was page-1 only), so `list`, `use <id>`, and create-time collision detection see every client in the account, not just the first page. - cli: `create` gathers name + location first, then shows one review + a single confirm (was confirm-mid-flow) — matches the dataset-push interactive flow. - tests: committed slug golden-parity test (24 pairs verified byte-identical against the Python slugify_dns1123, incl. NFKD ligatures/fractions/roman/ fullwidth); interactive create + cancel via the prompter seam; paginated list; collision-suffix end-to-end. - slug: doc-note the redundant dash-collapse (mirrors slug.py) and the ""/None fallback divergence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The deferred fast-follow from cli#92: pre-fill `client create`'s location prompt with a detected electricityMaps zone (backend ZONE_CHOICES), so a cloud-hosted client doesn't have to look up its own zone. - internal/geo: Detect() probes cloud instance metadata first (AWS IMDSv2/v1, GCP, Azure — concurrently under one short deadline, first wins → high confidence), then Cloudflare IP geolocation (low confidence, flagged). Returns an ISO country code, always a valid top-level zone; cloud regions map via a curated AWS/GCP/Azure table, and an unmapped region falls through to GeoIP for a valid zone rather than suggest something the backend would reject. - client create: the detected zone pre-fills the prompt default (the user confirms with Enter or overrides) — still never silent, never empty. A detectZone seam keeps the command tests hermetic. - Best-effort: offline / egress-restricted / bare metal → empty default (the prior behavior). Only runs interactively when --location is omitted. Tests: geo per-provider + GeoIP fallback + unmapped-region fallthrough + nothing-detected + region→country table; cli accepts-detected-zone end-to-end. go build/vet/test green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EdgeDevice.location is CharField(choices=ZONE_CHOICES) (electricityMaps zones),
so the backend rejects any zone not in the list. The CLI didn't validate, so a
bad --location (or a prompt typo) only failed as an opaque 400 at create time.
- internal/zones: the backend's ZONE_CHOICES vendored as zones.json (352 zones)
and go:embed-ed; Valid / Name / Suggest. scripts/sync-zones.sh regenerates it
from the backend's metaApi/models/zone_choices.py (the source of truth — a
private repo, so a local sibling read rather than a curl), with a --check
drift mode for CI, mirroring sync-schema.sh.
- client create: --location and the location prompt validate against the list
and fail fast with close-match suggestions ("germany" → did you mean DE?). The
cli#93 auto-detect only pre-fills a detected zone if it's valid.
Tests: zones Valid/Name/Suggest; cli rejects an invalid --location before any
API call. go build/vet/test green.
Closes#94.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>saadqbal
marked this pull request as draft
June 22, 2026 07:34
saadqbalforce-pushed
the
feat/84-location-autodetect
branch
from
July 6, 2026 14:27
7efdd9c to
edb8dedComparesaadqbal added a commit
that referenced
this pull request
Jul 7, 2026
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 added a commit
that referenced
this pull request
Jul 7, 2026
…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
commented
Jul 7, 2026
Collaborator
Closing — superseded by the cli#137 direction (PR #144). Per the review on #144 (finding #7), Reopen if we ever revisit client-side zone validation. Closing the linked #94 for the same reason. |
saadqbal added a commit
that referenced
this pull request
Jul 7, 2026
…ional (#137) (#144) * feat(cli): auto-name clients <firstname>-NN and make --location optional (#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> * fix(cli): address review — restore silent-path safety rails (#137) 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> * chore(cli): remove internal/geo — location auto-detect is gone, not parked (#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> * fix(cli): address Bugbot — 426 upgrade signal + slug-aware auto-name 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> * fix(cli): consent guard must not block idempotent adopt (#137, Bugbot 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> * fix(cli): re-run reuses the anchored client's name, not a fresh handle (#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> * fix(cli): honest message when the consent guard fires on a list failure (#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> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes#94.
EdgeDevice.locationisCharField(choices=ZONE_CHOICES)(electricityMaps zones), so the backend'sChoiceFieldrejects any zone not in the list. The CLI didn't validate--location/ the prompt, so a bad value (Germany,eu-central-1, a typo) only failed as an opaque 400 at create time. This validates against a vendored copy of the list and fails fast with suggestions.Stacked on #93 (
feat/84-location-autodetect, itself on #92) — retarget down the chain as each merges.What's here
internal/zones— the backend'sZONE_CHOICESvendored aszones.json(352 zones: countries + sub-zones likeUS-CAL-CISO), embedded viago:embed.Valid/Name/Suggest.scripts/sync-zones.sh— regenerateszones.jsonfrom the backend'smetaApi/models/zone_choices.py(the source of truth — it's what the API validates against), with a--checkdrift mode for CI.client create—--locationand the location prompt validate against the list, failing fast with close-match suggestions ("germany"→ did you mean: DE?;"de"→ DE). The feat(cli): location auto-detect for client create (#84) #93 auto-detect only pre-fills a detected zone if it's actually valid.Why a local-file sync (not a URL like sync-schema.sh)
sync-schema.shcurls data-ingestors (a public repo).backendis private, so an anonymouscurlofzone_choices.pywon't work — and the backend file is the authoritative validation source, so the script reads it from a sibling checkout (ZONES_SOURCEto override). The committedzones.jsonis what the binary uses; the script is the maintenance / CI-drift tool.Tests
internal/zones:Valid/Name(case-sensitivity + sub-zones),Suggest(name match, wrong-case, prefix, garbage → none).internal/cli: an invalid--locationerrors with aDEsuggestion before any API call; the existing flow/interactive tests still pass (DE/US/detectedFRare valid). Fullgo build/vet/test ./...green.🤖 Generated with Claude Code