From 952cdeb8bd9889adc5a350d9f3592068ececc049 Mon Sep 17 00:00:00 2001 From: Christopher Langton Date: Sat, 14 Nov 2026 00:58:48 +1100 Subject: [PATCH] fix(analyze): bound the forge pass to half the token quota and a short window in CI The vulnetix.yml analyze job read a year of pull requests, issues and commit reviews on every push to main, and kept spending the job token until 100 requests were left. That token's hourly quota belongs to the whole repository, so CodeQL's SARIF upload and the auto-version release dispatch failed with 'API rate limit exceeded for installation'. - forge budget: stop at max(100, half the hourly limit) instead of a fixed 100. - analyze: --forge-window-days bounds the GitHub pass apart from the git history window; the commit-review pass only asks about commits inside it. - gha init: on a push or a release the analyze job passes --forge-window-days=7; only a manual run reads the full window. Pull requests still pass --no-forge. - Regenerate .github/workflows/vulnetix.yml and docs/command-manifest.json. --- .github/workflows/vulnetix.yml | 2 +- cmd/analyze.go | 7 ++++ docs/command-manifest.json | 1 + docs/github-actions.md | 9 +++++ internal/analyze/collect_forge.go | 38 ++++++++++++------- internal/analyze/collect_forge_window_test.go | 34 +++++++++++++++++ internal/analyze/forge/budget.go | 28 +++++++++++--- internal/analyze/forge/forge_test.go | 38 +++++++++++++++++++ internal/analyze/run.go | 15 ++++++++ internal/ghainit/render.go | 20 +++++++--- internal/ghainit/render_test.go | 7 ++-- 11 files changed, 170 insertions(+), 29 deletions(-) create mode 100644 internal/analyze/collect_forge_window_test.go diff --git a/.github/workflows/vulnetix.yml b/.github/workflows/vulnetix.yml index eee5bb69e6..5f649483d3 100644 --- a/.github/workflows/vulnetix.yml +++ b/.github/workflows/vulnetix.yml @@ -214,7 +214,7 @@ jobs: artifact-name: analyze fail-on-findings: false github-token: ${{ github.event_name != 'pull_request' && github.token || '' }} - extra-args: ${{ github.event_name == 'pull_request' && '--no-forge' || '' }} + extra-args: ${{ github.event_name == 'pull_request' && '--no-forge' || github.event_name != 'workflow_dispatch' && '--forge-window-days=7' || '' }} sbom: name: SBOM diff --git a/cmd/analyze.go b/cmd/analyze.go index 2cbd40a3b2..9248e8b938 100644 --- a/cmd/analyze.go +++ b/cmd/analyze.go @@ -66,6 +66,11 @@ GitHub authentication than zero, because "we found no unreviewed commits" and "we did not look" are different claims and this report will not conflate them. + 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. + A repository whose remote is not GitHub skips the check automatically.`, Args: cobra.MaximumNArgs(1), RunE: runAnalyze, @@ -74,6 +79,7 @@ GitHub authentication func init() { analyzeCmd.Flags().String("path", ".", "Directory to analyze") analyzeCmd.Flags().Int("window-days", 365, "How far back to walk history") + analyzeCmd.Flags().Int("forge-window-days", 0, "How far back to read pull requests, issues and commit reviews from GitHub (0: --window-days). Each pull request costs several API calls") analyzeCmd.Flags().Int("max-commits", 20000, "Cap on commits walked; when hit, the report declares it") analyzeCmd.Flags().Int("complexity-threshold", 15, "Cyclomatic complexity at which a file counts as highly complex") analyzeCmd.Flags().StringP("output", "o", "pretty", "Terminal output format: pretty, json") @@ -98,6 +104,7 @@ func runAnalyze(cmd *cobra.Command, args []string) error { opts.Path = args[0] } opts.WindowDays, _ = cmd.Flags().GetInt("window-days") + opts.ForgeWindowDays, _ = cmd.Flags().GetInt("forge-window-days") opts.MaxCommits, _ = cmd.Flags().GetInt("max-commits") opts.ComplexityThreshold, _ = cmd.Flags().GetInt("complexity-threshold") opts.NoGit, _ = cmd.Flags().GetBool("no-git") diff --git a/docs/command-manifest.json b/docs/command-manifest.json index 01157a9faa..c61bf224a7 100644 --- a/docs/command-manifest.json +++ b/docs/command-manifest.json @@ -525,6 +525,7 @@ "complexity-threshold", "disable-memory", "fail-on-upload-error", + "forge-window-days", "max-commits", "no-analytics", "no-banner", diff --git a/docs/github-actions.md b/docs/github-actions.md index 1891f556a2..255b6c2ee6 100644 --- a/docs/github-actions.md +++ b/docs/github-actions.md @@ -66,6 +66,15 @@ Each CLI command also has its own composite action in a subdirectory of this rep `config-plan`, `config-apply`, `attest`, `jail`, `vex` and `github-publish`. Their inputs, outputs, permissions and examples are in [`github-actions/`](github-actions/README.md). +`analyze` reads pull requests, issues and commit reviews from GitHub with the job token, several +requests per pull request. That token's hourly quota (1,000 requests) belongs to the whole +repository, and every other job shares it, including CodeQL's SARIF upload and a workflow +dispatch. So the GitHub pass stops while half the token's hourly limit is still left, and the +report declares what it did not fetch. The workflow `vulnetix gha init` writes passes +`--no-forge` on a pull request and `--forge-window-days=7` on a push or a release. Only a manual +run (`workflow_dispatch`) reads the full `--window-days` window from GitHub. The git history +window stays at 365 days in every case. + They are generated, not hand-written: `tools/actiongen/spec.go` is the table, and `go generate ./tools/actiongen` writes every `/action.yml`, every `docs/github-actions/.md` and `action-lib/commands.sh`. `TestGeneratedFilesAreCurrent` diff --git a/internal/analyze/collect_forge.go b/internal/analyze/collect_forge.go index 9ff312ba96..f902a4d40a 100644 --- a/internal/analyze/collect_forge.go +++ b/internal/analyze/collect_forge.go @@ -38,11 +38,12 @@ const maxCommitReviewChecks = 300 func collectForge(b *Builder, client *forge.Client, repo forge.Repo, git *gitStats, opts Options, now time.Time, pr reporter) error { ctx := context.Background() - since := now.AddDate(0, 0, -opts.WindowDays) + days := opts.forgeWindowDays() + since := now.AddDate(0, 0, -days) window := &MetricWindow{ Since: since.UTC().Format(time.RFC3339), Until: now.UTC().Format(time.RFC3339), - Label: fmt.Sprintf("last_%d_days", opts.WindowDays), + Label: fmt.Sprintf("last_%d_days", days), } // This is the slowest collector by a distance: every pull request costs several API calls @@ -67,7 +68,7 @@ func collectForge(b *Builder, client *forge.Client, repo forge.Repo, git *gitSta emitIssues(b, issues, window) pr.Stage("Resolving which commits were reviewed") - emitCommitReview(b, client, repo, git, prRefs, window) + emitCommitReview(b, client, repo, git, prRefs, since, window) // If the budget ran out, every count above is short by an unknown amount — and an unknown // amount is exactly what must not be silently absorbed. Say it. @@ -396,9 +397,28 @@ func emitIssues(b *Builder, issues []forge.Issue, window *MetricWindow) { window, respSecs, respRefs) } +// commitsToReview picks the commits the compliance pass asks the forge about: non-merge +// commits inside the forge window, newest first. A merge commit's review status is the review +// status of the PR it merged, which we already have. The forge window can be shorter than the +// history walk, and every commit asked about costs an API call, so a commit older than the +// window is not asked about. +func commitsToReview(commits []*CommitRecord, since time.Time) []string { + shas := make([]string, 0, len(commits)) + for _, c := range commits { + if c.ParentCount > 1 { + continue + } + if at, err := time.Parse(time.RFC3339, c.CommittedAt); err == nil && at.Before(since) { + continue + } + shas = append(shas, c.SHA) + } + return shas +} + // emitCommitReview is github-metrics-aggregator's compliance metric. func emitCommitReview(b *Builder, client *forge.Client, repo forge.Repo, git *gitStats, - prRefs map[string]EvidenceRef, window *MetricWindow) { + prRefs map[string]EvidenceRef, since time.Time, window *MetricWindow) { if git == nil || len(git.commits) == 0 { b.Unmeasured(Metric{ @@ -410,16 +430,8 @@ func emitCommitReview(b *Builder, client *forge.Client, repo forge.Repo, git *gi return } - // Only non-merge commits, newest first, capped. A merge commit's review status is the - // review status of the PR it merged, which we already have. - shas := make([]string, 0, len(git.commits)) + shas := commitsToReview(git.commits, since) byShaRef := map[string]EvidenceRef{} - for _, c := range git.commits { - if c.ParentCount > 1 { - continue - } - shas = append(shas, c.SHA) - } omitted := 0 if len(shas) > maxCommitReviewChecks { diff --git a/internal/analyze/collect_forge_window_test.go b/internal/analyze/collect_forge_window_test.go new file mode 100644 index 0000000000..93e9215696 --- /dev/null +++ b/internal/analyze/collect_forge_window_test.go @@ -0,0 +1,34 @@ +package analyze + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// The forge window can be shorter than the history walk. The compliance pass costs an API +// call a commit, so it asks only about the commits inside the forge window, and never about +// a merge commit, whose review status is its pull request's. +func TestCommitsToReview_StaysInsideTheForgeWindow(t *testing.T) { + now := time.Date(2026, 11, 14, 0, 0, 0, 0, time.UTC) + at := func(days int) string { return now.AddDate(0, 0, -days).Format(time.RFC3339) } + commits := []*CommitRecord{ + {SHA: "new", CommittedAt: at(1)}, + {SHA: "merge", CommittedAt: at(2), ParentCount: 2}, + {SHA: "edge", CommittedAt: at(7)}, + {SHA: "old", CommittedAt: at(30)}, + {SHA: "undated"}, + } + + require.Equal(t, []string{"new", "edge", "undated"}, commitsToReview(commits, now.AddDate(0, 0, -7)), + "a commit with no parseable date is kept rather than dropped unseen") + require.Equal(t, []string{"new", "edge", "old", "undated"}, commitsToReview(commits, now.AddDate(0, 0, -365))) +} + +func TestForgeWindowDays_FollowsTheHistoryWindowUnlessSet(t *testing.T) { + o := DefaultOptions() + require.Equal(t, 365, o.forgeWindowDays()) + o.ForgeWindowDays = 7 + require.Equal(t, 7, o.forgeWindowDays()) +} diff --git a/internal/analyze/forge/budget.go b/internal/analyze/forge/budget.go index 518edceca5..2b070525aa 100644 --- a/internal/analyze/forge/budget.go +++ b/internal/analyze/forge/budget.go @@ -18,16 +18,31 @@ package forge // reason. It is not a failure, and it is not a silence. import ( + "fmt" "sync" "github.com/google/go-github/v66/github" ) -// reserve is the number of requests we refuse to spend. Leaving headroom matters because -// this token is usually shared — with other steps in the same CI job, and in vdb-manager's -// case with other processors — and draining it to zero breaks them, not us. +// reserve is the least number of requests we refuse to spend. Leaving headroom matters +// because this token is usually shared — with other steps in the same CI job, and in +// vdb-manager's case with other processors — and draining it to zero breaks them, not us. const reserve = 100 +// reserveShare is the fraction of the hourly limit we refuse to spend, when it is more than +// reserve. A fixed 100 was not enough headroom: inside GitHub Actions the job token's quota +// (1,000 an hour) belongs to the whole repository, and a forge pass on every push to the +// default branch spent it down to 100. CodeQL's SARIF upload and the release dispatch then +// failed with "API rate limit exceeded for installation". Keeping half the limit bounds +// what one run can take, whatever else runs beside it. +const reserveShare = 0.5 + +// floor is the remaining quota at which the collector stops, for a token with this hourly +// limit. +func floor(limit int) int { + return max(reserve, int(float64(limit)*reserveShare)) +} + type budget struct { mu sync.Mutex @@ -60,7 +75,7 @@ func (b *budget) Observe(resp *github.Response) { b.remaining = resp.Rate.Remaining b.limit = resp.Rate.Limit b.known = true - if b.remaining <= reserve { + if b.remaining <= floor(b.limit) { b.exhausted = true } } @@ -102,8 +117,9 @@ func (b *budget) Reason() string { return "" } - return "GitHub API rate limit reached; the remaining items were not fetched. " + - "Re-run when the quota resets, or narrow --window-days." + return fmt.Sprintf("Stopped at this run's share of the GitHub API rate limit (%d of %d requests left, "+ + "kept for other jobs that share the token); the remaining items were not fetched. "+ + "Re-run when the quota resets, or narrow --forge-window-days.", b.remaining, b.limit) } // IsRateLimit reports whether an error is GitHub telling us to stop. Distinguishing it from diff --git a/internal/analyze/forge/forge_test.go b/internal/analyze/forge/forge_test.go index b69f24942c..92c7cf858e 100644 --- a/internal/analyze/forge/forge_test.go +++ b/internal/analyze/forge/forge_test.go @@ -2,6 +2,7 @@ package forge import ( "context" + "fmt" "os" "testing" "time" @@ -128,6 +129,43 @@ func TestBudget(t *testing.T) { require.Equal(t, 2, b.Spent()) } +// Inside GitHub Actions the job token's 1,000 requests an hour belong to the whole repository. +// A forge pass that spent them down to a fixed 100 starved CodeQL's SARIF upload and the +// release dispatch on the same push, so the collector keeps half the limit for them. +func TestBudget_KeepsHalfTheLimitForOtherJobs(t *testing.T) { + cases := []struct { + limit, remaining int + exhausted bool + }{ + {1000, 501, false}, + {1000, 500, true}, + {1000, 300, true}, // already below the floor when we start: spend nothing more + {5000, 2501, false}, + {5000, 2500, true}, + {150, 101, false}, // a small limit still keeps at least the fixed reserve + {150, 100, true}, + } + for _, c := range cases { + b := newBudget() + b.Observe(&github.Response{Rate: github.Rate{Limit: c.limit, Remaining: c.remaining}}) + require.Equal(t, c.exhausted, b.Exhausted(), "limit %d, remaining %d", c.limit, c.remaining) + if c.exhausted { + require.Contains(t, b.Reason(), fmt.Sprintf("%d of %d requests left", c.remaining, c.limit)) + } + } +} + +// A response that carries no rate headers (some enterprise instances) cannot exhaust the +// budget on its own; it is still counted. +func TestBudget_UnknownLimit(t *testing.T) { + b := newBudget() + b.Observe(&github.Response{}) + require.False(t, b.Exhausted()) + _, known := b.Remaining() + require.False(t, known) + require.Equal(t, 1, b.Spent()) +} + func TestIsRateLimit(t *testing.T) { require.True(t, IsRateLimit(&github.RateLimitError{})) require.True(t, IsRateLimit(&github.AbuseRateLimitError{})) diff --git a/internal/analyze/run.go b/internal/analyze/run.go index 17a4fcdc68..0e3da9a103 100644 --- a/internal/analyze/run.go +++ b/internal/analyze/run.go @@ -31,6 +31,12 @@ type Options struct { WindowDays int MaxCommits int + // ForgeWindowDays bounds the forge pass (pull requests, issues, commit reviews) on its + // own. Every pull request in the window costs several GitHub API calls, so a CI run on + // every push can read a short recent window and leave the full one to a release or a + // manual run. Zero follows WindowDays. + ForgeWindowDays int + ComplexityThreshold int NoGit bool @@ -64,6 +70,15 @@ type Options struct { Context context.Context } +// forgeWindowDays is the window the forge pass reads: ForgeWindowDays when set, else the +// history window. +func (o Options) forgeWindowDays() int { + if o.ForgeWindowDays > 0 { + return o.ForgeWindowDays + } + return o.WindowDays +} + func (o Options) ctx() context.Context { if o.Context != nil { return o.Context diff --git a/internal/ghainit/render.go b/internal/ghainit/render.go index 517a9ac5aa..76ff023406 100644 --- a/internal/ghainit/render.go +++ b/internal/ghainit/render.go @@ -116,12 +116,7 @@ func Render(p *Policy, sig *Signals, opt Options) string { case "analyze": // it reads history and the repository's pull requests and issues j.fullHistory = true j.perms = map[string]string{"pull-requests": "read", "issues": "read"} - // The forge pass reads every pull request and issue in the window and - // spends the job token down to its reserve. That token shares the - // repository's hourly quota with every other job, CodeQL's SARIF - // upload among them, so on a pull request it would fail the run's - // required checks. The push to the default branch measures the forge. - j.steps[0].with = append(j.steps[0].with, [2]string{"extra-args", "${{ github.event_name == 'pull_request' && '--no-forge' || '' }}"}) + j.steps[0].with = append(j.steps[0].with, [2]string{"extra-args", analyzeForgeArgs}) case "aibom": // a commit scan is slow and this runs on every push j.steps[0].with = append(j.steps[0].with, [2]string{"extra-args", "--commit-scan-max\n1"}) } @@ -214,6 +209,19 @@ func (r *renderer) scanStep(action, gate string, with [][2]string, artifact stri // GitHub from these jobs; the push to the default branch keeps the token. const scanToken = "${{ github.event_name != 'pull_request' && github.token || '' }}" +// analyzeForgeArgs bounds the analyze job's forge pass, which reads pull requests, +// issues and commit reviews from GitHub with the job token, several requests a pull +// request. That token's hourly quota belongs to the whole repository and is shared with +// every other job, CodeQL's SARIF upload and the release dispatch among them. A pull +// request skips the pass. A push to the default branch, which happens on every merge, and +// a release, which an auto-versioned repository publishes on every merge too, read only +// the last recentForgeWindowDays days. A manual run (workflow_dispatch) reads the full +// window. The CLI also stops the pass while half the quota is left. +var analyzeForgeArgs = fmt.Sprintf("${{ github.event_name == 'pull_request' && '--no-forge' || github.event_name != 'workflow_dispatch' && '--forge-window-days=%d' || '' }}", recentForgeWindowDays) + +// recentForgeWindowDays is the forge window on a push or a release. +const recentForgeWindowDays = 7 + func (r *renderer) scanJob(id, name, action, gate string, with [][2]string, _ string) job { j := job{id: id, name: name, steps: []step{r.scanStep(action, gate, with, id)}} if r.p.Publish { diff --git a/internal/ghainit/render_test.go b/internal/ghainit/render_test.go index 7c0fa8a023..4d95b92fa3 100644 --- a/internal/ghainit/render_test.go +++ b/internal/ghainit/render_test.go @@ -99,8 +99,9 @@ func TestPublishAndTEAFollowTheLegs(t *testing.T) { } // The forge pass spends the repository's shared hourly token quota, so a pull -// request runs analyze without it and the push to the default branch keeps it. -func TestAnalyzeSkipsTheForgeOnPullRequests(t *testing.T) { +// request runs analyze without it, a push or a release reads a short recent +// window, and only a manual run reads the full one. +func TestAnalyzeBoundsTheForgePass(t *testing.T) { p := DefaultPolicy() p.Analyze = true out := Render(p, &Signals{}, Options{}) @@ -119,7 +120,7 @@ func TestAnalyzeSkipsTheForgeOnPullRequests(t *testing.T) { if !strings.HasPrefix(s.Uses, "Vulnetix/cli/analyze@") { continue } - want := "${{ github.event_name == 'pull_request' && '--no-forge' || '' }}" + want := "${{ github.event_name == 'pull_request' && '--no-forge' || github.event_name != 'workflow_dispatch' && '--forge-window-days=7' || '' }}" if got := s.With["extra-args"]; got != want { t.Errorf("analyze extra-args = %q, want %q", got, want) }