refactor(checks): migrate repository check suite to .mts - #7047
Conversation
Checks importing first-party CJS source use dynamic import so the ESM entrypoints resolve named exports at runtime under tsx. Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
v0.0.85
chore
|
🌿 Preview your docs: https://nvidia-preview-pr-7047.docs.buildwithfern.com/nemoclaw |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe repository check suite migrates execution paths to ChangesRepository check suite
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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 |
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: 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}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/checks/layer-import-boundaries.mts`:
- Around line 116-128: Update the path-resolution logic around the candidates
array so index.* files are checked before the bare base directory, ensuring
extensionless imports resolve to module/index.* when present. Preserve extension
handling and fallback behavior, and add a regression fixture covering an
extensionless directory import.
In `@scripts/checks/local-credential-helper-pin.mts`:
- Around line 61-68: The raw GitHub URL regex in the local credential helper
check must require the reviewed artifact path to be the complete URL path, with
no suffix, query, or fragment. Update the pattern used by the URL matching logic
around urlPattern and add a focused negative regression test covering a suffixed
path such as “.bak”, while preserving valid immutable URL detection.
In `@scripts/checks/no-coverage-ignore.mts`:
- Line 18: Add a focused positive test for the `.tsx` extension in the scanner
tests, alongside the existing `.mts` coverage, and retain or add a corresponding
false-positive assertion to verify unrelated paths are not detected. Ensure the
tests exercise the extension through the scanner’s public detection behavior.
In `@scripts/checks/no-unit-blocks-in-live-e2e.mts`:
- Around line 37-45: Update isItPrimitive() to recursively traverse chained it
modifier properties, recognizing conditional modifiers such as skipIf, runIf,
and for alongside the existing IT_PRIMITIVE_MEMBERS. Preserve the current
direct-member matches and add tests covering each conditional form plus nested
chains like it.concurrent.skip(...).
In `@scripts/checks/openshell-policy-mutation-read.mts`:
- Around line 82-109: The discoverPolicyReadSites implementation currently
counts raw source-text matches, including comments and string literals, and can
miss syntactically valid call forms. Replace the regex-based counting around
POLICY_GET_BUILDER_CALL and DIRECT_POLICY_GET_CALL with AST-based matching of
policy-read call and array expressions (or equivalent comment/string exclusion),
while preserving the existing productionTypeScriptFiles scope and result shape.
Add focused tests covering both genuine detections and commented/stringized
patterns that must not count.
In `@scripts/checks/test-create-require-budget.mts`:
- Around line 129-157: Update mergeBaseAllowlists and the surrounding
enforcement flow so the ratchet’s allowlists and comparison logic are loaded and
executed from the resolved immutable merge-base revision, rather than relying on
PR-workspace implementations that can be modified. Preserve the existing
base-branch resolution and fallback behavior while ensuring the early-return
path cannot bypass trusted-revision enforcement.
In `@scripts/checks/test-title-style.mts`:
- Around line 45-49: Update rootCallName and the surrounding title-check
analysis to collect aliases from Vitest imports and resolve aliased identifiers
to canonical call names before applying title rules. Preserve existing direct,
property-access, and nested-call detection, and add focused tests covering
aliased Vitest imports plus non-Vitest aliases to verify detection and avoid
false positives.
- Around line 136-147: Update walkTestFiles to call readdirSync with
withFileTypes enabled, skip entries where entry.isSymbolicLink() is true, and
use Dirent methods for directory and file checks instead of statSync. Preserve
the existing skip filtering, recursive traversal, and TEST_FILE_PATTERN matching
for regular files.
In `@test/no-coverage-ignore.test.ts`:
- Around line 36-45: Add the required local issue reference suffix in the parent
“scanned source path selection” describe title, using the final `(`#1234`)`
format; leave the child test titles unchanged.
🪄 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: a67047f5-c51b-4c8a-a387-827aad7c94b9
📒 Files selected for processing (35)
.agents/skills/nemoclaw-contributor-update-dependencies/SKILL.md.github/workflows/pr.yamlpackage.jsonscripts/checks/dependency-pins.mtsscripts/checks/direct-credential-env.mtsscripts/checks/e2e-mock-parity.mtsscripts/checks/hermes-light-skin-boundary.mtsscripts/checks/hermes-light-skin-boundary.tsscripts/checks/layer-import-boundaries.mtsscripts/checks/local-credential-helper-pin.mtsscripts/checks/no-coverage-ignore.mtsscripts/checks/no-test-dist-imports.mtsscripts/checks/no-unit-blocks-in-live-e2e.mtsscripts/checks/openshell-policy-mutation-read.mtsscripts/checks/run.mtsscripts/checks/test-create-require-budget.mtsscripts/checks/test-title-style.mtsscripts/checks/vitest-project-overlap.mtssrc/lib/messaging/manifest/types.test.tssrc/lib/security/credential-env.tstest/checks-runner.test.tstest/dependency-pins-check.test.tstest/e2e-mock-parity.test.tstest/hermes-light-skin-boundary.test.tstest/layer-import-boundaries.test.tstest/local-credential-helper-pin.test.tstest/no-coverage-ignore.test.tstest/no-direct-credential-env.test.tstest/no-unit-blocks-in-live-e2e.test.tstest/package-contract/credentials-shim.test.tstest/policy-mutation-read-discovery.test.tstest/pr-workflow-contract.test.tstest/test-boundary-guards.test.tstest/test-create-require-budget.test.tstest/test-title-style.test.ts
💤 Files with no reviewable changes (1)
- scripts/checks/hermes-light-skin-boundary.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🤖 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/checks/layer-import-boundaries.mts`:
- Around line 116-128: Update the path-resolution logic around the candidates
array so index.* files are checked before the bare base directory, ensuring
extensionless imports resolve to module/index.* when present. Preserve extension
handling and fallback behavior, and add a regression fixture covering an
extensionless directory import.
In `@scripts/checks/local-credential-helper-pin.mts`:
- Around line 61-68: The raw GitHub URL regex in the local credential helper
check must require the reviewed artifact path to be the complete URL path, with
no suffix, query, or fragment. Update the pattern used by the URL matching logic
around urlPattern and add a focused negative regression test covering a suffixed
path such as “.bak”, while preserving valid immutable URL detection.
In `@scripts/checks/no-coverage-ignore.mts`:
- Line 18: Add a focused positive test for the `.tsx` extension in the scanner
tests, alongside the existing `.mts` coverage, and retain or add a corresponding
false-positive assertion to verify unrelated paths are not detected. Ensure the
tests exercise the extension through the scanner’s public detection behavior.
In `@scripts/checks/no-unit-blocks-in-live-e2e.mts`:
- Around line 37-45: Update isItPrimitive() to recursively traverse chained it
modifier properties, recognizing conditional modifiers such as skipIf, runIf,
and for alongside the existing IT_PRIMITIVE_MEMBERS. Preserve the current
direct-member matches and add tests covering each conditional form plus nested
chains like it.concurrent.skip(...).
In `@scripts/checks/openshell-policy-mutation-read.mts`:
- Around line 82-109: The discoverPolicyReadSites implementation currently
counts raw source-text matches, including comments and string literals, and can
miss syntactically valid call forms. Replace the regex-based counting around
POLICY_GET_BUILDER_CALL and DIRECT_POLICY_GET_CALL with AST-based matching of
policy-read call and array expressions (or equivalent comment/string exclusion),
while preserving the existing productionTypeScriptFiles scope and result shape.
Add focused tests covering both genuine detections and commented/stringized
patterns that must not count.
In `@scripts/checks/test-create-require-budget.mts`:
- Around line 129-157: Update mergeBaseAllowlists and the surrounding
enforcement flow so the ratchet’s allowlists and comparison logic are loaded and
executed from the resolved immutable merge-base revision, rather than relying on
PR-workspace implementations that can be modified. Preserve the existing
base-branch resolution and fallback behavior while ensuring the early-return
path cannot bypass trusted-revision enforcement.
In `@scripts/checks/test-title-style.mts`:
- Around line 45-49: Update rootCallName and the surrounding title-check
analysis to collect aliases from Vitest imports and resolve aliased identifiers
to canonical call names before applying title rules. Preserve existing direct,
property-access, and nested-call detection, and add focused tests covering
aliased Vitest imports plus non-Vitest aliases to verify detection and avoid
false positives.
- Around line 136-147: Update walkTestFiles to call readdirSync with
withFileTypes enabled, skip entries where entry.isSymbolicLink() is true, and
use Dirent methods for directory and file checks instead of statSync. Preserve
the existing skip filtering, recursive traversal, and TEST_FILE_PATTERN matching
for regular files.
In `@test/no-coverage-ignore.test.ts`:
- Around line 36-45: Add the required local issue reference suffix in the parent
“scanned source path selection” describe title, using the final `(`#1234`)`
format; leave the child test titles unchanged.
🪄 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: a67047f5-c51b-4c8a-a387-827aad7c94b9
📒 Files selected for processing (35)
.agents/skills/nemoclaw-contributor-update-dependencies/SKILL.md.github/workflows/pr.yamlpackage.jsonscripts/checks/dependency-pins.mtsscripts/checks/direct-credential-env.mtsscripts/checks/e2e-mock-parity.mtsscripts/checks/hermes-light-skin-boundary.mtsscripts/checks/hermes-light-skin-boundary.tsscripts/checks/layer-import-boundaries.mtsscripts/checks/local-credential-helper-pin.mtsscripts/checks/no-coverage-ignore.mtsscripts/checks/no-test-dist-imports.mtsscripts/checks/no-unit-blocks-in-live-e2e.mtsscripts/checks/openshell-policy-mutation-read.mtsscripts/checks/run.mtsscripts/checks/test-create-require-budget.mtsscripts/checks/test-title-style.mtsscripts/checks/vitest-project-overlap.mtssrc/lib/messaging/manifest/types.test.tssrc/lib/security/credential-env.tstest/checks-runner.test.tstest/dependency-pins-check.test.tstest/e2e-mock-parity.test.tstest/hermes-light-skin-boundary.test.tstest/layer-import-boundaries.test.tstest/local-credential-helper-pin.test.tstest/no-coverage-ignore.test.tstest/no-direct-credential-env.test.tstest/no-unit-blocks-in-live-e2e.test.tstest/package-contract/credentials-shim.test.tstest/policy-mutation-read-discovery.test.tstest/pr-workflow-contract.test.tstest/test-boundary-guards.test.tstest/test-create-require-budget.test.tstest/test-title-style.test.ts
💤 Files with no reviewable changes (1)
- scripts/checks/hermes-light-skin-boundary.ts
🛑 Comments failed to post (9)
scripts/checks/layer-import-boundaries.mts (1)
116-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve
index.*before accepting the base directory.For
import "./module", an existingmodule/directory is selected beforemodule/index.mts, so boundary classification receives the directory path and can miss the protected target.Proposed fix
const candidates = [ - base, ...extensions.map((extension) => `${base}${extension}`), ...extensions.map((extension) => path.join(base, `index${extension}`)), + base, ];Add a regression fixture for an extensionless directory import.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const extensions = [".ts", ".tsx", ".mts", ".cts"]; const candidates = [ ...extensions.map((extension) => `${base}${extension}`), ...extensions.map((extension) => path.join(base, `index${extension}`)), base, ]; const found = candidates.find((candidate) => existsSync(candidate)); if (!found) return toRepoPath(`${base}.ts`); try { return toRepoPath(realpathSync(found)); } catch { return toRepoPath(found); }🤖 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 `@scripts/checks/layer-import-boundaries.mts` around lines 116 - 128, Update the path-resolution logic around the candidates array so index.* files are checked before the bare base directory, ensuring extensionless imports resolve to module/index.* when present. Preserve extension handling and fallback behavior, and add a regression fixture covering an extensionless directory import.scripts/checks/local-credential-helper-pin.mts (1)
61-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require the raw URL to end at the reviewed artifact path.
The regex accepts the path as a prefix, so a URL such as
local-credential-helper.mts.bakpasses both URL-count and commit checks when paired with the current digest. The guard can therefore approve a prompt that fetches the wrong path and subsequently breaks credential setup. Require a URL terminator with no suffix/query/fragment and add a negative regression case.As per path instructions, guardrails must enforce objective invariants deterministically and include focused detection and false-positive tests.
Also applies to: 405-410
🤖 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 `@scripts/checks/local-credential-helper-pin.mts` around lines 61 - 68, The raw GitHub URL regex in the local credential helper check must require the reviewed artifact path to be the complete URL path, with no suffix, query, or fragment. Update the pattern used by the URL matching logic around urlPattern and add a focused negative regression test covering a suffixed path such as “.bak”, while preserving valid immutable URL detection.Source: Path instructions
scripts/checks/no-coverage-ignore.mts (1)
18-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the newly added
.tsxextension.The new test only asserts existing
.mtsbehavior. Add a positive.tsxassertion so this extension cannot silently drop from the scanner.As per path instructions, “Require focused tests for both detection and false-positive behavior.”
🤖 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 `@scripts/checks/no-coverage-ignore.mts` at line 18, Add a focused positive test for the `.tsx` extension in the scanner tests, alongside the existing `.mts` coverage, and retain or add a corresponding false-positive assertion to verify unrelated paths are not detected. Ensure the tests exercise the extension through the scanner’s public detection behavior.Source: Path instructions
scripts/checks/no-unit-blocks-in-live-e2e.mts (1)
37-45: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== repo files ==\n' git ls-files | sed -n '1,120p' printf '\n== contributing ==\n' if git ls-files CONTRIBUTING.md >/dev/null 2>&1; then cat -n CONTRIBUTING.md | sed -n '1,220p' else echo 'CONTRIBUTING.md not found' fi printf '\n== outline: scripts/checks/no-unit-blocks-in-live-e2e.mts ==\n' ast-grep outline scripts/checks/no-unit-blocks-in-live-e2e.mts --view expanded || true printf '\n== relevant source slices ==\n' cat -n scripts/checks/no-unit-blocks-in-live-e2e.mts | sed -n '1,220p' printf '\n== related tests ==\n' fd -a 'no-unit-blocks-in-live-e2e' . || true rg -n "no-unit-blocks-in-live-e2e|skipIf|runIf|it\\.for|it\\.concurrent\\.skip|IT_PRIMITIVE_MEMBERS" scripts test . -g '!**/node_modules/**' || trueRepository: NVIDIA/NemoClaw
Length of output: 50373
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== current git status/stat ==\n' git status --short git diff --stat -- scripts/checks/no-unit-blocks-in-live-e2e.mts || trueRepository: NVIDIA/NemoClaw
Length of output: 184
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== test/no-unit-blocks-in-live-e2e.test.ts ==\n' cat -n test/no-unit-blocks-in-live-e2e.test.ts | sed -n '1,220p' printf '\n== live E2E usages around it.skipIf / it.runIf / chained modifiers ==\n' for f in \ test/e2e/live/issue-4434-tui-unreachable-inference.test.ts \ test/e2e/live/openshell-gateway-upgrade.test.ts \ test/e2e/live/overlayfs-autofix.test.ts do echo echo "--- $f ---" rg -n "it\\.|test\\." "$f" | sed -n '1,120p' done printf '\n== checker registration ==\n' cat -n scripts/checks/run.mts | sed -n '70,110p'Repository: NVIDIA/NemoClaw
Length of output: 6461
Recognize chained
itmodifiers too
isItPrimitive()only matchesitplus one direct property hop from a fixed allowlist, soit.skipIf(...),it.runIf(...),it.for(...), and nested chains likeit.concurrent.skip(...)bypass this guard. Make the matcher recursive over modifier chains and add tests for conditional and chained forms.🤖 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 `@scripts/checks/no-unit-blocks-in-live-e2e.mts` around lines 37 - 45, Update isItPrimitive() to recursively traverse chained it modifier properties, recognizing conditional modifiers such as skipIf, runIf, and for alongside the existing IT_PRIMITIVE_MEMBERS. Preserve the current direct-member matches and add tests covering each conditional form plus nested chains like it.concurrent.skip(...).scripts/checks/openshell-policy-mutation-read.mts (1)
82-109: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Parse calls rather than matching raw source text.
Comments and string literals containing these patterns are counted as policy reads. That can create CI false positives or preserve an expected count while an actual call is changed into an unrecognized form. Use AST call/array-expression matching (or at minimum exclude comments and strings), and add a regression case for commented/stringized patterns.
As per path instructions,
scripts/checks/**requires focused tests for both detection and false-positive behavior.🤖 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 `@scripts/checks/openshell-policy-mutation-read.mts` around lines 82 - 109, The discoverPolicyReadSites implementation currently counts raw source-text matches, including comments and string literals, and can miss syntactically valid call forms. Replace the regex-based counting around POLICY_GET_BUILDER_CALL and DIRECT_POLICY_GET_CALL with AST-based matching of policy-read call and array expressions (or equivalent comment/string exclusion), while preserving the existing productionTypeScriptFiles scope and result shape. Add focused tests covering both genuine detections and commented/stringized patterns that must not count.Source: Path instructions
scripts/checks/test-create-require-budget.mts (1)
129-157: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect how the checker is invoked and whether its implementation comes from trusted base code. rg -n -C4 'test-create-require-budget(\.mts|\.ts)?' .github package.json scriptsRepository: NVIDIA/NemoClaw
Length of output: 2304
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the checker and its runner around the relevant lines. ast-grep outline scripts/checks/test-create-require-budget.mts --view expanded || true printf '\n--- runner ---\n' ast-grep outline scripts/checks/run.mts --view expanded || true printf '\n--- checker excerpt ---\n' sed -n '1,220p' scripts/checks/test-create-require-budget.mts | cat -n printf '\n--- runner excerpt ---\n' sed -n '1,140p' scripts/checks/run.mts | cat -nRepository: NVIDIA/NemoClaw
Length of output: 17152
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find any other path that executes this checker or a trusted/base variant. rg -n -C3 'test-create-require-budget|create-require-budget' .github scripts package.jsonRepository: NVIDIA/NemoClaw
Length of output: 1987
Run the ratchet from a trusted revision. The check is executed from
scripts/checks/test-create-require-budget.mtsin the PR workspace, so changes tomergeBaseAllowlists(), the comparator, or the early-return path can weaken enforcement. Load the enforcement logic from an immutable/base commit instead.🤖 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 `@scripts/checks/test-create-require-budget.mts` around lines 129 - 157, Update mergeBaseAllowlists and the surrounding enforcement flow so the ratchet’s allowlists and comparison logic are loaded and executed from the resolved immutable merge-base revision, rather than relying on PR-workspace implementations that can be modified. Preserve the existing base-branch resolution and fallback behavior while ensuring the early-return path cannot bypass trusted-revision enforcement.Source: Path instructions
scripts/checks/test-title-style.mts (2)
45-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve aliased Vitest imports before applying title rules.
The textual root-name check misses valid calls such as
import { it as case_ } from "vitest"; case_("..."), allowing titles to bypass this guard. Track Vitest import aliases and map them to their canonical call names; add focused alias coverage.As per path instructions, guardrails must enforce objective invariants deterministically and require focused detection and false-positive tests.
Also applies to: 106-110
🤖 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 `@scripts/checks/test-title-style.mts` around lines 45 - 49, Update rootCallName and the surrounding title-check analysis to collect aliases from Vitest imports and resolve aliased identifiers to canonical call names before applying title rules. Preserve existing direct, property-access, and nested-call detection, and add focused tests covering aliased Vitest imports plus non-Vitest aliases to verify detection and avoid false positives.Sources: Coding guidelines, Path instructions
136-147: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Identify symlinks reachable from the configured scan roots. for root in src test nemoclaw/src; do if [ -e "$root" ]; then find "$root" -type l -print fi doneRepository: NVIDIA/NemoClaw
Length of output: 153
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant checker and the symlink-safe walker for comparison. for f in scripts/checks/test-title-style.mts scripts/checks/vitest-project-overlap.mts; do echo "===== $f =====" wc -l "$f" ast-grep outline "$f" --view expanded || true done echo "===== symlinks in repository =====" find . -type l -print | sed 's#^\./##' | sort | head -200Repository: NVIDIA/NemoClaw
Length of output: 2971
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "===== test-title-style.mts relevant lines =====" sed -n '125,160p' scripts/checks/test-title-style.mts | cat -n echo echo "===== vitest-project-overlap.mts relevant lines =====" sed -n '55,75p' scripts/checks/vitest-project-overlap.mts | cat -n echo echo "===== symlinks in repository =====" find . -type l -print | sed 's#^\./##' | sortRepository: NVIDIA/NemoClaw
Length of output: 2866
🏁 Script executed:
#!/bin/bash set -euo pipefail node <<'JS' const fs = require('fs'); const path = require('path'); const os = require('os'); const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'symlink-walk-')); const root = path.join(tmp, 'root'); const sub = path.join(root, 'sub'); fs.mkdirSync(sub, { recursive: true }); fs.writeFileSync(path.join(sub, 'foo.test.ts'), 'x'); // Symlink inside the scan root that points back to the scan root. const link = path.join(sub, 'loop'); fs.symlinkSync(root, link, 'dir'); const entries = fs.readdirSync(sub); const dirents = fs.readdirSync(sub, { withFileTypes: true }); console.log('readdirSync entries:', entries); console.log('dirent types:', dirents.map((d) => ({ name: d.name, isFile: d.isFile(), isDirectory: d.isDirectory(), isSymbolicLink: d.isSymbolicLink(), }))); const st = fs.statSync(link); console.log('statSync(link):', { isFile: st.isFile(), isDirectory: st.isDirectory(), isSymbolicLink: st.isSymbolicLink ? st.isSymbolicLink() : undefined, }); console.log('lstatSync(link):', { isFile: fs.lstatSync(link).isFile(), isDirectory: fs.lstatSync(link).isDirectory(), isSymbolicLink: fs.lstatSync(link).isSymbolicLink(), }); JSRepository: NVIDIA/NemoClaw
Length of output: 567
Do not follow symlinks while walking test roots.
statSync()resolves links, so a symlink under a scan root can recurse back into the tree or pull files in from outside the repo. UsereaddirSync(..., { withFileTypes: true }), skipentry.isSymbolicLink(), and switch toDirentchecks.🤖 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 `@scripts/checks/test-title-style.mts` around lines 136 - 147, Update walkTestFiles to call readdirSync with withFileTypes enabled, skip entries where entry.isSymbolicLink() is true, and use Dirent methods for directory and file checks instead of statSync. Preserve the existing skip filtering, recursive traversal, and TEST_FILE_PATTERN matching for regular files.Source: Path instructions
test/no-coverage-ignore.test.ts (1)
36-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required issue suffix to these test titles.
Neither the parent
describenor its child tests ends with a local(#1234)reference. Add the reference to the parent title; children then need not repeat it.As per coding guidelines, “place local issue references in a final
(#1234)suffix.” Based on learnings, a suffix on the parentdescribe(...)is sufficient.🤖 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/no-coverage-ignore.test.ts` around lines 36 - 45, Add the required local issue reference suffix in the parent “scanned source path selection” describe title, using the final `(`#1234`)` format; leave the child test titles unchanged.Sources: Coding guidelines, Learnings
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
HOLD at 5f0fa7c pending two enforcement fixes and exact-base refresh:
-
scripts/checks/test-create-require-budget.mts:129-156,258-273reads the baseline arrays from the merge base, but executes the extractor, comparator, and early-return flow from mutable PR-head code. A checked PR can therefore weaken the new ratchet while adding an allowlisted seam, contrary to.coderabbit.yaml:236. Run the monotonic comparison from base-trusted code and add a regression proving a head-side tamper cannot bypass it. -
scripts/checks/local-credential-helper-pin.mts:61-68matches the reviewed artifact path as a prefix. The current regex accepts.html.bak, query, and fragment suffixes even though the guard promises one full immutable URL. Require an exact URL-path boundary and add positive plus suffixed-path negative tests.
Please also disposition the remaining valid CodeRabbit guard findings before re-review, especially chained it modifiers (no-unit-blocks-in-live-e2e.mts:76-84) and raw-text policy-read discovery (openshell-policy-mutation-read.mts:82-109).
Finally, origin/main advanced to 3558a7a after the audited run. Refresh onto that base and rerun the exact head/base gates.
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
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 `@scripts/checks/openshell-policy-mutation-read.mts`:
- Around line 84-140: Update countPolicyReadCalls and its AST helpers to resolve
policy-builder references from the canonical import, including aliased imports,
rather than matching only terminal names; do not count unrelated member calls
such as fixture.buildPolicyGetCommand(). Add deterministic tests covering
aliased builder detection and this false-positive case.
🪄 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: ff438b0a-84c8-4412-8b2f-193ddb618e32
📒 Files selected for processing (15)
.github/actions/ci-static-checks/action.yaml.github/actions/ci-static-checks/create-require-ratchet.mtsscripts/checks/layer-import-boundaries.mtsscripts/checks/local-credential-helper-pin.mtsscripts/checks/no-unit-blocks-in-live-e2e.mtsscripts/checks/openshell-policy-mutation-read.mtsscripts/checks/test-title-style.mtstest/layer-import-boundaries.test.tstest/local-credential-helper-pin.test.tstest/no-coverage-ignore.test.tstest/no-unit-blocks-in-live-e2e.test.tstest/policy-mutation-read-discovery.test.tstest/pr-workflow-contract.test.tstest/test-create-require-budget.test.tstest/test-title-style.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- test/policy-mutation-read-discovery.test.ts
- test/no-unit-blocks-in-live-e2e.test.ts
- scripts/checks/no-unit-blocks-in-live-e2e.mts
- test/no-coverage-ignore.test.ts
- scripts/checks/test-title-style.mts
- test/local-credential-helper-pin.test.ts
- scripts/checks/local-credential-helper-pin.mts
- scripts/checks/layer-import-boundaries.mts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
HOLD at fef37a8e:
-
scripts/checks/openshell-policy-mutation-read.mts:84-140identifies builder calls only by their terminal property/name. An imported canonical alias such asimport { buildPolicyGetCommand as getPolicy } ...; getPolicy(name)currently counts 0, while unrelated callsfixture.buildPolicyGetCommand(name)andfixture["buildPolicyGetFullCommand"](name)each count 1. Resolve bindings from the canonical policy-command import, including aliases/namespaces, and add both alias-positive and unrelated-member-negative tests. This matches the new exact-head CodeRabbit Major. -
.github/actions/ci-static-checks/action.yaml:36-38adds the base-trusted ratchet, but this PR cannot execute that head-only addition through its base-trusted checkout. Exact-head run29537106568checked out the trusted action at base41a5885and went directly from dependency install to config validation; no ratchet step ran. Land the action/check as a prerequisite and refresh this PR onto that base, or record an explicit maintainer bootstrap waiver after verifying the unchanged allowlists.
The remaining eight original CodeRabbit items are correctly fixed with focused coverage. Required checks and E2E are still pending and must finish on the final exact head/base pair.
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Follow-up on exact head The policy scanner compares imported names rather than lexical symbols and recognizes canonical modules by suffix. Three adversarial cases reproduce as policy reads when each must count as zero:
That permits decoy calls to preserve the audited count while real policy reads change. Please resolve the call target through lexical scope/symbol identity, require the exact repository-root canonical module, and add adversarial regressions for all three cases. The new base-trusted GitHub's conflict result is stale: a local merge-tree against current |
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
## Summary Run the createRequire allowlist ratchet from the immutable base-trusted static-check action. Pull requests now compare their reviewed allowlists with the validated base commit even from a shallow checkout, rejecting additions while permitting removals. ## Related Issue Closes #7056 ## Changes - Bundle a trusted ratchet with the shared static-check action and run it immediately after dependency installation. - Read both the current `.ts` checker and the pending `.mts` form, validate literal allowlists, and compare them with the pull request base revision. - Validate the base SHA from the GitHub event and fetch that exact public commit only when a shallow checkout does not already contain it. - Add focused behavior and trusted-action wiring coverage. - Preserve credit for Tinson Lai, who originally authored this prerequisite in #7047; the commit retains Tinson as co-author. ## 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: internal CI enforcement only; no user-facing behavior changes - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project integration test/create-require-ratchet.test.ts test/pr-workflow-contract.test.ts` (2 files, 23 tests passed) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — hosted CI pending - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a “createRequire allowlist ratchet” static check that verifies observed `createRequire` usage against trusted allowlists from the base revision. * Extended the CI static-check composite action to install required verifier tooling and enforce the ratchet check before other validations. * Pin verifier dependencies (TypeScript) to a fixed version for consistent results. * **Bug Fixes** * Hardened verification to fail closed on malformed/ambiguous checker state and to reject unsupported inventory inputs (e.g., symlinks). * **Tests** * Added end-to-end and contract coverage for allowlist extraction, boundary detection, base revision resolution behavior, and dependency pinning/integrity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Approved. Security review: PASS. The check-suite module migration preserves CI entrypoints and scanner behavior, including credential-environment and policy-mutation guards, with direct .mts compatibility coverage. Focused suites and conventional CI pass; the secondary advisor model failure is non-authoritative and reported no actionable finding.
<!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 plain sentences: what changes and why. Describe before-and-after behavior when it applies. Use existing repository terms; do not invent a label for this PR. --> This follow-up hardens the trusted CI ratchet and OpenShell policy-read scanner after #7047 merged before the final maintainer fix could land. It removes a duplicate root-dependency execution path and prevents locally shadowed `require` or lookalike resolver calls from satisfying the policy-read check. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Follow-up to #7047. ## Changes <!-- List concrete changes. If this adds an abstraction, configuration, fallback, migration, or compatibility path, name its current requirement and consumer, explain why a direct change is insufficient, and identify the test that protects it. --> - Remove the duplicate root `npx tsx` invocation of the base-trusted createRequire ratchet. - Require CommonJS policy-builder imports to resolve to the ambient `require`, not a locally shadowed decoy. - Count `resolveOpenshellBinary()` policy reads only from the canonical policy command module and declaration. - Add workflow and scanner regressions covering the trust-boundary and AST-decoy cases. ## 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) ## Quality Gates <!-- Check one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: internal CI and static-analysis enforcement only; no user-facing behavior or configuration changes - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: exact diff reviewed for CI trust-boundary and policy-scanner false-negative behavior; focused negative regressions pass - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run --project integration test/policy-mutation-read-discovery.test.ts test/pr-workflow-contract.test.ts` (30 passed) - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: `npm run checks` passed; diff-scoped prek and the normal pre-push TypeScript gates passed - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved policy-read discovery to reduce false positives from shadowed `require` usage and non-canonical OpenShell resolver calls. * Strengthened canonical resolver detection to improve classification of direct policy reads. * **Tests** * Added Vitest cases to ensure resolver decoys are not counted and canonical resolver reads are counted correctly (including `--base` and `--full`). * Expanded workflow contract assertions to prevent invoking the ratchet script via `npx tsx`. * **Chores** * Updated the static-checks workflow to run the ratchet validation script directly with Node. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
7 participants
Summary
Migrates the repository check suite from
.tsentrypoints to.mtsand removes the final live.tsshim. This clean current-mainreplacement for #6940 preserves Tinson Lai as the author of the original four commits and credits him as co-author of the follow-up hardening.Related Issue
Closes #6921
Changes
scripts/checkssuite to.mtsand update package scripts, workflows, actions, tests, diagnostics, and contributor-skill references..mts-first compatibility selection while leaving no livescripts/checks/*.tsentrypoint in the tree..mtscoverage scanning, canonical Vitest project discovery, live-E2E AST detection, oclif command binding detection, and merge-base createRequire allowlist enforcement.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run checkspassed.npm run checkwas attempted, but its CLI coverage lane failed across 265 unrelated tests under the concurrent local environment: spawned Python fixtures reported missingtomllib/PyYAML and Node 26 deprecation warnings polluted expected-empty stderr. Focused suites,npm run check:diff, typecheck, and repository checks pass; hosted CI remains required.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
New Features
createRequireallowlists.Bug Fixes
Tests