Skip to content

ci(test): ratchet test conditional growth#5558

Merged
cv merged 8 commits into
mainfrom
ci/test-conditionals-guardrail
Jun 19, 2026
Merged

ci(test): ratchet test conditional growth#5558
cv merged 8 commits into
mainfrom
ci/test-conditionals-guardrail

Conversation

@cv

@cv cv commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a local test-conditional scanner and hooks a net-new if ratchet into the existing codebase growth guardrail workflow. This keeps current test branching debt visible while blocking changed test files from adding more conditional complexity.

Changes

  • Add scripts/find-test-conditionals.ts to report and score if statements in test/spec files.
  • Add npm run test-conditionals:scan and focused scanner unit coverage.
  • Extend .github/workflows/codebase-growth-guardrails.yaml with a data-only pull_request_target check that rejects net-new if statements in changed test files.

Type of Change

  • 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

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Tests added or updated for new or changed behavior
  • 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 (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

Release Notes

  • New Features
    • Added a CI guardrail that compares conditional if usage in changed test/spec files between the PR base and head, failing the build if it increases.
    • Introduced a new local scanner to analyze test/spec code for conditional patterns, returning ranked results in JSON or readable text with filtering.
    • Added an npm script to run the new scanner.
  • Tests
    • Added unit tests to verify detection within test bodies, correct handling of comments/strings, template cases, and result prioritization.
    • Updated an e2e scenario test to set an explicit timeout and adjust import ordering.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv self-assigned this Jun 19, 2026
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a test-conditionals enforcement system consisting of: a TypeScript AST scanner script that detects and scores if statements in test files with context classification and heuristics; a CLI wrapper with filtering and reporting; a new test-conditionals:scan npm script; Vitest tests for the scanner; a new CI workflow step that counts if tokens in changed test files and fails if the head adds more than the base; and minor e2e test import and timeout updates.

Changes

Test-conditionals guardrail

Layer / File(s) Summary
Scanner types, AST utilities, and context classification
scripts/find-test-conditionals.ts
Defines root constants, test-file pattern matching, filesystem walking, TypeScript AST location/expression helpers, callback-context detection, ancestor traversal, context-kind classification (test/hook/suite/helper/top-level), nested if-count, and loop detection.
Heuristic scoring and if-statement extraction
scripts/find-test-conditionals.ts
Implements computeScore (numeric score + reasons from context kind, assertion/control-flow flags, else-branch, loop, nesting depth, line counts), scanIfStatement (occurrence metadata extraction), and scanTextForTestConditionals (TypeScript AST walk that collects and returns scored occurrences).
Aggregation, reporting, filtering, CLI, and npm script
scripts/find-test-conditionals.ts, package.json
Implements summarizeFiles and collectTestConditionals (root walking + per-file scanning), formatReport (multi-section human-readable output), filterReport (threshold filtering + aggregate recomputation), CLI argument parsing (--json/--top/--min-score/--root), and main orchestration; registers the test-conditionals:scan npm script.
Vitest tests for scanTextForTestConditionals
test/test-conditionals-scanner.test.ts
Four test cases: verifies a single if in a virtual test body is detected with correct line/context/assertion flags while ignoring commented/stringified matches; verifies TypeScript with executable if logic in template interpolation; verifies score ordering between a test-body branch and a helper-guard occurrence, checking hasElse and containsControlFlow metadata.
CI workflow: if-count enforcement on PR test files
.github/workflows/codebase-growth-guardrails.yaml
Adds a pull_request_target step with embedded Node.js script that lists PR-changed test files via GitHub API pagination (including rename history via previous_filename), fetches base/head file contents at specific refs, strips comments and string/template/regex literals, counts if tokens via word-boundary match, and exits non-zero with per-file diagnostics if any file gains if statements.

E2E test housekeeping

Layer / File(s) Summary
Import reordering and timeout configuration
test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts
Reorders import statements to place workflow-boundary helpers before testTimeoutOptions; updates a single test case to explicitly set timeout to 60_000 milliseconds.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Suggested labels

chore

Poem

🐇 Hop hop, no sneaky if shall hide,
In test files where assertions reside.
The AST sniffs each branching tree,
Counts every fork for CI to see.
A score, a reason, a guardrail tight—
This bunny keeps your test code right! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and specifically refers to the primary change: adding a guardrail to prevent growth of test conditionals via a workflow check and scanner implementation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/test-conditionals-guardrail

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

@github-code-quality

github-code-quality Bot commented Jun 19, 2026

Copy link
Copy Markdown

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the branch is 96%. Coverage data for the branch is not yet available.

Show a code coverage summary of the most covered files.
File 6cb5385 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the branch is 46%. Coverage data for the branch is not yet available.

Show a code coverage summary of the most covered files.
File 6cb5385 +/-
src/lib/state/o...oard-session.ts 90%
src/lib/inference/local.ts 76%
src/lib/sandbox/config.ts 72%
src/lib/actions...dbox/rebuild.ts 67%
src/lib/onboard/preflight.ts 64%
src/lib/actions...licy-channel.ts 56%
src/lib/state/sandbox.ts 55%
src/lib/policy/index.ts 49%
src/lib/onboard...er-gpu-patch.ts 44%
src/lib/onboard.ts 18%

Updated June 19, 2026 23:34 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: None
Optional E2E: None

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • None. No E2E is recommended because this PR is limited to CI guardrails and test-analysis tooling, plus tests for that tooling. The diff does not modify NemoClaw runtime code, installer/onboarding paths, credential handling, sandbox lifecycle, security boundaries, network policy assets, inference routing, deployment logic, or real assistant user flows.

Optional E2E

  • None.

New E2E recommendations

  • None.

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: None
Optional Vitest E2E scenarios: None

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • None. No Vitest E2E scenario dispatch is required. The PR changes codebase-growth guardrail tooling and tests, plus a support-test-only timeout/import adjustment under test/e2e-scenario; it does not change the live Vitest scenario workflow, scenario registry, runtime support, live scenario files, manifests, fixtures, or dispatchable scenario behavior.

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts

@cv cv added the v0.0.66 Release target label Jun 19, 2026
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor

Findings: 0 needs attention, 2 worth checking, 0 nice ideas
Since last review: 1 prior item resolved, 1 still applies, 0 new items found

Review findings

🛠️ Needs attention

  • None.

🔎 Worth checking

  • Source-of-truth review needed: .github/workflows/codebase-growth-guardrails.yaml inline conditional counter versus scripts/find-test-conditionals.ts: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: The workflow uses a custom `stripTriviaAndLiterals` plus `/\bif\s*\(/` counter, while `scripts/find-test-conditionals.ts` records TypeScript AST `IfStatement` nodes; callable property tokens remain a concrete divergence risk.
  • Workflow and local scanner can still disagree on non-statement `if` calls (.github/workflows/codebase-growth-guardrails.yaml:458): The PR adds useful parity fixtures, but the trusted workflow still counts `if` with a lexical `/\bif\s*\(/` match after custom stripping, while the local scanner reports TypeScript AST `IfStatement` nodes. That means test code such as `obj.if()` or optional method calls using a property named `if` would be rejected by the workflow but absent from the contributor-facing scanner output. In a `pull_request_target` policy workflow, this kind of drift can make the enforced policy differ from the local remediation tool.
    • Recommendation: Prefer one base-owned implementation for both paths while preserving the no-PR-code-execution boundary, or extend the workflow/AST parity fixtures to cover callable property/accessor cases such as `obj.if()`, `client.if?.()`, and object methods named `if`, then either make the workflow match AST `IfStatement` semantics or explicitly document that the workflow intentionally bans these tokens too.
    • Evidence: Workflow enforcement uses `stripTriviaAndLiterals(text).match(/\bif\s*\(/g)?.length ?? 0`; `scripts/find-test-conditionals.ts` uses `ts.isIfStatement(node)`. Current parity tests cover property/type tokens like `{ if: true }` and `obj.if`, but not callable `obj.if()`.

🌱 Nice ideas

  • None.
Consider writing more tests for
  • **Runtime validation** — Workflow counter and AST scanner agree that `obj.if()`, `client.if?.()`, and object methods named `if` are not `if` statements, or the workflow intentionally rejects them with documented expectations.. The PR adds a token-bearing workflow enforcement path and a sizeable local parser. The added unit and extraction tests are strong, but behavior-specific parity/runtime validation would reduce policy drift risk.
  • **Runtime validation** — Workflow rejects malformed `HEAD_REPO` before attempting to fetch PR head content.. The PR adds a token-bearing workflow enforcement path and a sizeable local parser. The added unit and extraction tests are strong, but behavior-specific parity/runtime validation would reduce policy drift risk.
  • **Runtime validation** — Workflow compares renamed test-to-test files by reading `previous_filename` at the base SHA and `filename` at the head SHA.. The PR adds a token-bearing workflow enforcement path and a sizeable local parser. The added unit and extraction tests are strong, but behavior-specific parity/runtime validation would reduce policy drift risk.
  • **Runtime validation** — CLI scanner `--json`, `--min-score`, and repeated `--root` options produce the expected filtered report and reject missing option values.. The PR adds a token-bearing workflow enforcement path and a sizeable local parser. The added unit and extraction tests are strong, but behavior-specific parity/runtime validation would reduce policy drift risk.
  • **Acceptance clause:** Adds a local test-conditional scanner and hooks a net-new `if` ratchet into the existing codebase growth guardrail workflow. — add test evidence or identify existing coverage. The scanner and workflow ratchet are added, but the workflow's lexical counter can still disagree with the scanner's AST counter for callable non-statement `if` property tokens.
  • **.github/workflows/codebase-growth-guardrails.yaml inline conditional counter versus scripts/find-test-conditionals.ts** — The PR adds extraction tests comparing the workflow counter and local AST scanner for comments/strings, regex literals, real branches/else-if chains, nested template interpolation, and non-statement property/type tokens, plus workflow guard tests for per-file increases and removed/renamed-out files.. The workflow uses a custom `stripTriviaAndLiterals` plus `/\bif\s*\(/` counter, while `scripts/find-test-conditionals.ts` records TypeScript AST `IfStatement` nodes; callable property tokens remain a concrete divergence risk.
Since last review details

Current findings:

  • Source-of-truth review needed: .github/workflows/codebase-growth-guardrails.yaml inline conditional counter versus scripts/find-test-conditionals.ts: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: The workflow uses a custom `stripTriviaAndLiterals` plus `/\bif\s*\(/` counter, while `scripts/find-test-conditionals.ts` records TypeScript AST `IfStatement` nodes; callable property tokens remain a concrete divergence risk.
  • Workflow and local scanner can still disagree on non-statement `if` calls (.github/workflows/codebase-growth-guardrails.yaml:458): The PR adds useful parity fixtures, but the trusted workflow still counts `if` with a lexical `/\bif\s*\(/` match after custom stripping, while the local scanner reports TypeScript AST `IfStatement` nodes. That means test code such as `obj.if()` or optional method calls using a property named `if` would be rejected by the workflow but absent from the contributor-facing scanner output. In a `pull_request_target` policy workflow, this kind of drift can make the enforced policy differ from the local remediation tool.
    • Recommendation: Prefer one base-owned implementation for both paths while preserving the no-PR-code-execution boundary, or extend the workflow/AST parity fixtures to cover callable property/accessor cases such as `obj.if()`, `client.if?.()`, and object methods named `if`, then either make the workflow match AST `IfStatement` semantics or explicitly document that the workflow intentionally bans these tokens too.
    • Evidence: Workflow enforcement uses `stripTriviaAndLiterals(text).match(/\bif\s*\(/g)?.length ?? 0`; `scripts/find-test-conditionals.ts` uses `ts.isIfStatement(node)`. Current parity tests cover property/type tokens like `{ if: true }` and `obj.if`, but not callable `obj.if()`.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@scripts/find-test-conditionals.ts`:
- Around line 113-117: The scriptKindFor function currently returns
ts.ScriptKind.JS for JSX files, which causes JSX syntax to be misparsed as
comparison operators. Add a new conditional check in the scriptKindFor function
that specifically detects JSX file extensions (such as .jsx, .mjsx, .cjsx) and
returns ts.ScriptKind.JSX for these files. Place this check before the existing
JS check that matches /\.[cm]?jsx?$/i to ensure JSX files are correctly
identified and parsed with the appropriate ScriptKind.
🪄 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: 26dc3384-c089-4fe5-b304-a34d22bcaa2f

📥 Commits

Reviewing files that changed from the base of the PR and between 4d33291 and 247d5e8.

📒 Files selected for processing (4)
  • .github/workflows/codebase-growth-guardrails.yaml
  • package.json
  • scripts/find-test-conditionals.ts
  • test/test-conditionals-scanner.test.ts

Comment thread scripts/find-test-conditionals.ts
cv added 6 commits June 19, 2026 15:02
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv merged commit df54922 into main Jun 19, 2026
42 checks passed
@cv cv deleted the ci/test-conditionals-guardrail branch June 19, 2026 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.66 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant