refactor(ci): extract codebase growth guardrails into trusted tools - #6993
Merged
Conversation
The codebase-growth-guardrails workflow embedded two ~260/~366-line node heredocs that duplicated the entire GitHub REST/GraphQL blob client (retry, batching, REST fallback) and hand-rolled a second JavaScript lexer to count `if` statements. The heredoc-extraction test executed YAML-embedded code, and the `if` counter could drift from the local scanner. Move the shared PR blob client and the two policy evaluators into trusted tools/growth-guardrails/*.mts, run from the base checkout under pull_request_target. Line counting and budget parsing reuse scripts/check-test-file-size-budget.mts; `if` counting reuses scanTextForTestConditionals from scripts/find-test-conditionals.mts, so the workflow and the local scanner agree by construction and no second lexer remains in YAML. Blob text is fetched as data and parsed with the TypeScript AST, never executed, preserving the data-only trust boundary; a workflow-boundary module and test assert that boundary (pull_request_target trigger, read-only permissions, base-only checkout, --ignore-scripts install, no inline heredoc). The two inline shell checks stay inline as specified. Replace the heredoc-extraction test with direct module tests covering budget monotonicity, rename/delete, binary/truncated blobs, REST fallback, batching, retry, AST parity, and the trust boundary. Refs #6953 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR extracts inline growth guardrails into trusted TypeScript tools, adds shared GitHub PR/blob retrieval with retries and fallbacks, implements size and conditional policies, validates workflow trust boundaries, and adds focused tests and workflow integration. ChangesGrowth Guardrails
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant TestSizeBudget
participant TestConditionals
participant GitHubAPI
GitHubActions->>TestSizeBudget: execute trusted budget policy
GitHubActions->>TestConditionals: execute trusted conditional policy
TestSizeBudget->>GitHubAPI: fetch PR files and budget/test blobs
TestConditionals->>GitHubAPI: fetch PR files and base/head blobs
GitHubAPI-->>TestSizeBudget: return policy inputs
GitHubAPI-->>TestConditionals: return source contents
TestSizeBudget-->>GitHubActions: report budget result
TestConditionals-->>GitHubActions: report conditional result
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test/pr-workflow-contract.test.ts (1)
1171-1178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert each trusted tool step’s environment independently.
The aggregate
guardEnvcheck passes when only one policy step definesHEAD_REPO; the other then fails inreadEnv(). Locate each step by its trusted invocation and assert its required environment directly.Proposed test shape
-const guardEnv = JSON.stringify((guardJob.steps ?? []).map((step) => step.env ?? {})); -expect(guardEnv).toContain("HEAD_REPO"); +for (const tool of [ + "tools/growth-guardrails/test-size-budget.mts", + "tools/growth-guardrails/test-conditionals.mts", +]) { + const step = guardJob.steps?.find(({ run }) => run?.includes(tool)); + expect(step?.env).toHaveProperty("HEAD_REPO"); +}As per path instructions, review tests for behavioral confidence rather than broad implementation-level aggregation.
🤖 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/pr-workflow-contract.test.ts` around lines 1171 - 1178, Update the growth guardrails contract test around growthGuardrails and stepRuns to locate each trusted tool invocation separately, then assert that each corresponding step’s environment includes HEAD_REPO. Remove the aggregate guardEnv assertion so the test verifies the required environment for every trusted policy step independently.Source: Path instructions
🤖 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/growth-guardrails/pr-blob-client.mts`:
- Around line 157-191: The getContentViaRest fallback must request GitHub’s raw
contents media type and return the response text instead of parsing the
JSON/base64 shape. Update its fetch headers to include the raw Accept value and
read response.text() after status validation; in
test/growth-guardrails-pr-blob-client.test.ts lines 110-127, update the mock to
provide a raw body and assert the Accept header.
In `@tools/growth-guardrails/test-size-budget.mts`:
- Around line 51-91: The fallback budget must act as the baseline when the base
budget file is absent. In tools/growth-guardrails/test-size-budget.mts, update
the default-limit monotonicity check around baseWasFallback so
headBudget.defaultMaxLines is compared against the 1500-line fallback and
increases produce a violation. In
test/growth-guardrails-test-size-budget.test.ts, update the affected test to
expect an increased-default violation instead of passing.
- Around line 57-76: Update the legacy-budget comparison loops in the test-size
budget logic to honor retained previous_filename mappings when files are
renamed. Resolve each head entry’s base-path allowance through the rename
mapping, then compare the renamed head budget against that allowance instead of
treating it as removed and newly added; preserve existing behavior for
non-renamed files and ensure the related checks at the other indicated blocks
use the same mapping.
In `@tools/growth-guardrails/workflow-boundary.mts`:
- Around line 39-47: Extend WorkflowJob and the workflow validation logic to
inspect each jobs.<job_id>.permissions entry, alongside WorkflowDoc permissions.
Reject any job-level permissions granting contents: write, and add a test
covering this override while preserving existing workflow-level permission
checks.
- Around line 102-130: The workflow validation around REQUIRED_TOOL_INVOCATIONS
must fail closed by validating every executable run step against an exact
allowlist of permitted command shapes, rather than accepting trusted tool
strings found elsewhere. Reject any additional or unrecognized run step,
including steps that fetch or execute PR-controlled code, while preserving the
existing install, checkout, heredoc, and trusted-tool checks. Add a regression
test covering an extra run step that bypasses the trusted invocations.
---
Nitpick comments:
In `@test/pr-workflow-contract.test.ts`:
- Around line 1171-1178: Update the growth guardrails contract test around
growthGuardrails and stepRuns to locate each trusted tool invocation separately,
then assert that each corresponding step’s environment includes HEAD_REPO.
Remove the aggregate guardEnv assertion so the test verifies the required
environment for every trusted policy step independently.
🪄 Autofix (Beta)
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: b0db488c-ec8c-4338-a6ee-11b3af524e4c
📒 Files selected for processing (12)
.github/workflows/codebase-growth-guardrails.yamlci/source-shape-test-budget.jsontest/codebase-growth-guardrails-conditionals.test.tstest/growth-guardrails-pr-blob-client.test.tstest/growth-guardrails-test-conditionals.test.tstest/growth-guardrails-test-size-budget.test.tstest/growth-guardrails-workflow-boundary.test.tstest/pr-workflow-contract.test.tstools/growth-guardrails/pr-blob-client.mtstools/growth-guardrails/test-conditionals.mtstools/growth-guardrails/test-size-budget.mtstools/growth-guardrails/workflow-boundary.mts
💤 Files with no reviewable changes (1)
- test/codebase-growth-guardrails-conditionals.test.ts
Address CodeRabbit findings on the extracted growth-guardrail tools: - pr-blob-client: the REST fallback only runs for GraphQL-truncated (large) blobs, where the /contents object shape drops `content` above 1 MB. Request the `application/vnd.github.raw` media type and read the body text directly instead of decoding a base64 JSON field that is empty for those files. - test-size-budget: enforce default-limit monotonicity against the 1500-line fallback when the base budget file is absent, instead of skipping every check. - test-size-budget: follow test-file renames so a moved-but-unchanged legacy allowance is compared against its base-path entry rather than read as a newly added above-default budget. - workflow-boundary: also reject job-level `permissions:` write scopes, which override the read-only workflow default. - workflow-boundary: fail closed on run steps. Requiring the tool strings to appear somewhere allowed an added step to fetch and execute PR code; every run step must now carry a permitted signature and no forbidden execution primitive (| bash, | sh, curl, wget, eval, node heredoc). Refs #6953 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Contributor
Author
|
Thanks @coderabbitai — addressed all five findings in 8f15c3e:
Signed-off-by: Dongni Yang dongniy@nvidia.com |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin 8f15c3e13 --depth=2 2>&1 | tail -5 || true
git show 8f15c3e13 --stat 2>&1 | head -50Length of output: 2315 🏁 Script executed: #!/bin/bash
set -euo pipefail
git show 8f15c3e13f1842468226da311dfeb0233e6957d2 -- tools/growth-guardrails/pr-blob-client.mts | head -80
echo "----"
git show 8f15c3e13f1842468226da311dfeb0233e6957d2 -- tools/growth-guardrails/workflow-boundary.mts | head -150Length of output: 9800
Nice, thorough hardening pass — thanks for the detailed writeup. (ノ◕‿◕)ノ✧ |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/growth-guardrails-entrypoints.test.ts`:
- Line 70: Update the top-level describe title in the growth-guardrails
executable entrypoints test to append the local issue reference “(`#6953`)” as a
final suffix. Keep the existing behavior-oriented wording unchanged and do not
add the suffix to individual it titles.
🪄 Autofix (Beta)
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: 0efa87f7-2312-4f65-853e-d94da227203f
📒 Files selected for processing (4)
test/growth-guardrails-entrypoints.test.tstest/growth-guardrails-pr-blob-client.test.tstest/growth-guardrails-test-conditionals.test.tstest/growth-guardrails-test-size-budget.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/growth-guardrails-test-conditionals.test.ts
- test/growth-guardrails-test-size-budget.test.ts
- test/growth-guardrails-pr-blob-client.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: architecture
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
codebase-growth-guardrailspull_request_targetworkflow embedded two Node heredocs (~581 nonblank lines) that duplicated the entire GitHub REST/GraphQL blob client and hand-rolled a second JavaScript lexer to countifstatements. This extracts that logic into trustedtools/growth-guardrails/*.mtsmodules run from the base checkout, reusing the local scanners so the workflow and local checks agree by construction. Behavior is preserved for the test-size budget; theifcounter now uses the same TypeScript AST asfind-test-conditionals(no second lexer). One slice of epic #6952.Related Issue
Fixes #6953
This fully implements sub-issue #6953's acceptance criteria — extraction into trusted tools, pinned trusted-tool invocation from the base checkout, AST-parity conditional counting (no second lexer), preserved budget monotonicity and rename/delete behavior, correct truncated-blob REST fallback, batching/retry, and the trust-boundary test. The parent epic #6952 and its sibling slices remain open independently.
Changes
tools/growth-guardrails/pr-blob-client.mts— shared PR file/blob client (batched GraphQL, REST fallback, transient retry), deduplicated from the two heredocs.tools/growth-guardrails/test-size-budget.mts— budget monotonicity + changed-test evaluator, reusingparseBudget/countLinesfromscripts/check-test-file-size-budget.mts.tools/growth-guardrails/test-conditionals.mts—if-count evaluator reusingscanTextForTestConditionalsfromscripts/find-test-conditionals.mts(removes the hand-rolled lexer).tools/growth-guardrails/workflow-boundary.mts+ test — assert thepull_request_targettrust boundary (trigger, read-only permissions, base-only checkout,--ignore-scriptsinstall, no inline heredoc).npm ci --ignore-scripts+node --experimental-strip-types tools/growth-guardrails/*.mts. The two inline shell checks (JS-block,onboard.tsnet-growth) stay inline per the issue.Security note: the tools run from the trusted base revision and fetch PR blob text as data, parsing it with the TypeScript AST — they never execute PR-controlled code. The workflow-boundary module + test enforce this.
Type of Change
Quality Gates
pull_request_targetpolicy gate; the newworkflow-boundary.mts+test/growth-guardrails-workflow-boundary.test.tsassert data-only execution (base-only checkout, read-only permissions,--ignore-scripts, no inline heredoc, no PR-head checkout), and blob text is AST-parsed, never executed.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed — scoped to the changed files (hadolintnot installed locally, no Dockerfile in diff).npx vitest run test/growth-guardrails-*.test.ts(36 passed);test/pr-workflow-contract.test.tsgrowth assertion passes;npm run typecheck:cliclean; source-shape + test-file-size budgets pass; boundary validator PASSES on the real workflow.Signed-off-by: Dongni Yang dongniy@nvidia.com
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Tests