fix(hello): clarify account prompt and surface real validation errors#18
Merged
Merged
Conversation
Telemetry for "dhq hello" (interactive setup) showed 15 internal errors over 30d that were really auth/user failures dressed up as CLI bugs: - 8 were ".deployhq.com.deployhq.com" TLS errors (users typed their full hostname at the "Account subdomain:" prompt). - 4 were 403 (AccessDenied x2 + api_access_restricted x2). - 3 were 404 Not found on the validate-credentials API call. The URL-doubling case is already covered defensively in sdk.New (PR #15). What's left is helloLogin wrapping the validation error in *output.InternalError unconditionally, which bypasses ClassifyError's status-code fallback (PR #16) and gives users a generic "internal error" message instead of an actionable hint. Changes: - Clarify the interactive prompt to "Account subdomain (e.g. 'mycompany' for mycompany.deployhq.com):" — same example shown in the --account flag help. Lifts the hint into the moment it's needed. - Extract classifyHelloValidateErr that maps the validation API error to AuthError (401, 403) or UserError (404) or NetworkError, and returns nil for everything else so the raw error passes through. ClassifyError can then handle 5xx via its status-code fallback once PR #16 lands; if it doesn't, behavior matches main today. - Uses errors.As so the change is self-contained and doesn't depend on sdk.IsForbidden (added in PR #16). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR refactors the login credential validation error handling in the CLI. It extracts inline error classification into a dedicated helper function that maps HTTP status codes and network errors to specific typed error classes, applies this helper in the validation flow, and adds comprehensive test coverage for the new classification logic. ChangesLogin Validation Error Classification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Telemetry for the interactive
dhq helloonboarding showed 15 internal errors over 30 days that were really user/auth failures dressed up as CLI bugs:.deployhq.com.deployhq.comTLS errors (users typing full hostname at the subdomain prompt)AccessDenied/api_access_restrictedNot foundon the validate-credentials API callhellohas its own copy of the login flow (helloLogin), so the fixes in PR #15 and PR #16 only partially reach it:TrimSuffix(".deployhq.com")insdk.New— no change needed here.helloLoginwraps the validation error in*output.InternalErrorunconditionally — which beatsClassifyError's status-code fallback (typed errors win first).Changes
Account subdomain (e.g. 'mycompany' for mycompany.deployhq.com):— same example shown in the--accountflag help, lifted into the moment it's needed.classifyHelloValidateErrthat maps the validation API error into:*output.AuthErrorfor 401, 403 (with helpful hints for each)*output.UserErrorfor 404 (account-not-found hint)*output.NetworkErrorfor network failuresnilfor everything else, so the raw error passes through tooutput.ClassifyError's status-code fallback (once PR fix(errors): classify API errors by status code, not as internal #16 lands) — falls back to today's behavior if it doesn't.errors.As, so the change is self-contained and doesn't depend onsdk.IsForbidden(added in PR fix(errors): classify API errors by status code, not as internal #16).Test plan
go build,go vet,go test,golangci-lintTestClassifyHelloValidateErrcovers 401/403 (×2)/404/500/422/network/wrapped-403/generic — 9 cases.dhq hello, enter a bad email/key → see "Invalid credentials" with hint, not generic error.dhq helloon an account with API access disabled → see "Access denied" hint.Note on the 192s p90 duration
The earlier finding that
hellohas a 192s p90 (and 226-minute max) is interactive-UX latency, not a bug — users take time finding the API key in the dashboard. Not actionable here.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes