ci: guard oversized test files - #4899
Closed
cv wants to merge 9 commits into
Closed
Conversation
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)
📝 WalkthroughWalkthroughAdds an enforced test-file-size budgeting system (script, JSON budget, tests, pre-commit/npm/CI wiring and an inline GitHub Actions validator enforcing monotonicity/legacy rules) and expands NO_PROXY augmentation to include container aliases and the managed hostname inference.local, with related tests and docs updates. ChangesTest File Size Budget Enforcement
NO_PROXY augmentation and proxy preflight
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Validator as Inline Node.js validator
participant Budget as ci/test-file-size-budget.json
participant GHAPI as GitHub Contents API
participant Head as PR head commit
CI->>Validator: run "Require changed test files to stay within size budget" step
Validator->>Budget: download budget at PR base SHA (fallback if missing)
Validator->>Head: verify budget file retrievable at PR head if budget changed
Validator->>GHAPI: fetch raw contents of each changed test file from PR head
GHAPI-->>Validator: file contents
Validator->>Validator: count lines, compare to default/legacy limits, check monotonicity
Validator->>CI: report violations (fail) or PASS summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
2 tasks
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4899.docs.buildwithfern.com/nemoclaw |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ci/test-file-size-budget.json (1)
1-18:⚠️ Potential issue | 🟠 MajorAlign SPDX handling with strict JSON parsing for
ci/test-file-size-budget.json
ci/test-file-size-budget.jsonis strict JSON consumed viaJSON.parsein bothscripts/check-test-file-size-budget.tsand.github/workflows/codebase-growth-guardrails.yaml, so adding comment-style SPDX headers (///#) would break parsing.- Current repo SPDX hook
spdx-headersin.pre-commit-config.yamldoes not run on.jsonfiles (only*.ts/*.py/*.sh), so this file’s missing SPDX header is not enforced; if JSON SPDX is still required by policy, update the checker to explicitly exempt this file or introduce a JSON-safe SPDX approach.🤖 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 `@ci/test-file-size-budget.json` around lines 1 - 18, ci/test-file-size-budget.json is parsed with JSON.parse so comment-style SPDX headers would break consumers; update the repo checks to avoid adding/removing comment SPDX for this file by either (A) adding an explicit exclusion for "ci/test-file-size-budget.json" in the spdx-headers hook configuration in .pre-commit-config.yaml, or (B) altering the SPDX enforcement/checker to recognize a JSON-safe SPDX mechanism (e.g., a top-level "spdx" property) or to skip JSON files; ensure corresponding consumers in scripts/check-test-file-size-budget.ts and the .github/workflows/codebase-growth-guardrails.yaml continue to parse the file with JSON.parse without expecting comment headers.Source: Coding guidelines
.pre-commit-config.yaml (1)
1-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd SPDX header to this YAML config file.
Lines 1-3: this changed YAML file is missing the required SPDX copyright/license header.
As per coding guidelines, "**/*.{js,ts,tsx,jsx,sh,md,mdx,json,yaml,yml,css,svg}files must include SPDX headers."🤖 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 @.pre-commit-config.yaml around lines 1 - 3, This YAML config file (beginning with the comment "# NemoClaw — prek hook configuration") is missing the required SPDX header; add the SPDX header lines at the very top of .pre-commit-config.yaml (e.g., an SPDX copyright text and an SPDX-License-Identifier) so the file includes both SPDX-FileCopyrightText and SPDX-License-Identifier entries before the existing comments.Source: Coding guidelines
package.json (1)
1-1:⚠️ Potential issue | 🟠 MajorSPDX headers are missing from
package.json; apply the repo’s JSON$commentpattern or document an exception.
- Root
package.jsoncontains noSPDX-FileCopyrightText/SPDX-License-Identifierheader (and no JSON-safe$commentSPDX block), so the repo SPDX guideline for JSON files is unmet.- The repo already carries SPDX inside JSON via
$commentinschemas/router-pool-config.schema.jsonandnemoclaw-blueprint/model-specific-setup/schema.json.- Add a top-level
$commenttopackage.jsonwith the required SPDX strings (keeps npm parsing intact), or make an explicit documented exception forpackage.json.🤖 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 `@package.json` at line 1, Add the SPDX metadata to package.json by inserting a top-level JSON-safe $comment field containing the SPDX-FileCopyrightText and SPDX-License-Identifier strings (matching the repo pattern used in schemas/router-pool-config.schema.json and nemoclaw-blueprint/model-specific-setup/schema.json); update package.json's root object to include "$comment": "<SPDX-FileCopyrightText: ...> <SPDX-License-Identifier: ...>" or, if you opt not to modify package.json, add a short documented exception in the repo policy explaining why package.json is exempt and reference that exception in the repo README or CONTRIBUTING files.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.
Inline comments:
In @.github/workflows/codebase-growth-guardrails.yaml:
- Around line 123-124: The workflow's jq filter currently matches test/spec
files anywhere; update the jq selection used in the line containing --jq '.[] |
select((.status != "removed") and (.filename |
test("\\.(test|spec)\\.(ts|js|mts|mjs|cts|cjs)$"))) | [.filename, .raw_url] |
`@tsv`' to restrict matches to the same roots as the canonical checker
(scripts/check-test-file-size-budget.ts): only files with paths starting with
test/, src/, or nemoclaw/src/ and ending with .test.|.spec. + the existing
extensions; keep the status != "removed" check and output format intact but
change the regex to require the path prefix (e.g. ^(test|src|nemoclaw/src)/… )
so both enforcement layers target the same files.
- Around line 131-134: The gh api call that fetches
ci/test-file-size-budget.json using BASE_SHA can exit non-zero (404) and fail
the step; update the step around the gh api
"/repos/${REPO}/contents/ci/test-file-size-budget.json?ref=${BASE_SHA}" call so
it does not hard-fail: run the gh api and capture its exit status (or use a
conditional fallback), write decoded content to "$budget_file" only if the call
succeeded and produced content, otherwise create a safe default JSON into
"$budget_file" (or log and continue); ensure the logic references the same
BASE_SHA, gh api invocation and budget_file variable so missing-file cases use
the default instead of failing the workflow.
---
Outside diff comments:
In @.pre-commit-config.yaml:
- Around line 1-3: This YAML config file (beginning with the comment "# NemoClaw
— prek hook configuration") is missing the required SPDX header; add the SPDX
header lines at the very top of .pre-commit-config.yaml (e.g., an SPDX copyright
text and an SPDX-License-Identifier) so the file includes both
SPDX-FileCopyrightText and SPDX-License-Identifier entries before the existing
comments.
In `@ci/test-file-size-budget.json`:
- Around line 1-18: ci/test-file-size-budget.json is parsed with JSON.parse so
comment-style SPDX headers would break consumers; update the repo checks to
avoid adding/removing comment SPDX for this file by either (A) adding an
explicit exclusion for "ci/test-file-size-budget.json" in the spdx-headers hook
configuration in .pre-commit-config.yaml, or (B) altering the SPDX
enforcement/checker to recognize a JSON-safe SPDX mechanism (e.g., a top-level
"spdx" property) or to skip JSON files; ensure corresponding consumers in
scripts/check-test-file-size-budget.ts and the
.github/workflows/codebase-growth-guardrails.yaml continue to parse the file
with JSON.parse without expecting comment headers.
In `@package.json`:
- Line 1: Add the SPDX metadata to package.json by inserting a top-level
JSON-safe $comment field containing the SPDX-FileCopyrightText and
SPDX-License-Identifier strings (matching the repo pattern used in
schemas/router-pool-config.schema.json and
nemoclaw-blueprint/model-specific-setup/schema.json); update package.json's root
object to include "$comment": "<SPDX-FileCopyrightText: ...>
<SPDX-License-Identifier: ...>" or, if you opt not to modify package.json, add a
short documented exception in the repo policy explaining why package.json is
exempt and reference that exception in the repo README or CONTRIBUTING files.
🪄 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: 4091819e-3dab-4763-bb6f-bd91b33caa97
📒 Files selected for processing (7)
.github/workflows/codebase-growth-guardrails.yaml.github/workflows/pr.yaml.pre-commit-config.yamlci/test-file-size-budget.jsonpackage.jsonscripts/check-test-file-size-budget.tstest/test-file-size-budget.test.ts
## Summary Follow-up to PR #4331 (`fix(onboard): forward host proxy env into sandbox`). #4331 started forwarding the host `HTTP_PROXY` / `HTTPS_PROXY` / `NO_PROXY` into `openshell sandbox create -- env ...` so OpenShell-internal traffic could traverse the host proxy when one is configured. The forwarded `NO_PROXY` seed list did not yet include the OpenShell-managed inference hostname (`inference.local`) or the rootless container-host alias (`host.containers.internal`). When a host `HTTP_PROXY` is active on macOS + Colima, OpenShell's L7 proxy chains `inference.local` through the host proxy — which cannot reach an OpenShell-internal hostname — and streaming chat completions stall until the 120 s idle timeout fires (#4846). This PR seeds `inference.local` and `host.containers.internal` into the `withLocalNoProxy()` augmentation so the regression introduced by #4331's host-proxy forwarding stops short of the managed inference hostname. ## Related Issue Fixes #4846 (regression source: #4331). ## Changes - `src/lib/subprocess-env.ts` and `nemoclaw/src/lib/subprocess-env.ts` (mirrored): append `inference.local` and `host.containers.internal` to the seeded `NO_PROXY` host list in `withLocalNoProxy()`; JSDoc documents the boundary (host-side subprocesses + `openshell sandbox create -- env ...`) and the removal condition. - `src/lib/onboard/http-proxy-preflight.ts`: warning copy now names the managed inference hostname and includes it in the suggested `export NO_PROXY=` line for tools running outside the sandbox. Suppression now also requires `inference.local` so users with the previous loopback-only guidance still see the new export advice. - `docs/reference/troubleshooting.mdx`: document the full set of host names NemoClaw adds to `NO_PROXY` when a host HTTP proxy is detected. - Regression tests in `src/lib/subprocess-env.test.ts` and `src/lib/onboard/http-proxy-preflight.test.ts`, plus `test/host-proxy-inference-local-e2e.test.ts` (curl-driven E2E proving `inference.local` reaches a local listener directly when `HTTP_PROXY` is set). `subprocess-env.test.ts` also adds negative tests proving arbitrary `*.local` / `.local` hostnames are not added to the bypass and that a caller-provided `.local` entry does not expand the bypass surface. CLI / plugin sync test in `test/credential-exposure.test.ts` updated to match the new host list. ## Scope and boundaries - **`*.local` suffix.** Deliberately narrow: only exact `inference.local` is seeded. The reported failure path is the single OpenShell-managed inference hostname; widening to any `*.local` would change the bypass surface beyond the reported repro without evidence of other affected hostnames. A negative test (`subprocess-env.test.ts`) pins this behaviour. - **Sandbox-runtime `NO_PROXY` is unchanged.** `scripts/nemoclaw-start.sh` continues to set `NO_PROXY=localhost,127.0.0.1,::1,${PROXY_HOST}` inside the sandbox and intentionally **does not** include `inference.local` there — OpenShell's L7 proxy resolves that hostname internally, and bypassing it would force a direct DNS lookup that does not resolve from inside the container. This PR does not touch that runtime layer. - **Fix surface is host-side env propagation.** `withLocalNoProxy()` runs through `appendHostProxyEnvArgs()` into `openshell sandbox create -- env ...`. The `NO_PROXY` entry is consumed by OpenShell at sandbox-create time when it decides whether to chain its L7 proxy through the host `HTTP_PROXY` for a given hostname. With `inference.local` in `NO_PROXY` at sandbox-create time, OpenShell stops tunneling that hostname through the host proxy (the reported `Proxy connection error: Broken pipe (os error 32)` path). - **E2E coverage.** `test/host-proxy-inference-local-e2e.test.ts` exercises the env-construction primitive end-to-end via a real `curl` spawn driven by `buildSubprocessEnv()` against a local listener bound to `inference.local`. The full macOS + Colima sandbox path requires a macOS runner and is left to scenario E2E (`ubuntu-repo-cloud-openclaw` covers the closest cloud-onboard cross-section). ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Proxy bypass now also excludes additional local hostnames (inference.local, host.containers.internal, ::1, 0.0.0.0), preventing local services and managed inference from being routed through user proxies; preflight warning now requires inference.local to be present to suppress warnings. * **Documentation** * Updated proxy-preflight guidance and warning text to show suggested NO_PROXY/no_proxy exports including managed inference hosts and extra local aliases. * **Tests** * Added/updated unit and e2e tests validating the expanded NO_PROXY/no_proxy behavior and direct reachability of inference.local. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Tinson Lai <tinsonl@nvidia.com> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Feedback fixes pushed in 9c304c9.
Local validation passed: |
…x/test-size-guard # Conflicts: # ci/test-file-size-budget.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/codebase-growth-guardrails.yaml:
- Around line 244-254: The loop over headBudget.legacyMaxLines currently ignores
fetchHeadTextFile(file) returning null, so stale legacy entries slip through;
update the code in the loop (referencing headBudget.legacyMaxLines,
baseBudget.legacyMaxLines, fetchHeadTextFile, and violations) to push a
violation when fetchHeadTextFile(file) === null for a legacy entry that no
longer exists in the head (the same "stale-legacy-budget" semantics used by
scripts/check-test-file-size-budget.ts), and only run the countLines check when
text !== null so you don't call countLines on a missing file.
- Around line 127-145: The script currently records only .filename into
"$changed_files_file" and later greps for ci/test-file-size-budget.json purely
against filenames, so renames (which populate .previous_filename) are missed;
update the initial gh api call that writes to "$changed_files_file" to also
include .previous_filename (e.g., output TSV with status, filename,
previous_filename) and then change the grep/check logic that looks for the
budget file to consider either the current filename or previous_filename (so the
conditional that uses grep -q matches when either field equals
"ci/test-file-size-budget.json"); reference the existing variables
changed_files_file and the grep check when making these edits.
🪄 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: 056e75e3-6cbb-4ac1-b3d7-9cf2a62c3363
📒 Files selected for processing (5)
.github/workflows/codebase-growth-guardrails.yaml.pre-commit-config.yamlci/test-file-size-budget.jsonpackage.jsontest/pr-workflow-contract.test.ts
✅ Files skipped from review due to trivial changes (1)
- ci/test-file-size-budget.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .pre-commit-config.yaml
- package.json
Collaborator
Author
|
Follow-up advisor fix pushed in 20bf994.
Validation: focused prek file checks, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/pr-workflow-contract.test.ts (1)
154-156: ⚡ Quick winAssert
HEAD_SHAin the trusted-guard contract too.Line 154 currently verifies
HEAD_REPObut notHEAD_SHA. Add aHEAD_SHAassertion so this test catches regressions where the guard stops pinning content fetches to the PR head commit.Suggested patch
expect(guardRun).toContain("HEAD_REPO"); + expect(guardRun).toContain("HEAD_SHA"); expect(guardRun).not.toContain(".raw_url");Based on learnings from the PR objectives/comments summary: the trusted-boundary contract is intended to enforce contents API usage with both head repo and head SHA.
🤖 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 154 - 156, Add an assertion that the trusted-guard contract includes HEAD_SHA: update the test in pr-workflow-contract.test.ts where guardRun is checked (the block with expect(guardRun).toContain("HEAD_REPO") and expect(guardRun).not.toContain(".raw_url")) to also assert expect(guardRun).toContain("HEAD_SHA"); this will ensure the guard pins content fetches to the PR head commit (use the same guardRun variable and assertion style as the existing HEAD_REPO check).
🤖 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/pr-workflow-contract.test.ts`:
- Around line 154-156: Add an assertion that the trusted-guard contract includes
HEAD_SHA: update the test in pr-workflow-contract.test.ts where guardRun is
checked (the block with expect(guardRun).toContain("HEAD_REPO") and
expect(guardRun).not.toContain(".raw_url")) to also assert
expect(guardRun).toContain("HEAD_SHA"); this will ensure the guard pins content
fetches to the PR head commit (use the same guardRun variable and assertion
style as the existing HEAD_REPO check).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5f1d749e-2064-4fcb-993d-90c77d09b042
📒 Files selected for processing (2)
.github/workflows/codebase-growth-guardrails.yamltest/pr-workflow-contract.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/codebase-growth-guardrails.yaml
Collaborator
Author
|
Closing this noisy branch in favor of #4905. #4905 keeps the same test-size guard behavior but starts from current main and trims the trusted workflow guard, reducing the GitHub review diff from 968 additions / 17 files to 520 additions / 8 files. The old branch could not be force-pushed cleanly because repository rules reject force-pushes, so replacing the PR is the cleanest review path. |
cv
added a commit
that referenced
this pull request
Jun 7, 2026
## Summary Adds a test-file size budget so new or touched test files cannot keep growing into oversized catch-all suites. This is a trimmed replacement for #4899 with a clean branch from current `main`, reducing the review diff from ~1k lines to about 520 added lines. ## Related Issue Related to #4892. Replaces #4899. ## Changes - Add `scripts/check-test-file-size-budget.ts` and `ci/test-file-size-budget.json` to enforce a 1,500-line default budget with legacy ratchets for existing oversized tests. - Wire `npm run test-size:check` into pre-commit and PR static checks. - Add a compact trusted `pull_request_target` guard for changed test files and budget-policy tampering without using PR-controlled code or `raw_url` token fetches. - Add focused Vitest coverage and workflow-contract assertions for the new budget and trusted guard behavior. ## 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 Focused local checks passed: `npx prek run --files ... --skip test-cli --skip test-plugin`, `npx vitest run --project cli test/pr-workflow-contract.test.ts test/test-file-size-budget.test.ts`, `npm run test-size:check`, `npm run build:cli`, `npm run typecheck:cli`, `npm run source-shape:check`, and `git diff --check`. Full local commit hooks were attempted in the temporary clean worktree but the CLI coverage hook hit/stuck on local coverage-process instability, so the full matrix is left to CI. - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Introduced test file size budgeting system with configurable maximum line count limits for test files. * Legacy grandfathering support allows existing large test files to remain within their own budgets while preventing further growth. * Integrated automatic size validation into CI/CD workflow and pre-commit hooks. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
Adds a test-file size budget so oversized test files cannot quietly grow again after the CLI split work. The guard mirrors the existing growth-budget style by grandfathering current oversized files at their present size while requiring future shrinkage to ratchet the budget down.
Related Issue
Related to #4892
Changes
ci/test-file-size-budget.jsonwith a 1,500-line default ceiling and explicit legacy budgets for existing oversized test files.scripts/check-test-file-size-budget.tsplus unit coverage for default limits, legacy growth prevention, ratcheting, stale budget entries, and line counting.npm run test-size:checkinto prek, PR static checks, and the data-onlypull_request_targetgrowth guardrail workflow.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Focused checks run:
npm run test-size:checknpx vitest run --project cli test/test-file-size-budget.test.tsnpm run typecheck:clinpm run source-shape:checkumask 022 && npx prek run --files ci/test-file-size-budget.json scripts/check-test-file-size-budget.ts test/test-file-size-budget.test.ts package.json .pre-commit-config.yaml .github/workflows/pr.yaml .github/workflows/codebase-growth-guardrails.yamlworkflowscope, then the SSH push succeeded.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit