fix: avoid partial install when usage notice isn't accepted - #2694
fix: avoid partial install when usage notice isn't accepted#2694BenediktSchackenberg wants to merge 3 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoved the in-run onboarding prompt by calling Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer as "scripts/install.sh"
participant Node as "Node.js runtime"
participant CLI as "CLI install / Phase 2"
User->>Installer: start installer
Installer->>Node: install/verify Node.js
Node-->>Installer: Node available
Installer->>Installer: show_usage_notice()
Installer->>CLI: proceed with CLI installation / Phase 2
CLI-->>Installer: CLI installed
Installer->>User: finish onboarding
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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 `@scripts/install.sh`:
- Around line 1368-1370: The call to show_usage_notice is executed before
Node.js is guaranteed to exist, causing failures because show_usage_notice
invokes node; move the invocation of show_usage_notice so it runs after
install_nodejs has completed (i.e., call show_usage_notice after the
install_nodejs function/step), or alternatively rewrite the show_usage_notice
implementation to avoid using node and make it shell-native so it can run before
install_nodejs.
🪄 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: Enterprise
Run ID: 52ee291b-5719-4308-b6c4-af89a3849849
📒 Files selected for processing (1)
scripts/install.sh
There was a problem hiding this comment.
Pull request overview
This PR moves the third-party software usage notice earlier in the installer flow to ensure rejection/non-interactive failure happens before later phases, and removes a redundant usage notice invocation during onboarding.
Changes:
- Remove
show_usage_noticefromrun_onboard(). - Add a single
show_usage_noticecall early inmain()(before phase 1 “Node.js”), with an explanatory comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Show usage notice before any install steps so non-interactive/license | ||
| # rejection paths don't leave a partially-installed NemoClaw on PATH. | ||
| show_usage_notice | ||
|
|
| bash "${SCRIPT_DIR}/setup-jetson.sh" | ||
|
|
||
| # Show usage notice before any install steps so non-interactive/license | ||
| # rejection paths don't leave a partially-installed NemoClaw on PATH. | ||
| show_usage_notice | ||
|
|
… path Copilot review on NVIDIA#2694 correctly flagged that show_usage_notice() calls node under the hood. Moving it before step 1 (install_nodejs) breaks fresh installs where Node isn't present yet. Fix: keep the non-interactive/acceptance path as a cheap bash-only guard before step 1 (no node required since no prompt is shown). For the interactive/TTY path, defer show_usage_notice to after install_nodejs so Node is always available when the notice UI runs. Either way, the notice fires before step 2 (NemoClaw CLI install), so a rejection still leaves no partial install state. Per Copilot review on PR NVIDIA#2694. Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
Fixed based on Copilot review:
Either way the check fires before Phase 2 (NemoClaw CLI install), so rejection still leaves no partial install state. |
|
✨ Thanks for submitting this PR that proposes a fix to avoid partial installs when the usage notice isn't accepted, which improves the installation flow by checking the acceptance requirement early. |
…efore phase 2 Previous approach (bash-only early check + deferred TTY check) was overly complex and still invoked show_usage_notice on a code path where Node might not be present. Simpler fix: move show_usage_notice to immediately after install_nodejs and ensure_supported_runtime, but before Phase 2 (NemoClaw CLI install). Node.js is guaranteed available, and rejection still leaves no NemoClaw binary on PATH. Per Copilot/CodeRabbit review on PR NVIDIA#2694. Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
Simplified per review — removed the bash-only early-check approach entirely. show_usage_notice now runs immediately after install_nodejs + ensure_supported_runtime (end of Phase 1), before Phase 2. Node is guaranteed available, rejection still blocks NemoClaw install. |
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 `@scripts/install.sh`:
- Around line 1372-1377: The script currently calls show_usage_notice
unconditionally after install_nodejs, which still invokes node in
non-interactive mode (see show_usage_notice using node), causing headless
installs without --yes-i-accept-third-party-software to run install_nodejs
before failing; before calling install_nodejs (i.e., in Phase 0/1), add a
shell-native non-interactive acceptance gate that checks the presence of the
--yes-i-accept-third-party-software flag or a TTY and exits early with a clear
message if not accepted, and remove or guard the unconditional show_usage_notice
call so show_usage_notice is only run after the gate or in interactive installs
(references: show_usage_notice, install_nodejs, run_onboard).
🪄 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: Enterprise
Run ID: 6d46c355-7143-424d-b41f-a50add57c30e
📒 Files selected for processing (1)
scripts/install.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/install.sh (2)
1078-1087: 💤 Low valuevLLM installation assumes Debian/Ubuntu and may fail silently on other platforms.
The
apt-getfallback for pip3 only works on Debian-based systems. On macOS, RHEL, or other distros, users hittingNEMOCLAW_PROVIDER=vllmwithout pip3 will get a confusing failure. Consider adding a clearer error message when pip3 is unavailable on non-apt systems.💡 Suggested improvement
if ! python3 -c "import vllm" 2>/dev/null; then info "Installing vLLM…" if ! command_exists pip3; then - sudo apt-get install -y -qq python3-pip >/dev/null 2>&1 || true + if command_exists apt-get; then + sudo apt-get install -y -qq python3-pip >/dev/null 2>&1 || true + fi + if ! command_exists pip3; then + warn "pip3 not found. Install Python 3 pip manually, then re-run." + return 1 + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 1078 - 1087, The install script's vLLM branch relies on apt-get to install pip3 which only works on Debian/Ubuntu; update the logic in the block around command_exists, pip3, and the vLLM installation so it detects unsupported platforms and emits a clear error instead of failing silently: after checking command_exists pip3, if pip3 is missing try platform-specific installers (e.g., brew on macOS) or, if unknown, call warn with a descriptive message like "pip3 not found and automatic install not supported on this OS; please install pip3 manually" and return 1; ensure the same explanatory message is logged before attempting pip3 install and preserve the subsequent python3 -c "import vllm" check to fail fast if installation still doesn't succeed.
292-311: 💤 Low valueNested function definition is valid but may surprise readers.
stop_agent_forward_if_owned()is defined insiderestore_onboard_forward_after_post_checks(). This works in bash but is uncommon. Consider extracting it to file scope or adding a brief comment noting the intentional scoping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 292 - 311, The nested function stop_agent_forward_if_owned defined inside restore_onboard_forward_after_post_checks can surprise readers; extract stop_agent_forward_if_owned to file scope (move its full definition out of restore_onboard_forward_after_post_checks) or, if you intentionally want it scoped, add a clear comment immediately above the nested definition stating it is intentionally local to restore_onboard_forward_after_post_checks; ensure any references to openshell_bin, forward_list, owner, status and the call to "$openshell_bin" forward stop "$port" "$sandbox_name" still work after moving (or keep parameter/variable usage consistent) and run a quick shellcheck to verify no scope/variable issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Line 1745: The post-install call to restore_onboard_forward_after_post_checks
is non-fatal but currently invokes error "Hermes host forward restore failed."
which aborts after successful onboarding; change this to a non-fatal warning/log
so the script continues after onboarding. Locate the invocation of
restore_onboard_forward_after_post_checks and replace the error call with a
warning function or echo (e.g., warn or printf to stderr) that logs "Hermes host
forward restore failed. Post-install succeeded; follow remediation steps printed
earlier." so the script does not exit on this failure.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 1078-1087: The install script's vLLM branch relies on apt-get to
install pip3 which only works on Debian/Ubuntu; update the logic in the block
around command_exists, pip3, and the vLLM installation so it detects unsupported
platforms and emits a clear error instead of failing silently: after checking
command_exists pip3, if pip3 is missing try platform-specific installers (e.g.,
brew on macOS) or, if unknown, call warn with a descriptive message like "pip3
not found and automatic install not supported on this OS; please install pip3
manually" and return 1; ensure the same explanatory message is logged before
attempting pip3 install and preserve the subsequent python3 -c "import vllm"
check to fail fast if installation still doesn't succeed.
- Around line 292-311: The nested function stop_agent_forward_if_owned defined
inside restore_onboard_forward_after_post_checks can surprise readers; extract
stop_agent_forward_if_owned to file scope (move its full definition out of
restore_onboard_forward_after_post_checks) or, if you intentionally want it
scoped, add a clear comment immediately above the nested definition stating it
is intentionally local to restore_onboard_forward_after_post_checks; ensure any
references to openshell_bin, forward_list, owner, status and the call to
"$openshell_bin" forward stop "$port" "$sandbox_name" still work after moving
(or keep parameter/variable usage consistent) and run a quick shellcheck to
verify no scope/variable issues.
🪄 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: Enterprise
Run ID: bdf2bf83-2b6f-4057-88a3-7d1d646184f5
📒 Files selected for processing (1)
scripts/install.sh
| nemoclaw upgrade-sandboxes --auto 2>&1 || warn "Sandbox upgrade check failed (non-fatal)." | ||
| "$_CLI_BIN" upgrade-sandboxes --auto 2>&1 || warn "Sandbox upgrade check failed (non-fatal)." | ||
| fi | ||
| restore_onboard_forward_after_post_checks || error "Hermes host forward restore failed." |
There was a problem hiding this comment.
Fatal error after successful onboarding defeats the PR's partial-install fix.
restore_onboard_forward_after_post_checks is a non-critical post-install helper that restarts Hermes port forwarding. Aborting with error after onboarding has already completed introduces a new failure mode: the user loses their progress indicator and sees a scary error message even though installation succeeded.
This should be a warning, not a fatal error. The function already prints actionable remediation steps on failure (lines 369-370).
🔧 Proposed fix
- restore_onboard_forward_after_post_checks || error "Hermes host forward restore failed."
+ restore_onboard_forward_after_post_checks || warn "Hermes host forward could not be restored; see instructions above."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| restore_onboard_forward_after_post_checks || error "Hermes host forward restore failed." | |
| restore_onboard_forward_after_post_checks || warn "Hermes host forward could not be restored; see instructions above." |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install.sh` at line 1745, The post-install call to
restore_onboard_forward_after_post_checks is non-fatal but currently invokes
error "Hermes host forward restore failed." which aborts after successful
onboarding; change this to a non-fatal warning/log so the script continues after
onboarding. Locate the invocation of restore_onboard_forward_after_post_checks
and replace the error call with a warning function or echo (e.g., warn or printf
to stderr) that logs "Hermes host forward restore failed. Post-install
succeeded; follow remediation steps printed earlier." so the script does not
exit on this failure.
|
The restore_onboard_forward_after_post_checks issue at line 1745 is in upstream install.sh code that was pulled in during rebase, not part of this PR's changes. Filed separately if it's a real regression — happy to split it out if that's preferred. The vLLM/apt-get nitpick is also upstream code, same situation. |
Move usage notice gating ahead of phase 1 so non-interactive or rejected notice flows fail before Node/CLI installation. Also remove duplicate call from run_onboard(). Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
…efore phase 2 Previous approach (bash-only early check + deferred TTY check) was overly complex and still invoked show_usage_notice on a code path where Node might not be present. Simpler fix: move show_usage_notice to immediately after install_nodejs and ensure_supported_runtime, but before Phase 2 (NemoClaw CLI install). Node.js is guaranteed available, and rejection still leaves no NemoClaw binary on PATH. Per Copilot/CodeRabbit review on PR NVIDIA#2694. Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
…ase 2 Rebased on upstream/main (f9d21af). The upstream changes to install.sh (agent display name, clone_nemoclaw_ref refactor) are unrelated to this fix. Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
5e89a25 to
6d9da79
Compare
|
Closing as superseded by #2706, which merged as 3cdaf53 and closed #2671. I chose #2706 for the fix because it adds a shell-native fail-fast preflight before Phase 1/2 plus focused install-preflight regression coverage; this PR still moved the notice to after Node install, so it did not satisfy the fail-before-Phase-1/2 requirement as directly. |
wscurran
added
area: cli
Summary
show_usage_noticebefore install phase 1 (Node.js)install.shdescribing why this must happen earlyrun_onboard()Why
If the notice is rejected (or can't be accepted in non-interactive mode), the installer used to fail in phase 3 after Node.js + NemoClaw CLI were already installed. This change makes that path fail before any install steps run, so we don't leave a partial install behind.
Summary by CodeRabbit
New Features
Chores