ci(guardrails): batch conditional source reads - #6780
Conversation
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-advisor-raw-output.txt |
📝 WalkthroughWalkthroughThe conditional guardrail workflow now retrieves changed-file content through retryable, batched GraphQL requests with REST fallback, counts prefetched base and head text, and adds tests for batching, retries, and truncated-blob fallback. ChangesConditional scanning guardrail
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GuardrailTests
participant ConditionalScanningScript
participant GitHubGraphQLAPI
participant GitHubRESTContentsAPI
GuardrailTests->>ConditionalScanningScript: Execute conditional scan
ConditionalScanningScript->>GitHubGraphQLAPI: Fetch base and head blobs with retries
GitHubGraphQLAPI-->>ConditionalScanningScript: Return text or fallback metadata
ConditionalScanningScript->>GitHubRESTContentsAPI: Retrieve truncated or missing content
GitHubRESTContentsAPI-->>ConditionalScanningScript: Return decoded text or null
ConditionalScanningScript-->>GuardrailTests: Report conditional totals and status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Ho Lim <subhoya@gmail.com>
203b942 to
26e5f1f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/codebase-growth-guardrails-conditionals.test.ts (1)
216-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetry test asserts a fetch-ordering detail rather than only the retry outcome.
expect(result.stderr).toContain("retry: graphql NVIDIA/NemoClaw@base-sha attempt 1 failed")only holds because the source'sPromise.all([fetchBlobs(REPO, BASE_SHA, ...), fetchBlobs(HEAD_REPO, HEAD_SHA, ...)])evaluates the base call first and the mockfetchhas no internalawait, so the single simulated failure deterministically lands on the base request. That's correct today, but it couples the test to an internal call-ordering detail rather than purely the retry contract (transient failure → automatic recovery, exit 0). A regex that doesn't pin the repo/sha (e.g./retry: graphql .* attempt 1 failed/) would still prove the retry-and-recover behavior without breaking if the base/head fetch order or labeling changes.
Per path instructions, tests should "Prefer observable outcomes... over ... mock-call assertions" — the sha-specific match leans toward locking in an implementation detail.🔧 Suggested loosening of the assertion
- expect(result.stderr).toContain("retry: graphql NVIDIA/NemoClaw@base-sha attempt 1 failed"); + expect(result.stderr).toMatch(/retry: graphql \S+ attempt 1 failed/);🤖 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 `@test/codebase-growth-guardrails-conditionals.test.ts` around lines 216 - 234, Loosen the stderr assertion in the “retries a transient GraphQL failure” test to match the retry message and attempt number without requiring the specific repository or SHA. Keep the existing exit-status and request-count assertions so the test continues to verify transient-failure recovery without depending on fetch ordering.Source: Path instructions
.github/workflows/codebase-growth-guardrails.yaml (1)
397-459: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSecond copy of the entire retry/GraphQL batching stack — extract to a shared script instead of duplicating.
withRetry,requestJson,getJson,graphql,splitRepoName,buildBlobQuery,fetchBlobs, andgetContentViaResthere are near-verbatim copies of the same helpers already implemented in the "stay within size budget" step above (lines 130-292). This doubles the maintenance surface for ~150 lines of retry/batching logic and has already produced drift between the two copies:
- Step 1's
graphql()label usesvariables?.owner ?? ""}/${variables?.name ?? ""}@${variables?.oid ?? ""}(line 204); this copy usesvariables.owner}/${variables.name}@${variables.oid}(line 442) without the optional-chaining/fallback.- This copy's
getContentViaRestadds anif (!file) return null;guard (line 470) that step 1's equivalent (line 272) doesn't have.Neither drift is currently a live bug (both are always called with a defined
variables/file), but it shows the two copies are already silently diverging, which is exactly the risk of hand-duplicated infra code. Consider factoring these helpers into a single checked-in script (e.g.scripts/ci/github-api-helpers.mjs) that both steps invoke, so retry/backoff and batching semantics stay in one place.Also applies to: 469-529
🤖 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 397 - 459, The workflow contains a duplicated retry, GraphQL, batching, and content-fetch implementation that should be centralized. Extract withRetry, requestJson, getJson, graphql, splitRepoName, buildBlobQuery, fetchBlobs, and getContentViaRest into one checked-in shared script, then update both workflow steps to invoke or import that implementation and remove the duplicate inline helpers. Preserve the existing retry/backoff, batching, optional-value handling, and missing-file behavior consistently across both callers.
🤖 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 397-459: The workflow contains a duplicated retry, GraphQL,
batching, and content-fetch implementation that should be centralized. Extract
withRetry, requestJson, getJson, graphql, splitRepoName, buildBlobQuery,
fetchBlobs, and getContentViaRest into one checked-in shared script, then update
both workflow steps to invoke or import that implementation and remove the
duplicate inline helpers. Preserve the existing retry/backoff, batching,
optional-value handling, and missing-file behavior consistently across both
callers.
In `@test/codebase-growth-guardrails-conditionals.test.ts`:
- Around line 216-234: Loosen the stderr assertion in the “retries a transient
GraphQL failure” test to match the retry message and attempt number without
requiring the specific repository or SHA. Keep the existing exit-status and
request-count assertions so the test continues to verify transient-failure
recovery without depending on fetch ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a806f33b-8555-40b4-af35-f9179fbd86d7
📒 Files selected for processing (2)
.github/workflows/codebase-growth-guardrails.yamltest/codebase-growth-guardrails-conditionals.test.ts
Signed-off-by: Ho Lim <subhoya@gmail.com>
wscurran
added
area: ci
|
✨ Thanks for the optimization, @HOYALIM. Batching GraphQL blob reads and adding retries should make the guardrails more reliable and efficient. Ready for maintainer review. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/codebase-growth-guardrails-conditionals.test.ts (2)
92-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo REST-call counter to prove fallback is conditional on truncation.
REST content is pre-mocked for every
input.contentsentry (94-96), but nothing tracks how many times REST (contentsUrl) is actually invoked (onlyMOCK_GRAPHQL_REQUESTSis tracked, Lines 116-117, 120). This means a bug where the guard always falls back to REST regardless ofisTruncated— instead of only when GraphQL reports truncation — would still pass the truncated-blob test (261-280) and the batching test, since both scenarios yield identical observable results.Add a
MOCK_REST_REQUESTScounter (mirroringgraphqlRequests) and assert it's0in the non-truncated batching test and1here, to actually exercise the truncation-conditioned branch rather than just its side effect.As per path instructions, "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
Also applies to: 261-280
🤖 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 `@test/codebase-growth-guardrails-conditionals.test.ts` around lines 92 - 141, Add REST request counting to the generated fetch mock alongside graphqlRequests, incrementing it only when the requested URL matches contentsUrl. Emit the count on process exit as MOCK_REST_REQUESTS, then update the non-truncated batching test to assert zero REST requests and the truncated-blob test to assert exactly one, ensuring the truncation-dependent fallback is exercised.Source: Path instructions
92-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHigh-complexity inline mock script.
This block builds an entire mock GraphQL/REST server as a string template embedded in another string template, with nested regex/parsing logic (Lines 118-141). It's dense and hard to follow/modify. As per coding guidelines,
**/*.{ts,tsx,js,jsx}should "Keep function complexity low." Consider extracting the alias-parsing/response-building logic into small named helper functions (even if they still get serialized into the wrapper string) to improve readability.🤖 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 `@test/codebase-growth-guardrails-conditionals.test.ts` around lines 92 - 141, The generated wrapper in the test contains overly dense inline GraphQL mock logic. Refactor the wrapper construction around the `graphqlFailuresRemaining`/`global.fetch` script by extracting alias parsing and GraphQL response-building into small named helper functions, ensuring those helpers are included in the generated script and preserving the existing request, failure, and response behavior.Source: Coding guidelines
🤖 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 `@test/codebase-growth-guardrails-conditionals.test.ts`:
- Around line 92-141: Add REST request counting to the generated fetch mock
alongside graphqlRequests, incrementing it only when the requested URL matches
contentsUrl. Emit the count on process exit as MOCK_REST_REQUESTS, then update
the non-truncated batching test to assert zero REST requests and the
truncated-blob test to assert exactly one, ensuring the truncation-dependent
fallback is exercised.
- Around line 92-141: The generated wrapper in the test contains overly dense
inline GraphQL mock logic. Refactor the wrapper construction around the
`graphqlFailuresRemaining`/`global.fetch` script by extracting alias parsing and
GraphQL response-building into small named helper functions, ensuring those
helpers are included in the generated script and preserving the existing
request, failure, and response behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 88a780fe-314d-4463-ae01-bbaf7fa13a99
📒 Files selected for processing (1)
test/codebase-growth-guardrails-conditionals.test.ts
Summary
The changed-test conditional guard previously fetched base and head contents one file at a time and failed the whole check on a single transient GitHub API error. This change batches trusted blob reads through GraphQL and retries transient failures while preserving the existing counting policy and REST fallback.
For a PR with 11 changed test files, the normal request shape drops from 23 GitHub API calls (one file-list request plus 22 content requests) to 3 (one file-list request plus two blob batches), an approximately 87% reduction.
Changes
Type of Change
Quality Gates
pull_request_targetjob still executes only trusted base-branch code, validates repository names, sends PR-controlled paths as GraphQL variables, and performs read-only GitHub API operations.Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/codebase-growth-guardrails-conditionals.test.ts(11 passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Ho Lim subhoya@gmail.com
Summary by CodeRabbit