fix(onboard): enforce nemoclaw-blueprint min_openshell_version at preflight (#1317)

[ColinM-sys]

Summary

Closes #1317.

nemoclaw-blueprint/blueprint.yaml declares min_openshell_version: "0.1.0" but neither install.sh nor nemoclaw onboard ever validate the installed OpenShell against it. Users can complete a full install + onboard against openshell 0.0.20, then hit silent failures inside the sandbox at runtime when NemoClaw tries to call CLI surface that doesn't exist in the older binary (e.g. sandbox exec, --upload, divergent device-pairing behavior). The reporter saw a successful onboard followed by features quietly breaking with no clear cause.

Fix

Add the missing preflight check.

bin/lib/onboard.js

Two new helpers:

• versionGte(left, right) — compare semver-like x.y.z strings, defaulting missing components to 0. Mirrors the helper already present in bin/nemoclaw.js (kept independent rather than refactored to avoid touching unrelated callers in this PR).
• getBlueprintMinOpenshellVersion(rootDir = ROOT) — read nemoclaw-blueprint/blueprint.yaml, extract min_openshell_version. Intentionally lenient: missing file, missing field, malformed YAML, non-string value, and off-shape strings (e.g. "latest") all return null. This is so a busted local blueprint can never become a hard onboard blocker — the check only fires when the constraint is unambiguously legible.

In preflight(), immediately after the existing openshell version detection, compare installed against the blueprint minimum. If both are present and the installed version is below the minimum, abort with an actionable upgrade message:

  ✗ openshell 0.0.20 is below the minimum required by this NemoClaw release.
    blueprint.yaml min_openshell_version: 0.1.0

    Upgrade openshell and retry:
      https://github.com/NVIDIA/OpenShell/releases
    Or remove the existing binary so the installer can re-fetch a current build:
      command -v openshell && rm -f "$(command -v openshell)"

The second hint is the important one: it gives the user a one-liner that lets the bundled installer fetch a current build on the next run, rather than forcing them to figure out the manual upgrade path.

test/onboard.test.js

Four regression tests:

1. versionGte ordering edge cases — equal, greater, lesser, missing components, empty string defaults.
2. getBlueprintMinOpenshellVersion happy path — point at a temp blueprint with min_openshell_version: "0.1.0", assert "0.1.0" comes back.
3. getBlueprintMinOpenshellVersion returns null on every reachable failure mode — missing directory, missing field, malformed YAML, non-string value (min_openshell_version: 1.5), off-shape string (min_openshell_version: "latest").
4. Sanity check against the real shipped blueprint — getBlueprintMinOpenshellVersion(repoRoot) must return a parseable x.y.z string. Catches a future edit that accidentally drops or breaks the field, at CI time rather than at a user's onboard time.

Test plan

• vitest run test/onboard.test.js -t "1317" — 4/4 new regression tests pass.
• vitest run test/onboard.test.js -t "openshell" — pre-existing openshell tests still pass; same 2 unrelated failures appear on main with or without this change (verified by stash + re-run).
• Sanity-ran the new helpers against the real shipped blueprint.yaml from this branch with a Node REPL: getBlueprintMinOpenshellVersion() returns "0.1.0", and feeding "openshell 0.0.20" into the same getInstalledOpenshellVersion() → versionGte() chain that preflight() uses correctly returns false (would abort), while "openshell 0.1.0", "0.1.5", and "1.0.0" correctly return true (would allow onboard).
• Not verified: running nemoclaw onboard against an actual openshell 0.0.20 install on a Linux box. The helpers are unit-tested in isolation and the wiring point in preflight() reuses the same version output already captured for the existing display line, so the integration should follow, but a maintainer or reporter with the original repro environment is welcome to confirm.

Why this approach over silently upgrading openshell automatically

install.sh already attempts to install openshell when none is present, but it does not currently replace an existing-but-too-old binary, and silently overwriting a binary the user installed manually is intrusive enough that it deserves a separate design discussion. The error message in this PR points users at both the manual upgrade page and a one-liner that removes the stale binary so the existing installer logic re-fetches a current build on the next run — effectively letting them opt into the auto-upgrade path with a single command, without making that the default behavior.

Summary by CodeRabbit

• New Features

  • Added OpenShell version validation during onboarding to ensure the installed version meets minimum requirements; onboarding will fail with a clear error message if compatibility requirements aren't met.

• Tests

  • Added comprehensive test coverage for version comparison logic and blueprint configuration validation.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds OpenShell version validation to the onboarding preflight checks. It introduces semver comparison utilities and a blueprint reader to enforce minimum OpenShell version requirements, failing onboarding with an error if the installed version is below the configured minimum.

Changes

Cohort / File(s)	Summary
Version validation & comparison bin/lib/onboard.js	Added versionGte() semver comparison utility and getBlueprintMinOpenshellVersion() function to read and validate min_openshell_version from blueprint.yaml. Updated preflight() to capture installed OpenShell version, compare against blueprint minimum, and hard-fail onboarding with process.exit(1) if version is insufficient.
Test coverage test/onboard.test.js	Added regression tests for versionGte() behavior with equal, greater, and lesser versions, treating missing components as 0. Added tests for getBlueprintMinOpenshellVersion() covering success cases and graceful null returns for missing/malformed blueprint, directory absence, and invalid version formats. Included sanity check validating repository's shipped blueprint contains parseable min_openshell_version.

Sequence Diagram

sequenceDiagram
    participant Onboard as Onboard Process
    participant Preflight as Preflight Step
    participant CLI as OpenShell CLI
    participant FS as File System
    participant Compare as Version Comparison
    
    Onboard->>Preflight: Start preflight checks
    Preflight->>CLI: Execute 'openshell --version'
    CLI-->>Preflight: Return version string (e.g., 0.0.20)
    Preflight->>FS: Read blueprint.yaml
    FS-->>Preflight: Return YAML content
    Preflight->>Preflight: Parse installed version
    Preflight->>Preflight: Extract min_openshell_version from blueprint
    Preflight->>Compare: Compare(installed, minimum)
    alt Version >= Minimum
        Compare-->>Preflight: true
        Preflight-->>Onboard: Continue onboarding
    else Version < Minimum
        Compare-->>Preflight: false
        Preflight->>Onboard: Exit with error message
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A blueprint reads its wishes fine,
"Dear OpenShell, version must align!"
When too-old shells dare to appear,
We stop the dance and make it clear.
Compare and fail with grace, not fright—
Compatibility made right! 🔧✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the primary change: enforcing nemoclaw-blueprint's min_openshell_version in the preflight check, which is the core objective of this PR.
Linked Issues check	✅ Passed	The PR fully implements the primary objective from issue #1317: nemoclaw onboard preflight now compares installed OpenShell version against min_openshell_version and fails with an actionable upgrade message when below minimum.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the primary objective: adding versionGte and getBlueprintMinOpenshellVersion utilities, integrating the check into preflight, and adding comprehensive regression tests.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

test/onboard.test.js (1)

337-396: Add one malformed-shape case with a numeric prefix (e.g. 0.1.0-rc1).

That case protects against prefix-matching validation and ensures malformed-but-prefix-valid values are treated as null (non-blocking), which is the intended lenient behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/onboard.test.js` around lines 337 - 396, Add a new test case in the same
spec that creates a temp "nemoclaw-blueprint" directory and writes a
blueprint.yaml with min_openshell_version: "0.1.0-rc1" (a numeric prefix plus
suffix), then call getBlueprintMinOpenshellVersion(theTempDir) and assert it
returns null, finally removing the temp dir in a finally block; this follows the
pattern used for badShapeDir and ensures getBlueprintMinOpenshellVersion treats
prefix-like malformed values (e.g. "0.1.0-rc1") as null.

🤖 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 388-395: The versionGte function currently uses parseInt which
partially accepts non-numeric suffixes; instead validate each dot-separated
segment with a strict /^\d+$/ check and consider a version malformed if any
segment fails that test; then implement lenient/null behavior: if the minimum
version (the right parameter, e.g., min_openshell_version) is malformed, return
true to avoid enforcing a bogus minimum, and if the tested version (left) is
malformed return false (cannot confirm it meets the minimum). Update versionGte
(and the same logic used around the code at the other affected spots) to perform
these strict numeric checks before comparing segments.

---

Nitpick comments:
In `@test/onboard.test.js`:
- Around line 337-396: Add a new test case in the same spec that creates a temp
"nemoclaw-blueprint" directory and writes a blueprint.yaml with
min_openshell_version: "0.1.0-rc1" (a numeric prefix plus suffix), then call
getBlueprintMinOpenshellVersion(theTempDir) and assert it returns null, finally
removing the temp dir in a finally block; this follows the pattern used for
badShapeDir and ensures getBlueprintMinOpenshellVersion treats prefix-like
malformed values (e.g. "0.1.0-rc1") as null.

🪄 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: 6733bb0e-2351-4c1f-814f-728af2e20ac5

📥 Commits

Reviewing files that changed from the base of the PR and between f07c788 and 4426c84.

📒 Files selected for processing (2)

• bin/lib/onboard.js
• test/onboard.test.js

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Tighten version parsing/validation so malformed blueprint values can’t accidentally enforce a minimum.

Right now, min_openshell_version values with a valid prefix (e.g. 0.1.0-foo) pass the regex and can still influence gating. That conflicts with the intended “lenient/null on malformed” behavior. Also, parseInt partially parses non-numeric parts, which doesn’t match the function comment.

💡 Proposed fix

 function versionGte(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 toParts = (value) =>
+    String(value)
+      .split(".")
+      .map((part) => (/^[0-9]+$/.test(part) ? Number(part) : 0));
+  const lhs = toParts(left);
+  const rhs = toParts(right);
   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 (!/^[0-9]+\.[0-9]+\.[0-9]+/.test(trimmed)) return null;
+    if (!/^[0-9]+\.[0-9]+\.[0-9]+$/.test(trimmed)) return null;
     return trimmed;

Also applies to: 425-426

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bin/lib/onboard.js` around lines 388 - 395, The versionGte function currently
uses parseInt which partially accepts non-numeric suffixes; instead validate
each dot-separated segment with a strict /^\d+$/ check and consider a version
malformed if any segment fails that test; then implement lenient/null behavior:
if the minimum version (the right parameter, e.g., min_openshell_version) is
malformed, return true to avoid enforcing a bogus minimum, and if the tested
version (left) is malformed return false (cannot confirm it meets the minimum).
Update versionGte (and the same logic used around the code at the other affected
spots) to perform these strict numeric checks before comparing segments.

[wscurran]

✨ Thanks for submitting this fix, which proposes a way to enforce minimum OpenShell version requirements during preflight by reading the blueprint constraint. This helps prevent silent feature failures caused by outdated installations.