fix(codeintel): improve upload error hints for auth and permission failures

When upload of a scip index fails, the error message may be misleading or incomplete. The complexity comes from two aspects. First that we have 2 tokens - sourcegraph access token and code host token. This PR makes sure that whenever a token is invalid we get most probable causes. Second part is that even if a token is valid on its own, the user may not have sufficient permission. sourcegraph/sourcegraph#10077 makes the error codes and messages more consistent on the server side. This way user gets a hint if token was wrong or there was a permission problem.

To summarise. First of all user gets error message from the server which often is specific (ex. upload failed: must provide gitlab_token). However basing text returned by the server (server does not return structured response like JSON) is IMO fragile so except this error message user gets hints about possible problems.

I believe those two together should eliminate confusion and point user in the right direction how to remediate the issue. We also always show the link to the documentation.

Additionally existing code was deducing used code host based on the repository URL hostname, here we also check the specified command line parameter. Those hints which are displayed to the user is not a new invention, those were just improved to provide user with more accurate information and to utilise information from the server which effectively is best source of truth what the problem was.

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,135 @@ 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)
+	if httpErr := findAuthError(err); httpErr != nil {
+		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
+// findAuthError searches the error chain (including multi-errors from retries)
+// for a 401 or 403 ErrUnexpectedStatusCode. Returns nil if none is found.
+func findAuthError(err error) *ErrUnexpectedStatusCode {
+	// Check if it's a multi-error and scan all children.
+	if multi, ok := err.(errors.MultiError); ok {
+		for _, e := range multi.Errors() {
+			if found := findAuthError(e); found != nil {
+				return found
+			}
+		}
+		return nil
+	}
+
+	var httpErr *ErrUnexpectedStatusCode
+	if errors.As(err, &httpErr) && (httpErr.Code == 401 || httpErr.Code == 403) {
+		return httpErr
+	}
+	return nil
+}
+
+// uploadHints builds hint paragraphs for the Sourcegraph access token,
+// code host tokens, and a docs link.
+func uploadHints(accessToken string, isUnauthorized, isForbidden bool) string {
+	var causes []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 h := sourcegraphAccessTokenHint(accessToken, isUnauthorized, isForbidden); h != "" {
+		causes = append(causes, "- "+h)
 	}
 
-	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")}
+	for _, h := range codeHostTokenHints(isUnauthorized) {
+		causes = append(causes, "- "+h)
 	}
 
-	needsGitHubToken := strings.HasPrefix(codeintelUploadFlags.repo, "github.com")
-	needsGitLabToken := strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com")
+	var parts []string
+	parts = append(parts, "Possible causes:\n"+strings.Join(causes, "\n"))
+	parts = append(parts, "For more details on uploading SCIP indexes, see https://sourcegraph.com/docs/cli/references/code-intel/upload.")
 
-	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.",
-			)
-		} 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.",
-			)
+	return strings.Join(parts, "\n\n")
+}
+
+// sourcegraphAccessTokenHint returns a hint about the Sourcegraph access token
+// based on the error type and token state.
+func sourcegraphAccessTokenHint(accessToken string, isUnauthorized, isForbidden bool) string {
+	if isUnauthorized {
+		if accessToken == "" {
+			return "No Sourcegraph access token was provided. Set the SRC_ACCESS_TOKEN environment variable to a valid token."
 		}
-	} 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 {
-			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.",
-			)
+		if _, parseErr := accesstoken.ParsePersonalAccessToken(accessToken); parseErr != nil {
+			return "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 {
-		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.",
-		)
+		return "The Sourcegraph access token may be invalid, expired, or you may be connecting to the wrong Sourcegraph instance."
+	}
+	if isForbidden {
+		return "You may not have sufficient permissions on this Sourcegraph instance."
 	}
+	return ""
+}
 
-	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")}
+// codeHostTokenHints returns hints about GitHub or GitLab tokens.
+func codeHostTokenHints(isUnauthorized bool) []string {
+	if codeintelUploadFlags.gitHubToken != "" || strings.HasPrefix(codeintelUploadFlags.repo, "github.com") {
+		return []string{gitHubTokenHint(isUnauthorized)}
+	}
+	if codeintelUploadFlags.gitLabToken != "" || strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com") {
+		return []string{gitLabTokenHint(isUnauthorized)}
+	}
+	return []string{"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{})
+// gitHubTokenHint returns a hint about the GitHub token.
+// Only called when gitHubToken is set or repo starts with "github.com".
+func gitHubTokenHint(isUnauthorized bool) string {
+	if codeintelUploadFlags.gitHubToken == "" {
+		return 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 isUnauthorized {
+		return "The supplied -github-token may be invalid."
+	}
+	return "The supplied -github-token may lack the required permissions."
 }
 
-func mergeStringSlices(ss ...[]string) []string {
-	var combined []string
-	for _, s := range ss {
-		combined = append(combined, s...)
+// gitLabTokenHint returns a hint about the GitLab token.
+// Only called when gitLabToken is set or repo starts with "gitlab.com".
+func gitLabTokenHint(isUnauthorized bool) string {
+	if codeintelUploadFlags.gitLabToken == "" {
+		return 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)
+	}
+	if isUnauthorized {
+		return "The supplied -gitlab-token may be invalid."
 	}
+	return "The supplied -gitlab-token may lack the required permissions."
+}
+
+// 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{})
 }