fix/improve-error-messages

Author: chrapkowski-sg

cmd/src/code_intel_upload.go

@@ -71,7 +71,7 @@ func handleCodeIntelUpload(args []string) error {
 		}
 	}
 	if err != nil {
-		return handleUploadError(cfg.AccessToken, err)
+		return err
 	}
 
 	client := cfg.apiClient(codeintelUploadFlags.apiFlags, io.Discard)
@@ -212,94 +212,93 @@ func (e errorWithHint) Error() string {
 	return fmt.Sprintf("%s\n\n%s\n", e.err, e.hint)
 }
 
-// handleUploadError writes the given error to the given output. If the
-// given output object is nil then the error will be written to standard out.
-//
-// This method returns the error that should be passed back up to the runner.
+// handleUploadError attaches actionable hints to upload errors and returns
+// the enriched error. Only called for actual upload failures (not flag validation).
 func handleUploadError(accessToken string, err error) error {
-	if errors.Is(err, upload.ErrUnauthorized) {
-		err = attachHintsForAuthorizationError(accessToken, err)
+	var httpErr *ErrUnexpectedStatusCode
+	if errors.As(err, &httpErr) && (httpErr.Code == 401 || httpErr.Code == 403) {
+		isUnauthorized := httpErr.Code == 401
+		isForbidden := httpErr.Code == 403
+
+		displayErr := errors.Newf("upload failed: %s", uploadFailureReason(httpErr))
+
+		err = errorWithHint{
+			err:  displayErr,
+			hint: uploadHints(accessToken, isUnauthorized, isForbidden),
+		}
 	}
 
 	if codeintelUploadFlags.ignoreUploadFailures {
-		// Report but don't return the error
 		fmt.Println(err.Error())
 		return nil
 	}
 
 	return err
 }
 
-func attachHintsForAuthorizationError(accessToken string, originalError error) error {
-	var actionableHints []string
-
-	likelyTokenError := accessToken == ""
-	if _, parseErr := accesstoken.ParsePersonalAccessToken(accessToken); accessToken != "" && parseErr != nil {
-		likelyTokenError = true
-		actionableHints = append(actionableHints,
-			"However, the provided access token does not match expected format; was it truncated?",
-			"Typically the access token looks like sgp_<40 hex chars> or sgp_<instance-id>_<40 hex chars>.")
-	}
-
-	if likelyTokenError {
-		return errorWithHint{err: originalError, hint: strings.Join(mergeStringSlices(
-			[]string{"A Sourcegraph access token must be provided via SRC_ACCESS_TOKEN for uploading SCIP data."},
-			actionableHints,
-			[]string{"For more details, see https://sourcegraph.com/docs/cli/how-tos/creating_an_access_token."},
-		), "\n")}
+// uploadHints builds two independent hint blocks: one for the Sourcegraph
+// access token and one for code host tokens. Both are always included.
+func uploadHints(accessToken string, isUnauthorized, isForbidden bool) string {
+	var hints []string
+
+	// Paragraph 1: Sourcegraph access token
+	if isUnauthorized {
+		if accessToken == "" {
+			hints = append(hints, "No Sourcegraph access token was provided. Set the SRC_ACCESS_TOKEN environment variable to a valid token.")
+		} else if _, parseErr := accesstoken.ParsePersonalAccessToken(accessToken); parseErr != nil {
+			hints = append(hints, "The provided Sourcegraph access token does not match the expected format (sgp_<40 hex chars> or sgp_<instance-id>_<40 hex chars>). Was it copied incorrectly or truncated?")
+		} else {
+			hints = append(hints, "The Sourcegraph access token may be invalid, expired, or you may be connecting to the wrong Sourcegraph instance.")
+		}
+	} else if isForbidden {
+		hints = append(hints, "You may not have sufficient permissions on this Sourcegraph instance.")
 	}
 
-	needsGitHubToken := strings.HasPrefix(codeintelUploadFlags.repo, "github.com")
-	needsGitLabToken := strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com")
-
-	if needsGitHubToken {
-		if codeintelUploadFlags.gitHubToken != "" {
-			actionableHints = append(actionableHints,
-				fmt.Sprintf("The supplied -github-token does not indicate that you have collaborator access to %s.", codeintelUploadFlags.repo),
-				"Please check the value of the supplied token and its permissions on the code host and try again.",
-			)
+	// Paragraph 2: Code host token
+	if codeintelUploadFlags.gitHubToken != "" {
+		if isUnauthorized {
+			hints = append(hints, "The supplied -github-token may be invalid.")
 		} else {
-			actionableHints = append(actionableHints,
-				fmt.Sprintf("Please retry your request with a -github-token=XXX with collaborator access to %s.", codeintelUploadFlags.repo),
-				"This token will be used to check with the code host that the uploading user has write access to the target repository.",
-			)
+			hints = append(hints, "The supplied -github-token may lack the required permissions.")
 		}
-	} else if needsGitLabToken {
-		if codeintelUploadFlags.gitLabToken != "" {
-			actionableHints = append(actionableHints,
-				fmt.Sprintf("The supplied -gitlab-token does not indicate that you have write access to %s.", codeintelUploadFlags.repo),
-				"Please check the value of the supplied token and its permissions on the code host and try again.",
-			)
+	} else if strings.HasPrefix(codeintelUploadFlags.repo, "github.com") {
+		hints = append(hints, fmt.Sprintf("No -github-token was provided. If this Sourcegraph instance enforces code host authentication, retry with -github-token=<token> for a token with access to %s.", codeintelUploadFlags.repo))
+	}
+
+	if codeintelUploadFlags.gitLabToken != "" {
+		if isUnauthorized {
+			hints = append(hints, "The supplied -gitlab-token may be invalid.")
 		} else {
-			actionableHints = append(actionableHints,
-				fmt.Sprintf("Please retry your request with a -gitlab-token=XXX with write access to %s.", codeintelUploadFlags.repo),
-				"This token will be used to check with the code host that the uploading user has write access to the target repository.",
-			)
+			hints = append(hints, "The supplied -gitlab-token may lack the required permissions.")
 		}
-	} else {
-		actionableHints = append(actionableHints,
-			"Verification is supported for the following code hosts: github.com, gitlab.com.",
-			"Please request support for additional code host verification at https://github.com/sourcegraph/sourcegraph/issues/4967.",
-		)
+	} else if strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com") {
+		hints = append(hints, fmt.Sprintf("No -gitlab-token was provided. If this Sourcegraph instance enforces code host authentication, retry with -gitlab-token=<token> for a token with access to %s.", codeintelUploadFlags.repo))
 	}
 
-	return errorWithHint{err: originalError, hint: strings.Join(mergeStringSlices(
-		[]string{"This Sourcegraph instance has enforced auth for SCIP uploads."},
-		actionableHints,
-		[]string{"For more details, see https://docs.sourcegraph.com/cli/references/code-intel/upload."},
-	), "\n")}
-}
+	if codeintelUploadFlags.gitHubToken == "" && codeintelUploadFlags.gitLabToken == "" &&
+		!strings.HasPrefix(codeintelUploadFlags.repo, "github.com") &&
+		!strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com") {
+		hints = append(hints, "Code host verification is supported for github.com and gitlab.com repositories.")
+	}
 
-// emergencyOutput creates a default Output object writing to standard out.
-func emergencyOutput() *output.Output {
-	return output.NewOutput(os.Stdout, output.OutputOpts{})
+	hints = append(hints, "For more details on uploading SCIP indexes, see https://sourcegraph.com/docs/cli/references/code-intel/upload.")
+
+	return strings.Join(hints, "\n\n")
 }
 
-func mergeStringSlices(ss ...[]string) []string {
-	var combined []string
-	for _, s := range ss {
-		combined = append(combined, s...)
+// uploadFailureReason returns the server's response body if available, or a
+// generic reason derived from the HTTP status code.
+func uploadFailureReason(httpErr *ErrUnexpectedStatusCode) string {
+	if httpErr.Body != "" {
+		return httpErr.Body
+	}
+	if httpErr.Code == 401 {
+		return "unauthorized"
 	}
+	return "forbidden"
+}
 
-	return combined
+// emergencyOutput creates a default Output object writing to standard out.
+func emergencyOutput() *output.Output {
+	return output.NewOutput(os.Stdout, output.OutputOpts{})
 }

cmd/src/code_intel_upload_vendored.go

@@ -339,8 +339,20 @@ type uploadRequestOptions struct {
 	Done             bool      // Whether the request is a multipart finalize
 }
 
-// ErrUnauthorized occurs when the upload endpoint returns a 401 response.
-var ErrUnauthorized = errors.New("unauthorized upload")
+// ErrUnexpectedStatusCode is returned for HTTP error responses from the upload
+// endpoint. It carries the status code and response body so callers can inspect
+// them without parsing the error string.
+type ErrUnexpectedStatusCode struct {
+	Code int
+	Body string
+}
+
+func (e *ErrUnexpectedStatusCode) Error() string {
+	if e.Body != "" {
+		return fmt.Sprintf("unexpected status code: %d (%s)", e.Code, e.Body)
+	}
+	return fmt.Sprintf("unexpected status code: %d", e.Code)
+}
 
 // performUploadRequest performs an HTTP POST to the upload endpoint. The query string of the request
 // is constructed from the given request options and the body of the request is the unmodified reader.
@@ -419,17 +431,13 @@ func performRequest(ctx context.Context, req *http.Request, httpClient upload.Cl
 // returns a boolean flag indicating if the function can be retried on failure (error-dependent).
 func decodeUploadPayload(resp *http.Response, body []byte, target *int) (bool, error) {
 	if resp.StatusCode >= 300 {
-		if resp.StatusCode == http.StatusUnauthorized {
-			return false, ErrUnauthorized
-		}
-
-		suffix := ""
-		if !bytes.HasPrefix(bytes.TrimSpace(body), []byte{'<'}) {
-			suffix = fmt.Sprintf(" (%s)", bytes.TrimSpace(body))
+		detail := ""
+		if trimmed := bytes.TrimSpace(body); len(trimmed) > 0 && trimmed[0] != '<' {
+			detail = string(trimmed)
 		}
 
 		// Do not retry client errors
-		return resp.StatusCode >= 500, errors.Errorf("unexpected status code: %d%s", resp.StatusCode, suffix)
+		return resp.StatusCode >= 500, &ErrUnexpectedStatusCode{Code: resp.StatusCode, Body: detail}
 	}
 
 	if target == nil {