refactor(scripts): migrate low-hanging JavaScript to TypeScript#4367
Open
cv wants to merge 2 commits into
Open
refactor(scripts): migrate low-hanging JavaScript to TypeScript#4367cv wants to merge 2 commits into
cv wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR modernizes build scripts and test infrastructure by converting multiple scripts from CommonJS/untyped JavaScript to typed TypeScript with enhanced validation and module loading patterns. Package.json, biome.json, and test files are updated to support TypeScript execution and improve type safety throughout the codebase. ChangesScripts and Tests TypeScript Modernization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4367.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
Since last review detailsCurrent findings:
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: 4
🤖 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/benchmark-sandbox-image-build.ts`:
- Around line 41-47: The requireValue function currently treats tokens starting
with '-' as valid values, so when a flag is followed by another flag (e.g.,
--current-repo --cache) it should throw; update requireValue to check that the
captured value exists and does not start with '-' (or '--') and throw the same
Missing value error if it does. Apply the same validation wherever requireValue
is used (see the call sites around lines referenced) to ensure flag-shaped
tokens are rejected as option values.
- Around line 89-93: makeTempWorktree currently creates a temp dir
(worktreeRoot) then calls run("git", ["worktree", "add", ...]) and if that run
throws the temp dir is left behind; wrap the git call in a try/catch (or
try/finally) inside makeTempWorktree, and on any failure remove the created
directory (use fs.rmSync or fs.rmdirSync with recursive true for compatibility)
before rethrowing the error so the temp worktree is cleaned up; keep
worktreeRoot and the run(...) call names as the anchor points for the change.
In `@scripts/convert-docs-to-fern.ts`:
- Around line 89-93: The bug is that skillPriority can be undefined when nothing
is present, so the subsequent check (skillPriority !== "") still passes and
emits an empty skill.priority; fix by ensuring skillPriority is a true
empty-string fallback or by making the condition null-safe: set skillPriority =
firstPresent(nestedValue(metadata, "skill", "priority"),
metadata.skill_priority) ?? "" (or alternatively change the write condition to
check for null/undefined and emptiness, e.g. skillPriority != null &&
skillPriority !== "") so the exporter only writes skill.priority when a real
value exists; refer to the skillPriority variable, the firstPresent function,
nestedValue, and metadata.skill_priority to locate the code to change.
In `@scripts/watch-fern-preview.ts`:
- Around line 28-31: The current check allows whitespace-only versions because
it tests fernConfig.version without trimming; update the validation to trim the
value first and reject empty trimmed strings, then assign the trimmed result to
fernVersion so downstream uses (fernVersion and any construction like
`fern-api@${fernVersion}`) receive the cleaned value; specifically, replace the
validation of fernConfig.version with logic that computes const trimmedVersion =
fernConfig.version?.trim(), throws if typeof trimmedVersion !== "string" or
trimmedVersion.length === 0, and then set fernVersion = trimmedVersion.
🪄 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: fe8f6aa3-328b-41cd-a4c5-b3aa173795e2
📒 Files selected for processing (7)
package.jsonscripts/benchmark-sandbox-image-build.tsscripts/convert-docs-to-fern.tsscripts/dev-tier-selector.tsscripts/watch-fern-preview.tstest/credentials-shim.test.tstest/runner-basic.test.ts
miyoungc
reviewed
May 27, 2026
| @@ -3,20 +3,42 @@ | |||
|
|
|||
Collaborator
There was a problem hiding this comment.
@cv we can remove this file entirely, as we're done with the Fern migration.
miyoungc
approved these changes
May 27, 2026
Collaborator
Author
|
Addressed the PR feedback in 4b68528:
Re-ran:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
biome.json (1)
23-24: ⚡ Quick winPrefer a
test/**/*.tsglob over per-file test entries.These explicit paths are easy to miss during future test additions. Using a TS glob in both include blocks keeps lint coverage consistent as more tests migrate.
Proposed diff
"files": { "ignoreUnknown": true, "includes": [ @@ - "test/credentials-shim.test.ts", - "test/runner-basic.test.ts", + "test/**/*.ts", @@ "overrides": [ { "includes": [ @@ - "test/credentials-shim.test.ts", - "test/runner-basic.test.ts", + "test/**/*.ts",Also applies to: 100-101
🤖 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 `@biome.json` around lines 23 - 24, Replace the explicit test file entries ("test/credentials-shim.test.ts", "test/runner-basic.test.ts", and the other per-file strings seen in the include arrays) with a single glob pattern "test/**/*.ts" in both include blocks so new tests are picked up automatically; locate the include arrays that currently list those filenames and update them to the glob pattern to ensure consistent lint/test coverage as tests are added or moved.
🤖 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/watch-fern-preview.ts`:
- Around line 29-30: The semver test on trimmedFernVersion allows numeric
identifiers with leading zeros; replace the current regex used in the if test
with one that enforces numeric identifiers to be either "0" or non-zero-starting
(no leading zeros) for major/minor/patch and for numeric prerelease identifiers
(e.g., use a pattern that uses (?:0|[1-9]\d*) for numeric parts and applies that
rule inside the prerelease group), then throw the same Error when the tightened
regex test fails.
---
Nitpick comments:
In `@biome.json`:
- Around line 23-24: Replace the explicit test file entries
("test/credentials-shim.test.ts", "test/runner-basic.test.ts", and the other
per-file strings seen in the include arrays) with a single glob pattern
"test/**/*.ts" in both include blocks so new tests are picked up automatically;
locate the include arrays that currently list those filenames and update them to
the glob pattern to ensure consistent lint/test coverage as tests are added or
moved.
🪄 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: f6f210d4-0e0d-41b3-9f35-d65f313574fa
📒 Files selected for processing (4)
biome.jsonscripts/benchmark-sandbox-image-build.tsscripts/watch-fern-preview.tssrc/lib/credentials/store.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/credentials/store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/benchmark-sandbox-image-build.ts
Comment on lines
+29
to
+30
| if (!/^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(trimmedFernVersion)) { | ||
| throw new Error("fern.config.json must contain an exact semver version"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SemVer 2.0.0: are leading zeros allowed in MAJOR.MINOR.PATCH (e.g., 01.2.3)?
💡 Result:
No, leading zeros are not allowed in the MAJOR.MINOR.PATCH components of a version number under SemVer 2.0.0 [1][2]. The official Semantic Versioning 2.0.0 specification explicitly states: "A normal version number MUST take the form X.Y.Z where X, Y, and Z are non-negative integers, and MUST NOT contain leading zeroes [1][3]." This rule ensures that version numbers are parsed and sorted consistently, as leading zeros can cause ambiguity in numerical comparisons [4]. Additionally, the specification applies the same restriction to numeric identifiers within pre-release versions, which also must not include leading zeros [1][4].
Citations:
- 1: https://semver.org/spec/v2.0.0.html
- 2: https://github.com/semver/semver/blob/v2.0.0/semver.md
- 3: https://semver.org/
- 4: https://stackoverflow.com/questions/66607528/semantic-versioning-2-zero-cases
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show SPDX + the semver validation area
nl -ba scripts/watch-fern-preview.ts | sed -n '1,80p'
# Extract the current regex line(s) for quick reference
rg -n "trimmedFernVersion|fern\.config\.json|semver" -n scripts/watch-fern-preview.ts
# Quick semver regex behavior check (no package deps)
node <<'NODE'
const current = /^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/;
const proposed = /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-(?:0|[1-9]\d*|[0-9A-Za-z-]*[A-Za-z-][0-9A-Za-z-]*)(?:\.(?:0|[1-9]\d*|[0-9A-Za-z-]*[A-Za-z-][0-9A-Za-z-]*))*)?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$/
const cases = [
"1.2.3",
"01.2.3",
"1.02.3",
"1.2.03",
"0.0.0",
"1.2.3-alpha",
"1.2.3-alpha.1",
"1.2.3+build.1",
"1.2.3-alpha.01", // prerelease numeric identifier with leading zero
"1.2.3+01.2" // build metadata leading zeros (SemVer allows in build identifiers)
];
for (const s of cases) {
console.log(
s.padEnd(16),
"current=", current.test(s),
"proposed=", proposed.test(s)
);
}
NODERepository: NVIDIA/NemoClaw
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show top portion (including SPDX + nearby validation code)
sed -n '1,120p' scripts/watch-fern-preview.ts
# Locate the semver validation lines
rg -n "trimmedFernVersion|fern\.config\.json|\\^\\d+\\.\\d+\\.\\d+|semver" scripts/watch-fern-preview.ts
# Quick semver regex behavior check (no deps)
node <<'NODE'
const current = /^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/;
const proposed = /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-(?:0|[1-9]\d*|[0-9A-Za-z-]*[A-Za-z-][0-9A-Za-z-]*)(?:\.(?:0|[1-9]\d*|[0-9A-Za-z-]*[A-Za-z-][0-9A-Za-z-]*))*)?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$/
const cases = [
"1.2.3",
"01.2.3",
"1.02.3",
"1.2.03",
"0.0.0",
"1.2.3-alpha",
"1.2.3-alpha.1",
"1.2.3+build.1",
"1.2.3-alpha.01", // prerelease numeric identifier with leading zero
"1.2.3+01.2" // build metadata leading zeros (allowed in SemVer build identifiers)
];
for (const s of cases) {
console.log(
s.padEnd(16),
"current=", current.test(s),
"proposed=", proposed.test(s)
);
}
NODERepository: NVIDIA/NemoClaw
Length of output: 4243
scripts/watch-fern-preview.ts: tighten semver regex to reject leading zeros in numeric identifiers
The current regex accepts non-SemVer numeric identifiers like 01.2.3 (and 1.2.3-alpha.01), while the thrown error says the file must contain an “exact semver version” (regex located at lines 29-30).
Suggested patch
-if (!/^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(trimmedFernVersion)) {
+if (
+ !/^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-(?:0|[1-9]\d*|[0-9A-Za-z-]*[A-Za-z-][0-9A-Za-z-]*)(?:\.(?:0|[1-9]\d*|[0-9A-Za-z-]*[A-Za-z-][0-9A-Za-z-]*))*)?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$/.test(
+ trimmedFernVersion,
+ )
+) {
throw new Error("fern.config.json must contain an exact semver version");
}📝 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
| if (!/^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(trimmedFernVersion)) { | |
| throw new Error("fern.config.json must contain an exact semver version"); | |
| if ( | |
| !/^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-(?:0|[1-9]\d*|[0-9A-Za-z-]*[A-Za-z-][0-9A-Za-z-]*)(?:\.(?:0|[1-9]\d*|[0-9A-Za-z-]*[A-Za-z-][0-9A-Za-z-]*))*)?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$/.test( | |
| trimmedFernVersion, | |
| ) | |
| ) { | |
| throw new Error("fern.config.json must contain an exact semver version"); | |
| } |
🤖 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/watch-fern-preview.ts` around lines 29 - 30, The semver test on
trimmedFernVersion allows numeric identifiers with leading zeros; replace the
current regex used in the if test with one that enforces numeric identifiers to
be either "0" or non-zero-starting (no leading zeros) for major/minor/patch and
for numeric prerelease identifiers (e.g., use a pattern that uses (?:0|[1-9]\d*)
for numeric parts and applies that rule inside the prerelease group), then throw
the same Error when the tightened regex test fails.
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
Migrates a small batch of low-hanging JavaScript files to TypeScript while leaving runtime-critical CommonJS entrypoints untouched. This reduces the non-dist JavaScript footprint and puts dev scripts and shim tests under the existing CLI TypeScript check.
Changes
scripts/benchmark-sandbox-image-build.ts,scripts/dev-tier-selector.ts, andscripts/watch-fern-preview.ts.scripts/convert-docs-to-fern.tsmigration helper now that the Fern migration is complete.docs:preview:watchto run the TypeScript watcher withtsx, and hardened its Fern package version validation.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit