Skip to content

fix(hello): clarify account prompt and surface real validation errors#18

Merged
facundofarias merged 1 commit into
mainfrom
fix/hello-mirror-auth-login-improvements
May 29, 2026
Merged

fix(hello): clarify account prompt and surface real validation errors#18
facundofarias merged 1 commit into
mainfrom
fix/hello-mirror-auth-login-improvements

Conversation

@facundofarias
Copy link
Copy Markdown
Contributor

@facundofarias facundofarias commented May 29, 2026

Summary

Telemetry for the interactive dhq hello onboarding showed 15 internal errors over 30 days that were really user/auth failures dressed up as CLI bugs:

Cause Events
.deployhq.com.deployhq.com TLS errors (users typing full hostname at the subdomain prompt) 8
403 AccessDenied / api_access_restricted 4
404 Not found on the validate-credentials API call 3

hello has its own copy of the login flow (helloLogin), so the fixes in PR #15 and PR #16 only partially reach it:

Changes

  • Clarify the interactive prompt to Account subdomain (e.g. 'mycompany' for mycompany.deployhq.com): — same example shown in the --account flag help, lifted into the moment it's needed.
  • Extract classifyHelloValidateErr that maps the validation API error into:
    • *output.AuthError for 401, 403 (with helpful hints for each)
    • *output.UserError for 404 (account-not-found hint)
    • *output.NetworkError for network failures
    • nil for everything else, so the raw error passes through to output.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.
  • Uses errors.As, so the change is self-contained and doesn't depend on sdk.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-lint
  • TestClassifyHelloValidateErr covers 401/403 (×2)/404/500/422/network/wrapped-403/generic — 9 cases.
  • Smoke: run dhq hello, enter a bad email/key → see "Invalid credentials" with hint, not generic error.
  • Smoke: run dhq hello on an account with API access disabled → see "Access denied" hint.

Note on the 192s p90 duration

The earlier finding that hello has 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

    • Added example format to the Account subdomain prompt in the interactive login flow.
  • Bug Fixes

    • Enhanced credential validation error messages with improved classification for authentication, user, and network errors.

Review Change Stack

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>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8dedfac1-d3b4-4ade-9379-fd76be45dfc0

📥 Commits

Reviewing files that changed from the base of the PR and between 0387838 and 9547b06.

📒 Files selected for processing (2)
  • internal/commands/hello.go
  • internal/commands/hello_test.go

Walkthrough

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

Changes

Login Validation Error Classification

Layer / File(s) Summary
Error classification helper and tests
internal/commands/hello.go, internal/commands/hello_test.go
Imports errors and net/http to support typed error inspection. Adds classifyHelloValidateErr helper that maps API HTTP status codes (401/403/404) and network errors to typed *output.AuthError, *output.UserError, or *output.NetworkError; returns nil to signal pass-through of the original error. Includes table-driven test covering sdk.APIError status codes, wrapped errors via fmt.Errorf, and net.OpError network errors.
Validation flow and prompt integration
internal/commands/hello.go
Replaces inline credential-validation error branching with a call to classifyHelloValidateErr; typed errors are handled, otherwise the original error propagates. Updates the "Account subdomain" login prompt to include an example subdomain format.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the two main changes: clarifying the account subdomain prompt and improving error classification for validation failures.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/hello-mirror-auth-login-improvements

Comment @coderabbitai help to get the list of available commands and usage tips.

@facundofarias facundofarias merged commit c5cb8e5 into main May 29, 2026
3 checks passed
@facundofarias facundofarias deleted the fix/hello-mirror-auth-login-improvements branch May 29, 2026 06:47
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.

1 participant