From 630428b7521adaf27ff065976c02778159e0fd11 Mon Sep 17 00:00:00 2001 From: Facundo Farias Date: Fri, 29 May 2026 08:24:03 +0200 Subject: [PATCH 1/2] fix(errors): classify API errors by status code instead of bucketing as internal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- internal/commands/auth.go | 6 ++++++ internal/output/errors.go | 20 +++++++++++++++++ internal/output/errors_test.go | 39 ++++++++++++++++++++++++++++++++++ pkg/sdk/errors.go | 8 +++++++ 4 files changed, 73 insertions(+) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 48f9b5c..d432f69 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -136,6 +136,12 @@ func runAuthLogin(opts *AuthLoginOptions) error { Hint: "Check your email and API key at Profile > API Key in DeployHQ", } } + if sdk.IsForbidden(err) { + return &output.AuthError{ + Message: "Access denied", + Hint: "Your account may have API access restricted, or your user may not be a member of this account. Check Account Settings > API.", + } + } if output.IsNetworkErr(err) { return &output.NetworkError{Message: "validate credentials", Cause: err} } diff --git a/internal/output/errors.go b/internal/output/errors.go index 81af8fd..4c5a0b5 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -10,8 +10,11 @@ import ( "errors" "fmt" "net" + "net/http" "net/url" "syscall" + + "github.com/deployhq/deployhq-cli/pkg/sdk" ) // ExitCode constants for error classification. @@ -121,6 +124,11 @@ func IsNetworkErr(err error) bool { } // ClassifyError returns the appropriate exit code for an error. +// +// Precedence: typed errors (UserError, InternalError, etc.) the command code +// chose explicitly always win. Raw *sdk.APIError values returned without +// wrapping fall back to a status-code mapping so 4xx doesn't get reported as +// an internal CLI bug. func ClassifyError(err error) int { if err == nil { return ExitOK @@ -135,6 +143,18 @@ func ClassifyError(err error) int { case *NetworkError: return ExitNetworkError } + var apiErr *sdk.APIError + 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 + case apiErr.StatusCode >= 500: + return ExitInternalError + } + } if IsNetworkErr(err) { return ExitNetworkError } diff --git a/internal/output/errors_test.go b/internal/output/errors_test.go index dbe51b7..9f1281d 100644 --- a/internal/output/errors_test.go +++ b/internal/output/errors_test.go @@ -2,12 +2,14 @@ package output import ( "errors" + "fmt" "net" "net/url" "syscall" "testing" "time" + "github.com/deployhq/deployhq-cli/pkg/sdk" "github.com/stretchr/testify/assert" ) @@ -132,6 +134,43 @@ func TestClassifyError_DetectsURLNetworkError(t *testing.T) { assert.Equal(t, ExitNetworkError, ClassifyError(urlErr)) } +func TestClassifyError_APIErrorByStatus(t *testing.T) { + cases := []struct { + status int + want int + name string + }{ + {401, ExitAuthError, "401 unauthorized"}, + {403, ExitAuthError, "403 forbidden"}, + {404, ExitUserError, "404 not found is a user-supplied bad id"}, + {409, ExitUserError, "409 conflict is user state"}, + {422, ExitUserError, "422 validation is bad input"}, + {500, ExitInternalError, "500 server is internal"}, + {503, ExitInternalError, "503 unavailable is internal"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := &sdk.APIError{StatusCode: tc.status, Message: "x"} + assert.Equal(t, tc.want, ClassifyError(err)) + }) + } +} + +func TestClassifyError_APIErrorWrapped(t *testing.T) { + // errors.As should unwrap through a wrapping error to find the APIError. + apiErr := &sdk.APIError{StatusCode: 404, Message: "not found"} + wrapped := fmt.Errorf("fetch project: %w", apiErr) + assert.Equal(t, ExitUserError, ClassifyError(wrapped)) +} + +func TestClassifyError_TypedErrorWinsOverAPIError(t *testing.T) { + // If the command explicitly wraps an API 5xx as a UserError, honor that. + apiErr := &sdk.APIError{StatusCode: 500, Message: "boom"} + wrapped := &UserError{Message: "you triggered the bug", Hint: "blah"} + _ = apiErr // sanity: classification of the typed error doesn't peek inside + assert.Equal(t, ExitUserError, ClassifyError(wrapped)) +} + // Sanity check: a deadline-style error wrapped in net.Error.Timeout() classifies // even when accessed through deadline-exceeded chains used by net/http. func TestIsNetworkErr_DeadlineExceededViaNetError(t *testing.T) { diff --git a/pkg/sdk/errors.go b/pkg/sdk/errors.go index 8f2d224..0012f1f 100644 --- a/pkg/sdk/errors.go +++ b/pkg/sdk/errors.go @@ -69,3 +69,11 @@ func IsUnauthorized(err error) bool { } return false } + +// IsForbidden checks whether err is an APIError with status 403. +func IsForbidden(err error) bool { + if apiErr, ok := err.(*APIError); ok { + return apiErr.IsForbidden() + } + return false +} From 269dbc70bd320b1520f9649df19bccab64727ef7 Mon Sep 17 00:00:00 2001 From: Facundo Farias Date: Fri, 29 May 2026 08:43:04 +0200 Subject: [PATCH 2/2] fix(errors): mirror status mapping in JSON envelope, drop misleading test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- internal/output/breadcrumbs.go | 33 ++++++++++++++++++++++++++++- internal/output/breadcrumbs_test.go | 33 +++++++++++++++++++++++++++++ internal/output/errors.go | 4 ++++ internal/output/errors_test.go | 14 +++--------- 4 files changed, 72 insertions(+), 12 deletions(-) diff --git a/internal/output/breadcrumbs.go b/internal/output/breadcrumbs.go index a6d0e57..1aff631 100644 --- a/internal/output/breadcrumbs.go +++ b/internal/output/breadcrumbs.go @@ -1,6 +1,12 @@ package output -import "strings" +import ( + "errors" + "net/http" + "strings" + + "github.com/deployhq/deployhq-cli/pkg/sdk" +) // Breadcrumb represents a suggested next action. type Breadcrumb struct { @@ -98,6 +104,31 @@ func ErrorResponseFromErr(err error) *Response { case *InternalError: code = "internal_error" message = e.Message + case *NetworkError: + code = "network_error" + message = e.Message + } + + // Mirror the status-code mapping from ClassifyError so a raw *sdk.APIError + // returned without wrapping produces a `code` field consistent with the + // exit_code agents already follow. + if code == "error" { + var apiErr *sdk.APIError + if errors.As(err, &apiErr) { + switch { + case apiErr.StatusCode == http.StatusUnauthorized, + apiErr.StatusCode == http.StatusForbidden: + code = "auth_error" + case apiErr.StatusCode == http.StatusNotFound: + code = "not_found" + case apiErr.StatusCode == http.StatusConflict: + code = "conflict" + case apiErr.StatusCode >= 400 && apiErr.StatusCode < 500: + code = "user_error" + case apiErr.StatusCode >= 500: + code = "internal_error" + } + } } // Enrich errors with actionable suggestions when no hint is set diff --git a/internal/output/breadcrumbs_test.go b/internal/output/breadcrumbs_test.go index 126c35b..09f16b8 100644 --- a/internal/output/breadcrumbs_test.go +++ b/internal/output/breadcrumbs_test.go @@ -2,8 +2,10 @@ package output import ( "encoding/json" + "net/http" "testing" + "github.com/deployhq/deployhq-cli/pkg/sdk" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -116,6 +118,37 @@ func TestErrorResponseFromErr_Retryable(t *testing.T) { assert.True(t, data.Retryable) } +func TestErrorResponseFromErr_APIErrorByStatus(t *testing.T) { + cases := []struct { + status int + wantCode string + wantExit int + }{ + {401, "auth_error", ExitAuthError}, + {403, "auth_error", ExitAuthError}, + {404, "not_found", ExitNotFoundError}, + {409, "conflict", ExitConflictError}, + {422, "user_error", ExitUserError}, + {500, "internal_error", ExitInternalError}, + {503, "internal_error", ExitInternalError}, + } + for _, tc := range cases { + t.Run(http.StatusText(tc.status), func(t *testing.T) { + err := &sdk.APIError{StatusCode: tc.status, Message: "x"} + data := ErrorResponseFromErr(err).Data.(ErrorData) + assert.Equal(t, tc.wantCode, data.Code, "code mismatch") + assert.Equal(t, tc.wantExit, data.ExitCode, "exit_code mismatch") + }) + } +} + +func TestErrorResponseFromErr_NetworkError(t *testing.T) { + err := &NetworkError{Message: "validate credentials"} + data := ErrorResponseFromErr(err).Data.(ErrorData) + assert.Equal(t, "network_error", data.Code) + assert.Equal(t, ExitNetworkError, data.ExitCode) +} + func TestErrorData_JSONShape(t *testing.T) { resp := ErrorResponseFromErr(&UserError{Message: "test error"}) diff --git a/internal/output/errors.go b/internal/output/errors.go index 4c5a0b5..e984caa 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -149,6 +149,10 @@ func ClassifyError(err error) int { case apiErr.StatusCode == http.StatusUnauthorized, apiErr.StatusCode == http.StatusForbidden: return ExitAuthError + case apiErr.StatusCode == http.StatusNotFound: + return ExitNotFoundError + case apiErr.StatusCode == http.StatusConflict: + return ExitConflictError case apiErr.StatusCode >= 400 && apiErr.StatusCode < 500: return ExitUserError case apiErr.StatusCode >= 500: diff --git a/internal/output/errors_test.go b/internal/output/errors_test.go index 9f1281d..5b4c418 100644 --- a/internal/output/errors_test.go +++ b/internal/output/errors_test.go @@ -142,8 +142,8 @@ func TestClassifyError_APIErrorByStatus(t *testing.T) { }{ {401, ExitAuthError, "401 unauthorized"}, {403, ExitAuthError, "403 forbidden"}, - {404, ExitUserError, "404 not found is a user-supplied bad id"}, - {409, ExitUserError, "409 conflict is user state"}, + {404, ExitNotFoundError, "404 not found uses dedicated exit code"}, + {409, ExitConflictError, "409 conflict uses dedicated exit code"}, {422, ExitUserError, "422 validation is bad input"}, {500, ExitInternalError, "500 server is internal"}, {503, ExitInternalError, "503 unavailable is internal"}, @@ -160,15 +160,7 @@ func TestClassifyError_APIErrorWrapped(t *testing.T) { // errors.As should unwrap through a wrapping error to find the APIError. apiErr := &sdk.APIError{StatusCode: 404, Message: "not found"} wrapped := fmt.Errorf("fetch project: %w", apiErr) - assert.Equal(t, ExitUserError, ClassifyError(wrapped)) -} - -func TestClassifyError_TypedErrorWinsOverAPIError(t *testing.T) { - // If the command explicitly wraps an API 5xx as a UserError, honor that. - apiErr := &sdk.APIError{StatusCode: 500, Message: "boom"} - wrapped := &UserError{Message: "you triggered the bug", Hint: "blah"} - _ = apiErr // sanity: classification of the typed error doesn't peek inside - assert.Equal(t, ExitUserError, ClassifyError(wrapped)) + assert.Equal(t, ExitNotFoundError, ClassifyError(wrapped)) } // Sanity check: a deadline-style error wrapped in net.Error.Timeout() classifies