fix(install): keep nemoclaw available after install#1468
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughInstaller now marks versioned payload runs with 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 docstrings
🧪 Generate unit tests (beta)
Comment |
cv
approved these changes
Apr 4, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install.sh (1)
455-486:⚠️ Potential issue | 🟠 MajorUpdate uninstall.sh to handle script-based shims.
The shim is now created as an executable script (lines 479-486), but
uninstall.shonly removes symlinks (line 341:[ -L ... ]). Regular files matching the shim name trigger the warning "Leaving ... in place because it is not an installer-managed shim." (line 344) instead of being removed, leaving installer-created shims behind after uninstallation.Add logic to detect and remove script-based shims, for example by checking for the
#!/usr/bin/env bashmarker or another identifier that distinguishes installer-managed shims from user-created files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 455 - 486, The uninstall script currently only removes symlink shims and leaves installer-created script shims behind; update uninstall.sh to detect and remove regular files created by ensure_nemoclaw_shim by checking for the installer marker (e.g. the shebang "#!/usr/bin/env bash" or the expected exec line that the shim writes) inside "$NEMOCLAW_SHIM_DIR/nemoclaw" (or the same path used by ensure_nemoclaw_shim) — ensure you only remove the file when it is a regular file and its contents match the installer-created shim pattern, otherwise keep the existing behavior of leaving user-created files and emitting the same warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/install.sh`:
- Around line 455-486: The uninstall script currently only removes symlink shims
and leaves installer-created script shims behind; update uninstall.sh to detect
and remove regular files created by ensure_nemoclaw_shim by checking for the
installer marker (e.g. the shebang "#!/usr/bin/env bash" or the expected exec
line that the shim writes) inside "$NEMOCLAW_SHIM_DIR/nemoclaw" (or the same
path used by ensure_nemoclaw_shim) — ensure you only remove the file when it is
a regular file and its contents match the installer-created shim pattern,
otherwise keep the existing behavior of leaving user-created files and emitting
the same warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2959ebde-d097-4a5c-a8bc-c557241937e5
📒 Files selected for processing (3)
install.shscripts/install.shtest/install-preflight.test.js
Contributor
Author
|
Addressing Coderabbit review |
Contributor
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`:
- Around line 329-345: The is_installer_managed_nemoclaw_shim function currently
allows extra content before/after the three expected lines, causing false
positives; tighten the matcher so the file must consist exactly of the
three-line structure by anchoring the pattern to the whole contents (remove the
leading/trailing wildcards and only allow the intended intraline wildcards),
e.g. change the case pattern in is_installer_managed_nemoclaw_shim to match
'^#!/usr/bin/env
bash\n'"$path_line"*"$path_suffix"'\n'"$exec_line"*"$exec_suffix"'$' (i.e. no
extra '*' before/after) so only exact three-line installer-generated shims are
detected and user wrappers are preserved.
🪄 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: e83e39a7-526e-45b0-8a28-b5ac8021f832
📒 Files selected for processing (2)
test/uninstall.test.jsuninstall.sh
ericksoa
pushed a commit
that referenced
this pull request
Apr 4, 2026
…ty (#1470) ## Summary - unify installer and onboarding host detection around shared TypeScript preflight logic - move `deploy` behavior into TypeScript, thin the Brev compatibility wrapper, and harden Brev readiness handling - demote or remove legacy platform-specific setup paths (`setup-spark`, `brev-setup.sh`) in favor of the canonical installer + onboard flow - update docs, CLI help, and Brev E2E coverage to match the new behavior ## What Changed - added shared host assessment and remediation planning in `src/lib/preflight.ts` - wired installer and onboard flows to the same host preflight decisions - changed Podman handling from hard block to unsupported-runtime warning - migrated deploy logic into `src/lib/deploy.ts` - updated `nemoclaw deploy` to use the authenticated Brev CLI, current Brev create flags, explicit GCP provider default, stricter readiness checks, and standard installer/onboard flow - removed `scripts/setup-spark.sh` and reduced `scripts/brev-setup.sh` to a deprecated compatibility wrapper - updated README/docs/help text and hardened the Brev E2E cleanup path ## Validation - `npm run build:cli` - targeted Vitest coverage for `src/lib/preflight.test.ts`, `src/lib/deploy.test.ts`, `test/install-preflight.test.js`, `test/cli.test.js`, `test/runner.test.js` - live Brev validation with `TEST_SUITE=deploy-cli` on `cpu-e2.4vcpu-16gb` - confirmed successful end-to-end remote deploy after waiting for Brev `status=RUNNING`, `build_status=COMPLETED`, `shell_status=READY` ## Related Issues - Fixes #1377 - Addresses #1330 - Addresses #1390 - Related to #1404 ## Credit / Prior Work This branch builds on ideas and prior work from: - #1368 by @zyang-dev for simplifying Spark setup and removing the old cgroup workaround - #1395 and #1468 by @kjw3 for the thin installer/bootstrap direction and installer path reliability - #1450 by @cjagwani for switching Brev flows toward GCP for reliability - #1383 by @13ernkastel for the current Brev create flag compatibility work - #1364 by @WuKongAI-CMU for deploy sync-path fixes - #1362 and #1266 by @jyaunches for the Brev E2E/launchable infrastructure direction - issue ideas from #1377 and #1404 by @zNeill, #1330 by @Marcelo5444, and #1390 by @ericksoa <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved host diagnostics with actionable remediation guidance surfaced during installer/onboard preflight. * **Improvements** * macOS (Intel) now recommends Docker Desktop; DGX Spark guidance now uses the standard installer + `nemoclaw onboard`. * Preflight output shows detected runtime and WSL notes; installer prints remediation actions and will skip onboarding on blocking issues. * **Deprecations** * `nemoclaw deploy`, `nemoclaw setup-spark`, and the legacy bootstrap wrapper are now deprecated compatibility paths. * **Documentation** * Quickstart, troubleshooting, and command reference updated to reflect installer+onboard flow and deprecation guidance. * **Tests** * Added/updated tests covering preflight, deploy compatibility, CLI aliases, and deploy e2e scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
2 participants
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.
Summary
Fixes a CLI install regression where
nemoclawwas reported as installed but the command was broken after install completed.This changes the installer so:
~/.local/bin/nemoclawwrapperPATHbefore execing the installed CLIcurl | bashinstalls no longernpm linka temporary cloned checkoutRoot Cause
There were two installer-side problems:
The thin bootstrap
install.shcloned the selected ref into a temporary directory and then executedscripts/install.shfrom that clone. The payload installer treated that temp clone as a source checkout and rannpm link, which created a global package symlink into the temp directory. Once the bootstrap temp directory was cleaned up,nemoclawwas broken.Even when the CLI binary existed, future shells could still fail to run it because the shebang relied on
nodebeing onPATH. On hosts usingnvm, sourcingnvm.shwas not sufficient to guarantee that a Node version was active in later shells.Related Issues
Related: #569, #989, #1429
Notes:
#569is the closest installer/runtime-path issue.#989and#1429track the developer-doc path that requiresnpm link.#989or#1429.Related Prior Work
This overlaps with earlier installer investigation/work in:
#485by @afurm#517by @MilanKieleThose PRs helped surface the same general npm/prefix/PATH fragility from adjacent angles.
What Changed
install.shNEMOCLAW_BOOTSTRAP_PAYLOAD=1scripts/install.sh~/.local/bin/nemoclawwrapperPATHand execs the installednemoclawis_source_checkout()now rejects bootstrap payload clonestest/install-preflight.test.jsSupported Paths
This PR fixes the fully supported installer paths:
curl | bash./install.shDeveloper repo usage remains:
git clonenpm installnpm linknemoclaw onboardPlain
npm installalone is not treated as a supported CLI install path.Validation
Local:
npm test -- test/install-preflight.test.jsLive:
brev-cpukj-nemoclaw-cpu-test./install.sh --non-interactivecurl | bashcommand -v nemoclawandnemoclaw --versionsparkspark-d8c8./install.sh --non-interactivecommand -v nemoclawandnemoclaw --versionNotes
I only live-tested non-interactive installs on this branch. Interactive install behavior was not live-retested here.
Summary by CodeRabbit
Bug Fixes / Improvements
Tests