feat(ci): add a self-serve build-health tool, raise the bazel matrix cap - #656
feat(ci): add a self-serve build-health tool, raise the bazel matrix cap#656balajinvda wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds a Go CI health CLI with GitHub data collection, performance analysis, text reports, and a self-contained HTML dashboard. It adds tests, a Bash launcher, Go module metadata, build-output exclusion, and increases Bazel matrix concurrency from 8 to 12. ChangesCI health and workflow operations
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Operator
participant CIHealth
participant GHCLI
participant GitHub
participant Dashboard
Operator->>CIHealth: Select report mode and limits
CIHealth->>GHCLI: Request runs, jobs, caches, or PRs
GHCLI->>GitHub: Fetch paginated JSON data
GitHub-->>GHCLI: Return API records
GHCLI-->>CIHealth: Return decoded data
CIHealth->>CIHealth: Analyze timing, cache, and diagnostic data
CIHealth->>Dashboard: Render self-contained HTML
Dashboard-->>Operator: Display report
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ci/ci-health`:
- Around line 174-191: Add focused tests in tools/ci/test-ci-health covering
mocked gh responses, pagination behavior, percentile edge cases, and main()
command dispatch for workflow, merge-time, cache, and --all options. Keep the
tests targeted to the existing cache_report, workflow_report, merge_time_report,
and main symbols without changing tool behavior.
- Line 81: Update the GitHub API retrieval used by the cache, workflow-run, and
pull-request metrics to follow pagination and aggregate every page needed for
the report window. In particular, replace the single-page call around gh with a
pagination-aware fetch, then apply args.prs only as a client-side limit after
all relevant pull requests are collected, preserving existing metric
calculations.
- Around line 118-123: Update percentile to use a documented percentile
calculation method that interpolates between adjacent sorted values, ensuring
percentile([1, 2], 0.5) returns 1.5 and p90 for ten ascending values reflects
the 90th percentile rather than automatically selecting the maximum. Preserve
the existing empty-input return of 0.0.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee52de18-52b9-4966-9a97-e99e95c715b7
📒 Files selected for processing (3)
.github/workflows/bazel.yml.github/workflows/license-dependencies.ymltools/ci/ci-health
balajinvda
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bazel.yml (1)
741-754: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winSave the caches after the test step, not between build and test.
The previous combined
actions/cache@v4step saved in the post-job phase, so the entry contained everything the build and the test step fetched.actions/cache/save@v4saves at the point where the step runs.bazel test //...can fetch further external repositories into~/.cache/bazel/_bazel_*/cache(test-only dependencies and toolchains reached only by the test configuration). Those fetches now land after the save, so the next run restores an incomplete repository cache and re-downloads them.Move this step below
bazel test //...(after line 876). The conditions themselves are correct.♻️ Proposed move
- - name: Save Bazel repository + disk caches - # Never from a merge-queue ref: gh-readonly-queue/** is deleted when the - # queue drains, so the entry is unrestorable but still occupies quota. - if: >- - steps.precheck.outputs.skip == 'false' - && steps.bazel_cache.outputs.cache-hit != 'true' - && !startsWith(github.ref, 'refs/heads/gh-readonly-queue/') - uses: actions/cache/save@v4 - with: - path: | - ~/.cache/bazel/_bazel_${{ env.USER || 'root' }}/install - ~/.cache/bazel/_bazel_${{ env.USER || 'root' }}/cache - key: bazel-${{ matrix.subtree.workdir == '.' && 'rootmodule' || matrix.subtree.id }}-${{ hashFiles(format('{0}/MODULE.bazel.lock', matrix.subtree.workdir), format('{0}/.bazelversion', matrix.subtree.workdir)) }} - - name: bazel test //...Then add the same step immediately after the
bazel test //...step:- name: Save Bazel repository + disk caches # Runs after build and test so the entry holds every fetched external # repository. Never from a merge-queue ref: gh-readonly-queue/** is # deleted when the queue drains, so the entry is unrestorable but still # occupies quota. if: >- always() && steps.precheck.outputs.skip == 'false' && steps.bazel_cache.outputs.cache-hit != 'true' && !startsWith(github.ref, 'refs/heads/gh-readonly-queue/') uses: actions/cache/save@v4 with: path: | ~/.cache/bazel/_bazel_${{ env.USER || 'root' }}/install ~/.cache/bazel/_bazel_${{ env.USER || 'root' }}/cache key: bazel-${{ matrix.subtree.workdir == '.' && 'rootmodule' || matrix.subtree.id }}-${{ hashFiles(format('{0}/MODULE.bazel.lock', matrix.subtree.workdir), format('{0}/.bazelversion', matrix.subtree.workdir)) }}Drop
always()if you want a failed test to skip the save.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/bazel.yml around lines 741 - 754, Move the “Save Bazel repository + disk caches” step to immediately after the `bazel test //...` step so test-only downloads are included. Preserve its existing conditions, or add `always()` if failed tests should still save the cache; do not leave the original pre-test save step in place.
🧹 Nitpick comments (2)
tools/ci/test-ci-health (1)
59-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the API-shaped helpers.
The pure analysis functions are well covered.
gh_pagedandmerge_time_reportare not, and that is where thekey="items"defect ontools/ci/ci-healthline 568 sits. Add a case that stubsci.gh_jsonwith a bare list and a 100-item page, then asserts thatgh_pagedandmerge_time_reportaggregate correctly.As per coding guidelines, "For changed tool behavior, add or update focused tests."
Also applies to: 219-235
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/ci/test-ci-health` around lines 59 - 70, Add focused tests for the API-shaped helpers `gh_paged` and `merge_time_report` in the existing test suite. Stub `ci.gh_json` to return a bare list containing a full 100-item page, then assert both helpers aggregate the results correctly and preserve the expected behavior without assuming an `items` wrapper.Source: Coding guidelines
tools/ci/ci-health (1)
65-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the
ghcall.
subprocess.runhas no timeout.fetch_jobsissues these calls from eight threads, so one stalled request blocks the tool with no output. Add a timeout and convert the expiry into the existingRuntimeErrorpath, whichmainalready reports throughsys.exit.♻️ Proposed change
def gh_json(path, repo): cmd = ["gh", "api", f"repos/{repo}/{path}"] - out = subprocess.run(cmd, capture_output=True, text=True) + try: + out = subprocess.run(cmd, capture_output=True, text=True, timeout=60) + except subprocess.TimeoutExpired: + raise RuntimeError(f"gh api {path} timed out") if out.returncode != 0: raise RuntimeError(f"gh api {path} failed: {out.stderr.strip()}") return json.loads(out.stdout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/ci/ci-health` around lines 65 - 70, Update gh_json to pass a finite timeout to subprocess.run and catch subprocess.TimeoutExpired, converting it into the existing RuntimeError path with a clear failure message. Preserve the current nonzero-return handling and JSON parsing behavior for completed requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ci/ci-health`:
- Around line 567-575: Update merge_time_report so every --prs value uses
gh_paged with key=None for the pulls endpoint, including limits at or below 100,
and rely on its client-side limit. Remove the conditional gh_json path while
preserving the existing pull request filtering and latency calculation.
In `@tools/ci/test-ci-health`:
- Around line 16-23: Add tools/ci/test-ci-health to the appropriate CI job in
.github/workflows/bazel.yml, following the existing invocation pattern used for
tools/ci/test-bazel-cache-upload-mode. Preserve the executable entrypoint and
ensure the test runs as part of the workflow.
- Around line 25-36: Add an explicit import for importlib.machinery alongside
the existing imports in the test module so the SourceFileLoader reference used
during ci module loading is available reliably; leave the existing _spec
initialization and execution flow unchanged.
---
Outside diff comments:
In @.github/workflows/bazel.yml:
- Around line 741-754: Move the “Save Bazel repository + disk caches” step to
immediately after the `bazel test //...` step so test-only downloads are
included. Preserve its existing conditions, or add `always()` if failed tests
should still save the cache; do not leave the original pre-test save step in
place.
---
Nitpick comments:
In `@tools/ci/ci-health`:
- Around line 65-70: Update gh_json to pass a finite timeout to subprocess.run
and catch subprocess.TimeoutExpired, converting it into the existing
RuntimeError path with a clear failure message. Preserve the current
nonzero-return handling and JSON parsing behavior for completed requests.
In `@tools/ci/test-ci-health`:
- Around line 59-70: Add focused tests for the API-shaped helpers `gh_paged` and
`merge_time_report` in the existing test suite. Stub `ci.gh_json` to return a
bare list containing a full 100-item page, then assert both helpers aggregate
the results correctly and preserve the expected behavior without assuming an
`items` wrapper.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6f3bcbbc-02cf-45bc-840a-967d7306d398
📒 Files selected for processing (3)
.github/workflows/bazel.ymltools/ci/ci-healthtools/ci/test-ci-health
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ci-health/github.go`:
- Around line 170-195: Update fetchJobs to paginate each run’s jobs endpoint
until all pages are retrieved, preserving the worker limit and aggregating every
page’s Jobs into that run’s result. Use the API’s pagination metadata or
next-page indicator and stop when no further page exists. Add a focused test
covering a run whose jobs span multiple pages and verifying all jobs are
returned.
In `@tools/ci-health/main.go`:
- Around line 156-162: Maintain a separate run slice matching the --runs
job-timing window, and pass that slice to diagnose instead of runsList. Update
printWhy and the dashboard diagnostic summary to use the same job-sample slice,
while retaining runsList exclusively for trend-chart data.
- Around line 143-154: Update the command flow around the usage fetch,
fetchCaches, and summariseCaches calls to compute a needCache condition and
perform those requests only for the default, --why, --all, and dashboard modes.
Ensure --durations and --merge-times bypass all cache access while preserving
the existing cache summary behavior for cache-dependent modes.
- Around line 89-93: Validate the parsed count options before dispatch so
--runs, --history, --weeks, and --prs reject negative values instead of reaching
slicing logic. Update the flag-handling flow around the IntVar bindings and
return a clear validation error or usage failure for any negative value, while
preserving valid zero and positive inputs.
In `@tools/ci-health/render.go`:
- Around line 183-196: The quotaBar function currently caps the calculated used
percentage before displaying it. Preserve the raw percentage for threshold
selection and the quotatxt output, while introducing a separately capped value
only for the CSS width attribute. Add a regression test covering utilization
above quota, such as 12 GB, verifying the text reports 120% while the bar width
remains 100%.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 79e3a8e1-7fba-4df1-a6ff-3ec263ba77c8
📒 Files selected for processing (9)
tools/ci-health/.gitignoretools/ci-health/analysis.gotools/ci-health/analysis_test.gotools/ci-health/github.gotools/ci-health/github_test.gotools/ci-health/go.modtools/ci-health/main.gotools/ci-health/render.gotools/ci/ci-health
…tool The repository was at 9.42 GB of its 10 GB Actions cache quota across 16 entries, so every new entry was evicting one another job needed. Nothing reports this: jobs still pass, they are just colder. Two causes. dependency-docs-bazel was 4.19 GB, 42% of the whole budget, in three copies. It caches a Bazel build of five Java runtime inventories, and every branch wrote its own full-size copy. It now restores everywhere and saves only from main, so one authoritative entry serves every branch. Merge-queue refs were duplicating the largest keys. gh-readonly-queue/** branches are deleted when the queue drains, so an entry saved there can never be restored, but it still counts against quota until evicted. 1.90 GB was sat in exactly that state, from the pr-593 and pr-595 merges. The bazel cache is now split into restore plus a save that skips those refs. Adds tools/ci/ci-health so this is answerable without hand-written gh api calls: cache totals against quota, largest families with their share, duplicate keys across refs with the merge-queue waste called out, workflow duration percentiles by day, and PR open-to-merge latency. It also records the trap in reading those numbers: a low median duration usually means change detection skipped rows, not that builds got faster, so the report prints run counts and p90 alongside. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
The duration report pulled `actions/runs?per_page=100` and filtered by workflow name in Python, so it sampled the last 100 runs across all workflows and kept whichever happened to be bazel. In practice that was 14 runs out of 1275, and it under-reported worse as the repo got busier. It now reads the per-workflow endpoint and pages through it. That truncation also produced a false conclusion: with only two days of runs visible there appeared to be no history to trend against. There are three full weeks. Adds `--dashboard`, which writes a self-contained HTML report and opens it. No CDN and no JavaScript dependency; the charts are inline SVG, so the file works offline and adds nothing to the dependency surface. The report separates queue wait from execution time, because the Actions UI shows only their sum. On current data that distinction matters: several matrix rows spend more wall clock waiting for a runner than building. Also drops the oldest week from the trend when the history window was truncated. Only the tail of that week is held, so its median came from an arbitrary slice and plotted as a misleading near-zero point. Adds focused tests for the pure analysis functions, one per defect the tool has actually shipped with. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
A 25-row merge-queue run needs three waves at max-parallel 8 and two at 12, and full-matrix runs are what set the p90 (20.1 min against a 6.8 min median). The cap exists to keep simultaneous actions/checkout downloads under GitHub's rate limit, which previously failed runs at "Set up job" with HTTP 429. That constraint is unchanged, so this is a measured step rather than a jump to 16: combined with the bazel-docker matrix the checkout burst goes from 12 to 16. Walk this back first if 429s reappear. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
Review feedback: tools/AGENTS.md prefers Go for non-trivial repo tooling and says to avoid new Python, partly because some CI environments here do not guarantee a Python interpreter. This tool is squarely in that category: structured API parsing, concurrent fetches, and logic that benefits from unit tests. It should not have been Python to begin with. Behaviour is unchanged. The Go build produces the same ranked causes, the same weekly trend, and a visually identical dashboard against live data. Layout follows the existing convention: the tool lives in its own module at tools/ci-health/, with tools/ci/ci-health kept as the stable entrypoint. Two fixes carried over from review of the Python version: - The pulls endpoint returns a bare array, so paging it with a wrapper key crashed once --prs exceeded one page. The Go paging helper takes an empty key for bare-array endpoints, and a test covers both response shapes. - The cache listing read a single page, silently capping the report at 100 entries. It now pages. 37 tests, no third-party dependencies. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
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>
4fbd9d9 to
b21a78c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
balajinvda
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ci-health/analysis.go`:
- Around line 212-214: Update the label-trimming logic for the truncated report
so it removes the first label whenever truncated is true and labels is
non-empty, including the single-bucket case. Add a regression test covering
truncated input with exactly one bucket and verify the incomplete bucket is
excluded.
In `@tools/ci-health/github_test.go`:
- Around line 34-36: Handle fmt.Sscanf errors in both page-parsing stubs:
tools/ci-health/github_test.go lines 34-36 and 224-226. Check the scan result
before decrementing idx or using page, and return a contextual wrapped error
with %w when parsing fails; preserve normal response-page selection for valid
page numbers.
In `@tools/ci-health/github.go`:
- Around line 29-38: Update the fetch function’s *exec.ExitError branch to wrap
the original err with %w while retaining the trimmed stderr in the formatted
message; leave the generic error branch and successful output behavior
unchanged.
- Around line 29-38: Instrument the fetch function around the outbound gh api
command by creating a client span, injecting W3C trace context into the command
via the -H request header, and recording command failures on the span before
returning the existing errors. Reuse the repository’s existing tracing package
and provider setup where available; otherwise add the required OpenTelemetry
dependency with Apache-2.0 license and attribution review. Add focused tests
covering span creation, trace-header propagation, and failure recording.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 80b02766-f1cc-4a21-bc8e-dd76c4181204
📒 Files selected for processing (10)
.github/workflows/bazel.ymltools/ci-health/.gitignoretools/ci-health/analysis.gotools/ci-health/analysis_test.gotools/ci-health/github.gotools/ci-health/github_test.gotools/ci-health/go.modtools/ci-health/main.gotools/ci-health/render.gotools/ci/ci-health
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/ci-health/go.mod
- tools/ci/ci-health
- tools/ci-health/.gitignore
- tools/ci-health/render.go
- tools/ci-health/analysis_test.go
| if truncated && len(labels) > 1 { | ||
| labels = labels[1:] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Drop the only partial week.
When truncated is true and one bucket exists, this condition retains incomplete data. The report then shows a percentile for an arbitrary slice of that week. Drop the first label whenever truncated is true and labels is non-empty. Add a one-bucket regression test.
Proposed fix
- if truncated && len(labels) > 1 {
+ if truncated && len(labels) > 0 {
labels = labels[1:]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if truncated && len(labels) > 1 { | |
| labels = labels[1:] | |
| } | |
| if truncated && len(labels) > 0 { | |
| labels = labels[1:] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/ci-health/analysis.go` around lines 212 - 214, Update the
label-trimming logic for the truncated report so it removes the first label
whenever truncated is true and labels is non-empty, including the single-bucket
case. Add a regression test covering truncated input with exactly one bucket and
verify the incomplete bucket is excluded.
| if i := strings.Index(path, "&page="); i >= 0 { | ||
| fmt.Sscanf(path[i+len("&page="):], "%d", &idx) | ||
| idx-- |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'fmt\.Sscanf\(' tools/ci-health/github_test.goRepository: NVIDIA/nvcf
Length of output: 554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- guidance ---'
find .. -name AGENTS.md -print
printf '%s\n' '--- github_test.go outline ---'
ast-grep outline tools/ci-health/github_test.go
printf '%s\n' '--- relevant source ---'
sed -n '1,90p' tools/ci-health/github_test.go
sed -n '200,250p' tools/ci-health/github_test.go
printf '%s\n' '--- call sites and test coverage ---'
rg -n -C 3 'httptest|page=|github_test|mock|ServeHTTP|RoundTripper|Sscanf' tools/ci-healthRepository: NVIDIA/nvcf
Length of output: 10737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- applicable guidance ---'
sed -n '1,220p' AGENTS.md
sed -n '1,220p' tools/AGENTS.md
printf '%s\n' '--- standalone fmt.Sscanf behavior ---'
cat >/tmp/verify_scan.go <<'EOF'
package main
import (
"fmt"
)
func main() {
for _, input := range []string{"2", "", "x", "2x", "2&page=3"} {
value := 0
n, err := fmt.Sscanf(input, "%d", &value)
fmt.Printf("%q: n=%d value=%d err=%T:%v\n", input, n, value, err, err)
}
}
EOF
go run /tmp/verify_scan.go
rm -f /tmp/verify_scan.go
printf '%s\n' '--- production pagination inputs ---'
sed -n '50,85p' tools/ci-health/github.go
sed -n '165,205p' tools/ci-health/github.goRepository: NVIDIA/nvcf
Length of output: 15051
Handle page-number parse failures in both test stubs.
Check each fmt.Sscanf result and return a wrapped error when parsing fails. In the first stub, a failed scan decrements idx to -1 and can panic when indexing pages. In the second stub, a failed scan leaves page as 1 and selects the wrong response page. Preserve the scan error with %w.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 35-35: Error return value of fmt.Sscanf is not checked
(errcheck)
📍 Affects 1 file
tools/ci-health/github_test.go#L34-L36(this comment)tools/ci-health/github_test.go#L224-L226
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/ci-health/github_test.go` around lines 34 - 36, Handle fmt.Sscanf
errors in both page-parsing stubs: tools/ci-health/github_test.go lines 34-36
and 224-226. Check the scan result before decrementing idx or using page, and
return a contextual wrapped error with %w when parsing fails; preserve normal
response-page selection for valid page numbers.
Sources: Coding guidelines, Linters/SAST tools
| var fetch = func(path, repo string) ([]byte, error) { | ||
| out, err := exec.Command("gh", "api", "repos/"+repo+"/"+path).Output() | ||
| if err != nil { | ||
| var ee *exec.ExitError | ||
| if ok := asExitError(err, &ee); ok { | ||
| return nil, fmt.Errorf("gh api %s: %s", path, strings.TrimSpace(string(ee.Stderr))) | ||
| } | ||
| return nil, fmt.Errorf("gh api %s: %w", path, err) | ||
| } | ||
| return out, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Preserve the gh command error.
Line 34 returns only formatted stderr. This discards the originating *exec.ExitError.
Wrap err with %w and retain stderr in the message.
Proposed fix
- return nil, fmt.Errorf("gh api %s: %s", path, strings.TrimSpace(string(ee.Stderr)))
+ return nil, fmt.Errorf("gh api %s: %s: %w", path, strings.TrimSpace(string(ee.Stderr)), err)As per coding guidelines, preserve the originating error with %w.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var fetch = func(path, repo string) ([]byte, error) { | |
| out, err := exec.Command("gh", "api", "repos/"+repo+"/"+path).Output() | |
| if err != nil { | |
| var ee *exec.ExitError | |
| if ok := asExitError(err, &ee); ok { | |
| return nil, fmt.Errorf("gh api %s: %s", path, strings.TrimSpace(string(ee.Stderr))) | |
| } | |
| return nil, fmt.Errorf("gh api %s: %w", path, err) | |
| } | |
| return out, nil | |
| var fetch = func(path, repo string) ([]byte, error) { | |
| out, err := exec.Command("gh", "api", "repos/"+repo+"/"+path).Output() | |
| if err != nil { | |
| var ee *exec.ExitError | |
| if ok := asExitError(err, &ee); ok { | |
| return nil, fmt.Errorf("gh api %s: %s: %w", path, strings.TrimSpace(string(ee.Stderr)), err) | |
| } | |
| return nil, fmt.Errorf("gh api %s: %w", path, err) | |
| } | |
| return out, nil |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 29-29: An argument passed to exec.Command/exec.CommandContext is built by concatenating a string literal with dynamic input. If that input is attacker-controlled (and especially when the command is a shell such as sh -c/bash -c), this enables OS command injection. Pass untrusted data as separate, fixed arguments instead of interpolating it into a command string, avoid invoking a shell, and validate/escape the input where a shell is unavoidable.
Context: exec.Command("gh", "api", "repos/"+repo+"/"+path)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-concat-arg-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/ci-health/github.go` around lines 29 - 38, Update the fetch function’s
*exec.ExitError branch to wrap the original err with %w while retaining the
trimmed stderr in the formatted message; leave the generic error branch and
successful output behavior unchanged.
Source: Coding guidelines
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Go modules ---'
fd -HI -t f '^go\.mod$' .
printf '%s\n' '--- Existing OpenTelemetry usage ---'
rg -n -C 3 'go\.opentelemetry\.io|otel\.|traceparent|propagation\.' . -g '*.go' -g 'go.mod'Repository: NVIDIA/nvcf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Guidance files ---'
find .. -name AGENTS.md -print
printf '%s\n' '--- Tool files and module declarations ---'
git ls-files 'tools/ci-health/*' 'go.mod' 'go.sum' '*go.mod' '*go.sum' | sed -n '1,160p'
printf '%s\n' '--- Relevant tool source ---'
sed -n '1,90p' tools/ci-health/github.go
sed -n '1,80p' tools/ci-health/main.go 2>/dev/null || true
printf '%s\n' '--- Non-vendor tracing usage ---'
rg -n -g '*.go' -g 'go.mod' -g '!**/vendor/**' \
'go\.opentelemetry\.io|otel\.|traceparent|propagation\.' . \
| sed -n '1,240p'
printf '%s\n' '--- Tool tests and callers ---'
rg -n -g '!**/vendor/**' 'tools/ci-health|fetch\(|gh api|github\.go' . \
| sed -n '1,240p'Repository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Applicable guidance ---'
for f in AGENTS.md tools/AGENTS.md tools/ci-health/AGENTS.md; do
if [ -f "$f" ]; then
echo "### $f"
cat "$f"
fi
done
printf '%s\n' '--- ci-health module ---'
cat tools/ci-health/go.mod
if [ -f tools/ci-health/go.sum ]; then
sed -n '1,160p' tools/ci-health/go.sum
fi
printf '%s\n' '--- ci-health source outline and tests ---'
wc -l tools/ci-health/*.go
sed -n '1,260p' tools/ci-health/github_test.go
printf '%s\n' '--- Existing tracing helpers and module boundaries ---'
sed -n '1,180p' src/libraries/go/worker/tracing/tracing.go
sed -n '1,120p' src/libraries/go/lib/pkg/otel/otel.go
sed -n '1,180p' src/libraries/go/lib/go.modRepository: NVIDIA/nvcf
Length of output: 44413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- License policy ---'
if [ -f .allowed-licenses.txt ]; then
rg -n -i 'apache|otel|mit|bsd' .allowed-licenses.txt
else
echo '.allowed-licenses.txt not found'
fi
printf '%s\n' '--- Existing module references to shared tracing packages ---'
rg -n -g 'go.mod' -g '!**/vendor/**' \
'src/libraries/go/(worker|lib)|pkg/otel|pkg/nvkit/tracing' . \
| sed -n '1,160p'
printf '%s\n' '--- GitHub CLI invocation capabilities in repository docs ---'
rg -n -i -g '!**/vendor/**' -g '!**/*.js' \
'gh api|--header|-H[[:space:]].*trace|TRACEPARENT|TRACESTATE' . \
| sed -n '1,200p'Repository: NVIDIA/nvcf
Length of output: 25565
🌐 Web query:
GitHub CLI gh api custom HTTP request headers -H documentation
💡 Result:
To add custom HTTP request headers when using the GitHub CLI (gh), use the --header or -H flag with the gh api command [1][2]. The syntax follows the standard key:value format: gh api -H 'Header-Name: header-value' You can provide multiple headers by repeating the -H flag [1]. Key points regarding headers in the GitHub CLI: 1. Per-request Headers: The -H flag is specifically for the gh api command and only applies to the request being executed [3]. 2. Response Headers: If you need to view the HTTP response headers returned by the server, you can use the --include flag with your gh api command [1][2]. 3. Global/Per-host Headers: As of August 2026, the GitHub CLI does not natively support configuring custom global or per-host HTTP headers that apply automatically to all gh commands (such as gh pr or gh issue) [3]. Users must explicitly add headers to each gh api call [3]. 4. Preview Headers: While the --header flag can be used for any custom header, the gh api command also provides a dedicated --preview flag for simplifying GitHub API preview requests [1]. For example, --preview name is a shorthand for adding an Accept header with the appropriate media type [1].
Citations:
- 1: https://cli.github.com/manual/gh_api
- 2: https://commandmasters.com/commands/gh-api-common/
- 3: Add per-host custom HTTP headers support cli/cli#12753
Instrument the outbound GitHub call.
Create a client span around fetch, propagate W3C trace context with gh api -H, and record command failures as span errors. Add focused instrumentation tests.
tools/ci-health has no tracing dependency or provider setup. Reuse the existing repository tracing package or add OpenTelemetry with the required Apache-2.0 license and attribution review.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 29-29: An argument passed to exec.Command/exec.CommandContext is built by concatenating a string literal with dynamic input. If that input is attacker-controlled (and especially when the command is a shell such as sh -c/bash -c), this enables OS command injection. Pass untrusted data as separate, fixed arguments instead of interpolating it into a command string, avoid invoking a shell, and validate/escape the input where a shell is unavoidable.
Context: exec.Command("gh", "api", "repos/"+repo+"/"+path)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-concat-arg-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/ci-health/github.go` around lines 29 - 38, Instrument the fetch
function around the outbound gh api command by creating a client span, injecting
W3C trace context into the command via the -H request header, and recording
command failures on the span before returning the existing errors. Reuse the
repository’s existing tracing package and provider setup where available;
otherwise add the required OpenTelemetry dependency with Apache-2.0 license and
attribution review. Add focused tests covering span creation, trace-header
propagation, and failure recording.
Source: Coding guidelines
3 participants
Why
Two questions came up that nobody could answer without hand-rolling
gh apicalls: are the caches healthy, and why is the build slow.
The caches were not healthy. The repo sat at 9.42 GB of its 10 GB quota (94%,
and 97% by the time this was written), which means GitHub is evicting
least-recently-used entries continuously. That failure mode is invisible in a
green pipeline: jobs still pass, they just stop being fast. Two specific causes:
dependency-docs-bazelheld 4.19 GB across three copies, 42% of the entirequota, because every branch saved its own full-size copy.
gh-readonly-queue/**refs. Those branches are deleted whenthe merge queue drains, so the entries can never be restored, but they still
count against quota until evicted.
Answering the second question needed a tool, and the first version of that tool
was wrong in a way worth calling out: it sampled
actions/runs?per_page=100andfiltered by workflow name client-side, so it saw 14 of 1275 bazel runs. That led
me to state there was no history to trend against. There are three full weeks.
What changed
Cache:
actions/cache/restore+ guardedactions/cache/save. Dependency-docs saves only frommain; bazel skipsmerge-queue refs. Restores are unchanged everywhere, so no job gets colder.
tools/ci-health(Go, withtools/ci/ci-healthas the stable entrypoint):--dashboardwrites a self-contained HTML report and opens it. Inline SVG,no CDN and no JavaScript dependency, so it works offline and adds nothing to
the dependency surface.
and the distinction turns out to matter:
stargatespends roughly 6.1 minwaiting for a runner against roughly 3.4 min building.
only the tail of that week is held, so its median plotted as a misleading
near-zero point.
misread as a speedup.
Matrix:
max-parallel8 to 12 on the bazel matrix.Customer Release Notes
Not customer visible.
Plan Summary
Not applicable.
Usage
Testing
go test ./tools/ci-health/...: 37 tests, no third-party dependencies. Eachcase corresponds to a defect this tool actually shipped with, including the two
found in review: bare-array pagination and single-page cache listing. Coverage
includes percentile interpolation, the partial-week drop, skipped-matrix
classification, the queue/execution split, long-pole attribution excluding gate
jobs, tolerance of failed job fetches, and that the dashboard stays
self-contained.
Every subcommand was run against live data, including error paths. The dashboard
was rendered headless and inspected, not just parsed.
The full 25-row matrix passed on this branch with no HTTP 429s at the new cap.
Notes
On the
max-parallelchange, the measured result was weaker than predicted.Comparing two 25-row
pull_requestruns, same branch and same runner class:Wall clock improved 9%, but per-row time rose 22%. Both runs used GitHub-hosted
runners (separate VMs, so not CPU contention between rows); the shared remote
cache endpoint is the likeliest explanation. This is n=1 on each side, so it may
be noise. Worth a week of samples via
tools/ci/ci-health --durationsbeforeconcluding either way, and easy to revert if not.
The cap also exists to keep simultaneous
actions/checkoutdownloads underGitHub's rate limit, which previously failed runs at "Set up job" with HTTP 429.
That is why this is 12 and not 16: combined with the
bazel-dockermatrix thecheckout burst goes from 12 to 16. If 429s reappear, walk this back first.
This fix stops the bleed but does not reclaim space. Deleting the two stranded
merge-queue entries frees 1.90 GB immediately.
GitHub Pages is not enabled on this repo, and enabling it would publish the
dashboard publicly. A scheduled workflow uploading the HTML as an artifact is
the better route if we want it without running the tool locally.
References
None
Related Merge Requests/Pull Requests
None
Dependencies
None. The dashboard is inline SVG specifically to avoid adding a charting
library, and the Go module has no requires.
Github commit:
fix(ci): cut cache-quota waste, add a self-serve build-health dashboard
Co-authored-by: Balaji Ganesan bganesan@nvidia.com
Summary by CodeRabbit