From 8a38cf4d48416d38d3235596e3e9dfd243a248ed Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 22 Apr 2026 13:13:12 +0000 Subject: [PATCH 1/5] Enhance GitHub API error handling with structured ApiError type and actionable suggestions for SAML, rate-limit, and authorization errors --- cli/azd/pkg/templates/gh_errors.go | 113 ++++++++ cli/azd/pkg/templates/gh_source.go | 137 +++++++++- cli/azd/pkg/templates/gh_source_test.go | 59 +++++ cli/azd/pkg/tools/github/api_error.go | 245 ++++++++++++++++++ cli/azd/pkg/tools/github/api_error_test.go | 216 +++++++++++++++ cli/azd/pkg/tools/github/github.go | 6 +- .../pkg/tools/github/github_methods_test.go | 5 +- cli/azd/resources/error_suggestions.yaml | 10 + 8 files changed, 784 insertions(+), 7 deletions(-) create mode 100644 cli/azd/pkg/templates/gh_errors.go create mode 100644 cli/azd/pkg/tools/github/api_error.go create mode 100644 cli/azd/pkg/tools/github/api_error_test.go diff --git a/cli/azd/pkg/templates/gh_errors.go b/cli/azd/pkg/templates/gh_errors.go new file mode 100644 index 00000000000..77e88dc68f3 --- /dev/null +++ b/cli/azd/pkg/templates/gh_errors.go @@ -0,0 +1,113 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package templates + +import ( + "errors" + + "github.com/azure/azure-dev/cli/azd/internal" + "github.com/azure/azure-dev/cli/azd/pkg/errorhandler" + "github.com/azure/azure-dev/cli/azd/pkg/tools/github" +) + +// withGitHubSuggestion wraps the supplied error in an *internal.ErrorWithSuggestion +// when the error is a recognized GitHub failure (typed *github.ApiError or +// *RepoNotAccessibleError). The wrapping carries actionable guidance inline +// in the error chain, which means: +// +// - The core CLI ErrorMiddleware renders it directly without consulting the +// YAML rules pipeline (ErrorMiddleware short-circuits on +// *ErrorWithSuggestion). +// - Callers that don't have access to the YAML pipeline (e.g., extensions +// receiving stringified errors over gRPC) still see the suggestion as part +// of the wrapped error, because *ApiError keeps formatting itself with +// status + message. +// +// Returns the original error unchanged when no specific suggestion applies, so +// other typed-error pipelines (auth, unknown gh failures) behave as before. +func withGitHubSuggestion(err error) error { + if err == nil { + return nil + } + + if apiErr, ok := errors.AsType[*github.ApiError](err); ok { + if s := suggestionForApiError(apiErr); s != nil { + s.Err = err + return s + } + } + + if repoErr, ok := errors.AsType[*RepoNotAccessibleError](err); ok { + s := suggestionForRepoNotAccessible(repoErr) + s.Err = err + return s + } + + return err +} + +func suggestionForApiError(apiErr *github.ApiError) *internal.ErrorWithSuggestion { + switch apiErr.Kind { + case github.KindSAMLBlocked: + return &internal.ErrorWithSuggestion{ + Message: "The GitHub organization that owns this repository requires SAML SSO " + + "authorization for your token before it can be used.", + Suggestion: "Open https://github.com/settings/tokens, find the personal access " + + "token you're using, click 'Configure SSO', and authorize the organization. " + + "If you're signed in with `gh auth login`, run " + + "`gh auth refresh -h github.com` and complete the SSO flow in the browser.", + Links: []errorhandler.ErrorLink{ + { + URL: "https://docs.github.com/enterprise-cloud@latest/authentication/" + + "authenticating-with-saml-single-sign-on/" + + "authorizing-a-personal-access-token-for-use-with-saml-single-sign-on", + Title: "Authorizing a personal access token for use with SAML SSO", + }, + }, + } + case github.KindRateLimited: + return &internal.ErrorWithSuggestion{ + Message: "GitHub API rate limit exceeded.", + Suggestion: "Authenticated requests have a much higher limit than anonymous ones. " + + "Run `gh auth login` (or set GITHUB_TOKEN / GH_TOKEN) and retry. " + + "If you're already authenticated, wait for the rate-limit window to reset " + + "(typically up to one hour).", + Links: []errorhandler.ErrorLink{ + { + URL: "https://docs.github.com/rest/overview/rate-limits-for-the-rest-api", + Title: "GitHub REST API rate limits", + }, + }, + } + case github.KindUnauthorized: + return &internal.ErrorWithSuggestion{ + Message: "GitHub rejected the request as unauthenticated (HTTP 401).", + Suggestion: "Run `gh auth login` to sign in, or refresh an expired token with " + + "`gh auth refresh`. If you're using GITHUB_TOKEN / GH_TOKEN, regenerate the " + + "token and ensure it has the required scopes.", + } + case github.KindForbidden: + return &internal.ErrorWithSuggestion{ + Message: "GitHub denied access to the requested resource (HTTP 403). The " + + "repository may be private, your token may be missing required scopes, or " + + "your account may not have permission.", + Suggestion: "Verify you can access the repository in a browser while signed in " + + "as the same GitHub account. If you're using a personal access token, ensure " + + "it includes the 'repo' scope. Run `gh auth status` to confirm which account " + + "gh is using.", + } + } + return nil +} + +func suggestionForRepoNotAccessible(_ *RepoNotAccessibleError) *internal.ErrorWithSuggestion { + // Leave Message empty so the renderer falls back to RepoNotAccessibleError.Error(), + // which is already user-friendly and includes the repo slug — avoids duplication. + return &internal.ErrorWithSuggestion{ + Suggestion: "Confirm the repository URL is correct and that the active gh account " + + "can see it. Run `gh auth status` to check which account is active. For Enterprise " + + "Managed Users (EMU), make sure the active account is the EMU account that owns " + + "this repository — a github.com URL may need to target a different host.", + } +} diff --git a/cli/azd/pkg/templates/gh_source.go b/cli/azd/pkg/templates/gh_source.go index 27b8dda6dcd..6480ae3ef54 100644 --- a/cli/azd/pkg/templates/gh_source.go +++ b/cli/azd/pkg/templates/gh_source.go @@ -5,6 +5,7 @@ package templates import ( "context" + "errors" "fmt" "net/url" "strings" @@ -145,6 +146,20 @@ func resolveBranchAndPath( hostname string, repoSlug string, branchAndPath string, +) (branch string, filePath string, err error) { + branch, filePath, err = resolveBranchAndPathInner(ctx, ghCli, hostname, repoSlug, branchAndPath) + if err != nil { + return "", "", withGitHubSuggestion(err) + } + return branch, filePath, nil +} + +func resolveBranchAndPathInner( + ctx context.Context, + ghCli *github.Cli, + hostname string, + repoSlug string, + branchAndPath string, ) (branch string, filePath string, err error) { if branchAndPath == "" { return "", "", fmt.Errorf("branch and path cannot be empty") @@ -153,7 +168,11 @@ func resolveBranchAndPath( parts := strings.Split(branchAndPath, "/") if len(parts) == 1 { // Only one segment - try it as a branch first - if branchExists(ctx, ghCli, hostname, repoSlug, parts[0]) { + exists, accessErr := branchExists(ctx, ghCli, hostname, repoSlug, parts[0]) + if accessErr != nil { + return "", "", accessErr + } + if exists { return parts[0], "", nil } // If not a branch, assume it's a file in the default branch @@ -166,21 +185,129 @@ func resolveBranchAndPath( candidateBranch := strings.Join(parts[:i], "/") candidatePath := strings.Join(parts[i:], "/") - if branchExists(ctx, ghCli, hostname, repoSlug, candidateBranch) { + exists, accessErr := branchExists(ctx, ghCli, hostname, repoSlug, candidateBranch) + if accessErr != nil { + return "", "", accessErr + } + if exists { return candidateBranch, candidatePath, nil } } - // If no valid branch found, return error + // If no valid branch found, probe the repo itself to disambiguate + // "repo exists but branch is genuinely weird" from the much more common + // "repo isn't accessible to this account" (e.g., wrong URL, private + // repo, or — for EMU users — a github.com URL that should be on the + // enterprise instance). GitHub returns 404 for both private and + // not-existing repos when the caller isn't authorized, so a 404 here + // is a much better signal than the branch walk's 404s. + if repoErr := checkRepoAccessible(ctx, ghCli, hostname, repoSlug); repoErr != nil { + return "", "", repoErr + } + return "", "", fmt.Errorf("could not find a valid branch in the URL path. "+ "Tried branch names from '%s' to '%s'", parts[0], strings.Join(parts, "/")) } +// checkRepoAccessible probes /repos/{slug} to determine why no branch could +// be resolved. Returns nil if the repo is accessible (so the caller should +// emit the original "no valid branch" message), or a typed error describing +// the access failure otherwise. +func checkRepoAccessible( + ctx context.Context, + ghCli *github.Cli, + hostname string, + repoSlug string, +) error { + apiPath := fmt.Sprintf("/repos/%s", repoSlug) + _, err := ghCli.ApiCall(ctx, hostname, apiPath, github.ApiCallOptions{}) + if err == nil { + return nil + } + apiErr, ok := errors.AsType[*github.ApiError](err) + if !ok { + return err + } + // Auth/SAML/rate-limit already carry actionable typed info — surface as-is. + if apiErr.IsAuthError() || apiErr.Kind == github.KindRateLimited { + return apiErr + } + // 404 on the repo itself: the user can't see this repo, and the original + // branch error would mislead. Wrap as a RepoNotAccessibleError so the + // error_suggestions.yaml pipeline can attach EMU/private-repo guidance. + if apiErr.IsNotFound() { + return &RepoNotAccessibleError{ + Hostname: hostname, + RepoSlug: repoSlug, + Cause: apiErr, + } + } + return apiErr +} + +// RepoNotAccessibleError indicates that /repos/{owner}/{repo} returned 404 — +// either the repo doesn't exist, it's private and the caller isn't authorized, +// or the URL targets the wrong GitHub host (e.g., a github.com URL for an +// EMU/enterprise account that lives on a different instance). +type RepoNotAccessibleError struct { + Hostname string + RepoSlug string + Cause error +} + +func (e *RepoNotAccessibleError) Error() string { + return fmt.Sprintf( + "repository %s/%s is not accessible (HTTP 404). "+ + "It may not exist, may be private, or your account may not have access", + e.Hostname, e.RepoSlug, + ) +} + +func (e *RepoNotAccessibleError) Unwrap() error { return e.Cause } + // branchExists checks if a branch exists in the repository using the GitHub API. -func branchExists(ctx context.Context, ghCli *github.Cli, hostname string, repoSlug string, branchName string) bool { +// +// Returns: +// - (true, nil) — the branch exists (HTTP 200). +// - (false, nil) — the branch does not exist (HTTP 404), or the error could +// not be classified as an access failure. In the unknown case we keep +// walking branch candidates rather than failing the whole resolution, +// preserving long-standing behavior. +// - (false, err) — the API call failed for an authentication, authorization, +// rate-limit, or server-side reason. The error is the underlying +// *github.ApiError so the caller can short-circuit the branch-walk and +// the error_suggestions.yaml pipeline can surface an actionable message. +func branchExists( + ctx context.Context, + ghCli *github.Cli, + hostname string, + repoSlug string, + branchName string, +) (bool, error) { apiPath := fmt.Sprintf("/repos/%s/branches/%s", repoSlug, url.PathEscape(branchName)) _, err := ghCli.ApiCall(ctx, hostname, apiPath, github.ApiCallOptions{}) - return err == nil + if err == nil { + return true, nil + } + if apiErr, ok := errors.AsType[*github.ApiError](err); ok { + // Only surface errors we have positive evidence are access failures. + // For 404 or unclassifiable errors (e.g., Kind == KindUnknown because + // gh's output wasn't parseable) keep walking the candidate branches — + // this matches the historical "treat any non-2xx as not-a-branch" + // behavior and avoids regressing on transient or unexpected gh + // output formats. + switch apiErr.Kind { + case github.KindSAMLBlocked, + github.KindRateLimited, + github.KindUnauthorized, + github.KindForbidden, + github.KindServerError: + return false, apiErr + } + return false, nil + } + // Unknown non-API error (e.g., gh not installed, ctx cancelled). Surface it. + return false, err } // ensureGitHubAuthenticated checks if the user is authenticated to GitHub and initiates login if not. diff --git a/cli/azd/pkg/templates/gh_source_test.go b/cli/azd/pkg/templates/gh_source_test.go index 08c1996a216..4fc768649ed 100644 --- a/cli/azd/pkg/templates/gh_source_test.go +++ b/cli/azd/pkg/templates/gh_source_test.go @@ -5,6 +5,7 @@ package templates import ( "encoding/json" + "errors" "fmt" "path/filepath" "strings" @@ -683,3 +684,61 @@ func Test_ParseGitHubUrl_NotAuthenticated(t *testing.T) { require.Equal(t, "main", urlInfo.Branch) require.Equal(t, "path/to/file.yaml", urlInfo.FilePath) } + +// Test_ParseGitHubUrl_AccessErrorShortCircuits verifies that when the +// GitHub API returns a typed access failure (e.g., 403 SAML enforcement), +// resolveBranchAndPath returns the underlying *github.ApiError immediately +// instead of walking every candidate branch and emitting the misleading +// "could not find a valid branch in the URL path" message. The error +// surface is the typed error so the YAML error-suggestion pipeline can +// attach an actionable message. +func Test_ParseGitHubUrl_AccessErrorShortCircuits(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "--version" + }).Respond(exec.RunResult{Stdout: github.Version.String()}) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "auth" && args.Args[1] == "status" + }).Respond(exec.RunResult{Stdout: "Logged in to"}) + + // Track number of branch lookups so we can assert short-circuit. + callCount := 0 + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "api" + }).RespondFn(func(args exec.RunArgs) (exec.RunResult, error) { + callCount++ + // Real gh CLI emits stderr in this format; parseApiError reads it. + stderr := "gh: Resource protected by organization SAML enforcement. " + + "You must grant your OAuth token access to this organization. (HTTP 403)" + return exec.RunResult{Stdout: "", Stderr: stderr, ExitCode: 1}, + fmt.Errorf("exit code: 1, stdout: , stderr: %s", stderr) + }) + + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) + + _, err := ParseGitHubUrl( + *mockContext.Context, + "https://github.com/org/repo/blob/feature/sub/path/file.yaml", + ghCli, + ) + require.Error(t, err) + + // Must surface as the typed *github.ApiError so the YAML pipeline + // can map it to a SAML-specific suggestion. + apiErr, ok := errors.AsType[*github.ApiError](err) + require.True(t, ok, "expected *github.ApiError, got %T: %v", err, err) + require.Equal(t, github.KindSAMLBlocked, apiErr.Kind) + require.Equal(t, 403, apiErr.StatusCode) + + // The walk must short-circuit on the first auth failure rather than + // trying every candidate branch (would be 4 calls for "feature/sub/path/file.yaml"). + require.Equal(t, 1, callCount, "expected branch walk to short-circuit on first access error") + + // And the misleading "could not find a valid branch" message must NOT appear. + require.NotContains(t, err.Error(), "could not find a valid branch") +} diff --git a/cli/azd/pkg/tools/github/api_error.go b/cli/azd/pkg/tools/github/api_error.go new file mode 100644 index 00000000000..564bc908a79 --- /dev/null +++ b/cli/azd/pkg/tools/github/api_error.go @@ -0,0 +1,245 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package github + +import ( + "encoding/json" + "fmt" + "regexp" + "strconv" + "strings" +) + +// ApiErrorKind classifies the cause of a `gh api` failure. A single error +// has exactly one Kind. Use the predicate methods on *ApiError (IsAuthError, +// IsNotFound) for boolean checks; switch on Kind when you need to distinguish +// SAML vs. plain auth vs. rate-limit. +type ApiErrorKind int + +const ( + // KindUnknown indicates the failure couldn't be classified — typically + // because gh failed before issuing the request (network error, missing + // binary, ctx cancelled) and there's no HTTP status to inspect. + KindUnknown ApiErrorKind = iota + // KindSAMLBlocked indicates the owning organization enforces SAML SSO + // and the caller's token has not been authorized for it. Resolution + // requires an out-of-band step in the GitHub UI; `gh auth login` alone + // does not fix it. + KindSAMLBlocked + // KindRateLimited indicates GitHub rejected the request because rate + // limits were exceeded. Authenticated requests have higher limits. + KindRateLimited + // KindUnauthorized indicates HTTP 401 — the request was unauthenticated + // or the token is invalid/expired. + KindUnauthorized + // KindForbidden indicates HTTP 403 that is NOT SAML or rate-limit + // related — typically missing token scopes or a permission denial. + KindForbidden + // KindNotFound indicates HTTP 404 — the resource doesn't exist OR is + // private and the caller isn't authorized to know it exists. + KindNotFound + // KindServerError indicates a 5xx response from GitHub. + KindServerError + // KindOther indicates a non-2xx response that doesn't fit any of the + // categories above (e.g., 4xx codes other than 401/403/404). + KindOther +) + +// String returns a human-readable name for the kind, used in error messages. +func (k ApiErrorKind) String() string { + switch k { + case KindSAMLBlocked: + return "SAMLBlocked" + case KindRateLimited: + return "RateLimited" + case KindUnauthorized: + return "Unauthorized" + case KindForbidden: + return "Forbidden" + case KindNotFound: + return "NotFound" + case KindServerError: + return "ServerError" + case KindOther: + return "Other" + default: + return "Unknown" + } +} + +// ApiError represents a structured error returned from a `gh api` invocation. +// +// It is built from the underlying gh CLI's output (the JSON body that GitHub's +// REST API writes to stdout on errors, plus the human-readable stderr) so +// callers can branch on the failure mode without doing brittle substring +// matching against opaque error strings. +// +// Use Kind for switch-based dispatch and the IsAuthError / IsNotFound +// predicates for common boolean checks. +type ApiError struct { + // URL is the API URL that was requested (e.g., "https://api.github.com/repos/o/r/branches/main"). + URL string + // Kind classifies the failure (SAML vs. rate-limit vs. plain 4xx etc.). + Kind ApiErrorKind + // StatusCode is the HTTP status code returned by the GitHub API (e.g., 401, 403, 404). + // It is 0 when no status code could be parsed (e.g., gh failed before issuing the request). + StatusCode int + // Message is the human-readable message GitHub returned in the JSON + // error body (e.g., "Resource protected by organization SAML enforcement."). + // Empty when the failure was not an HTTP-level error (e.g., gh not installed). + Message string + // Stderr is the raw stderr output from `gh api`, kept for diagnostics. + Stderr string + // Underlying is the original error from the command runner (typically *exec.ExitError). + Underlying error +} + +// Error implements the error interface. +func (e *ApiError) Error() string { + switch e.Kind { + case KindSAMLBlocked: + return fmt.Sprintf("gh api %s: SAML SSO enforcement blocked the request (HTTP %d)", e.URL, e.StatusCode) + case KindRateLimited: + return fmt.Sprintf("gh api %s: GitHub API rate limit exceeded (HTTP %d)", e.URL, e.StatusCode) + case KindUnknown: + return fmt.Sprintf("gh api %s: %s", e.URL, e.Underlying) + default: + if e.Message != "" { + return fmt.Sprintf("gh api %s: HTTP %d: %s", e.URL, e.StatusCode, e.Message) + } + return fmt.Sprintf("gh api %s: HTTP %d", e.URL, e.StatusCode) + } +} + +// Unwrap exposes the underlying command-runner error so errors.Is/errors.As +// continue to work against the original error chain. +func (e *ApiError) Unwrap() error { + return e.Underlying +} + +// IsAuthError reports whether the error indicates an authentication or +// authorization failure (401, 403, or SAML enforcement). +func (e *ApiError) IsAuthError() bool { + switch e.Kind { + case KindUnauthorized, KindForbidden, KindSAMLBlocked: + return true + default: + return false + } +} + +// IsNotFound reports whether the API returned 404. +func (e *ApiError) IsNotFound() bool { + return e.Kind == KindNotFound +} + +// httpStatusRe matches gh CLI's standard "(HTTP )" suffix that appears +// at the end of error messages from `gh api` when the request was issued and +// returned a non-2xx response. Example: +// +// gh: Resource protected by organization SAML enforcement. ... (HTTP 403) +var httpStatusRe = regexp.MustCompile(`\(HTTP (\d{3})\)`) + +// githubErrorBody mirrors the structured error envelope GitHub's REST API +// writes to stdout for non-2xx responses. Only fields we care about are +// captured; unknown fields are ignored. +type githubErrorBody struct { + Message string `json:"message"` + // Status is a string in the response (e.g., "403"), not an int. + Status string `json:"status"` +} + +// parseApiError converts a failed `gh api` invocation into a structured +// *ApiError. stdout typically contains GitHub's JSON error envelope and +// stderr contains gh's human-readable rendering with the "(HTTP NNN)" suffix. +// Both are inspected: the JSON body is the primary signal (stable, structured), +// stderr is a fallback for cases where stdout isn't valid JSON (gh failed +// before issuing the request, network error, etc.). +// +// Returns nil if err is nil. Always returns a non-nil *ApiError otherwise so +// callers can rely on errors.AsType[*ApiError] succeeding for any failed +// `gh api` call. +func parseApiError(url, stdout, stderr string, err error) *ApiError { + if err == nil { + return nil + } + + apiErr := &ApiError{URL: url, Stderr: stderr, Underlying: err} + + // Primary signal: GitHub's JSON error envelope on stdout. + if body := parseGitHubErrorBody(stdout); body != nil { + apiErr.Message = body.Message + if code, convErr := strconv.Atoi(body.Status); convErr == nil { + apiErr.StatusCode = code + } + } + + // Fallback: stderr "(HTTP NNN)" marker. Always check, in case the JSON + // body was missing or malformed (e.g., proxy returning HTML). + if apiErr.StatusCode == 0 { + if m := httpStatusRe.FindStringSubmatch(stderr); len(m) == 2 { + if code, convErr := strconv.Atoi(m[1]); convErr == nil { + apiErr.StatusCode = code + } + } + } + + apiErr.Kind = classifyKind(apiErr.StatusCode, apiErr.Message+"\n"+stderr) + return apiErr +} + +// parseGitHubErrorBody returns the GitHub JSON error envelope if stdout +// contains one, otherwise nil. We tolerate leading/trailing whitespace and +// require both Message and Status to be non-empty so we don't misinterpret +// arbitrary JSON success bodies as errors. +func parseGitHubErrorBody(stdout string) *githubErrorBody { + stdout = strings.TrimSpace(stdout) + if !strings.HasPrefix(stdout, "{") { + return nil + } + var body githubErrorBody + if err := json.Unmarshal([]byte(stdout), &body); err != nil { + return nil + } + if body.Message == "" || body.Status == "" { + return nil + } + return &body +} + +// classifyKind picks an ApiErrorKind from the status code and the combined +// message + stderr text. SAML and rate-limit checks take precedence over the +// raw status code (both surface as 403 from GitHub) so the more specific +// classification wins. Phrase matching is intentionally narrow (e.g., requires +// "saml enforcement", not bare "saml") to avoid misclassifying repo names +// that happen to contain those substrings. +func classifyKind(statusCode int, text string) ApiErrorKind { + lower := strings.ToLower(text) + + if strings.Contains(lower, "saml enforcement") || + strings.Contains(lower, "saml sso") || + strings.Contains(lower, "sso authorization") || + strings.Contains(lower, "sso required") { + return KindSAMLBlocked + } + if strings.Contains(lower, "api rate limit exceeded") || + strings.Contains(lower, "secondary rate limit") { + return KindRateLimited + } + + switch { + case statusCode == 0: + return KindUnknown + case statusCode == 401: + return KindUnauthorized + case statusCode == 403: + return KindForbidden + case statusCode == 404: + return KindNotFound + case statusCode >= 500: + return KindServerError + default: + return KindOther + } +} diff --git a/cli/azd/pkg/tools/github/api_error_test.go b/cli/azd/pkg/tools/github/api_error_test.go new file mode 100644 index 00000000000..ad7a0552a1e --- /dev/null +++ b/cli/azd/pkg/tools/github/api_error_test.go @@ -0,0 +1,216 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package github + +import ( + "errors" + "fmt" + "testing" + + "github.com/azure/azure-dev/cli/azd/pkg/exec" + "github.com/stretchr/testify/require" +) + +const samlDocURL = "https://docs.github.com/articles/" + + "authenticating-to-a-github-organization-with-saml-single-sign-on/" + +const samlStdoutBody = `{"message":"Resource protected by organization SAML enforcement. ` + + `You must grant your OAuth token access to this organization.",` + + `"documentation_url":"` + samlDocURL + `",` + + `"status":"403"}` + +const samlStderrLine = "gh: Resource protected by organization SAML enforcement. " + + "You must grant your OAuth token access to this organization. (HTTP 403)" + +func TestParseApiError_NilError(t *testing.T) { + t.Parallel() + require.Nil(t, parseApiError("https://api.github.com/x", "", "", nil)) +} + +func TestParseApiError_StatusFromJSONBody(t *testing.T) { + t.Parallel() + cases := []struct { + name string + stdout string + stderr string + want int + }{ + { + "401 from JSON", + `{"message":"Bad credentials","documentation_url":"...","status":"401"}`, + "gh: Bad credentials (HTTP 401)", + 401, + }, + { + "403 SAML from JSON", + samlStdoutBody, + samlStderrLine, + 403, + }, + { + "404 from JSON", + `{"message":"Not Found","documentation_url":"...","status":"404"}`, + "gh: Not Found (HTTP 404)", + 404, + }, + { + "stderr fallback when stdout has no JSON", + "", + "gh: Server error (HTTP 500)", + 500, + }, + { + "stderr fallback when stdout JSON missing fields", + `{"foo":"bar"}`, + "gh: weird (HTTP 502)", + 502, + }, + { + "no marker anywhere", + "", + "gh: failed talking to api/repos/team-401k", + 0, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := errors.New("exit 1") + apiErr := parseApiError("https://api.github.com/r", tc.stdout, tc.stderr, err) + require.NotNil(t, apiErr) + require.Equal(t, tc.want, apiErr.StatusCode) + }) + } +} + +func TestParseApiError_DetectsSAML(t *testing.T) { + t.Parallel() + apiErr := parseApiError( + "https://api.github.com/repos/o/r", + samlStdoutBody, + samlStderrLine, + errors.New("exit 1"), + ) + require.NotNil(t, apiErr) + require.Equal(t, KindSAMLBlocked, apiErr.Kind) + require.True(t, apiErr.IsAuthError()) + require.Equal(t, 403, apiErr.StatusCode) + require.Contains(t, apiErr.Message, "SAML enforcement") +} + +func TestParseApiError_DetectsRateLimit(t *testing.T) { + t.Parallel() + apiErr := parseApiError( + "https://api.github.com/repos/o/r", + `{"message":"API rate limit exceeded for user ID 12345.","documentation_url":"...","status":"403"}`, + "gh: API rate limit exceeded for user ID 12345. (HTTP 403)", + errors.New("exit 1"), + ) + require.NotNil(t, apiErr) + require.Equal(t, KindRateLimited, apiErr.Kind) + require.Equal(t, 403, apiErr.StatusCode) +} + +func TestParseApiError_DoesNotMisclassifySsoInRepoName(t *testing.T) { + t.Parallel() + // Repo name "sso-tools" must not trip SAML detection: we look for + // specific phrases ("saml enforcement", "saml sso", "sso authorization", + // "sso required"), not a bare "sso" substring. + apiErr := parseApiError( + "https://api.github.com/repos/o/sso-tools/branches/main", + `{"message":"Not Found","documentation_url":"...","status":"404"}`, + "gh: Not Found (HTTP 404) at /repos/o/sso-tools/branches/main", + errors.New("exit 1"), + ) + require.NotNil(t, apiErr) + require.Equal(t, KindNotFound, apiErr.Kind) + require.False(t, apiErr.IsAuthError()) + require.True(t, apiErr.IsNotFound()) +} + +func TestParseApiError_NoOutputAvailable(t *testing.T) { + t.Parallel() + // gh failed before issuing the request (network error, missing binary, + // ctx cancelled, etc.) — we still return a non-nil ApiError with + // StatusCode=0 so callers' errors.AsType[*ApiError] checks succeed. + underlying := errors.New("dial tcp: lookup api.github.com: no such host") + apiErr := parseApiError("https://api.github.com/repos/o/r", "", "", underlying) + require.NotNil(t, apiErr) + require.Equal(t, KindUnknown, apiErr.Kind) + require.Equal(t, 0, apiErr.StatusCode) + require.Same(t, underlying, apiErr.Underlying) +} + +func TestApiError_UnwrapPreservesChain(t *testing.T) { + t.Parallel() + sentinel := errors.New("sentinel") + apiErr := &ApiError{URL: "u", Underlying: sentinel} + require.ErrorIs(t, apiErr, sentinel) +} + +func TestApiError_ErrorMessage(t *testing.T) { + t.Parallel() + cases := []struct { + name string + err *ApiError + want string + }{ + { + "saml", + &ApiError{URL: "https://api.github.com/x", StatusCode: 403, Kind: KindSAMLBlocked}, + "gh api https://api.github.com/x: SAML SSO enforcement blocked the request (HTTP 403)", + }, + { + "rate limit", + &ApiError{URL: "https://api.github.com/x", StatusCode: 403, Kind: KindRateLimited}, + "gh api https://api.github.com/x: GitHub API rate limit exceeded (HTTP 403)", + }, + { + "plain status with message", + &ApiError{URL: "https://api.github.com/x", StatusCode: 404, Kind: KindNotFound, Message: "Not Found"}, + "gh api https://api.github.com/x: HTTP 404: Not Found", + }, + { + "plain status without message", + &ApiError{URL: "https://api.github.com/x", StatusCode: 500, Kind: KindServerError}, + "gh api https://api.github.com/x: HTTP 500", + }, + { + "unknown kind falls back to underlying", + &ApiError{URL: "https://api.github.com/x", Kind: KindUnknown, Underlying: errors.New("boom")}, + "gh api https://api.github.com/x: boom", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tc.want, tc.err.Error()) + }) + } +} + +// Integration-style: ApiCall returns *ApiError on failure with status +// extracted from the captured stdout JSON body. +func TestApiCall_ReturnsTypedApiErrorOnFailure(t *testing.T) { + t.Parallel() + cli, mockCtx := newTestCli(t) + mockCtx.CommandRunner.When( + func(args exec.RunArgs, _ string) bool { + return len(args.Args) > 0 && args.Args[0] == "api" + }, + ).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { + stdout := `{"message":"Bad credentials","documentation_url":"...","status":"401"}` + stderr := "gh: Bad credentials (HTTP 401)" + return exec.NewRunResult(1, stdout, stderr), + fmt.Errorf("exit code: 1, stdout: %s, stderr: %s", stdout, stderr) + }) + + _, err := cli.ApiCall(t.Context(), "github.com", "/repos/o/r", ApiCallOptions{}) + require.Error(t, err) + apiErr, ok := errors.AsType[*ApiError](err) + require.True(t, ok, "expected error to unwrap to *ApiError, got: %v", err) + require.Equal(t, 401, apiErr.StatusCode) + require.True(t, apiErr.IsAuthError()) + require.Equal(t, "Bad credentials", apiErr.Message) +} diff --git a/cli/azd/pkg/tools/github/github.go b/cli/azd/pkg/tools/github/github.go index 1a32a2969ca..90f04f44f62 100644 --- a/cli/azd/pkg/tools/github/github.go +++ b/cli/azd/pkg/tools/github/github.go @@ -232,7 +232,11 @@ func (cli *Cli) ApiCall(ctx context.Context, hostname, path string, options ApiC runArgs := cli.newRunArgs(args...) result, err := cli.commandRunner.Run(ctx, runArgs) if err != nil { - return "", fmt.Errorf("failed running gh api: %s: %w", url, err) + // Build a typed *ApiError from the captured stdout (GitHub's JSON + // error envelope) and stderr. Callers can branch on HTTP status, + // SAML enforcement, and rate-limit conditions without sniffing + // error strings. The original error is preserved via Unwrap(). + return "", parseApiError(url, result.Stdout, result.Stderr, err) } return result.Stdout, nil diff --git a/cli/azd/pkg/tools/github/github_methods_test.go b/cli/azd/pkg/tools/github/github_methods_test.go index 8558806d2e1..d8ac5609d27 100644 --- a/cli/azd/pkg/tools/github/github_methods_test.go +++ b/cli/azd/pkg/tools/github/github_methods_test.go @@ -198,7 +198,10 @@ func TestApiCall(t *testing.T) { ApiCallOptions{}, ) require.Error(t, err) - require.Contains(t, err.Error(), "failed running gh api") + // Should be a typed *ApiError so callers can branch on status, etc. + apiErr, ok := errors.AsType[*ApiError](err) + require.True(t, ok, "expected error to be *ApiError, got: %v", err) + require.Equal(t, "https://api.github.com/bad", apiErr.URL) }) } diff --git a/cli/azd/resources/error_suggestions.yaml b/cli/azd/resources/error_suggestions.yaml index 3bb7baeddfb..df571c9b8c9 100644 --- a/cli/azd/resources/error_suggestions.yaml +++ b/cli/azd/resources/error_suggestions.yaml @@ -544,6 +544,16 @@ rules: - url: "https://learn.microsoft.com/azure/developer/azure-developer-cli/azd-schema" title: "azure.yaml schema reference" + # ============================================================================ + # GitHub API Errors (gh api) + # Suggestions for *github.ApiError (SAML/rate-limit/401/403) and + # *RepoNotAccessibleError are emitted directly as *internal.ErrorWithSuggestion + # by pkg/templates/gh_errors.go, so they don't need YAML rules here. This + # keeps the suggestion text co-located with the code that classifies the + # failure and lets the actionable text survive boundaries (e.g., gRPC) where + # the YAML pipeline doesn't run. + # ============================================================================ + # ============================================================================ # Text Pattern Rules — Specific patterns first # These are fallbacks for errors without typed Go structs. From cbc30b6d42908c22572ba9f6a7c5bf2a614cca73 Mon Sep 17 00:00:00 2001 From: Jeffrey Chen Date: Sat, 25 Apr 2026 00:11:51 +0000 Subject: [PATCH 2/5] Fix cspell --- cli/azd/pkg/templates/gh_source.go | 2 +- cli/azd/pkg/tools/github/api_error.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/azd/pkg/templates/gh_source.go b/cli/azd/pkg/templates/gh_source.go index 6480ae3ef54..e53e69415ed 100644 --- a/cli/azd/pkg/templates/gh_source.go +++ b/cli/azd/pkg/templates/gh_source.go @@ -292,7 +292,7 @@ func branchExists( if apiErr, ok := errors.AsType[*github.ApiError](err); ok { // Only surface errors we have positive evidence are access failures. // For 404 or unclassifiable errors (e.g., Kind == KindUnknown because - // gh's output wasn't parseable) keep walking the candidate branches — + // gh's output wasn't parsable) keep walking the candidate branches — // this matches the historical "treat any non-2xx as not-a-branch" // behavior and avoids regressing on transient or unexpected gh // output formats. diff --git a/cli/azd/pkg/tools/github/api_error.go b/cli/azd/pkg/tools/github/api_error.go index 564bc908a79..f237cb6529f 100644 --- a/cli/azd/pkg/tools/github/api_error.go +++ b/cli/azd/pkg/tools/github/api_error.go @@ -70,7 +70,7 @@ func (k ApiErrorKind) String() string { // ApiError represents a structured error returned from a `gh api` invocation. // -// It is built from the underlying gh CLI's output (the JSON body that GitHub's +// It is built from the underlying gh CLI output (the JSON body that GitHub's // REST API writes to stdout on errors, plus the human-readable stderr) so // callers can branch on the failure mode without doing brittle substring // matching against opaque error strings. @@ -134,7 +134,7 @@ func (e *ApiError) IsNotFound() bool { return e.Kind == KindNotFound } -// httpStatusRe matches gh CLI's standard "(HTTP )" suffix that appears +// httpStatusRe matches the standard "(HTTP )" suffix that gh CLI prints // at the end of error messages from `gh api` when the request was issued and // returned a non-2xx response. Example: // From 328148f4354123b68a0db8b79f0a946ab8185124 Mon Sep 17 00:00:00 2001 From: Jeffrey Chen Date: Thu, 30 Apr 2026 21:47:46 +0000 Subject: [PATCH 3/5] Address PR review feedback for GitHub API error handling --- cli/azd/pkg/templates/gh_errors.go | 13 +++++ cli/azd/pkg/templates/gh_source_test.go | 68 ++++++++++++++++++++++ cli/azd/pkg/tools/github/api_error.go | 3 + cli/azd/pkg/tools/github/api_error_test.go | 5 ++ 4 files changed, 89 insertions(+) diff --git a/cli/azd/pkg/templates/gh_errors.go b/cli/azd/pkg/templates/gh_errors.go index 77e88dc68f3..dbf5bf9b1f6 100644 --- a/cli/azd/pkg/templates/gh_errors.go +++ b/cli/azd/pkg/templates/gh_errors.go @@ -97,6 +97,19 @@ func suggestionForApiError(apiErr *github.ApiError) *internal.ErrorWithSuggestio "it includes the 'repo' scope. Run `gh auth status` to confirm which account " + "gh is using.", } + case github.KindServerError: + return &internal.ErrorWithSuggestion{ + Message: "GitHub returned a server error (HTTP 5xx). This usually indicates a " + + "transient issue on GitHub's side rather than a problem with your request.", + Suggestion: "Wait a few minutes and try again. If the problem persists, check " + + "https://www.githubstatus.com/ for ongoing incidents.", + Links: []errorhandler.ErrorLink{ + { + URL: "https://www.githubstatus.com/", + Title: "GitHub Status", + }, + }, + } } return nil } diff --git a/cli/azd/pkg/templates/gh_source_test.go b/cli/azd/pkg/templates/gh_source_test.go index 4fc768649ed..a7ddc12a6ba 100644 --- a/cli/azd/pkg/templates/gh_source_test.go +++ b/cli/azd/pkg/templates/gh_source_test.go @@ -742,3 +742,71 @@ func Test_ParseGitHubUrl_AccessErrorShortCircuits(t *testing.T) { // And the misleading "could not find a valid branch" message must NOT appear. require.NotContains(t, err.Error(), "could not find a valid branch") } + +// Test_ParseGitHubUrl_RepoNotAccessibleFallback verifies the private/EMU +// codepath: when every branch lookup returns 404 (no positive access error), +// resolveBranchAndPath probes /repos/{slug}; if that also returns 404 the +// error surfaces as *RepoNotAccessibleError instead of the misleading +// "could not find a valid branch" message. This is the other half of the +// PR's value alongside Test_ParseGitHubUrl_AccessErrorShortCircuits. +func Test_ParseGitHubUrl_RepoNotAccessibleFallback(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "--version" + }).Respond(exec.RunResult{Stdout: github.Version.String()}) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "auth" && args.Args[1] == "status" + }).Respond(exec.RunResult{Stdout: "Logged in to"}) + + // Every gh api call (branch lookups + repo probe) returns a 404 with the + // real GitHub JSON error envelope so parseApiError classifies as KindNotFound. + notFoundStdout := `{"message":"Not Found","documentation_url":"...","status":"404"}` + notFoundStderr := "gh: Not Found (HTTP 404)" + branchCallCount := 0 + repoProbeCount := 0 + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "api" + }).RespondFn(func(args exec.RunArgs) (exec.RunResult, error) { + apiURL := args.Args[1] + switch { + case strings.Contains(apiURL, "/branches/"): + branchCallCount++ + case strings.HasSuffix(apiURL, "/repos/owner/repo"): + repoProbeCount++ + } + return exec.RunResult{Stdout: notFoundStdout, Stderr: notFoundStderr, ExitCode: 1}, + fmt.Errorf("exit code: 1, stdout: %s, stderr: %s", notFoundStdout, notFoundStderr) + }) + + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) + + // 4-segment branch+path so branch walker exhausts all candidates before + // falling back to the repo probe. + _, err := ParseGitHubUrl( + *mockContext.Context, + "https://github.com/owner/repo/blob/feature/sub/path/file.yaml", + ghCli, + ) + require.Error(t, err) + + // All 4 branch candidates were tried (no short-circuit, since 404 is + // "not a branch", not an access failure)... + require.Equal(t, 4, branchCallCount, "expected branch walk to try every candidate on 404") + // ...followed by exactly one /repos/{slug} probe. + require.Equal(t, 1, repoProbeCount, "expected exactly one /repos/{slug} probe after branch walk") + + // The error must surface as *RepoNotAccessibleError so the suggestion + // pipeline (gh_errors.go) attaches EMU/private-repo guidance. + repoErr, ok := errors.AsType[*RepoNotAccessibleError](err) + require.True(t, ok, "expected *RepoNotAccessibleError, got %T: %v", err, err) + require.Equal(t, "github.com", repoErr.Hostname) + require.Equal(t, "owner/repo", repoErr.RepoSlug) + + // And the misleading "could not find a valid branch" message must NOT appear. + require.NotContains(t, err.Error(), "could not find a valid branch") +} diff --git a/cli/azd/pkg/tools/github/api_error.go b/cli/azd/pkg/tools/github/api_error.go index f237cb6529f..3584494accd 100644 --- a/cli/azd/pkg/tools/github/api_error.go +++ b/cli/azd/pkg/tools/github/api_error.go @@ -103,6 +103,9 @@ func (e *ApiError) Error() string { case KindRateLimited: return fmt.Sprintf("gh api %s: GitHub API rate limit exceeded (HTTP %d)", e.URL, e.StatusCode) case KindUnknown: + if e.Underlying == nil { + return fmt.Sprintf("gh api %s: unknown error", e.URL) + } return fmt.Sprintf("gh api %s: %s", e.URL, e.Underlying) default: if e.Message != "" { diff --git a/cli/azd/pkg/tools/github/api_error_test.go b/cli/azd/pkg/tools/github/api_error_test.go index ad7a0552a1e..8431d888a9a 100644 --- a/cli/azd/pkg/tools/github/api_error_test.go +++ b/cli/azd/pkg/tools/github/api_error_test.go @@ -181,6 +181,11 @@ func TestApiError_ErrorMessage(t *testing.T) { &ApiError{URL: "https://api.github.com/x", Kind: KindUnknown, Underlying: errors.New("boom")}, "gh api https://api.github.com/x: boom", }, + { + "unknown kind with nil underlying", + &ApiError{URL: "https://api.github.com/x", Kind: KindUnknown}, + "gh api https://api.github.com/x: unknown error", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { From 5a67833744007a65f7fefd564168098fbe13e623 Mon Sep 17 00:00:00 2001 From: Jeffrey Chen Date: Thu, 30 Apr 2026 22:21:12 +0000 Subject: [PATCH 4/5] Address feedback --- cli/azd/pkg/templates/gh_errors.go | 26 ++++++++++++++-------- cli/azd/pkg/templates/gh_source.go | 8 ++++--- cli/azd/pkg/templates/gh_source_test.go | 4 ++-- cli/azd/pkg/tools/github/api_error.go | 8 ++++--- cli/azd/pkg/tools/github/api_error_test.go | 26 ++++++++++++++++++++++ 5 files changed, 55 insertions(+), 17 deletions(-) diff --git a/cli/azd/pkg/templates/gh_errors.go b/cli/azd/pkg/templates/gh_errors.go index dbf5bf9b1f6..6076a0a47e6 100644 --- a/cli/azd/pkg/templates/gh_errors.go +++ b/cli/azd/pkg/templates/gh_errors.go @@ -53,16 +53,24 @@ func suggestionForApiError(apiErr *github.ApiError) *internal.ErrorWithSuggestio return &internal.ErrorWithSuggestion{ Message: "The GitHub organization that owns this repository requires SAML SSO " + "authorization for your token before it can be used.", - Suggestion: "Open https://github.com/settings/tokens, find the personal access " + - "token you're using, click 'Configure SSO', and authorize the organization. " + - "If you're signed in with `gh auth login`, run " + - "`gh auth refresh -h github.com` and complete the SSO flow in the browser.", + Suggestion: "If you signed in with `gh auth login`, run `gh auth refresh` and " + + "complete the SSO authorization in the browser when prompted (use " + + "`gh auth refresh -h ` for non-default hosts). If you're using a " + + "personal access token, open your GitHub token settings, locate the " + + "token, click 'Configure SSO', and authorize the organization that owns " + + "this repository.", Links: []errorhandler.ErrorLink{ { URL: "https://docs.github.com/enterprise-cloud@latest/authentication/" + - "authenticating-with-saml-single-sign-on/" + - "authorizing-a-personal-access-token-for-use-with-saml-single-sign-on", - Title: "Authorizing a personal access token for use with SAML SSO", + "authenticating-with-single-sign-on/" + + "about-authentication-with-single-sign-on", + Title: "About authentication with single sign-on", + }, + { + URL: "https://docs.github.com/enterprise-cloud@latest/authentication/" + + "authenticating-with-single-sign-on/" + + "authorizing-a-personal-access-token-for-use-with-single-sign-on", + Title: "Authorizing a personal access token for use with single sign-on", }, }, } @@ -75,8 +83,8 @@ func suggestionForApiError(apiErr *github.ApiError) *internal.ErrorWithSuggestio "(typically up to one hour).", Links: []errorhandler.ErrorLink{ { - URL: "https://docs.github.com/rest/overview/rate-limits-for-the-rest-api", - Title: "GitHub REST API rate limits", + URL: "https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api", + Title: "Rate limits for the REST API", }, }, } diff --git a/cli/azd/pkg/templates/gh_source.go b/cli/azd/pkg/templates/gh_source.go index e53e69415ed..1c1680ad1bb 100644 --- a/cli/azd/pkg/templates/gh_source.go +++ b/cli/azd/pkg/templates/gh_source.go @@ -233,8 +233,9 @@ func checkRepoAccessible( return apiErr } // 404 on the repo itself: the user can't see this repo, and the original - // branch error would mislead. Wrap as a RepoNotAccessibleError so the - // error_suggestions.yaml pipeline can attach EMU/private-repo guidance. + // branch error would mislead. Wrap as a RepoNotAccessibleError so + // withGitHubSuggestion can attach EMU/private-repo guidance from + // pkg/templates/gh_errors.go (inline, not via the YAML pipeline). if apiErr.IsNotFound() { return &RepoNotAccessibleError{ Hostname: hostname, @@ -276,7 +277,8 @@ func (e *RepoNotAccessibleError) Unwrap() error { return e.Cause } // - (false, err) — the API call failed for an authentication, authorization, // rate-limit, or server-side reason. The error is the underlying // *github.ApiError so the caller can short-circuit the branch-walk and -// the error_suggestions.yaml pipeline can surface an actionable message. +// withGitHubSuggestion (pkg/templates/gh_errors.go) can attach an +// actionable message inline. func branchExists( ctx context.Context, ghCli *github.Cli, diff --git a/cli/azd/pkg/templates/gh_source_test.go b/cli/azd/pkg/templates/gh_source_test.go index a7ddc12a6ba..3b8ab7048fd 100644 --- a/cli/azd/pkg/templates/gh_source_test.go +++ b/cli/azd/pkg/templates/gh_source_test.go @@ -728,8 +728,8 @@ func Test_ParseGitHubUrl_AccessErrorShortCircuits(t *testing.T) { ) require.Error(t, err) - // Must surface as the typed *github.ApiError so the YAML pipeline - // can map it to a SAML-specific suggestion. + // Must surface as the typed *github.ApiError so withGitHubSuggestion + // can map it to a SAML-specific suggestion inline. apiErr, ok := errors.AsType[*github.ApiError](err) require.True(t, ok, "expected *github.ApiError, got %T: %v", err, err) require.Equal(t, github.KindSAMLBlocked, apiErr.Kind) diff --git a/cli/azd/pkg/tools/github/api_error.go b/cli/azd/pkg/tools/github/api_error.go index 3584494accd..5e314241f5b 100644 --- a/cli/azd/pkg/tools/github/api_error.go +++ b/cli/azd/pkg/tools/github/api_error.go @@ -194,8 +194,10 @@ func parseApiError(url, stdout, stderr string, err error) *ApiError { // parseGitHubErrorBody returns the GitHub JSON error envelope if stdout // contains one, otherwise nil. We tolerate leading/trailing whitespace and -// require both Message and Status to be non-empty so we don't misinterpret -// arbitrary JSON success bodies as errors. +// require Message to be non-empty so we don't misinterpret arbitrary JSON +// success bodies as errors. Status is optional — many GitHub REST error +// bodies include only message/documentation_url, and the HTTP status is +// then recovered from stderr's "(HTTP NNN)" marker by the caller. func parseGitHubErrorBody(stdout string) *githubErrorBody { stdout = strings.TrimSpace(stdout) if !strings.HasPrefix(stdout, "{") { @@ -205,7 +207,7 @@ func parseGitHubErrorBody(stdout string) *githubErrorBody { if err := json.Unmarshal([]byte(stdout), &body); err != nil { return nil } - if body.Message == "" || body.Status == "" { + if body.Message == "" { return nil } return &body diff --git a/cli/azd/pkg/tools/github/api_error_test.go b/cli/azd/pkg/tools/github/api_error_test.go index 8431d888a9a..5e8dce9cf0f 100644 --- a/cli/azd/pkg/tools/github/api_error_test.go +++ b/cli/azd/pkg/tools/github/api_error_test.go @@ -66,6 +66,15 @@ func TestParseApiError_StatusFromJSONBody(t *testing.T) { "gh: weird (HTTP 502)", 502, }, + { + // GitHub error bodies often include only message+documentation_url + // (no status). We must still capture the message and recover the + // HTTP code from stderr's "(HTTP NNN)" marker. + "message-only JSON falls back to stderr for status", + `{"message":"Bad credentials","documentation_url":"https://docs.github.com/rest"}`, + "gh: Bad credentials (HTTP 401)", + 401, + }, { "no marker anywhere", "", @@ -129,6 +138,23 @@ func TestParseApiError_DoesNotMisclassifySsoInRepoName(t *testing.T) { require.True(t, apiErr.IsNotFound()) } +func TestParseApiError_MessageOnlyBodyCapturesMessage(t *testing.T) { + t.Parallel() + // Verify the diagnostic Message is captured even when the JSON body + // omits "status" — a common shape for GitHub REST errors. The HTTP code + // still comes from the stderr "(HTTP NNN)" marker. + apiErr := parseApiError( + "https://api.github.com/repos/o/r", + `{"message":"Bad credentials","documentation_url":"https://docs.github.com/rest"}`, + "gh: Bad credentials (HTTP 401)", + errors.New("exit 1"), + ) + require.NotNil(t, apiErr) + require.Equal(t, "Bad credentials", apiErr.Message) + require.Equal(t, 401, apiErr.StatusCode) + require.Equal(t, KindUnauthorized, apiErr.Kind) +} + func TestParseApiError_NoOutputAvailable(t *testing.T) { t.Parallel() // gh failed before issuing the request (network error, missing binary, From 20f15d58449c04a2610e37711df692f00a47bd3c Mon Sep 17 00:00:00 2001 From: Jeffrey Chen Date: Thu, 30 Apr 2026 22:40:46 +0000 Subject: [PATCH 5/5] Add test coverage and address review feedback - F1: Add gh_errors_test.go covering suggestionForApiError per-kind, suggestionForRepoNotAccessible, and withGitHubSuggestion dispatch. - F3: Add Test_ParseGitHubUrl_RepoAccessibleFallsThrough and Test_ParseGitHubUrl_RepoProbeReturnsClassifiedError covering the two remaining /repos/{slug} probe outcomes. - F4: Add TestClassifyKind_HttpStatusToKind covering all status->Kind mappings and TestClassifyKind_SAMLPhraseVariants for SAML detection. - F5: Add malformed-JSON, HTML-body, and message-only fallback rows to TestParseApiError_StatusFromJSONBody. - F6: Wrap ParseGitHubUrl error in newGhTemplateSource with the failing template URL so users see which operation failed. - F9: Tighten KindOther doc comment to match classifyKind behavior. - F13: Add TestParseApiError_StderrHttpStatusFallback covering the '(HTTP NNN)' regex fallback edge cases (multiple markers, missing marker, malformed patterns, case sensitivity). - Use t.Context() in new gh_source_test.go subtests to match repo convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/templates/gh_errors_test.go | 157 ++++++++++++++++++++ cli/azd/pkg/templates/gh_source.go | 2 +- cli/azd/pkg/templates/gh_source_test.go | 110 +++++++++++++- cli/azd/pkg/tools/github/api_error.go | 6 +- cli/azd/pkg/tools/github/api_error_test.go | 163 +++++++++++++++++++++ 5 files changed, 433 insertions(+), 5 deletions(-) create mode 100644 cli/azd/pkg/templates/gh_errors_test.go diff --git a/cli/azd/pkg/templates/gh_errors_test.go b/cli/azd/pkg/templates/gh_errors_test.go new file mode 100644 index 00000000000..1790ffff7db --- /dev/null +++ b/cli/azd/pkg/templates/gh_errors_test.go @@ -0,0 +1,157 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package templates + +import ( + "errors" + "strings" + "testing" + + "github.com/azure/azure-dev/cli/azd/internal" + "github.com/azure/azure-dev/cli/azd/pkg/tools/github" + "github.com/stretchr/testify/require" +) + +// TestSuggestionForApiError_PerKind verifies that every classified +// ApiErrorKind that is meant to surface user guidance produces a non-nil +// *internal.ErrorWithSuggestion containing the substrings users rely on +// (the suggestion text and the relevant doc link). Kinds that intentionally +// return nil (NotFound, Other, Unknown) are also asserted to ensure we +// don't silently start emitting suggestions where none are expected. +func TestSuggestionForApiError_PerKind(t *testing.T) { + t.Parallel() + cases := []struct { + name string + kind github.ApiErrorKind + wantNil bool + wantSnippet string // substring required in the rendered suggestion text + wantLink string // substring required in at least one Links[].URL (empty = skip) + }{ + {"SAMLBlocked", github.KindSAMLBlocked, false, + "SAML SSO", "authenticating-with-single-sign-on"}, + {"RateLimited", github.KindRateLimited, false, + "rate limit", "rate-limits-for-the-rest-api"}, + {"Unauthorized", github.KindUnauthorized, false, + "gh auth login", ""}, + {"Forbidden", github.KindForbidden, false, + "gh auth status", ""}, + {"ServerError", github.KindServerError, false, + "server error", "githubstatus.com"}, + {"NotFound returns nil — RepoNotAccessibleError handles that path", github.KindNotFound, true, "", ""}, + {"Other returns nil — falls through to typed *ApiError.Error()", github.KindOther, true, "", ""}, + {"Unknown returns nil — surfaces underlying error", github.KindUnknown, true, "", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + apiErr := &github.ApiError{ + URL: "https://api.github.com/repos/o/r", + Kind: tc.kind, + StatusCode: 500, // arbitrary, not asserted + } + got := suggestionForApiError(apiErr) + if tc.wantNil { + require.Nil(t, got, "expected nil suggestion for %s", tc.kind) + return + } + require.NotNil(t, got, "expected non-nil suggestion for %s", tc.kind) + combined := got.Message + " " + got.Suggestion + require.Contains(t, combined, tc.wantSnippet) + if tc.wantLink != "" { + var found bool + for _, l := range got.Links { + if strings.Contains(l.URL, tc.wantLink) { + found = true + break + } + } + require.True(t, found, "expected a Links[] URL containing %q, got %+v", tc.wantLink, got.Links) + } + }) + } +} + +// TestSuggestionForRepoNotAccessible verifies the dedicated +// RepoNotAccessibleError suggestion is emitted with EMU/private-repo guidance +// and a non-empty Suggestion field — Message is intentionally empty so the +// renderer falls back to RepoNotAccessibleError.Error() (which already +// contains the repo slug) and we don't duplicate the same text twice. +func TestSuggestionForRepoNotAccessible(t *testing.T) { + t.Parallel() + got := suggestionForRepoNotAccessible(&RepoNotAccessibleError{ + Hostname: "github.com", + RepoSlug: "owner/repo", + }) + require.NotNil(t, got) + require.Empty(t, got.Message, "Message must be empty so renderer uses RepoNotAccessibleError.Error()") + require.Contains(t, got.Suggestion, "gh auth status") + require.Contains(t, got.Suggestion, "EMU") +} + +// TestWithGitHubSuggestion_Dispatch verifies the wrapper: +// - returns nil for nil input +// - wraps *github.ApiError with a classified suggestion (preserving the +// original error in ErrorWithSuggestion.Err so the chain still unwraps +// to the typed *ApiError) +// - wraps *RepoNotAccessibleError into a suggestion (always — never nil, +// because RepoNotAccessibleError is itself a strong signal) +// - returns the original error unchanged for unrecognized types +// - returns the original error unchanged when the typed error has a kind +// that intentionally has no suggestion (e.g., KindNotFound on its own — +// RepoNotAccessibleError is the surface for the "repo invisible" case) +func TestWithGitHubSuggestion_Dispatch(t *testing.T) { + t.Parallel() + + t.Run("nil in nil out", func(t *testing.T) { + t.Parallel() + require.NoError(t, withGitHubSuggestion(nil)) + }) + + t.Run("ApiError with suggestion-bearing kind is wrapped", func(t *testing.T) { + t.Parallel() + original := &github.ApiError{ + URL: "https://api.github.com/repos/o/r/branches/main", + Kind: github.KindSAMLBlocked, + StatusCode: 403, + } + wrapped := withGitHubSuggestion(original) + ews, ok := errors.AsType[*internal.ErrorWithSuggestion](wrapped) + require.True(t, ok, "expected *internal.ErrorWithSuggestion, got %T", wrapped) + // The ErrorWithSuggestion wraps the original so error chain still + // unwraps to the typed ApiError for downstream consumers. + got, ok := errors.AsType[*github.ApiError](ews) + require.True(t, ok, "ErrorWithSuggestion must preserve *ApiError in chain") + require.Same(t, original, got) + }) + + t.Run("ApiError without suggestion (KindNotFound) returned unchanged", func(t *testing.T) { + t.Parallel() + original := &github.ApiError{ + URL: "https://api.github.com/repos/o/r/branches/main", + Kind: github.KindNotFound, + StatusCode: 404, + } + got := withGitHubSuggestion(original) + require.Same(t, original, got, "no suggestion → return original error untouched") + }) + + t.Run("RepoNotAccessibleError is always wrapped", func(t *testing.T) { + t.Parallel() + original := &RepoNotAccessibleError{Hostname: "github.com", RepoSlug: "owner/repo"} + wrapped := withGitHubSuggestion(original) + ews, ok := errors.AsType[*internal.ErrorWithSuggestion](wrapped) + require.True(t, ok) + // Chain still unwraps to the typed RepoNotAccessibleError. + got, ok := errors.AsType[*RepoNotAccessibleError](ews) + require.True(t, ok) + require.Same(t, original, got) + }) + + t.Run("unrecognized error returned unchanged", func(t *testing.T) { + t.Parallel() + original := errors.New("some random error") + got := withGitHubSuggestion(original) + require.Same(t, original, got) + }) +} diff --git a/cli/azd/pkg/templates/gh_source.go b/cli/azd/pkg/templates/gh_source.go index 1c1680ad1bb..403b89d5629 100644 --- a/cli/azd/pkg/templates/gh_source.go +++ b/cli/azd/pkg/templates/gh_source.go @@ -344,7 +344,7 @@ func newGhTemplateSource( // Parse the GitHub URL to extract repository information urlInfo, err := ParseGitHubUrl(ctx, urlArg, ghCli) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create template source for %q: %w", urlArg, err) } authResult, err := ghCli.GetAuthStatus(ctx, urlInfo.Hostname) diff --git a/cli/azd/pkg/templates/gh_source_test.go b/cli/azd/pkg/templates/gh_source_test.go index 3b8ab7048fd..4fe50e52a00 100644 --- a/cli/azd/pkg/templates/gh_source_test.go +++ b/cli/azd/pkg/templates/gh_source_test.go @@ -693,7 +693,7 @@ func Test_ParseGitHubUrl_NotAuthenticated(t *testing.T) { // surface is the typed error so the YAML error-suggestion pipeline can // attach an actionable message. func Test_ParseGitHubUrl_AccessErrorShortCircuits(t *testing.T) { - mockContext := mocks.NewMockContext(context.Background()) + mockContext := mocks.NewMockContext(t.Context()) mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { return strings.Contains(command, string(filepath.Separator)+"gh") && @@ -750,7 +750,7 @@ func Test_ParseGitHubUrl_AccessErrorShortCircuits(t *testing.T) { // "could not find a valid branch" message. This is the other half of the // PR's value alongside Test_ParseGitHubUrl_AccessErrorShortCircuits. func Test_ParseGitHubUrl_RepoNotAccessibleFallback(t *testing.T) { - mockContext := mocks.NewMockContext(context.Background()) + mockContext := mocks.NewMockContext(t.Context()) mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { return strings.Contains(command, string(filepath.Separator)+"gh") && @@ -810,3 +810,109 @@ func Test_ParseGitHubUrl_RepoNotAccessibleFallback(t *testing.T) { // And the misleading "could not find a valid branch" message must NOT appear. require.NotContains(t, err.Error(), "could not find a valid branch") } + +// Test_ParseGitHubUrl_RepoAccessibleFallsThrough covers the "repo exists, +// branch genuinely doesn't" path: every branch candidate returns 404 but +// the /repos/{slug} probe returns 200, so checkRepoAccessible returns nil +// and the original "could not find a valid branch" message surfaces. +func Test_ParseGitHubUrl_RepoAccessibleFallsThrough(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "--version" + }).Respond(exec.RunResult{Stdout: github.Version.String()}) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "auth" && args.Args[1] == "status" + }).Respond(exec.RunResult{Stdout: "Logged in to"}) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "api" + }).RespondFn(func(args exec.RunArgs) (exec.RunResult, error) { + apiURL := args.Args[1] + // Repo probe succeeds; branch probes all 404. + if strings.HasSuffix(apiURL, "/repos/owner/repo") { + return exec.RunResult{Stdout: `{"name":"repo"}`}, nil + } + stdout := `{"message":"Not Found","documentation_url":"...","status":"404"}` + stderr := "gh: Not Found (HTTP 404)" + return exec.RunResult{Stdout: stdout, Stderr: stderr, ExitCode: 1}, + fmt.Errorf("exit code: 1, stdout: %s, stderr: %s", stdout, stderr) + }) + + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) + _, err := ParseGitHubUrl( + *mockContext.Context, + "https://github.com/owner/repo/blob/feature/sub/path/file.yaml", + ghCli, + ) + require.Error(t, err) + + // Repo IS accessible, so RepoNotAccessibleError must NOT appear. + _, ok := errors.AsType[*RepoNotAccessibleError](err) + require.False(t, ok, "repo is accessible — must not surface RepoNotAccessibleError") + // Original "no valid branch" message is the right surface here. + require.Contains(t, err.Error(), "could not find a valid branch") +} + +// Test_ParseGitHubUrl_RepoProbeReturnsClassifiedError covers the case +// where the branch walk exhausts on 404 but the /repos/{slug} probe itself +// returns a classified access error (e.g., SAML on the org). The repo +// probe's typed error must propagate (not the misleading branch message +// and not RepoNotAccessibleError, since this is an auth failure rather +// than a not-found). +func Test_ParseGitHubUrl_RepoProbeReturnsClassifiedError(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "--version" + }).Respond(exec.RunResult{Stdout: github.Version.String()}) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "auth" && args.Args[1] == "status" + }).Respond(exec.RunResult{Stdout: "Logged in to"}) + + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, string(filepath.Separator)+"gh") && + args.Args[0] == "api" + }).RespondFn(func(args exec.RunArgs) (exec.RunResult, error) { + apiURL := args.Args[1] + if strings.HasSuffix(apiURL, "/repos/owner/repo") { + // Repo probe returns SAML 403 (e.g., the org enforces SSO and + // the user's token isn't authorized). + samlStdout := `{"message":"Resource protected by organization SAML enforcement.",` + + `"documentation_url":"...","status":"403"}` + samlStderr := "gh: Resource protected by organization SAML enforcement. (HTTP 403)" + return exec.RunResult{Stdout: samlStdout, Stderr: samlStderr, ExitCode: 1}, + fmt.Errorf("exit code: 1, stdout: %s, stderr: %s", samlStdout, samlStderr) + } + // Branch probes all 404. + stdout := `{"message":"Not Found","documentation_url":"...","status":"404"}` + stderr := "gh: Not Found (HTTP 404)" + return exec.RunResult{Stdout: stdout, Stderr: stderr, ExitCode: 1}, + fmt.Errorf("exit code: 1, stdout: %s, stderr: %s", stdout, stderr) + }) + + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) + _, err := ParseGitHubUrl( + *mockContext.Context, + "https://github.com/owner/repo/blob/feature/sub/path/file.yaml", + ghCli, + ) + require.Error(t, err) + + // Must surface the typed *github.ApiError from the repo probe — not + // RepoNotAccessibleError (which is reserved for repo-probe 404s) and + // not the misleading "no valid branch" text. + apiErr, ok := errors.AsType[*github.ApiError](err) + require.True(t, ok, "expected *github.ApiError from repo probe, got %T: %v", err, err) + require.Equal(t, github.KindSAMLBlocked, apiErr.Kind) + require.NotContains(t, err.Error(), "could not find a valid branch") + _, isRepoErr := errors.AsType[*RepoNotAccessibleError](err) + require.False(t, isRepoErr, "SAML on repo probe must not surface as RepoNotAccessibleError") +} diff --git a/cli/azd/pkg/tools/github/api_error.go b/cli/azd/pkg/tools/github/api_error.go index 5e314241f5b..1938ae8cf76 100644 --- a/cli/azd/pkg/tools/github/api_error.go +++ b/cli/azd/pkg/tools/github/api_error.go @@ -41,8 +41,10 @@ const ( KindNotFound // KindServerError indicates a 5xx response from GitHub. KindServerError - // KindOther indicates a non-2xx response that doesn't fit any of the - // categories above (e.g., 4xx codes other than 401/403/404). + // KindOther indicates a classified non-2xx response that doesn't match + // any of the more specific kinds above (e.g., 4xx codes other than + // 401/403/404, or rare status codes outside the standard buckets). Has + // a known StatusCode but no targeted suggestion is provided. KindOther ) diff --git a/cli/azd/pkg/tools/github/api_error_test.go b/cli/azd/pkg/tools/github/api_error_test.go index 5e8dce9cf0f..d904ef652ee 100644 --- a/cli/azd/pkg/tools/github/api_error_test.go +++ b/cli/azd/pkg/tools/github/api_error_test.go @@ -48,6 +48,26 @@ func TestParseApiError_StatusFromJSONBody(t *testing.T) { samlStderrLine, 403, }, + { + // Plain 403 without SAML/rate-limit phrases must classify as + // KindForbidden (asserted in TestClassifyKind_HttpStatusToKind). + "403 plain forbidden", + `{"message":"Resource not accessible by integration","documentation_url":"...","status":"403"}`, + "gh: Resource not accessible by integration (HTTP 403)", + 403, + }, + { + "500 server error from JSON", + `{"message":"Server Error","documentation_url":"...","status":"500"}`, + "gh: Server Error (HTTP 500)", + 500, + }, + { + "422 unprocessable (KindOther bucket)", + `{"message":"Validation Failed","documentation_url":"...","status":"422"}`, + "gh: Validation Failed (HTTP 422)", + 422, + }, { "404 from JSON", `{"message":"Not Found","documentation_url":"...","status":"404"}`, @@ -66,6 +86,24 @@ func TestParseApiError_StatusFromJSONBody(t *testing.T) { "gh: weird (HTTP 502)", 502, }, + { + // Truncated/invalid JSON (e.g., proxy returning HTML, gh + // crashing mid-write). We must not panic on json.Unmarshal + // and must still recover the status code from stderr. + "malformed JSON falls back to stderr", + `{"message":"oops`, + "gh: oops (HTTP 503)", + 503, + }, + { + // Proxy-style HTML response: not JSON at all. The early + // "not a JSON object" guard short-circuits without invoking + // the unmarshaler, and stderr still recovers the status. + "HTML body falls back to stderr", + "Bad gateway", + "gh: Bad gateway (HTTP 502)", + 502, + }, { // GitHub error bodies often include only message+documentation_url // (no status). We must still capture the message and recover the @@ -155,6 +193,131 @@ func TestParseApiError_MessageOnlyBodyCapturesMessage(t *testing.T) { require.Equal(t, KindUnauthorized, apiErr.Kind) } +// TestClassifyKind_HttpStatusToKind exercises the full status-code → Kind +// mapping in classifyKind, including the buckets (401/403/404/5xx/other) +// and the SAML / rate-limit phrase overrides on a 403. Each row uses +// realistic stderr/JSON shapes from gh CLI. +func TestClassifyKind_HttpStatusToKind(t *testing.T) { + t.Parallel() + cases := []struct { + name string + status int + text string + want ApiErrorKind + }{ + {"401 → Unauthorized", 401, "Bad credentials", KindUnauthorized}, + {"403 plain → Forbidden", 403, "Resource not accessible by integration", KindForbidden}, + {"403 SAML → SAMLBlocked", 403, + "Resource protected by organization SAML enforcement", KindSAMLBlocked}, + {"403 secondary rate limit → RateLimited", 403, + "You have exceeded a secondary rate limit", KindRateLimited}, + {"403 primary rate limit → RateLimited", 403, + "API rate limit exceeded for user ID 12345", KindRateLimited}, + {"404 → NotFound", 404, "Not Found", KindNotFound}, + {"422 → Other", 422, "Validation Failed", KindOther}, + {"409 → Other", 409, "Conflict", KindOther}, + {"500 → ServerError", 500, "Server Error", KindServerError}, + {"502 → ServerError", 502, "Bad Gateway", KindServerError}, + {"503 → ServerError", 503, "Service Unavailable", KindServerError}, + {"0 (no status) → Unknown", 0, "", KindUnknown}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tc.want, classifyKind(tc.status, tc.text)) + }) + } +} + +// TestClassifyKind_SAMLPhraseVariants verifies all phrase patterns the +// SAML detector recognizes. Adding a new phrase to classifyKind should be +// accompanied by a new row here. +func TestClassifyKind_SAMLPhraseVariants(t *testing.T) { + t.Parallel() + phrases := []string{ + "Resource protected by organization SAML enforcement", + "You must use SAML SSO before accessing this resource", + "Your token has not been granted SSO authorization for this organization", + "SSO required for this organization", + // Casing must not matter — classifier lowercases the input first. + "resource protected by organization saml enforcement", + "SAML SSO REQUIRED", + } + for _, p := range phrases { + t.Run(p, func(t *testing.T) { + t.Parallel() + require.Equal(t, KindSAMLBlocked, classifyKind(403, p)) + }) + } +} + +// TestParseApiError_StderrHttpStatusFallback exercises the "(HTTP NNN)" +// regex fallback that recovers the status code from gh's stderr when the +// JSON body on stdout is missing/unusable. Documents what the regex does +// and does NOT match (3-digit form only, first match wins). +func TestParseApiError_StderrHttpStatusFallback(t *testing.T) { + t.Parallel() + cases := []struct { + name string + stderr string + wantStatus int + wantKind ApiErrorKind + }{ + { + name: "standard gh suffix", + stderr: "gh: Not Found (HTTP 404)", + wantStatus: 404, + wantKind: KindNotFound, + }, + { + name: "extra trailing text after marker", + stderr: "gh: Bad credentials (HTTP 401)\nTry running gh auth login", + wantStatus: 401, + wantKind: KindUnauthorized, + }, + { + name: "first marker wins when multiple are present", + stderr: "gh: outer (HTTP 502) inner (HTTP 500)", + wantStatus: 502, + wantKind: KindServerError, + }, + { + name: "no marker present", + stderr: "gh: connection refused", + wantStatus: 0, + wantKind: KindUnknown, + }, + { + name: "extra whitespace inside parens does not match", + stderr: "gh: weird ( HTTP 500 )", + wantStatus: 0, + wantKind: KindUnknown, + }, + { + name: "two-digit code does not match (regex requires 3 digits)", + stderr: "gh: weird (HTTP 99)", + wantStatus: 0, + wantKind: KindUnknown, + }, + { + name: "lowercase 'http' does not match (case sensitive)", + stderr: "gh: weird (http 500)", + wantStatus: 0, + wantKind: KindUnknown, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + // Empty stdout forces reliance on the stderr fallback only. + apiErr := parseApiError("https://api.github.com/x", "", tc.stderr, errors.New("gh failed")) + require.NotNil(t, apiErr) + require.Equal(t, tc.wantStatus, apiErr.StatusCode) + require.Equal(t, tc.wantKind, apiErr.Kind) + }) + } +} + func TestParseApiError_NoOutputAvailable(t *testing.T) { t.Parallel() // gh failed before issuing the request (network error, missing binary,