ci: batch codebase-growth-guardrails blob fetches and add retry - #6641
Conversation
The 'Require changed test files to stay within size budget' step issued one REST /contents/ call per legacy budget entry plus one per changed test file (13+T calls), with no retry. GitHub's contents endpoint returns intermittent 5xx (HTTP 502 today); with the current \~11 legacy entries, a single ambient 502 anywhere in that sequence fails the whole job. Observed today: 9/30 (30%) runs of this workflow failed across all PRs, all with 'HTTP 502' at api.github.com/repos/.../contents/<file>. Reruns are non-deterministic because each rerun is a fresh coin flip over the same N calls. This change: - Batches every needed HEAD-side blob fetch into one GraphQL request per repo/SHA, using aliases (up to 25 blobs per request). The BASE budget file is fetched separately so we can decide which HEAD blobs we still need. Typical run: 1 REST call for /pulls/files, 1 GraphQL call for the BASE budget, and 1 GraphQL call for the HEAD batch. - Wraps every outbound request in a retry-with-jittered-exponential- backoff (4 attempts, 250ms base, 4s cap) that only retries on 408, 425, 429, 5xx, and network errors. - Falls back to REST /contents/ for individual blobs when GraphQL returns null text or is truncated (e.g. very large files). Policy semantics are unchanged: budget monotonicity, legacy line-count enforcement, defaultMaxLines guarding, and changed-file line-count checks all match the previous implementation. The workflow remains data-only and never checks out PR code (still pull_request_target). With p \~= 0.05 per call, current run-level failure ~= 51%; batched + retried run-level failure ~= 0.04%. Signed-off-by: J. Yaunches <jyaunches@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
📝 WalkthroughWalkthroughThe workflow now retrieves repository blobs through retryable batched GraphQL queries with REST fallback, then uses the shared loader for budget monotonicity, line-count, and changed-test-file validation. ChangesGrowth guardrail retrieval
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GuardrailScript
participant GraphQLAPI
participant RESTContentsAPI
participant BudgetChecks
GuardrailScript->>GraphQLAPI: Request batched base and HEAD blobs
GraphQLAPI-->>GuardrailScript: Return blob text or unavailable paths
GuardrailScript->>RESTContentsAPI: Fetch unavailable content
RESTContentsAPI-->>GuardrailScript: Return decoded file content or null
GuardrailScript->>BudgetChecks: Validate budgets and changed test files
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/codebase-growth-guardrails.yaml (1)
163-200: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
isTransientis dead code; retriability is decided by parsing the error message instead.
isTransient(Line 163) is never called.requestJsonthrows a plainHTTP <status>error at Line 194 without setting.transient, sowithRetrycan only classify HTTP failures by regex-matchingerror.message. That couples retry behavior to the exact message string and duplicates the status logic thatisTransientalready encodes. Route HTTP-error classification throughisTransientat the throw sites (here and ingetContentViaRestat Line 285) so status codes are the single source of truth.♻️ Set
error.transientfromisTransient- if (!response.ok) throw new Error(`${url}: HTTP ${response.status}`); - return response.json(); + if (!response.ok) { + const error = new Error(`${url}: HTTP ${response.status}`); + if (isTransient(response.status)) error.transient = true; + throw error; + } + return response.json();Apply the same pattern at Line 285 in
getContentViaRest, after whichwithRetrycan rely onerror.transientalone.🤖 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/codebase-growth-guardrails.yaml around lines 163 - 200, Use the existing isTransient(status) helper as the single source of truth for HTTP retry classification: in requestJson and getContentViaRest, assign the result of isTransient(response.status) to the thrown HTTP error’s transient property before throwing. Update withRetry to rely on error.transient rather than parsing error.message, while preserving network-error handling.
🤖 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.
Nitpick comments:
In @.github/workflows/codebase-growth-guardrails.yaml:
- Around line 163-200: Use the existing isTransient(status) helper as the single
source of truth for HTTP retry classification: in requestJson and
getContentViaRest, assign the result of isTransient(response.status) to the
thrown HTTP error’s transient property before throwing. Update withRetry to rely
on error.transient rather than parsing error.message, while preserving
network-error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 02cb4c6d-d1a4-4c72-ab07-70874d06f5b6
📒 Files selected for processing (1)
.github/workflows/codebase-growth-guardrails.yaml
…IA#6641) ## Summary `.github/workflows/codebase-growth-guardrails.yaml` — batch HEAD-side blob fetches through GraphQL aliases and add retry-with-backoff on transient GitHub API failures. The "Require changed test files to stay within size budget" step is currently the highest-flake required check in the repo. ## Problem The size-budget step issues one `REST /repos/{owner}/{name}/contents/{path}?ref=SHA` call per legacy budget entry (currently 11 in `ci/test-file-size-budget.json`) plus one per changed test file. Each `fetch()` has zero retry, and GitHub's contents endpoint intermittently returns HTTP 502/503. Any single 5xx anywhere in the sequence fails the entire required check. Evidence: - Today, `codebase-growth-guardrails` failed **9/30 runs (30%)** across all PRs, every failure with the same signature: `Error: https://api.github.com/repos/<owner>/NemoClaw/contents/<file>?ref=<sha>: HTTP 502`. - On PR NVIDIA#6616 the workflow failed **3 consecutive rerun attempts** on three *different* files (`ci/test-file-size-budget.json`, `nemoclaw/src/commands/migration-state.test.ts`, `src/lib/inference/nim.test.ts`) — confirms intermittent per-request 5xx rather than a hard outage, and confirms reruns don't converge because each rerun is a fresh coin flip over the same N calls. - `main` runs of this workflow are essentially always green (last failure 2026-06-26), because they run against the trusted repo and hit the endpoint less. Fork PRs are disproportionately affected. ## Change - Batch every needed HEAD-side blob fetch into one GraphQL request per `repo/SHA` using aliases (up to 25 blobs per request, larger sets are chunked). The BASE-side budget file is fetched separately so the script can parse legacy entries before deciding which HEAD blobs to load. - Wrap every outbound request (REST and GraphQL) in `withRetry` — 4 attempts with jittered exponential backoff (250 ms base, 4 s cap) that only retries on 408, 425, 429, 5xx, and network errors. - Fall back to REST `/contents/` for any individual blob GraphQL returns as `null` text or `isTruncated: true` (e.g. very large files). Policy is unchanged: budget monotonicity, legacy line-count enforcement (`lines > maxLines` and `lines < maxLines`), `defaultMaxLines` guarding, changed-file line-count checks, and the "removed legacy budget must not exceed defaultMaxLines" invariant all match the previous implementation. This is data-only; the workflow stays on `pull_request_target` and still does not check out PR code. ## API-call and flake math | | Current | After | |---|---|---| | Sequential calls per run | ~13 + T | 3 (1 REST files + 2 GraphQL) | | Retries on transient 5xx | none | 4 attempts, backoff | | Run-level failure at p = 0.05 per call | ~51% | ~0.04% | `T` = number of changed test files matching the size-budget regex. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - YAML lints (repo `check yaml` hook passed at commit time). - Manual dry-run of the new script against PR NVIDIA#6616's file set (13 blobs) succeeds locally with `GH_TOKEN` set; batched query returns all 13 blobs in one round-trip. - Retry path exercised by pointing `fetch` at a 502-returning mock; verified backoff order and eventual success. - `pull_request_target` guarantee preserved: no `actions/checkout`, no PR-authored code executed, tokens read-only. Refs the ambient flake surfaced on PR NVIDIA#6616 and every other fork PR hitting `codebase-growth-guardrails` today. Signed-off-by: J. Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Improved CI validation reliability when checking changed test file size budgets. * Reduced the number of repository content requests through batched retrieval. * Added automatic retries for transient network, rate-limit, and server errors. * Added fallback handling for unavailable or truncated content and explicit detection of binary files. * Preserved budget monotonicity checks, including legacy budget comparisons and line-count validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: J. Yaunches <jyaunches@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Summary
.github/workflows/codebase-growth-guardrails.yaml— batch HEAD-side blob fetches through GraphQL aliases and add retry-with-backoff on transient GitHub API failures. The "Require changed test files to stay within size budget" step is currently the highest-flake required check in the repo.Problem
The size-budget step issues one
REST /repos/{owner}/{name}/contents/{path}?ref=SHAcall per legacy budget entry (currently 11 inci/test-file-size-budget.json) plus one per changed test file. Eachfetch()has zero retry, and GitHub's contents endpoint intermittently returns HTTP 502/503. Any single 5xx anywhere in the sequence fails the entire required check.Evidence:
codebase-growth-guardrailsfailed 9/30 runs (30%) across all PRs, every failure with the same signature:Error: https://api.github.com/repos/<owner>/NemoClaw/contents/<file>?ref=<sha>: HTTP 502.ci/test-file-size-budget.json,nemoclaw/src/commands/migration-state.test.ts,src/lib/inference/nim.test.ts) — confirms intermittent per-request 5xx rather than a hard outage, and confirms reruns don't converge because each rerun is a fresh coin flip over the same N calls.mainruns of this workflow are essentially always green (last failure 2026-06-26), because they run against the trusted repo and hit the endpoint less. Fork PRs are disproportionately affected.Change
repo/SHAusing aliases (up to 25 blobs per request, larger sets are chunked). The BASE-side budget file is fetched separately so the script can parse legacy entries before deciding which HEAD blobs to load.withRetry— 4 attempts with jittered exponential backoff (250 ms base, 4 s cap) that only retries on 408, 425, 429, 5xx, and network errors./contents/for any individual blob GraphQL returns asnulltext orisTruncated: true(e.g. very large files).Policy is unchanged: budget monotonicity, legacy line-count enforcement (
lines > maxLinesandlines < maxLines),defaultMaxLinesguarding, changed-file line-count checks, and the "removed legacy budget must not exceed defaultMaxLines" invariant all match the previous implementation. This is data-only; the workflow stays onpull_request_targetand still does not check out PR code.API-call and flake math
T= number of changed test files matching the size-budget regex.Type of Change
Verification
check yamlhook passed at commit time).GH_TOKENset; batched query returns all 13 blobs in one round-trip.fetchat a 502-returning mock; verified backoff order and eventual success.pull_request_targetguarantee preserved: noactions/checkout, no PR-authored code executed, tokens read-only.Refs the ambient flake surfaced on PR #6616 and every other fork PR hitting
codebase-growth-guardrailstoday.Signed-off-by: J. Yaunches jyaunches@nvidia.com
Summary by CodeRabbit