[feat] Agent friendly error handling

Author: nieblara

internal/errors/suggestions.go

@@ -0,0 +1,24 @@
+package errors
+
+import "strings"
+
+var suggestions = map[int]string{
+	401: "Your access token may be invalid or expired. Run `ldcli login` or set LD_ACCESS_TOKEN. " +
+		"Create a new token at {baseURI}/settings/authorization.",
+	403: "You don't have permission for this action. Check your access token's role " +
+		"and custom role policies in LaunchDarkly settings.",
+	404: "Resource not found. Verify the project key, flag key, or environment key. " +
+		"Use `ldcli projects list` or `ldcli flags list` to see available resources.",
+	409: "Conflict: the resource may have been modified since you last read it. " +
+		"Fetch the latest version and retry.",
+	429: "Rate limited. Wait a moment and retry. If this persists, check your account's " +
+		"rate limit allocation.",
+}
+
+func SuggestionForStatus(statusCode int, baseURI string) string {
+	s, ok := suggestions[statusCode]
+	if !ok {
+		return ""
+	}
+	return strings.ReplaceAll(s, "{baseURI}", baseURI)
+}

internal/errors/suggestions_test.go

@@ -0,0 +1,75 @@
+package errors_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/launchdarkly/ldcli/internal/errors"
+)
+
+func TestSuggestionForStatus(t *testing.T) {
+	t.Run("401 returns access token suggestion with baseURI", func(t *testing.T) {
+		result := errors.SuggestionForStatus(401, "https://app.launchdarkly.com")
+
+		assert.Contains(t, result, "ldcli login")
+		assert.Contains(t, result, "LD_ACCESS_TOKEN")
+		assert.Contains(t, result, "https://app.launchdarkly.com/settings/authorization")
+		assert.NotContains(t, result, "{baseURI}")
+	})
+
+	t.Run("403 returns permission suggestion", func(t *testing.T) {
+		result := errors.SuggestionForStatus(403, "https://app.launchdarkly.com")
+
+		assert.Contains(t, result, "permission")
+		assert.Contains(t, result, "custom role")
+	})
+
+	t.Run("404 returns resource not found suggestion", func(t *testing.T) {
+		result := errors.SuggestionForStatus(404, "https://app.launchdarkly.com")
+
+		assert.Contains(t, result, "not found")
+		assert.Contains(t, result, "ldcli projects list")
+		assert.Contains(t, result, "ldcli flags list")
+	})
+
+	t.Run("409 returns conflict suggestion", func(t *testing.T) {
+		result := errors.SuggestionForStatus(409, "https://app.launchdarkly.com")
+
+		assert.Contains(t, result, "Conflict")
+		assert.Contains(t, result, "latest version")
+	})
+
+	t.Run("429 returns rate limit suggestion", func(t *testing.T) {
+		result := errors.SuggestionForStatus(429, "https://app.launchdarkly.com")
+
+		assert.Contains(t, result, "Rate limited")
+		assert.Contains(t, result, "retry")
+	})
+
+	t.Run("unknown status code returns empty string", func(t *testing.T) {
+		result := errors.SuggestionForStatus(502, "https://app.launchdarkly.com")
+
+		assert.Empty(t, result)
+	})
+
+	t.Run("200 returns empty string", func(t *testing.T) {
+		result := errors.SuggestionForStatus(200, "https://app.launchdarkly.com")
+
+		assert.Empty(t, result)
+	})
+
+	t.Run("replaces baseURI placeholder with actual value", func(t *testing.T) {
+		result := errors.SuggestionForStatus(401, "https://custom.ld.example.com")
+
+		assert.Contains(t, result, "https://custom.ld.example.com/settings/authorization")
+		assert.NotContains(t, result, "{baseURI}")
+	})
+
+	t.Run("empty baseURI does not panic", func(t *testing.T) {
+		result := errors.SuggestionForStatus(401, "")
+
+		assert.Contains(t, result, "ldcli login")
+		assert.Contains(t, result, "/settings/authorization")
+	})
+}

internal/output/output_test.go

@@ -71,6 +71,21 @@ func TestCmdOutputSingular(t *testing.T) {
 			fn:       output.ErrorPlaintextOutputFn,
 			input:    `{}`,
 		},
+		"with an error with a suggestion": {
+			expected: "Not Found (code: not_found)\nSuggestion: Check the resource key.",
+			fn:       output.ErrorPlaintextOutputFn,
+			input:    `{"code": "not_found", "message": "Not Found", "suggestion": "Check the resource key."}`,
+		},
+		"with an error with an empty suggestion": {
+			expected: "Internal Server Error (code: internal_server_error)",
+			fn:       output.ErrorPlaintextOutputFn,
+			input:    `{"code": "internal_server_error", "message": "Internal Server Error", "suggestion": ""}`,
+		},
+		"with an error with only a message and a suggestion": {
+			expected: "an error\nSuggestion: Try again.",
+			fn:       output.ErrorPlaintextOutputFn,
+			input:    `{"message": "an error", "suggestion": "Try again."}`,
+		},
 		"with a singular resource": {
 			expected: "test-name (test-key)",
 			fn:       output.SingularPlaintextOutputFn,

internal/output/plaintext_fns.go

@@ -28,16 +28,24 @@ var ConfigPlaintextOutputFn = func(r resource) string {
 // An error response could have a code and message or just a message. It's also possible that
 // there isn't either property.
 var ErrorPlaintextOutputFn = func(r resource) string {
+	var parts []string
+
 	switch {
 	case r["code"] == nil && (r["message"] == "" || r["message"] == nil):
-		return "unknown error occurred"
+		parts = append(parts, "unknown error occurred")
 	case r["code"] == nil:
-		return r["message"].(string)
+		parts = append(parts, fmt.Sprint(r["message"]))
 	case r["message"] == "":
-		return fmt.Sprintf("an error occurred (code: %s)", r["code"])
+		parts = append(parts, fmt.Sprintf("an error occurred (code: %v)", r["code"]))
 	default:
-		return fmt.Sprintf("%s (code: %s)", r["message"], r["code"])
+		parts = append(parts, fmt.Sprintf("%v (code: %v)", r["message"], r["code"]))
+	}
+
+	if suggestion, ok := r["suggestion"]; ok && suggestion != nil && suggestion != "" {
+		parts = append(parts, fmt.Sprintf("\nSuggestion: %s", suggestion))
 	}
+
+	return strings.Join(parts, "")
 }
 
 // MultiplePlaintextOutputFn converts the resource to plain text.

internal/output/plaintext_fns_internal_test.go

@@ -6,6 +6,32 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+func TestErrorPlaintextOutputFn(t *testing.T) {
+	t.Run("with a non-string message does not panic", func(t *testing.T) {
+		r := resource{"message": float64(404)}
+
+		result := ErrorPlaintextOutputFn(r)
+
+		assert.Equal(t, "404", result)
+	})
+
+	t.Run("with a non-string code renders via fmt formatting", func(t *testing.T) {
+		r := resource{"code": 123, "message": "an error"}
+
+		result := ErrorPlaintextOutputFn(r)
+
+		assert.Equal(t, "an error (code: 123)", result)
+	})
+
+	t.Run("with non-string message and code does not panic", func(t *testing.T) {
+		r := resource{"code": true, "message": 42}
+
+		result := ErrorPlaintextOutputFn(r)
+
+		assert.Equal(t, "42 (code: true)", result)
+	})
+}
+
 func TestSingularPlaintextOutputFn(t *testing.T) {
 	tests := map[string]struct {
 		resource resource

internal/output/resource_output_test.go

@@ -360,4 +360,38 @@ func TestCmdOutputError(t *testing.T) {
 			assert.Equal(t, "something went wrong", result)
 		})
 	})
+
+	t.Run("with an error containing statusCode and suggestion", func(t *testing.T) {
+		errJSON := `{"code":"not_found","message":"Not Found","statusCode":404,"suggestion":"Resource not found. Use ldcli flags list."}`
+
+		t.Run("with JSON output includes all fields", func(t *testing.T) {
+			err := errors.NewError(errJSON)
+
+			result := output.CmdOutputError("json", err)
+
+			assert.JSONEq(t, errJSON, result)
+		})
+
+		t.Run("with plaintext output includes suggestion line", func(t *testing.T) {
+			err := errors.NewError(errJSON)
+
+			result := output.CmdOutputError("plaintext", err)
+
+			assert.Contains(t, result, "Not Found (code: not_found)")
+			assert.Contains(t, result, "\nSuggestion: Resource not found. Use ldcli flags list.")
+		})
+	})
+
+	t.Run("with an error with statusCode but no suggestion", func(t *testing.T) {
+		errJSON := `{"code":"internal_server_error","message":"Internal Server Error","statusCode":500}`
+
+		t.Run("with plaintext output does not include suggestion line", func(t *testing.T) {
+			err := errors.NewError(errJSON)
+
+			result := output.CmdOutputError("plaintext", err)
+
+			assert.Equal(t, "Internal Server Error (code: internal_server_error)", result)
+			assert.NotContains(t, result, "Suggestion:")
+		})
+	})
 }

internal/resources/client.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"strings"
 
 	"github.com/launchdarkly/ldcli/internal/errors"
 )
@@ -73,18 +74,39 @@ func (c ResourcesClient) MakeRequest(
 		return body, nil
 	}
 
+	var baseURI string
+	if parsed, err := url.Parse(path); err == nil && parsed.Scheme != "" {
+		baseURI = parsed.Scheme + "://" + parsed.Host
+	}
+
 	if len(body) > 0 {
+		var errMap map[string]interface{}
+		if err := json.Unmarshal(body, &errMap); err != nil {
+			errMap = map[string]interface{}{
+				"code":       strings.ToLower(strings.ReplaceAll(http.StatusText(res.StatusCode), " ", "_")),
+				"message":    string(body),
+				"statusCode": res.StatusCode,
+			}
+		} else {
+			if _, exists := errMap["statusCode"]; !exists {
+				errMap["statusCode"] = res.StatusCode
+			}
+		}
+		if suggestion := errors.SuggestionForStatus(res.StatusCode, baseURI); suggestion != "" {
+			errMap["suggestion"] = suggestion
+		}
+		body, _ = json.Marshal(errMap)
 		return body, errors.NewError(string(body))
 	}
 
-	switch res.StatusCode {
-	case http.StatusMethodNotAllowed:
-		resp, _ := json.Marshal(map[string]string{
-			"code":    "method_not_allowed",
-			"message": "method not allowed",
-		})
-		return body, errors.NewError(string(resp))
-	default:
-		return body, errors.NewError(fmt.Sprintf("could not complete the request: %d", res.StatusCode))
+	errMap := map[string]interface{}{
+		"code":       strings.ToLower(strings.ReplaceAll(http.StatusText(res.StatusCode), " ", "_")),
+		"message":    http.StatusText(res.StatusCode),
+		"statusCode": res.StatusCode,
+	}
+	if suggestion := errors.SuggestionForStatus(res.StatusCode, baseURI); suggestion != "" {
+		errMap["suggestion"] = suggestion
 	}
+	resp, _ := json.Marshal(errMap)
+	return body, errors.NewError(string(resp))
 }