fix(ci): address review findings in ci-health

Four issues from review, all real, all verified rather than taken on trust.

fetchJobs requested per_page=100 but never asked for page 2, so any run with
more than 100 jobs reported partial timings. Wide matrix runs are exactly the
ones whose numbers matter, so this silently understated the busiest runs. It
now pages until total_count is reached.

Negative counts panicked instead of erroring: --runs, --history, --weeks and
--prs all reach slice bounds, and `--runs=-1` died with "slice bounds out of
range [:-1]". Reproduced before fixing. They are now rejected with a message
naming the flag.

--durations and --merge-times read no cache data but still fetched it, so they
failed whenever cache access failed, for reports that never used the result.
The cache calls are now made only for the modes that read them.

diagnose divided job medians drawn from the --runs sample by a wall-clock
median drawn from the full --history window. Mixing two windows can misstate
each cause's share and reorder them. Diagnosis now runs on the sampled window;
the full history still feeds the trend charts, where it belongs.

Tests cover pagination across two pages, the single-page stop, and rejection of
every negative count flag. 40 tests pass.

Verified live afterwards: --runs=-1 now prints a clean error, --durations
completes without touching the cache, and --why reports over the window it
actually sampled.

Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>

Author: balaji-g

tools/ci-health/analysis_test.go

@@ -16,7 +16,10 @@
 package main
 
 import (
+	"flag"
+	"io"
 	"math"
+	"os"
 	"strings"
 	"testing"
 	"time"
@@ -345,3 +348,22 @@ func TestDashboardHasNoExternalReferences(t *testing.T) {
 		}
 	}
 }
+
+// --runs, --history, --weeks and --prs all reach slice bounds, where a negative
+// value panics instead of erroring.
+func TestNegativeCountFlagsAreRejected(t *testing.T) {
+	for _, name := range []string{"runs", "history", "weeks", "prs"} {
+		t.Run(name, func(t *testing.T) {
+			flag.CommandLine = flag.NewFlagSet("ci-health", flag.ContinueOnError)
+			flag.CommandLine.SetOutput(io.Discard)
+			os.Args = []string{"ci-health", "--" + name + "=-1", "--repo", "o/r"}
+			err := run()
+			if err == nil {
+				t.Fatalf("--%s=-1 was accepted; it panics when used as a slice bound", name)
+			}
+			if !strings.Contains(err.Error(), "--"+name+" must be at least 1") {
+				t.Fatalf("error = %q, want it to name --%s", err, name)
+			}
+		})
+	}
+}

tools/ci-health/github.go

@@ -178,13 +178,25 @@ func fetchJobs(repo string, runs []Run, workers int) []Job {
 			defer wg.Done()
 			sem <- struct{}{}
 			defer func() { <-sem }()
-			var body struct {
-				Jobs []Job `json:"jobs"`
-			}
-			if err := getJSON(fmt.Sprintf("actions/runs/%d/jobs?per_page=100", id), repo, &body); err != nil {
-				return
+			// Paginate. A run with more than 100 jobs would otherwise report
+			// partial timings, and the wide matrix rows are exactly the runs
+			// whose numbers matter most.
+			var all []Job
+			for page := 1; ; page++ {
+				var body struct {
+					Total int   `json:"total_count"`
+					Jobs  []Job `json:"jobs"`
+				}
+				path := fmt.Sprintf("actions/runs/%d/jobs?per_page=100&page=%d", id, page)
+				if err := getJSON(path, repo, &body); err != nil {
+					return
+				}
+				all = append(all, body.Jobs...)
+				if len(body.Jobs) < 100 || len(all) >= body.Total {
+					break
+				}
 			}
-			out[i] = result{jobs: body.Jobs}
+			out[i] = result{jobs: all}
 		}(i, run.ID)
 	}
 	wg.Wait()

tools/ci-health/github_test.go

@@ -212,3 +212,55 @@ func TestJobDecodesNullTimestamps(t *testing.T) {
 		t.Fatal("created_at should have decoded")
 	}
 }
+
+// A run with more than 100 jobs used to report only the first page, and the
+// wide matrix runs are exactly the ones whose timings matter most.
+func TestFetchJobsPaginatesRunsWithManyJobs(t *testing.T) {
+	var calls []string
+	prev := fetch
+	fetch = func(path, repo string) ([]byte, error) {
+		calls = append(calls, path)
+		page := 1
+		if i := strings.Index(path, "&page="); i >= 0 {
+			fmt.Sscanf(path[i+len("&page="):], "%d", &page)
+		}
+		n := 100
+		if page == 2 {
+			n = 40
+		}
+		if page > 2 {
+			n = 0
+		}
+		jobs := make([]string, 0, n)
+		for i := 0; i < n; i++ {
+			jobs = append(jobs, `{"name":"bazel (x)","run_id":1,"conclusion":"success"}`)
+		}
+		return []byte(fmt.Sprintf(`{"total_count":140,"jobs":[%s]}`, strings.Join(jobs, ","))), nil
+	}
+	t.Cleanup(func() { fetch = prev })
+
+	got := fetchJobs("o/r", []Run{{ID: 1}}, 1)
+	if len(got) != 140 {
+		t.Fatalf("got %d jobs, want 140 (page 1 + page 2)", len(got))
+	}
+	if len(calls) != 2 {
+		t.Fatalf("made %d requests, want 2; must stop once total_count is reached", len(calls))
+	}
+}
+
+func TestFetchJobsStopsOnSinglePage(t *testing.T) {
+	var calls int
+	prev := fetch
+	fetch = func(path, repo string) ([]byte, error) {
+		calls++
+		return []byte(`{"total_count":1,"jobs":[{"name":"bazel (x)","run_id":1,"conclusion":"success"}]}`), nil
+	}
+	t.Cleanup(func() { fetch = prev })
+
+	if got := fetchJobs("o/r", []Run{{ID: 1}}, 1); len(got) != 1 {
+		t.Fatalf("got %d jobs, want 1", len(got))
+	}
+	if calls != 1 {
+		t.Fatalf("made %d requests, want 1; a short page means no more pages", calls)
+	}
+}

tools/ci-health/main.go

@@ -108,6 +108,16 @@ func parseFlags() *options {
 func run() error {
 	o := parseFlags()
 
+	// These reach slice bounds, so a negative value panics rather than erroring.
+	for _, c := range []struct {
+		name string
+		v    int
+	}{{"runs", o.runs}, {"history", o.history}, {"weeks", o.weeks}, {"prs", o.prs}} {
+		if c.v < 1 {
+			return fmt.Errorf("--%s must be at least 1, got %d", c.name, c.v)
+		}
+	}
+
 	workflow := o.workflow
 	if filepath.Ext(workflow) != ".yml" && filepath.Ext(workflow) != ".yaml" {
 		workflow += ".yml"
@@ -132,33 +142,40 @@ func run() error {
 		}
 		truncated = len(runsList) >= o.history
 	}
+	var sampled []Run
 	if needJobs {
-		sample := runsList
-		if len(sample) > o.runs {
-			sample = sample[:o.runs]
+		sampled = runsList
+		if len(sampled) > o.runs {
+			sampled = sampled[:o.runs]
 		}
-		jobs = fetchJobs(o.repo, sample, 8)
+		jobs = fetchJobs(o.repo, sampled, 8)
 	}
 
-	usage := struct {
-		Size  int64 `json:"active_caches_size_in_bytes"`
-		Count int   `json:"active_caches_count"`
-	}{}
-	if err := getJSON("actions/cache/usage", o.repo, &usage); err != nil {
-		return err
-	}
-	caches, err := fetchCaches(o.repo)
-	if err != nil {
-		return err
+	// --durations and --merge-times read no cache data, so they should neither
+	// pay for these calls nor fail when cache access does.
+	needCache := needJobs || (!o.why && !o.durations && !o.mergeTimes)
+	var cache CacheState
+	if needCache {
+		usage := struct {
+			Size  int64 `json:"active_caches_size_in_bytes"`
+			Count int   `json:"active_caches_count"`
+		}{}
+		if err := getJSON("actions/cache/usage", o.repo, &usage); err != nil {
+			return err
+		}
+		caches, err := fetchCaches(o.repo)
+		if err != nil {
+			return err
+		}
+		cache = summariseCaches(caches, usage.Size, usage.Count)
 	}
-	cache := summariseCaches(caches, usage.Size, usage.Count)
 
 	stats := analyseJobs(jobs)
 	poles, poleRuns := longPoles(jobs)
 	var causes []Cause
 	var wall float64
 	if needJobs {
-		causes, wall = diagnose(runsList, stats, poles, poleRuns, cache)
+		causes, wall = diagnose(sampled, stats, poles, poleRuns, cache)
 	}
 
 	if o.dashboard != "" {
@@ -179,7 +196,7 @@ func run() error {
 	}
 
 	if o.why || o.all {
-		printWhy(causes, wall, runsList)
+		printWhy(causes, wall, sampled)
 	}
 	if o.durations || o.all {
 		printDurations(runsList, workflow, o.weeks, truncated)