Fix uninstall.sh for curl-to-bash execution - #426
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:
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as nemoclaw CLI
participant FS as Local FS
participant Shell as Shell
participant Remote as Remote URL
rect rgba(200,200,255,0.5)
User->>CLI: run `nemoclaw uninstall [args]`
CLI->>FS: resolveUninstallScript() (check `ROOT/uninstall.sh`, `../uninstall.sh`)
end
alt local script found
CLI->>Shell: spawnSync("bash", ["<localScript>", ...args]) (inherit stdio)
Shell-->>CLI: exit status
CLI-->>User: exit with status
else no local script
CLI->>Remote: build `curl -fsSL <url> | bash` with shell-quoted args
CLI->>Shell: spawn remote command (bash -c ...)
Shell-->>Remote: fetch & pipe script
Remote-->>Shell: script stream
Shell-->>CLI: exit status
CLI-->>User: exit with status
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)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@uninstall.sh`:
- Line 457: The current guard calling main runs even when the file is sourced in
an interactive shell because $0 == "bash"/"-bash"; update the guard to call main
only when the script is being executed (when "${BASH_SOURCE[0]}" = "$0") OR when
BASH_SOURCE is unset (meaning input came from stdin/pipe) and $0 is "bash" or
"-bash"; specifically replace the existing if condition with a compound check
that requires either BASH_SOURCE[0] == $0 OR (BASH_SOURCE[0] is unset AND ($0 ==
"bash" OR $0 == "-bash")), so interactive sourcing (where BASH_SOURCE is set to
the sourced filename) will not trigger main.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: deeda9cc-c9f6-4d43-86ce-d86411b0fc0e
📒 Files selected for processing (1)
uninstall.sh
|
Verification update: Validated the curl-to-bash uninstall flow on the fix branch using a sanitized command form: curl -fsSL https://raw.githubusercontent.com/NVIDIA/NemoClaw/refs/heads/fix/uninstall-stdin-entrypoint/uninstall.sh | bashObserved behavior:
Representative output confirmed the expected sequence:
Local machine/user-specific prompt prefixes and paths omitted from this note. |
|
Additional verification update: Also validated the same stdin execution path on macOS with the fix branch: curl -fsSL https://raw.githubusercontent.com/NVIDIA/NemoClaw/refs/heads/fix/uninstall-stdin-entrypoint/uninstall.sh | bashObserved behavior on macOS:
This matches the previously verified stdin behavior on Linux and confirms the prompt handling works in both environments. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
README.md (2)
82-84: Align the console block with markdownlint MD014The snippet at Line 83 uses
$prompts without output, which triggers MD014 in many markdownlint setups. Either remove$or include expected output.Proposed lint-friendly edit
-```console -$ curl -fsSL https://raw.githubusercontent.com/NVIDIA/NemoClaw/refs/heads/main/uninstall.sh | bash -``` +```bash +curl -fsSL https://raw.githubusercontent.com/NVIDIA/NemoClaw/refs/heads/main/uninstall.sh | bash +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 82 - 84, The markdown snippet includes a console block with a leading "$" prompt which triggers markdownlint MD014; update the fenced code block around the curl command (the block containing "curl -fsSL https://raw.githubusercontent.com/NVIDIA/NemoClaw/refs/heads/main/uninstall.sh | bash") to be lint-friendly by removing the "$" prompt and changing the fence language to bash (i.e., replace the "console" fence and "$ curl..." line with a "bash" fence followed by "curl -fsSL ... | bash" so the example contains no prompts).
83-83: Prefer an immutable uninstall URL in curl-to-bash docsLine 83 points to
refs/heads/main, which is mutable. For safer reproducibility, document a tagged release (or commit-pinned) URL for the uninstall script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 83, The curl command in README.md uses a mutable refs/heads/main URL; update that example to point to an immutable release or commit (e.g., use refs/tags/<release-tag> or refs/commits/<commit-sha>) so the uninstall script URL is pinned and reproducible—replace the existing curl URL string shown on the line with one that references a specific tag or commit SHA and mention the chosen tag/sha in the README example.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Around line 82-84: The markdown snippet includes a console block with a
leading "$" prompt which triggers markdownlint MD014; update the fenced code
block around the curl command (the block containing "curl -fsSL
https://raw.githubusercontent.com/NVIDIA/NemoClaw/refs/heads/main/uninstall.sh |
bash") to be lint-friendly by removing the "$" prompt and changing the fence
language to bash (i.e., replace the "console" fence and "$ curl..." line with a
"bash" fence followed by "curl -fsSL ... | bash" so the example contains no
prompts).
- Line 83: The curl command in README.md uses a mutable refs/heads/main URL;
update that example to point to an immutable release or commit (e.g., use
refs/tags/<release-tag> or refs/commits/<commit-sha>) so the uninstall script
URL is pinned and reproducible—replace the existing curl URL string shown on the
line with one that references a specific tag or commit SHA and mention the
chosen tag/sha in the README example.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/nemoclaw.js`:
- Around line 182-200: The current exit logic uses process.exit(result.status ||
0) which treats a null result.status (e.g., subprocess killed by signal or spawn
failure) as success; update the termination handling in the blocks that call
spawnSync (the variables named result from spawnSync and the subsequent
process.exit calls) to: if result.status !== null then exit with result.status;
else if result.signal is set then exit with a nonzero code derived from the
signal (common convention: 128 + signal number) or at minimum a distinct nonzero
code; otherwise exit with 1 to indicate failure. Ensure you reference the
spawnSync return object’s status and signal fields and replace the current
process.exit(...) use with this three-way logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65ad17df-97d5-42a0-85ec-55406e770482
📒 Files selected for processing (1)
bin/nemoclaw.js
|
@ericksoa @jacobtomlinson or @liveaverage could I get a review on this to allow curl based uninstalls and a convenience wrapper in nemoclaw cli |
liveaverage
left a comment
liveaverage
left a comment
There was a problem hiding this comment.
Ran on Spark and Brev with success. Correctly resolved local uninstall.sh when present and fallback to remote uninstall.sh worked as expected
* fix uninstall script stdin entrypoint guard * fix uninstall entrypoint guard when sourced from bash * fix uninstall cleanup order and tty prompt * docs add curl uninstall command to readme * feat add nemoclaw uninstall wrapper * fix uninstall docs pin and cli exit handling * docs restore main uninstall url in readme * docs remove shell prompts from readme commands * docs match uninstall code fence style
* fix uninstall script stdin entrypoint guard * fix uninstall entrypoint guard when sourced from bash * fix uninstall cleanup order and tty prompt * docs add curl uninstall command to readme * feat add nemoclaw uninstall wrapper * fix uninstall docs pin and cli exit handling * docs restore main uninstall url in readme * docs remove shell prompts from readme commands * docs match uninstall code fence style
4 participants
Summary
set -ucurl -fsSL ... | bashTesting
bash -n uninstall.shCloses #425
Summary by CodeRabbit