chore: unify vitest workspace and replace husky with prek#673
Merged
ericksoa merged 11 commits intoNVIDIA:mainfrom Mar 23, 2026
Merged
chore: unify vitest workspace and replace husky with prek#673ericksoa merged 11 commits intoNVIDIA:mainfrom
ericksoa merged 11 commits intoNVIDIA:mainfrom
Conversation
Convert all 21 root test files from CJS to ESM:
- require() → import (static for top-level, createRequire bridge for
registry.test.js where HOME must be set before module load)
- __dirname → import.meta.dirname
- Add test/package.json with "type": "module"
- Add explicit `import { describe, it, expect } from "vitest"`
- Normalize all built-in imports to use node: prefix
Unify both test suites under a single vitest 4 config:
- Upgrade root vitest from ^3.1.1 to ^4.1.0, add @vitest/coverage-v8
- Remove vitest and coverage-v8 from nemoclaw/package.json
- Replace vitest.config.ts + nemoclaw/vitest.config.ts with a single
root vitest.config.ts using test.projects (vitest 4 API)
- One `npx vitest run --coverage` runs both cli and plugin tests
- Coverage scoped to nemoclaw/src/**/*.ts (plugin code only)
Update CI and hooks:
- pr.yaml: merge two vitest steps into one
- pre-commit: run plugin tests from root (--project plugin)
- Coverage ratchet: read from coverage/ instead of nemoclaw/coverage/
Cleanup from code review:
- Remove dead afterEach in runtime-shell.test.js
- Remove unnecessary createRequire in onboard-readiness.test.js and
runner.test.js (replaced with static ESM imports)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…workspace # Conflicts: # .husky/pre-commit
husky-env.sh is sourced (not executed directly), so it doesn't have a shebang. Add a shellcheck shell=sh directive so shellcheck knows the target shell. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMoved Vitest config to a root multi-project setup, migrated tests to ESM, consolidated CI to a single coverage-enabled Vitest run, removed Husky hook scripts and helper in favor of prek-managed hooks, updated coverage summary path to repo root, and bumped Node engine to >=22.0.0. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Git as Git Client
participant Prek as Prek (local hooks)
participant CI as GitHub Actions
participant Vitest as Vitest (root)
participant Coverage as V8 Coverage
participant FS as Repo FS
Git->>Prek: commit / push triggers hooks
Prek->>Vitest: run configured hooks (e.g., `npx vitest run --project plugin`)
Vitest->>FS: load tests (ESM) from `test/` and `nemoclaw/src/`
Vitest->>Coverage: record coverage (v8)
Coverage->>FS: write `coverage/coverage-summary.json`
Dev->>CI: push / open PR
CI->>Vitest: run `npx vitest run --coverage`
Vitest->>Coverage: produce reports
Coverage->>FS: emit root `coverage/coverage-summary.json`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The eslint strict-type-checked rules in nemoclaw/ need vitest's type definitions to resolve describe/it/expect in test files. Without vitest in nemoclaw/node_modules, tsc and eslint report 473 no-unsafe-call errors. Test execution still happens from the root workspace — this dep is only needed for type checking and linting. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
scripts/check-coverage-ratchet.sh (1)
21-24:⚠️ Potential issue | 🟡 MinorError message references outdated path.
The error message still instructs users to run coverage "in nemoclaw/" but coverage is now generated at the repository root. This could mislead contributors debugging coverage issues.
Proposed fix
if [ ! -f "$SUMMARY_FILE" ]; then echo "ERROR: Coverage summary not found: $SUMMARY_FILE" - echo "Run 'npx vitest run --coverage' in nemoclaw/ first." + echo "Run 'npx vitest run --coverage' first." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-coverage-ratchet.sh` around lines 21 - 24, The script's error message refers to an outdated path ("nemoclaw/"); update the echo that mentions running coverage so it points to the repository root (e.g., "Run 'npx vitest run --coverage' in the repository root") and/or reference the SUMMARY_FILE variable so the message is unambiguous; modify the echo lines near the SUMMARY_FILE check (the block that prints "ERROR: Coverage summary not found: $SUMMARY_FILE" and the following guidance) to reflect the new location and include actionable guidance referencing SUMMARY_FILE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/install-preflight.test.js`:
- Around line 10-12: Replace uses of import.meta.dirname when constructing paths
(e.g., the constants INSTALLER and CURL_PIPE_INSTALLER and any other occurrences
at lines referenced) with a Node-20-compatible approach: derive __dirname using
fileURLToPath(import.meta.url) and path.dirname, then use that directory to
build paths via path.join; update all usages (including the other occurrences
noted) to reference the new dirname variable so the test runs on Node
20.0.0–20.10.x as well as newer versions.
In `@test/runner.test.js`:
- Around line 4-11: Add an explicit CommonJS require for this ESM test by
importing createRequire from "module" and initializing require with the current
module URL (e.g., const require = createRequire(import.meta.url)); place this
near the top of test/runner.test.js before any usage (so calls like require(),
require.resolve(), and require.cache used later in the file work correctly),
ensuring subsequent code that references require, require.resolve, or
require.cache operates without ReferenceError.
---
Outside diff comments:
In `@scripts/check-coverage-ratchet.sh`:
- Around line 21-24: The script's error message refers to an outdated path
("nemoclaw/"); update the echo that mentions running coverage so it points to
the repository root (e.g., "Run 'npx vitest run --coverage' in the repository
root") and/or reference the SUMMARY_FILE variable so the message is unambiguous;
modify the echo lines near the SUMMARY_FILE check (the block that prints "ERROR:
Coverage summary not found: $SUMMARY_FILE" and the following guidance) to
reflect the new location and include actionable guidance referencing
SUMMARY_FILE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff458c2c-58cd-4789-9fce-3bd2ec0298e3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
.github/workflows/pr.yaml.husky/pre-commitnemoclaw/package.jsonnemoclaw/vitest.config.tspackage.jsonscripts/check-coverage-ratchet.shscripts/husky-env.shtest/cli.test.jstest/credentials.test.jstest/inference-config.test.jstest/install-preflight.test.jstest/local-inference.test.jstest/nim.test.jstest/onboard-readiness.test.jstest/onboard-selection.test.jstest/onboard.test.jstest/package.jsontest/platform.test.jstest/policies.test.jstest/preflight.test.jstest/registry.test.jstest/runner.test.jstest/runtime-shell.test.jstest/security-c2-dockerfile-injection.test.jstest/security-c4-manifest-traversal.test.jstest/service-env.test.jstest/setup-sandbox-name.test.jstest/smoke-macos-install.test.jstest/uninstall.test.jsvitest.config.ts
💤 Files with no reviewable changes (1)
- nemoclaw/vitest.config.ts
Comment on lines
+10
to
12
| const INSTALLER = path.join(import.meta.dirname, "..", "install.sh"); | ||
| const CURL_PIPE_INSTALLER = path.join(import.meta.dirname, "..", "scripts", "install.sh"); | ||
| const GITHUB_INSTALL_URL = "git+https://github.com/NVIDIA/NemoClaw.git"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What exact Node.js version introduced import.meta.dirname support, and is it available in all Node 20.x releases?
💡 Result:
import.meta.dirname (and import.meta.filename) first shipped in Node.js v20.11.0. [1][2]
It is not available in all Node 20.x releases—only in Node 20.11.0 and later (so 20.0.0–20.10.x do not have it). [1][2]
Sources: [1] Node.js v20.11.0 release notes, [2] Node.js v20 ESM docs (“Added in: v20.11.0”).
🏁 Script executed:
# Check package.json for Node version requirement
if [ -f package.json ]; then
echo "=== package.json engines field ==="
jq '.engines' package.json 2>/dev/null || grep -A 2 '"engines"' package.json
fi
# Check for other version markers
echo "=== Other version files ==="
for file in .node-version .nvmrc .nanorc; do
if [ -f "$file" ]; then
echo "$file:"; cat "$file"
fi
done
# Check README or docs
if [ -f README.md ]; then
echo "=== Node version mentions in README ==="
grep -i "node.*version\|require.*node\|>=.*20" README.md | head -5
fiRepository: NVIDIA/NemoClaw
Length of output: 186
import.meta.dirname breaks on Node 20.0.0–20.10.x; use fileURLToPath instead.
Lines 10–11 and related uses of import.meta.dirname will fail on Node versions 20.0.0–20.10.x. Since package.json declares "node": ">=20.0.0", this code is incompatible with a significant range of supported versions. import.meta.dirname was introduced only in Node 20.11.0.
Use fileURLToPath(import.meta.url) with path.dirname() to maintain compatibility across the full Node 20.x range:
Patch
import fs from "node:fs";
import os from "node:os";
import path from "node:path";
import { spawnSync } from "node:child_process";
+import { fileURLToPath } from "node:url";
-const INSTALLER = path.join(import.meta.dirname, "..", "install.sh");
-const CURL_PIPE_INSTALLER = path.join(import.meta.dirname, "..", "scripts", "install.sh");
+const THIS_DIR = path.dirname(fileURLToPath(import.meta.url));
+const REPO_ROOT = path.join(THIS_DIR, "..");
+const INSTALLER = path.join(REPO_ROOT, "install.sh");
+const CURL_PIPE_INSTALLER = path.join(REPO_ROOT, "scripts", "install.sh");- cwd: path.join(import.meta.dirname, ".."),
+ cwd: REPO_ROOT,Also applies to lines 91, 351, 483, 500, 510.
📝 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.
Suggested change
| const INSTALLER = path.join(import.meta.dirname, "..", "install.sh"); | |
| const CURL_PIPE_INSTALLER = path.join(import.meta.dirname, "..", "scripts", "install.sh"); | |
| const GITHUB_INSTALL_URL = "git+https://github.com/NVIDIA/NemoClaw.git"; | |
| import fs from "node:fs"; | |
| import os from "node:os"; | |
| import path from "node:path"; | |
| import { spawnSync } from "node:child_process"; | |
| import { fileURLToPath } from "node:url"; | |
| const THIS_DIR = path.dirname(fileURLToPath(import.meta.url)); | |
| const REPO_ROOT = path.join(THIS_DIR, ".."); | |
| const INSTALLER = path.join(REPO_ROOT, "install.sh"); | |
| const CURL_PIPE_INSTALLER = path.join(REPO_ROOT, "scripts", "install.sh"); | |
| const GITHUB_INSTALL_URL = "git+https://github.com/NVIDIA/NemoClaw.git"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/install-preflight.test.js` around lines 10 - 12, Replace uses of
import.meta.dirname when constructing paths (e.g., the constants INSTALLER and
CURL_PIPE_INSTALLER and any other occurrences at lines referenced) with a
Node-20-compatible approach: derive __dirname using
fileURLToPath(import.meta.url) and path.dirname, then use that directory to
build paths via path.join; update all usages (including the other occurrences
noted) to reference the new dirname variable so the test runs on Node
20.0.0–20.10.x as well as newer versions.
Comment on lines
+4
to
+11
| import { describe, it, expect } from "vitest"; | ||
| import fs from "node:fs"; | ||
| import os from "node:os"; | ||
| import path from "node:path"; | ||
| import childProcess from "node:child_process"; | ||
| const { spawnSync } = childProcess; | ||
|
|
||
| const runnerPath = path.join(__dirname, "..", "bin", "lib", "runner"); | ||
| const runnerPath = path.join(import.meta.dirname, "..", "bin", "lib", "runner"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ESM file still uses require without createRequire bootstrap.
rg -nP '\brequire\s*\(|\brequire\.cache\b|\brequire\.resolve\b' test/runner.test.js
rg -n 'createRequire|node:module' test/runner.test.jsRepository: NVIDIA/NemoClaw
Length of output: 908
Define require explicitly in this ESM test file.
After the ESM migration, require(), require.cache, and require.resolve() calls throughout this file (lines 16, 43, 44, 49, 67, 68, 78, 88, 93, 98, 107, 117, 124, 132, 139) will throw ReferenceError unless you initialize require via createRequire.
Minimal fix
import { describe, it, expect } from "vitest";
import fs from "node:fs";
import os from "node:os";
import path from "node:path";
import childProcess from "node:child_process";
+import { createRequire } from "node:module";
const { spawnSync } = childProcess;
+const require = createRequire(import.meta.url);
const runnerPath = path.join(import.meta.dirname, "..", "bin", "lib", "runner");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/runner.test.js` around lines 4 - 11, Add an explicit CommonJS require
for this ESM test by importing createRequire from "module" and initializing
require with the current module URL (e.g., const require =
createRequire(import.meta.url)); place this near the top of test/runner.test.js
before any usage (so calls like require(), require.resolve(), and require.cache
used later in the file work correctly), ensuring subsequent code that references
require, require.resolve, or require.cache operates without ReferenceError.
- Bump engines.node from >=20.0.0 to >=22.0.0 in both package.json files. import.meta.dirname (used in all ESM test files) requires Node 20.11+, and CI already runs Node 22. - Fix stale error message in coverage ratchet script that referenced the old nemoclaw/ coverage path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Husky managed git hooks via `.husky/` shell scripts that invoked lint-staged, vitest, commitlint, tsc, and pyright. prek can do all of this from `.pre-commit-config.yaml`, giving us a single source of truth. - Add `@j178/prek` as a devDependency (npm package) - Move lint-staged, vitest, commitlint, tsc, pyright into `.pre-commit-config.yaml` as local hooks with proper stages - Remove lint-staged + shellcheck devDeps (redundant with prek-native hooks that already run the same tools at priorities 5-10) - Delete `.husky/`, `scripts/husky-env.sh`, and husky devDep - `prepare` script now runs `prek install` (only inside a git repo) Existing contributors with husky's `core.hooksPath=.husky/_` in their local git config will need to run: git config --unset-all --local core.hooksPath Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
changed the title
The test landed on main as CJS (require/__dirname/bare describe) which breaks under our vitest 4 ESM config. Convert to ESM imports with explicit vitest globals. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The pre-commit hook was cd-ing into nemoclaw/ before running vitest, causing the root vitest.config.ts project paths to resolve incorrectly (nemoclaw/nemoclaw/src/...) and find zero test files. prek already runs hooks from the repo root, so the cd is unnecessary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a pre-commit hook that rejects force-added files matching .gitignore patterns (catches `git add -f` bypasses). The hook uses `git ls-files --ignored --cached` so .gitignore stays the single source of truth — no duplicate pattern list. Also expand .gitignore with additional sensitive file patterns (key.json, token.json, secrets.json/yaml, .envrc, .direnv/, .pypirc, *.tfvars) and reorganize into labeled sections. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
ericksoa
approved these changes
Mar 23, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
LGTM — mechanical CJS→ESM conversion, vitest workspace unification, and husky→prek migration all look correct. Node >=22 requirement is the only breaking change; acceptable given import.meta.dirname dependency. Will create merge conflicts with our open security PRs (#675, #679, #617) but those can rebase after.
ericksoa
approved these changes
Mar 23, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
LGTM — mechanical CJS→ESM conversion, vitest workspace unification, and husky→prek migration all look correct. Node >=22 requirement is the only breaking change; acceptable given import.meta.dirname dependency. Will create merge conflicts with our open security PRs (#675, #679, #617) but those can rebase after.
ericksoa
approved these changes
Mar 23, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
LGTM — mechanical CJS to ESM conversion, vitest workspace unification, and husky to prek migration all look correct. Node >=22 requirement is the only breaking change; acceptable given import.meta.dirname dependency. Will create merge conflicts with our open security PRs (#675, #679, #617) but those can rebase after.
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
2 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
require→import,__dirname→import.meta.dirname)test.projects@j178/prek) as the sole git hook managerVitest / ESM changes
Test files (21 files):
require()→import(ESM static imports)createRequirebridge only where needed (registry.test.js — HOME must be set before module load)node:prefixtest/package.jsonwith"type": "module"Config:
vitest.config.ts+nemoclaw/vitest.config.ts→ single rootvitest.config.tswithtest.projects@vitest/coverage-v8to root>=22Husky → prek migration
All git hooks now live in
.pre-commit-config.yaml— single source of truth.tsc --noEmit), Pyright@j178/prekas devDependency;preparescript runsprek install.husky/directory andscripts/husky-env.shTest plan
npx vitest run— all tests passnpx vitest run --coverage --project plugin— coverage report generates correctly🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores
Documentation