feat: Add script to install the OpenShell CLI and a utility to resolve its binary path.#969
Open
Abmarne wants to merge 2 commits intoNVIDIA:mainfrom
Open
feat: Add script to install the OpenShell CLI and a utility to resolve its binary path.#969Abmarne wants to merge 2 commits intoNVIDIA:mainfrom
Abmarne wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
…e its binary path.
Contributor
📝 WalkthroughWalkthroughAdded runtime validation to ensure resolved Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer Script
participant Resolver as resolve-openshell.js
participant Candidate as Candidate Binary
participant Timeout as timeout/gtimeout
Installer->>Resolver: ask for `openshell` path
Resolver->>Candidate: attempt `command -v` or candidate path
alt candidate found
Resolver->>Timeout: run `Candidate -V` (2s)
Timeout->>Candidate: execute
Candidate-->>Timeout: stdout/stderr (maybe hangs)
Timeout-->>Resolver: output or timeout
Resolver->>Resolver: match /^openshell \d+\.\d+\.\d+/
alt match
Resolver-->>Installer: return candidate path
else no match / timeout / error
Resolver-->>Installer: reject candidate (continue search)
end
else no candidate
Resolver-->>Installer: no binary found
end
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}
Contributor
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/resolve-openshell.js`:
- Around line 24-33: The isRustCli function currently uses execSync with a
shell-interpolated command (`${path} -V`) which allows command injection and
breaks on paths with spaces; change to use execFileSync to invoke the binary
directly: add execFileSync to the child_process import (e.g., include
execFileSync alongside execSync) and replace the execSync call in isRustCli with
execFileSync(path, ["-V"], { encoding: "utf-8", stdio: ["ignore","pipe","pipe"],
timeout: 2000 }) so the version check runs without shell parsing and handles
spaced paths safely.
In `@scripts/install-openshell.sh`:
- Line 56: The unbounded probe VERSION_OUT="$(openshell -V 2>&1 || echo "")" can
hang; replace it with a bounded timeout call: attempt to use the GNU timeout
binary first (timeout -s KILL 2s ...) and fall back to gtimeout for BSD, running
openshell -V under that 2s timeout and capturing stdout/stderr into VERSION_OUT
(preserving the existing || echo "" fallback if the timeout/command fails);
update the assignment that uses VERSION_OUT so it calls the timeout wrapper
around the openshell -V invocation.
🪄 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: 919e053b-322e-4503-8193-78b12f435ba4
📒 Files selected for processing (2)
bin/lib/resolve-openshell.jsscripts/install-openshell.sh
… install the openshell CLI.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/resolve-openshell.js (1)
7-36: JSDoc forresolveOpenshellis now separated from its function.The original JSDoc block (lines 7-18) documents
resolveOpenshell, but the newisRustClifunction was inserted between them, creating visual confusion. Consider reorganizing:📝 Suggested reorganization
Move
isRustCliabove the JSDoc forresolveOpenshell, or place it afterresolveOpenshell(hoisting will still work):const { execSync, execFileSync } = require("child_process"); const fs = require("fs"); +/** + * Verify if the binary is the OpenShell Rust CLI (and not a shadowing NPM package). + * `@param` {string} path Absolute path to binary + * `@returns` {boolean} + */ +function isRustCli(path) { + try { + const output = execFileSync(path, ["-V"], { + encoding: "utf-8", + stdio: ["ignore", "pipe", "pipe"], + timeout: 2000, + }).trim(); + return /^openshell\s+[0-9]+\.[0-9]+\.[0-9]+/i.test(output); + } catch { + return false; + } +} + /** * Resolve the openshell binary path. * ... */ -/** - * Verify if the binary is the OpenShell Rust CLI... - */ -function isRustCli(path) { ... } - function resolveOpenshell(opts = {}) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/resolve-openshell.js` around lines 7 - 36, The JSDoc for resolveOpenshell is separated from its function by the newly inserted isRustCli, which causes visual confusion; fix it by relocating isRustCli so the docblock immediately precedes resolveOpenshell (either move isRustCli above the JSDoc or place it after the resolveOpenshell function), or alternatively move the JSDoc to sit directly above isRustCli and add a new JSDoc for resolveOpenshell—ensure the resolveOpenshell JSDoc and the resolveOpenshell function are adjacent and keep the isRustCli declaration near related utility functions.
🤖 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/resolve-openshell.js`:
- Around line 7-36: The JSDoc for resolveOpenshell is separated from its
function by the newly inserted isRustCli, which causes visual confusion; fix it
by relocating isRustCli so the docblock immediately precedes resolveOpenshell
(either move isRustCli above the JSDoc or place it after the resolveOpenshell
function), or alternatively move the JSDoc to sit directly above isRustCli and
add a new JSDoc for resolveOpenshell—ensure the resolveOpenshell JSDoc and the
resolveOpenshell function are adjacent and keep the isRustCli declaration near
related utility functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b55bf1e-eee5-4a81-99a3-8e97c5709497
📒 Files selected for processing (2)
bin/lib/resolve-openshell.jsscripts/install-openshell.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install-openshell.sh
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
1 participant
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.
Fixes: #967
I have identified and addressed the two core issues preventing successful NemoClaw installation and connectivity on Ubuntu 22.04 and 24.04: the cgroup v2 incompatibility on newer Ubuntu versions and the shadowing of the OpenShell binary by an NPM package of the same name.
Summary of Fixes
Resolved cgroup v2 Incompatibility (Ubuntu 24.04):
cgroupns=hoston Ubuntu 24.04 due to the default shift to cgroup v2.sudo ./scripts/setup-spark.sh(or manually configure/etc/docker/daemon.json) to adjust the Docker daemon configuration. This ensures the k3s cluster inside the gateway container can manage pod network namespaces correctly.Resolved OpenShell Binary Shadowing (NPM conflict):
PATH, causing NemoClaw to run the wrong binary and crash during step 3/7 of the onboarding.PATHis the Rust CLI (checked via-Vand specific version string regex) and not the Node.js package.openshell -Vfor version detection. It now correctly identifies shadowing NPM packages and proceeds with the CLI installation even if that name is already taken on thePATH.nemoclaw onboardnow uses the robust resolution logic, ensuring it properly detects and fixes shadowing issues during the initial setup.Detailed Actions Taken
Verification of Fix
With these changes, even if a user has the openshell NPM package installed,
nemoclaw onboardwill:~/.local/binor/usr/local/bin.Summary by CodeRabbit