From 31441a06b4d840f150c808042d964ee716acce53 Mon Sep 17 00:00:00 2001 From: Christopher Langton Date: Sat, 14 Nov 2026 02:44:49 +1100 Subject: [PATCH] fix(analyze): a spent GitHub quota at preflight skips the forge pass instead of failing the run When the job token's quota was already spent by other jobs, the preflight repository read got 403 'API rate limit exceeded for installation'. repoAccessError reported that as 'cannot read Vulnetix/cli', and analyze exited without a report. A rate-limit error at preflight is now forge.ErrRateLimited, and analyze.Check skips the forge pass: its metrics are not measured, and the git, file, dependency and trust metrics are still reported. IsRateLimit also matches wrapped errors. --- cmd/analyze.go | 4 +- internal/analyze/forge/auth.go | 20 ++++++++++ internal/analyze/forge/auth_check_test.go | 45 +++++++++++++++++++++++ internal/analyze/forge/budget.go | 14 ++----- internal/analyze/run.go | 8 ++++ 5 files changed, 80 insertions(+), 11 deletions(-) diff --git a/cmd/analyze.go b/cmd/analyze.go index 9248e8b938..05a76b9abf 100644 --- a/cmd/analyze.go +++ b/cmd/analyze.go @@ -69,7 +69,9 @@ GitHub authentication The token's hourly quota is usually shared (in Actions, with every other job in the repository), so the GitHub pass stops while half the quota is still left and reports what it did not fetch. --forge-window-days reads a shorter window from GitHub than - --window-days reads from git, which costs fewer requests. + --window-days reads from git, which costs fewer requests. A token whose quota is already + spent skips GitHub, reporting those metrics as not measured; the rest of the report is + unaffected. A repository whose remote is not GitHub skips the check automatically.`, Args: cobra.MaximumNArgs(1), diff --git a/internal/analyze/forge/auth.go b/internal/analyze/forge/auth.go index 0e53efbf4b..ca9e6206c2 100644 --- a/internal/analyze/forge/auth.go +++ b/internal/analyze/forge/auth.go @@ -13,6 +13,7 @@ package forge import ( "context" + "errors" "fmt" "net/http" "os" @@ -75,6 +76,16 @@ var ErrNoCredentials = fmt.Errorf(`no GitHub credentials found. Or run with --no-forge to skip these metrics entirely. They will be reported as "not measured" rather than zero — the report never claims we looked when we did not`) +// ErrRateLimited means the preflight could not run because the token's quota is spent. It is +// not an access problem: inside GitHub Actions the job token's quota belongs to the whole +// repository, and other jobs can spend it before analyze starts. A caller should skip the +// forge pass and report its metrics as not measured, not fail the whole analysis. +var ErrRateLimited = errors.New("the GitHub API rate limit for this token is exhausted") + +func rateLimitedError(creds *Credentials, err error) error { + return fmt.Errorf("%w (token from %s): %v", ErrRateLimited, creds.Source, err) +} + // repoAccessError explains a token that works but cannot see this repository. // // The raw error is "404 Not Found", which is a lie of omission: the repository exists, and @@ -172,11 +183,17 @@ func Check(ctx context.Context, creds *Credentials, host string, repo Repo) (*Ch user, _, err := client.gh.Users.Get(ctx, "") if err != nil { + if IsRateLimit(err) { + return nil, rateLimitedError(creds, err) + } if repo.Owner == "" || repo.Name == "" { return nil, fmt.Errorf("GitHub rejected the token from %s: %w", creds.Source, err) } if _, resp, rerr := client.gh.Repositories.Get(ctx, repo.Owner, repo.Name); rerr != nil { + if IsRateLimit(rerr) { + return nil, rateLimitedError(creds, rerr) + } return nil, repoAccessError(creds, "repository-scoped token", repo, resp, rerr) } @@ -197,6 +214,9 @@ func Check(ctx context.Context, creds *Credentials, host string, repo Repo) (*Ch if repo.Owner != "" && repo.Name != "" { if _, resp, rerr := client.gh.Repositories.Get(ctx, repo.Owner, repo.Name); rerr != nil { + if IsRateLimit(rerr) { + return nil, rateLimitedError(creds, rerr) + } return nil, repoAccessError(creds, user.GetLogin(), repo, resp, rerr) } } diff --git a/internal/analyze/forge/auth_check_test.go b/internal/analyze/forge/auth_check_test.go index 95f58eb6a9..9fde2f58f5 100644 --- a/internal/analyze/forge/auth_check_test.go +++ b/internal/analyze/forge/auth_check_test.go @@ -20,11 +20,23 @@ import ( type fakeGitHub struct { userStatus, repoStatus, rateStatus int calls []string + + // exhausted answers every endpoint but /rate_limit the way GitHub does once a token's + // quota is spent: 403, X-RateLimit-Remaining: 0, "API rate limit exceeded". + exhausted bool } func (f *fakeGitHub) ServeHTTP(w http.ResponseWriter, r *http.Request) { f.calls = append(f.calls, r.URL.Path) w.Header().Set("Content-Type", "application/json") + if f.exhausted && r.URL.Path != "/api/v3/rate_limit" { + w.Header().Set("X-RateLimit-Limit", "1000") + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Reset", fmt.Sprint(time.Now().Add(time.Hour).Unix())) + w.WriteHeader(http.StatusForbidden) + _, _ = fmt.Fprint(w, `{"message":"API rate limit exceeded for installation."}`) + return + } reply := func(status int, body string) { w.WriteHeader(status) if status == http.StatusOK { @@ -125,6 +137,39 @@ func TestCheck_UserTokenCannotSeeTheRepo(t *testing.T) { require.Contains(t, msg, "almost certainly\n an access problem") } +// A spent quota is "come back later", not an access problem. In GitHub Actions the job token's +// quota is the repository's, and other jobs can spend it first; reporting that as "cannot read +// Vulnetix/cli" sent the reader to check permissions that were fine. +func TestCheck_ExhaustedQuotaIsNotAnAccessError(t *testing.T) { + for _, repo := range []Repo{testRepo, {}} { + host := startFakeGitHub(t, &fakeGitHub{exhausted: true}) + + _, err := Check(context.Background(), testCreds, host, repo) + require.ErrorIs(t, err, ErrRateLimited, "repo %+v", repo) + require.NotContains(t, err.Error(), "cannot read") + require.Contains(t, err.Error(), "GH_TOKEN") + } +} + +// GITHUB_TOKEN cannot read /user (403, not a rate limit); the repository read is then the one +// that meets the spent quota. +func TestCheck_RepositoryScopedTokenWithExhaustedQuota(t *testing.T) { + f := &fakeGitHub{userStatus: 403, rateStatus: 200} + host := startFakeGitHub(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + f.exhausted = r.URL.Path == "/api/v3/repos/acme/widgets" + f.ServeHTTP(w, r) + })) + + _, err := Check(context.Background(), testCreds, host, testRepo) + require.ErrorIs(t, err, ErrRateLimited) + require.NotContains(t, err.Error(), "cannot read") +} + +func TestIsRateLimit_Wrapped(t *testing.T) { + require.True(t, IsRateLimit(fmt.Errorf("fetch: %w", &github.RateLimitError{}))) + require.False(t, IsRateLimit(fmt.Errorf("fetch: %w", errors.New("boom")))) +} + // The quota is informational: failing to read it does not fail the check. func TestCheck_RateLimitFailureIsNotFatal(t *testing.T) { host := startFakeGitHub(t, &fakeGitHub{userStatus: 200, repoStatus: 200, rateStatus: 500}) diff --git a/internal/analyze/forge/budget.go b/internal/analyze/forge/budget.go index 2b070525aa..13f61d13e9 100644 --- a/internal/analyze/forge/budget.go +++ b/internal/analyze/forge/budget.go @@ -18,6 +18,7 @@ package forge // reason. It is not a failure, and it is not a silence. import ( + "errors" "fmt" "sync" @@ -126,15 +127,8 @@ func (b *budget) Reason() string { // a real failure matters: one means "come back later", the other means something is broken, // and treating them the same makes both harder to diagnose. func IsRateLimit(err error) bool { - if err == nil { - return false - } - if _, ok := err.(*github.RateLimitError); ok { - return true - } - if _, ok := err.(*github.AbuseRateLimitError); ok { - return true - } + var rl *github.RateLimitError + var abuse *github.AbuseRateLimitError - return false + return errors.As(err, &rl) || errors.As(err, &abuse) } diff --git a/internal/analyze/run.go b/internal/analyze/run.go index 0e3da9a103..b6813d16aa 100644 --- a/internal/analyze/run.go +++ b/internal/analyze/run.go @@ -8,6 +8,7 @@ package analyze import ( "context" + "errors" "fmt" "net/url" "os" @@ -165,6 +166,13 @@ func Check(ctx context.Context, opts Options) (*Preflight, error) { // unable to read a private repo it was never granted — and GitHub reports that as a 404, so // the failure looks like a typo rather than a permission. result, err := forge.Check(ctx, creds, host, forgeRepo) + if errors.Is(err, forge.ErrRateLimited) { + // The token is fine; its quota is spent, often by other jobs sharing it. That costs the + // forge metrics, which are then not measured, and never the rest of the report. + p.ForgeStatus = fmt.Sprintf("GitHub: skipped (%v). Pull-request, review and issue metrics will be reported as not measured.", err) + + return p, nil + } if err != nil { return nil, err }