fix(errors): classify API errors by status code, not as internal#16
Conversation
…as internal Telemetry was showing inflated internal-error rates on commands that were actually failing on user-supplied bad input. Examples from last 30d: - deployments show: 73/74 errors were 404 record_not_found (bad deployment id) — classified as internal. - deployments watch: 34/34 errors were 404 — classified as internal. - deployments create: 3/3 errors were 422 parent must exist — classified as internal. - api: 9 errors were 404 Not Found, 1 was 400 missing param, 2 were 422 parent must exist — all classified as internal. - auth login: 11 errors were 403 AccessDenied and 4 were 403 api_access_restricted — classified as internal because the auth login special-case only catches 401. Root cause: most commands return the raw *sdk.APIError directly, and ClassifyError has no case for it, so the default ExitInternalError fires. The dashboard's "internal" bucket therefore confuses real CLI bugs with user input errors and access denials. Changes: - ClassifyError uses errors.As to find a wrapped *sdk.APIError, then maps 401/403 -> auth, other 4xx -> user, 5xx -> internal. Typed errors that the command code chose explicitly still win, preserving the existing override path. - sdk.IsForbidden added as a package-level helper mirroring IsUnauthorized. - runAuthLogin now classifies 403 as AuthError with a hint covering the two observed causes (account API access disabled, user not in account). - Tests cover 401/403/404/409/422/500/503 and wrapping via fmt.Errorf. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 2 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe PR adds HTTP 403 Forbidden error handling across the CLI stack. It introduces ChangesForbidden Response Recognition and Classification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 630428b752
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if errors.As(err, &apiErr) { | ||
| switch { | ||
| case apiErr.StatusCode == http.StatusUnauthorized, | ||
| apiErr.StatusCode == http.StatusForbidden: | ||
| return ExitAuthError | ||
| case apiErr.StatusCode >= 400 && apiErr.StatusCode < 500: | ||
| return ExitUserError |
There was a problem hiding this comment.
When a command returns a raw *sdk.APIError in --json/piped mode, main builds the envelope with ErrorResponseFromErr, which only sets Data.Code for *UserError, *AuthError, and *InternalError and otherwise leaves it as the default "error" while taking exit_code from ClassifyError. This new status-code branch therefore produces inconsistent JSON such as code: "error", exit_code: 3 for 401/403 or code: "error", exit_code: 1 for 4xx API failures, so agents that follow the documented auth_error/user_error codes will miss these newly reclassified cases; please mirror the API status mapping in the JSON error response as well.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/output/errors_test.go`:
- Around line 166-171: The test currently only classifies a plain UserError;
instead construct a single error value that actually contains both types (e.g.,
wrap the *sdk.APIError inside the UserError or wrap the UserError around the
*sdk.APIError using fmt.Errorf("%w", ...) / errors.Wrap so both are present) and
then call ClassifyError on that combined error and assert it returns
ExitUserError; update TestClassifyError_TypedErrorWinsOverAPIError to build that
wrapped error (referencing UserError, *sdk.APIError and ClassifyError) and
assert precedence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bdbc3fc-d097-41ce-98f5-8edd55aa8175
📒 Files selected for processing (4)
internal/commands/auth.gointernal/output/errors.gointernal/output/errors_test.gopkg/sdk/errors.go
…test Addresses two review findings on PR #16: 1. Codex (P2): ErrorResponseFromErr left code="error" for raw *sdk.APIError, producing inconsistent JSON like {code:"error", exit_code:3} for 401/403 — agents that follow the documented auth_error/user_error/not_found/conflict codes would miss the newly reclassified cases. Mirror the same status-code mapping there so the JSON code field and exit_code agree. Bonus: added a NetworkError case while in there — it was also falling through to "error" despite getting the correct exit code. 2. CodeRabbit: TestClassifyError_TypedErrorWinsOverAPIError never actually exercised precedence — apiErr was unused. Given the current types, there's no way to construct a single value that's both *UserError and reachable as *sdk.APIError via errors.As (none of the typed errors implement Unwrap), so the test was structurally vacuous. Remove it; TestUserError / TestAuthError / TestNetworkError already prove the type switch fires first. Also caught an inconsistency between paths: ExitNotFoundError (5) and ExitConflictError (6) already exist and exitCodeForErrorCode returns them on the JSON code path, but my original ClassifyError APIError mapping was returning ExitUserError (1) for both. Align ClassifyError with the dedicated exit codes so 404/409 produce 5/6 through both paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#18) 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>
1 participant
Summary
Telemetry analysis showed the dashboard's
internalerror bucket was inflated by user-input failures and access-denied errors. Examples from last 30 days:deployments show: 73 of 74 internal errors were404 record_not_found(user passed bad deployment id).deployments watch: 34 of 34 were404 record_not_found.deployments create: 3 of 3 were422 parent must exist.api: 9 were404 Not Found, 1 was400 missing param, 2 were422 parent must exist.auth login: 11 were403 AccessDeniedand 4 were403 api_access_restricted.Root cause: Most commands return the raw
*sdk.APIErrordirectly.ClassifyErrorhas no case for it, so the defaultExitInternalErrorfires. The dashboard therefore mixes real CLI bugs with user input errors and access denials.Changes
output.ClassifyErrornow useserrors.Asto find a wrapped*sdk.APIErrorafter the typed-error switch. Mapping:401,403→AuthError4xx(404/409/422/…) →UserError5xx→InternalErrorsdk.IsForbiddenadded as a package-level helper, mirroringIsUnauthorized.runAuthLoginclassifies403asAuthErrorwith a hint that covers the two observed causes (account API access disabled, user not in account).fmt.Errorf.Expected dashboard impact
Roughly ~150 events that were classified as
internalin the last 30 days should move touser(110+) orauth(15+), giving a much cleaner signal for real CLI bugs going forward.Test plan
go build ./...,go vet ./...,go test ./...,golangci-lint runTestClassifyError_APIErrorByStatuscovers 401, 403, 404, 409, 422, 500, 503.TestClassifyError_APIErrorWrappedcovers wrapping viafmt.Errorf(errors.Aschain).TestClassifyError_TypedErrorWinsOverAPIErrorconfirms typed errors still take precedence.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests