Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 76 additions & 3 deletions bin/lib/onboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,54 @@ function getInstalledOpenshellVersion(versionOutput = null) {
return match[1];
}

/**
* Compare two semver-like x.y.z strings. Returns true iff `left >= right`.
* Non-numeric or missing components are treated as 0.
*/
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 length = Math.max(lhs.length, rhs.length);
Comment on lines +388 to +395
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

for (let index = 0; index < length; index += 1) {
const a = lhs[index] || 0;
const b = rhs[index] || 0;
if (a > b) return true;
if (a < b) return false;
}
return true;
}

/**
* Read `min_openshell_version` from nemoclaw-blueprint/blueprint.yaml. Returns
* null if the blueprint or field is missing or unparseable — callers must
* treat null as "no constraint configured" so a malformed install does not
* become a hard onboard blocker. See #1317.
*/
function getBlueprintMinOpenshellVersion(rootDir = ROOT) {
try {
// Lazy require: yaml is already a dependency via bin/lib/policies.js but
// pulling it at module load would slow down `nemoclaw --help` for users
// who never reach the preflight path.
// eslint-disable-next-line @typescript-eslint/no-require-imports
const YAML = require("yaml");
const blueprintPath = path.join(rootDir, "nemoclaw-blueprint", "blueprint.yaml");
if (!fs.existsSync(blueprintPath)) return null;
const raw = fs.readFileSync(blueprintPath, "utf8");
const parsed = YAML.parse(raw);
const value = parsed && parsed.min_openshell_version;
if (typeof value !== "string") return null;
const trimmed = value.trim();
if (!/^[0-9]+\.[0-9]+\.[0-9]+/.test(trimmed)) return null;
return trimmed;
} catch {
return null;
}
}

function getStableGatewayImageRef(versionOutput = null) {
const version = getInstalledOpenshellVersion(versionOutput);
if (!version) return null;
Expand Down Expand Up @@ -1559,9 +1607,32 @@ async function preflight() {
process.exit(1);
}
}
console.log(
` ✓ openshell CLI: ${runCaptureOpenshell(["--version"], { ignoreError: true }) || "unknown"}`,
);
const openshellVersionOutput = runCaptureOpenshell(["--version"], { ignoreError: true });
console.log(` ✓ openshell CLI: ${openshellVersionOutput || "unknown"}`);
// Enforce nemoclaw-blueprint/blueprint.yaml's min_openshell_version. Without
// this check, users can complete a full onboard against an OpenShell that
// pre-dates required CLI surface (e.g. `sandbox exec`, `--upload`) and hit
// silent failures inside the sandbox at runtime. See #1317.
const installedOpenshellVersion = getInstalledOpenshellVersion(openshellVersionOutput);
const minOpenshellVersion = getBlueprintMinOpenshellVersion();
if (
installedOpenshellVersion &&
minOpenshellVersion &&
!versionGte(installedOpenshellVersion, minOpenshellVersion)
) {
console.error("");
console.error(
` ✗ openshell ${installedOpenshellVersion} is below the minimum required by this NemoClaw release.`,
);
console.error(` blueprint.yaml min_openshell_version: ${minOpenshellVersion}`);
console.error("");
console.error(" Upgrade openshell and retry:");
console.error(" https://github.com/NVIDIA/OpenShell/releases");
console.error(" Or remove the existing binary so the installer can re-fetch a current build:");
console.error(" command -v openshell && rm -f \"$(command -v openshell)\"");
console.error("");
process.exit(1);
}
if (openshellInstall.futureShellPathHint) {
console.log(
` Note: openshell was installed to ${openshellInstall.localBin} for this onboarding run.`,
Expand Down Expand Up @@ -4159,6 +4230,8 @@ module.exports = {
getNavigationChoice,
getSandboxInferenceConfig,
getInstalledOpenshellVersion,
getBlueprintMinOpenshellVersion,
versionGte,
getRequestedModelHint,
getRequestedProviderHint,
getStableGatewayImageRef,
Expand Down
104 changes: 104 additions & 0 deletions test/onboard.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import {
getFutureShellPathHint,
getSandboxInferenceConfig,
getInstalledOpenshellVersion,
getBlueprintMinOpenshellVersion,
versionGte,
getRequestedModelHint,
getRequestedProviderHint,
getRequestedSandboxNameHint,
Expand Down Expand Up @@ -301,6 +303,108 @@ describe("onboard helpers", () => {
});
});

it("regression #1317: versionGte handles equal, greater, and lesser semvers", () => {
expect(versionGte("0.1.0", "0.1.0")).toBe(true);
expect(versionGte("0.1.0", "0.0.20")).toBe(true);
expect(versionGte("0.0.20", "0.1.0")).toBe(false);
expect(versionGte("1.2.3", "1.2.4")).toBe(false);
expect(versionGte("1.2.4", "1.2.3")).toBe(true);
expect(versionGte("0.0.21", "0.0.20")).toBe(true);
// Defensive: missing components default to 0
expect(versionGte("1.0", "1.0.0")).toBe(true);
expect(versionGte("", "0.0.0")).toBe(true);
});

it("regression #1317: getBlueprintMinOpenshellVersion reads min_openshell_version from blueprint.yaml", () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-blueprint-min-version-"));
const blueprintDir = path.join(tmpDir, "nemoclaw-blueprint");
fs.mkdirSync(blueprintDir, { recursive: true });
fs.writeFileSync(
path.join(blueprintDir, "blueprint.yaml"),
[
'version: "0.1.0"',
'min_openshell_version: "0.1.0"',
'min_openclaw_version: "2026.3.0"',
].join("\n"),
);
try {
expect(getBlueprintMinOpenshellVersion(tmpDir)).toBe("0.1.0");
} finally {
fs.rmSync(tmpDir, { recursive: true, force: true });
}
});

it("regression #1317: getBlueprintMinOpenshellVersion returns null on missing or unparseable blueprint", () => {
// Missing directory
const missingDir = path.join(os.tmpdir(), "nemoclaw-blueprint-missing-" + Date.now().toString());
expect(getBlueprintMinOpenshellVersion(missingDir)).toBe(null);

// Present file, missing field — must NOT block onboard
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-blueprint-no-field-"));
const blueprintDir = path.join(tmpDir, "nemoclaw-blueprint");
fs.mkdirSync(blueprintDir, { recursive: true });
fs.writeFileSync(
path.join(blueprintDir, "blueprint.yaml"),
'version: "0.1.0"\n',
);
try {
expect(getBlueprintMinOpenshellVersion(tmpDir)).toBe(null);
} finally {
fs.rmSync(tmpDir, { recursive: true, force: true });
}

// Present file, malformed YAML — must NOT throw, just return null
const badDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-blueprint-bad-yaml-"));
const badBlueprintDir = path.join(badDir, "nemoclaw-blueprint");
fs.mkdirSync(badBlueprintDir, { recursive: true });
fs.writeFileSync(path.join(badBlueprintDir, "blueprint.yaml"), "this is: : not valid: yaml: [");
try {
expect(getBlueprintMinOpenshellVersion(badDir)).toBe(null);
} finally {
fs.rmSync(badDir, { recursive: true, force: true });
}

// Present file, non-string value (yaml parses unquoted 1.5 as number) —
// must NOT block onboard, just return null
const wrongTypeDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-blueprint-wrong-type-"));
const wrongTypeBlueprintDir = path.join(wrongTypeDir, "nemoclaw-blueprint");
fs.mkdirSync(wrongTypeBlueprintDir, { recursive: true });
fs.writeFileSync(
path.join(wrongTypeBlueprintDir, "blueprint.yaml"),
"min_openshell_version: 1.5\n",
);
try {
expect(getBlueprintMinOpenshellVersion(wrongTypeDir)).toBe(null);
} finally {
fs.rmSync(wrongTypeDir, { recursive: true, force: true });
}

// Present file, string value that doesn't look like x.y.z — must NOT
// block onboard. Defends against typos like "latest" or "0.1".
const badShapeDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-blueprint-bad-shape-"));
const badShapeBlueprintDir = path.join(badShapeDir, "nemoclaw-blueprint");
fs.mkdirSync(badShapeBlueprintDir, { recursive: true });
fs.writeFileSync(
path.join(badShapeBlueprintDir, "blueprint.yaml"),
'min_openshell_version: "latest"\n',
);
try {
expect(getBlueprintMinOpenshellVersion(badShapeDir)).toBe(null);
} finally {
fs.rmSync(badShapeDir, { recursive: true, force: true });
}
});

it("regression #1317: shipped blueprint.yaml exposes a parseable min_openshell_version", () => {
// Sanity check against the real on-disk blueprint so a future edit that
// accidentally drops or breaks the field is caught by CI rather than at
// a user's onboard time.
const repoRoot = path.resolve(__dirname, "..");
const v = getBlueprintMinOpenshellVersion(repoRoot);
expect(v).not.toBe(null);
expect(/^[0-9]+\.[0-9]+\.[0-9]+/.test(v)).toBe(true);
});

it("pins the gateway image to the installed OpenShell release version", () => {
expect(getInstalledOpenshellVersion("openshell 0.0.12")).toBe("0.0.12");
expect(getInstalledOpenshellVersion("openshell 0.0.13-dev.8+gbbcaed2ea")).toBe("0.0.13");
Expand Down