Skip to content

Commit 28b61c7

Browse files
chrapkowski-sgchrapkowski-sg
committed
fix/improve-error-messages
1 parent f404049 commit 28b61c7

File tree

2 files changed

+80
-77
lines changed

2 files changed

+80
-77
lines changed

‎cmd/src/code_intel_upload.go‎

Lines changed: 62 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func handleCodeIntelUpload(args []string) error {
7171
}
7272
}
7373
if err != nil {
74-
return handleUploadError(cfg.AccessToken, err)
74+
return err
7575
}
7676

7777
client := cfg.apiClient(codeintelUploadFlags.apiFlags, io.Discard)
@@ -212,94 +212,89 @@ func (e errorWithHint) Error() string {
212212
return fmt.Sprintf("%s\n\n%s\n", e.err, e.hint)
213213
}
214214

215-
// handleUploadError writes the given error to the given output. If the
216-
// given output object is nil then the error will be written to standard out.
217-
//
218-
// This method returns the error that should be passed back up to the runner.
215+
// handleUploadError attaches actionable hints to upload errors and returns
216+
// the enriched error. Only called for actual upload failures (not flag validation).
219217
func handleUploadError(accessToken string, err error) error {
220-
if errors.Is(err, upload.ErrUnauthorized) {
221-
err = attachHintsForAuthorizationError(accessToken, err)
218+
var httpErr *ErrUnexpectedStatusCode
219+
if errors.As(err, &httpErr) && (httpErr.Code == 401 || httpErr.Code == 403) {
220+
isUnauthorized := httpErr.Code == 401
221+
isForbidden := httpErr.Code == 403
222+
223+
// Primary error line: prefer server message, fall back to generic.
224+
var displayErr error
225+
if httpErr.Body != "" {
226+
displayErr = errors.Newf("upload rejected by server: %s", httpErr.Body)
227+
} else if isUnauthorized {
228+
displayErr = errors.New("upload failed: unauthorized")
229+
} else {
230+
displayErr = errors.New("upload failed: forbidden")
231+
}
232+
233+
err = errorWithHint{
234+
err: displayErr,
235+
hint: uploadHints(accessToken, isUnauthorized, isForbidden),
236+
}
222237
}
223238

224239
if codeintelUploadFlags.ignoreUploadFailures {
225-
// Report but don't return the error
226240
fmt.Println(err.Error())
227241
return nil
228242
}
229243

230244
return err
231245
}
232246

233-
func attachHintsForAuthorizationError(accessToken string, originalError error) error {
234-
var actionableHints []string
235-
236-
likelyTokenError := accessToken == ""
237-
if _, parseErr := accesstoken.ParsePersonalAccessToken(accessToken); accessToken != "" && parseErr != nil {
238-
likelyTokenError = true
239-
actionableHints = append(actionableHints,
240-
"However, the provided access token does not match expected format; was it truncated?",
241-
"Typically the access token looks like sgp_<40 hex chars> or sgp_<instance-id>_<40 hex chars>.")
242-
}
243-
244-
if likelyTokenError {
245-
return errorWithHint{err: originalError, hint: strings.Join(mergeStringSlices(
246-
[]string{"A Sourcegraph access token must be provided via SRC_ACCESS_TOKEN for uploading SCIP data."},
247-
actionableHints,
248-
[]string{"For more details, see https://sourcegraph.com/docs/cli/how-tos/creating_an_access_token."},
249-
), "\n")}
247+
// uploadHints builds two independent hint blocks: one for the Sourcegraph
248+
// access token and one for code host tokens. Both are always included.
249+
func uploadHints(accessToken string, isUnauthorized, isForbidden bool) string {
250+
var hints []string
251+
252+
// Paragraph 1: Sourcegraph access token
253+
if isUnauthorized {
254+
if accessToken == "" {
255+
hints = append(hints, "No Sourcegraph access token was provided. Set the SRC_ACCESS_TOKEN environment variable to a valid token.")
256+
} else if _, parseErr := accesstoken.ParsePersonalAccessToken(accessToken); parseErr != nil {
257+
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?")
258+
} else {
259+
hints = append(hints, "The Sourcegraph access token may be invalid, expired, or you may be connecting to the wrong Sourcegraph instance.")
260+
}
261+
} else if isForbidden {
262+
hints = append(hints, "You may not have sufficient permissions on this Sourcegraph instance.")
250263
}
251264

252-
needsGitHubToken := strings.HasPrefix(codeintelUploadFlags.repo, "github.com")
253-
needsGitLabToken := strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com")
254-
255-
if needsGitHubToken {
256-
if codeintelUploadFlags.gitHubToken != "" {
257-
actionableHints = append(actionableHints,
258-
fmt.Sprintf("The supplied -github-token does not indicate that you have collaborator access to %s.", codeintelUploadFlags.repo),
259-
"Please check the value of the supplied token and its permissions on the code host and try again.",
260-
)
265+
// Paragraph 2: Code host token
266+
if codeintelUploadFlags.gitHubToken != "" {
267+
if isUnauthorized {
268+
hints = append(hints, "The supplied -github-token may be invalid.")
261269
} else {
262-
actionableHints = append(actionableHints,
263-
fmt.Sprintf("Please retry your request with a -github-token=XXX with collaborator access to %s.", codeintelUploadFlags.repo),
264-
"This token will be used to check with the code host that the uploading user has write access to the target repository.",
265-
)
270+
hints = append(hints, "The supplied -github-token may lack the required permissions.")
266271
}
267-
} else if needsGitLabToken {
268-
if codeintelUploadFlags.gitLabToken != "" {
269-
actionableHints = append(actionableHints,
270-
fmt.Sprintf("The supplied -gitlab-token does not indicate that you have write access to %s.", codeintelUploadFlags.repo),
271-
"Please check the value of the supplied token and its permissions on the code host and try again.",
272-
)
272+
} else if strings.HasPrefix(codeintelUploadFlags.repo, "github.com") {
273+
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))
274+
}
275+
276+
if codeintelUploadFlags.gitLabToken != "" {
277+
if isUnauthorized {
278+
hints = append(hints, "The supplied -gitlab-token may be invalid.")
273279
} else {
274-
actionableHints = append(actionableHints,
275-
fmt.Sprintf("Please retry your request with a -gitlab-token=XXX with write access to %s.", codeintelUploadFlags.repo),
276-
"This token will be used to check with the code host that the uploading user has write access to the target repository.",
277-
)
280+
hints = append(hints, "The supplied -gitlab-token may lack the required permissions.")
278281
}
279-
} else {
280-
actionableHints = append(actionableHints,
281-
"Verification is supported for the following code hosts: github.com, gitlab.com.",
282-
"Please request support for additional code host verification at https://github.com/sourcegraph/sourcegraph/issues/4967.",
283-
)
282+
} else if strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com") {
283+
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))
284+
}
285+
286+
if codeintelUploadFlags.gitHubToken == "" && codeintelUploadFlags.gitLabToken == "" &&
287+
!strings.HasPrefix(codeintelUploadFlags.repo, "github.com") &&
288+
!strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com") {
289+
hints = append(hints, "Code host verification is supported for github.com and gitlab.com repositories.")
284290
}
285291

286-
return errorWithHint{err: originalError, hint: strings.Join(mergeStringSlices(
287-
[]string{"This Sourcegraph instance has enforced auth for SCIP uploads."},
288-
actionableHints,
289-
[]string{"For more details, see https://docs.sourcegraph.com/cli/references/code-intel/upload."},
290-
), "\n")}
292+
hints = append(hints, "For more details on uploading SCIP indexes, see https://sourcegraph.com/docs/cli/references/code-intel/upload.")
293+
294+
return strings.Join(hints, "\n\n")
291295
}
292296

293297
// emergencyOutput creates a default Output object writing to standard out.
294298
func emergencyOutput() *output.Output {
295299
return output.NewOutput(os.Stdout, output.OutputOpts{})
296300
}
297-
298-
func mergeStringSlices(ss ...[]string) []string {
299-
var combined []string
300-
for _, s := range ss {
301-
combined = append(combined, s...)
302-
}
303-
304-
return combined
305-
}

‎cmd/src/code_intel_upload_vendored.go‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -339,8 +339,20 @@ type uploadRequestOptions struct {
339339
Done bool // Whether the request is a multipart finalize
340340
}
341341

342-
// ErrUnauthorized occurs when the upload endpoint returns a 401 response.
343-
var ErrUnauthorized = errors.New("unauthorized upload")
342+
// ErrUnexpectedStatusCode is returned for HTTP error responses from the upload
343+
// endpoint. It carries the status code and response body so callers can inspect
344+
// them without parsing the error string.
345+
type ErrUnexpectedStatusCode struct {
346+
Code int
347+
Body string
348+
}
349+
350+
func (e *ErrUnexpectedStatusCode) Error() string {
351+
if e.Body != "" {
352+
return fmt.Sprintf("unexpected status code: %d (%s)", e.Code, e.Body)
353+
}
354+
return fmt.Sprintf("unexpected status code: %d", e.Code)
355+
}
344356

345357
// performUploadRequest performs an HTTP POST to the upload endpoint. The query string of the request
346358
// 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
419431
// returns a boolean flag indicating if the function can be retried on failure (error-dependent).
420432
func decodeUploadPayload(resp *http.Response, body []byte, target *int) (bool, error) {
421433
if resp.StatusCode >= 300 {
422-
if resp.StatusCode == http.StatusUnauthorized {
423-
return false, ErrUnauthorized
424-
}
425-
426-
suffix := ""
427-
if !bytes.HasPrefix(bytes.TrimSpace(body), []byte{'<'}) {
428-
suffix = fmt.Sprintf(" (%s)", bytes.TrimSpace(body))
434+
detail := ""
435+
if trimmed := bytes.TrimSpace(body); len(trimmed) > 0 && trimmed[0] != '<' {
436+
detail = string(trimmed)
429437
}
430438

431439
// Do not retry client errors
432-
return resp.StatusCode >= 500, errors.Errorf("unexpected status code: %d%s", resp.StatusCode, suffix)
440+
return resp.StatusCode >= 500, &ErrUnexpectedStatusCode{Code: resp.StatusCode, Body: detail}
433441
}
434442

435443
if target == nil {

0 commit comments

Comments
 (0)