fix(onboard): upgrade stale openshell during preflight (Fixes #1404)#1435
fix(onboard): upgrade stale openshell during preflight (Fixes #1404)#1435deepujain wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes dynamically resolve OpenShell minimum version requirements from blueprint configuration instead of using hardcoded values. A new Node.js utility module parses versions from YAML files, and the onboarding logic is refactored with improved version comparison to evaluate upgrade decisions based on configured requirements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
454-460: Keep the blueprint minimum-version parser shared with the installer.This helper duplicates
scripts/install-openshell.sh's parsing/fallback logic, and the two implementations already accept different inputs. Sharing one parser, or at least mirroring the shell rules here, would make it much harder for preflight and the installer to disagree on whether an upgrade is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 454 - 460, getMinimumOpenshellVersion duplicates parsing/fallback logic from scripts/install-openshell.sh which can cause preflight/installer mismatch; extract and centralize the parsing logic (or import a shared parser) so both the installer script and this function use the same implementation, and update getMinimumOpenshellVersion to delegate to that shared parser while preserving its existing input options (accepting blueprintText or reading the default blueprint file). Ensure the shared parser reproduces the shell rules (same regex, quoting, and fallback behavior) and reference the function name getMinimumOpenshellVersion and the installer script scripts/install-openshell.sh when locating where to replace logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 454-460: getMinimumOpenshellVersion duplicates parsing/fallback
logic from scripts/install-openshell.sh which can cause preflight/installer
mismatch; extract and centralize the parsing logic (or import a shared parser)
so both the installer script and this function use the same implementation, and
update getMinimumOpenshellVersion to delegate to that shared parser while
preserving its existing input options (accepting blueprintText or reading the
default blueprint file). Ensure the shared parser reproduces the shell rules
(same regex, quoting, and fallback behavior) and reference the function name
getMinimumOpenshellVersion and the installer script scripts/install-openshell.sh
when locating where to replace logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4942884e-5c56-43a0-a110-5dc9cbe76a41
📒 Files selected for processing (4)
bin/lib/onboard.jsscripts/install-openshell.shtest/install-openshell.test.jstest/onboard.test.js
|
✨ |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
458-463: Consider adding error handling for missing blueprint file.Unlike
getMinimumOpenshellVersionFromFilein the helper module which checks file existence first, this function will throw ifblueprint.yamlis missing. While the file should always exist in a valid NemoClaw installation, a more informative error message would improve debuggability.🔧 Optional: Add graceful fallback or clearer error
function getMinimumOpenshellVersion(blueprintText = null) { + const blueprintPath = path.join(ROOT, "nemoclaw-blueprint", "blueprint.yaml"); const rawBlueprint = blueprintText == null - ? fs.readFileSync(path.join(ROOT, "nemoclaw-blueprint", "blueprint.yaml"), "utf-8") + ? (fs.existsSync(blueprintPath) + ? fs.readFileSync(blueprintPath, "utf-8") + : "") : String(blueprintText); return parseMinimumOpenshellVersion(rawBlueprint); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 458 - 463, getMinimumOpenshellVersion currently reads blueprint.yaml directly and will throw a raw error if the file is missing; update it to check for the file before reading (e.g., use fs.existsSync or a try/catch around fs.readFileSync) and throw or log a clearer, contextual error if the blueprint file is absent, then call parseMinimumOpenshellVersion(rawBlueprint) as before; reference the function getMinimumOpenshellVersion and keep parseMinimumOpenshellVersion usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 458-463: getMinimumOpenshellVersion currently reads blueprint.yaml
directly and will throw a raw error if the file is missing; update it to check
for the file before reading (e.g., use fs.existsSync or a try/catch around
fs.readFileSync) and throw or log a clearer, contextual error if the blueprint
file is absent, then call parseMinimumOpenshellVersion(rawBlueprint) as before;
reference the function getMinimumOpenshellVersion and keep
parseMinimumOpenshellVersion usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6166862e-9f58-47fc-9426-4a2ed7f05a30
📒 Files selected for processing (4)
bin/lib/onboard.jsscripts/install-openshell.shscripts/lib/openshell-version.jstest/onboard.test.js
✅ Files skipped from review due to trivial changes (1)
- test/onboard.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install-openshell.sh
|
Addressed the parser nit by moving the minimum-version parsing into a shared helper and having the installer use the same logic with the existing shell fallback still in place. I reran |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
1785-1808: Consider caching theisOpenshellInstalled()result.The
isOpenshellInstalled()check is performed once implicitly at line 1786 (via ternary) and again at line 1789 (in the if-condition). While this works correctly and isn't a bug, caching the result would be slightly cleaner.♻️ Optional: Cache the installation check
+ const openshellInstalled = isOpenshellInstalled(); const requiredOpenshellVersion = getMinimumOpenshellVersion(); - const installedOpenshellVersion = isOpenshellInstalled() - ? getInstalledOpenshellVersion() - : null; - if (!isOpenshellInstalled()) { + const installedOpenshellVersion = openshellInstalled ? getInstalledOpenshellVersion() : null; + if (!openshellInstalled) { console.log(" openshell CLI not found. Installing...");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1785 - 1808, Cache the result of isOpenshellInstalled() into a local boolean (e.g., openshellPresent) and use that value when computing installedOpenshellVersion and in the subsequent conditional instead of calling isOpenshellInstalled() twice; update the references to installedOpenshellVersion, requiredOpenshellVersion, openshellInstall, and installOpenshell() accordingly so the logic is identical but avoids the redundant call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 1785-1808: Cache the result of isOpenshellInstalled() into a local
boolean (e.g., openshellPresent) and use that value when computing
installedOpenshellVersion and in the subsequent conditional instead of calling
isOpenshellInstalled() twice; update the references to
installedOpenshellVersion, requiredOpenshellVersion, openshellInstall, and
installOpenshell() accordingly so the logic is identical but avoids the
redundant call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 893a8ab0-e8c1-45c1-96a5-cbefcce24f23
📒 Files selected for processing (1)
bin/lib/onboard.js
|
Picked up the follow-up nit too: |
deepujain
force-pushed
the
fix/1404-refresh-openshell-on-upgrade
branch
from
d403cb2 to
30e87fb
Compare
|
Rebased this branch onto the latest |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 1578-1582: The console.error message that prints the GitHub
releases URL should be replaced with an allowed official docs link or an
internal docs path; locate the block that references requiredOpenshellVersion
and the two console.error calls (the one with the GitHub URL) and change the
second console.error string from "https://github.com/NVIDIA/OpenShell/releases"
to the approved documentation URL/path (e.g., an internal docs page or official
tool documentation) and keep the surrounding message and the process.exit(1)
behavior unchanged.
- Around line 373-398: Normalize and more robustly parse version strings before
comparison: in compareVersions and getMinimumOpenshellVersion call-sites strip
common prefixes like leading "v" (e.g., .replace(/^v/i, "") and .trim()), split
on non-digit separators if needed, and coerce parts with Number.parseInt(part,
10) falling back to 0 for NaN; then update
shouldUpgradeOpenshell(installedVersion, minimumVersion) to treat an unparseable
or empty installedVersion as "requires upgrade" (return true) rather than false
so preflight won't silently skip upgrades. Use the function names
compareVersions, getMinimumOpenshellVersion, and shouldUpgradeOpenshell to find
where to apply these changes.
🪄 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: Pro
Run ID: 0ef1a106-63fc-4b18-acd1-5d9e0d02e503
📒 Files selected for processing (5)
bin/lib/onboard.jsscripts/install-openshell.shscripts/lib/openshell-version.jstest/install-openshell.test.jstest/onboard.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/install-openshell.sh
- test/install-openshell.test.js
- scripts/lib/openshell-version.js
- test/onboard.test.js
| function compareVersions(left = "0.0.0", right = "0.0.0") { | ||
| const lhs = String(left).split(".").map((part) => Number.parseInt(part, 10) || 0); | ||
| const rhs = String(right).split(".").map((part) => Number.parseInt(part, 10) || 0); | ||
| const length = Math.max(lhs.length, rhs.length); | ||
| for (let index = 0; index < length; index += 1) { | ||
| const a = lhs[index] || 0; | ||
| const b = rhs[index] || 0; | ||
| if (a > b) return 1; | ||
| if (a < b) return -1; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| function getMinimumOpenshellVersion(blueprintText = null) { | ||
| if (blueprintText == null) { | ||
| return getMinimumOpenshellVersionFromFile( | ||
| path.join(ROOT, "nemoclaw-blueprint", "blueprint.yaml"), | ||
| ); | ||
| } | ||
| return parseMinimumOpenshellVersion(String(blueprintText)); | ||
| } | ||
|
|
||
| function shouldUpgradeOpenshell(installedVersion = null, minimumVersion = null) { | ||
| if (!installedVersion || !minimumVersion) return false; | ||
| return compareVersions(installedVersion, minimumVersion) < 0; | ||
| } |
There was a problem hiding this comment.
Harden version normalization before compare to avoid skipped upgrades.
Current coercion can mis-handle non-canonical inputs (for example, v1.2.3), and shouldUpgradeOpenshell() returns false on unparseable installed versions. That can silently skip a required upgrade in preflight.
🔧 Proposed fix
+const SEMVER_3PART_PATTERN = /^v?(\d+)\.(\d+)\.(\d+)$/i;
+
+function normalizeOpenshellVersion(version) {
+ const match = String(version ?? "").trim().match(SEMVER_3PART_PATTERN);
+ if (!match) return null;
+ return `${Number.parseInt(match[1], 10)}.${Number.parseInt(match[2], 10)}.${Number.parseInt(match[3], 10)}`;
+}
+
function compareVersions(left = "0.0.0", right = "0.0.0") {
- const lhs = String(left).split(".").map((part) => Number.parseInt(part, 10) || 0);
- const rhs = String(right).split(".").map((part) => Number.parseInt(part, 10) || 0);
+ const normalizedLeft = normalizeOpenshellVersion(left);
+ const normalizedRight = normalizeOpenshellVersion(right);
+ if (!normalizedLeft || !normalizedRight) return 0;
+ const lhs = normalizedLeft.split(".").map((part) => Number.parseInt(part, 10) || 0);
+ const rhs = normalizedRight.split(".").map((part) => Number.parseInt(part, 10) || 0);
const length = Math.max(lhs.length, rhs.length);
for (let index = 0; index < length; index += 1) {
const a = lhs[index] || 0;
@@
function getMinimumOpenshellVersion(blueprintText = null) {
- if (blueprintText == null) {
- return getMinimumOpenshellVersionFromFile(
- path.join(ROOT, "nemoclaw-blueprint", "blueprint.yaml"),
- );
- }
- return parseMinimumOpenshellVersion(String(blueprintText));
+ const parsed =
+ blueprintText == null
+ ? getMinimumOpenshellVersionFromFile(
+ path.join(ROOT, "nemoclaw-blueprint", "blueprint.yaml"),
+ )
+ : parseMinimumOpenshellVersion(String(blueprintText));
+ return normalizeOpenshellVersion(parsed) || DEFAULT_MIN_OPENSHELL_VERSION;
}
function shouldUpgradeOpenshell(installedVersion = null, minimumVersion = null) {
- if (!installedVersion || !minimumVersion) return false;
- return compareVersions(installedVersion, minimumVersion) < 0;
+ const normalizedMinimum = normalizeOpenshellVersion(minimumVersion);
+ if (!normalizedMinimum) return false;
+ const normalizedInstalled = normalizeOpenshellVersion(installedVersion);
+ if (!normalizedInstalled) return true;
+ return compareVersions(normalizedInstalled, normalizedMinimum) < 0;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 373 - 398, Normalize and more robustly parse
version strings before comparison: in compareVersions and
getMinimumOpenshellVersion call-sites strip common prefixes like leading "v"
(e.g., .replace(/^v/i, "") and .trim()), split on non-digit separators if
needed, and coerce parts with Number.parseInt(part, 10) falling back to 0 for
NaN; then update shouldUpgradeOpenshell(installedVersion, minimumVersion) to
treat an unparseable or empty installedVersion as "requires upgrade" (return
true) rather than false so preflight won't silently skip upgrades. Use the
function names compareVersions, getMinimumOpenshellVersion, and
shouldUpgradeOpenshell to find where to apply these changes.
| console.error( | ||
| ` Failed to upgrade openshell CLI to meet NemoClaw's minimum ${requiredOpenshellVersion}.`, | ||
| ); | ||
| console.error(" Install manually: https://github.com/NVIDIA/OpenShell/releases"); | ||
| process.exit(1); |
There was a problem hiding this comment.
Replace newly added GitHub release link with an allowed docs link/path.
The newly added URL at Line 1581 points to a third-party code repository and conflicts with the repository link policy for JS files.
As per coding guidelines, "Do not add links to third-party code repositories, community collections, or unofficial resources; only link to official tool documentation (Node.js, Python, uv)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 1578 - 1582, The console.error message that
prints the GitHub releases URL should be replaced with an allowed official docs
link or an internal docs path; locate the block that references
requiredOpenshellVersion and the two console.error calls (the one with the
GitHub URL) and change the second console.error string from
"https://github.com/NVIDIA/OpenShell/releases" to the approved documentation
URL/path (e.g., an internal docs page or official tool documentation) and keep
the surrounding message and the process.exit(1) behavior unchanged.
Fixes NVIDIA#1404 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Fixes NVIDIA#1404 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Fixes NVIDIA#1404 Signed-off-by: Deepak Jain <deepujain@gmail.com>
deepujain
force-pushed
the
fix/1404-refresh-openshell-on-upgrade
branch
from
30e87fb to
b6d8a3c
Compare
|
Rebased this onto current main and ported the OpenShell upgrade path through the TS-era onboard flow. The branch now uses the shared minimum-version loader, keeps the current blueprint min/max checks, and both plus pass locally. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
1935-1956: Consider extracting version enforcement into a helper function.The min/max version enforcement blocks (lines 1935-1956 for min, 1962-1979 for max) share similar structure. This is acceptable given the distinct error messages, but could be consolidated if this pattern is reused elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 1935 - 1956, Extract the duplicated version-check-and-fail logic into a helper like enforceVersionConstraint(currentVersionGetter, requiredVersionGetter, comparatorFn, failureMessageBuilder) and replace the min/max blocks that use getInstalledOpenshellVersion, getBlueprintMinOpenshellVersion/getBlueprintMaxOpenshellVersion and versionGte/versionLte with calls to this helper; the helper should call the provided getters, run the comparator (e.g., versionGte/versionLte), and when the check fails emit the same series of console.error lines (including TROUBLESHOOTING_GUIDE_URL and the remove-binary hint) and call process.exit(1), so the error text remains identical while removing duplicated structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 1935-1956: Extract the duplicated version-check-and-fail logic
into a helper like enforceVersionConstraint(currentVersionGetter,
requiredVersionGetter, comparatorFn, failureMessageBuilder) and replace the
min/max blocks that use getInstalledOpenshellVersion,
getBlueprintMinOpenshellVersion/getBlueprintMaxOpenshellVersion and
versionGte/versionLte with calls to this helper; the helper should call the
provided getters, run the comparator (e.g., versionGte/versionLte), and when the
check fails emit the same series of console.error lines (including
TROUBLESHOOTING_GUIDE_URL and the remove-binary hint) and call process.exit(1),
so the error text remains identical while removing duplicated structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 97ec2350-aefc-47ad-864f-6e5ac0b5eb92
📒 Files selected for processing (3)
scripts/install-openshell.shscripts/lib/openshell-version.jssrc/lib/onboard.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/lib/openshell-version.js
Fixes NVIDIA#1404 Signed-off-by: Deepak Jain <deepujain@gmail.com>
deepujain
force-pushed
the
fix/1404-refresh-openshell-on-upgrade
branch
from
b6d8a3c to
209c29c
Compare
|
Rebased this onto the latest main and folded the duplicated OpenShell version checks into a shared helper. npm run build:cli plus npm test -- test/onboard.test.ts test/install-openshell.test.ts pass locally. |
|
Thanks for the stale OpenShell upgrade fix. This was addressed by |
wscurran
added
area: cli
wscurran
removed
NemoClaw CLI
feature
2 participants
Summary
Fixes #1404.
NemoClaw already declares a minimum supported OpenShell version in the blueprint, but onboarding only installed OpenShell when the CLI was missing. If an older CLI was already present, upgrading NemoClaw could leave that stale OpenShell binary in place and keep using it.
Changes
bin/lib/onboard.jsto:min_openshell_versionfrom the blueprintscripts/lib/openshell-version.jsso onboarding and the installer resolve the minimum OpenShell version with the same parsing and fallback behavior.scripts/install-openshell.shto use that shared parser when available, with the existing shell fallback and env override path intact.test/onboard.test.jsfor the new minimum-version helpers.test/install-openshell.test.jsto verify the shell installer honors the blueprint minimum and explicit override path without attempting a network download.Testing
npm run build:clinpm test -- test/onboard.test.js test/install-openshell.test.jsEvidence it works
test/onboard.test.jsnow covers the version comparison path that decides whether OpenShell needs an upgrade.test/install-openshell.test.jsverifies that the shell installer reads the blueprint minimum version and respects an explicit minimum-version override.npm testsuite in this worktree. The branch-specific tests are green, but the broader suite still hit unrelated existing failures in:src/lib/preflight.test.tstest/install-preflight.test.jsSigned-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
Release Notes
New Features
Refactor