fix(install): fail fast on license gate so partial install can't land#2706
Merged
ericksoa merged 2 commits intoMay 4, 2026
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: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe installer now aborts early if interactive third‑party license acceptance cannot proceed: when not non‑interactive, stdin isn't a TTY, and ChangesEarly Termination Guard
Installer Atomicity Tests
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer
participant TTY as "/dev/tty"
participant Phase1 as "Phase 1: Node.js"
participant Phase2 as "Phase 2: CLI"
User->>Installer: invoke scripts/install.sh
Installer->>Installer: check NON_INTERACTIVE env / accept flag
Installer->>TTY: attempt to open /dev/tty
alt /dev/tty unavailable AND not non-interactive AND no accept flag
Installer->>User: print "Interactive third-party software acceptance requires a TTY"
Installer-->>User: exit 1
else accept flag present or non-interactive
Installer->>Phase1: run Node.js install
Phase1-->>Installer: success
Installer->>Phase2: run CLI install
Phase2-->>Installer: success
Installer->>User: continue to onboarding
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
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 `@scripts/install.sh`:
- Around line 1373-1381: The current TTY preflight lets
ACCEPT_THIRD_PARTY_SOFTWARE=1 bypass the TTY check but phase 3 still needs
NON_INTERACTIVE=1, causing later failures in show_usage_notice(); fix by
ensuring acceptance implies non-interactive: when ACCEPT_THIRD_PARTY_SOFTWARE is
set, set NON_INTERACTIVE=1 early (or change the TTY preflight to not skip the
TTY check for ACCEPT_THIRD_PARTY_SOFTWARE), and then keep the existing TTY
check/early error around the conditional that references NON_INTERACTIVE and
ACCEPT_THIRD_PARTY_SOFTWARE so phase 3 (show_usage_notice()) will run in the
non-interactive path without later failing.
In `@test/install-preflight.test.ts`:
- Around line 2482-2533: The bypass tests currently only assert the TTY message
is absent which can false-pass; update the two bypass tests that call
runInstaller(...) (the ones around the existing assertions at lines ~2482-2533
and ~2550-2563) to also assert that the returned phases string is non-empty
(i.e., phases !== "" or expect(phases).not.toBe("")). This ensures the installer
actually progressed past preflight; reference the runInstaller helper and its
returned phases property when adding the additional assertion.
🪄 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: a5fed121-4213-43e4-97a1-e68e408e0a64
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.ts
Contributor
|
✨ Thanks for submitting this PR that fixes a bug where the installation process fails to check for license acceptance before proceeding with installation. Related open PRs: Related open issues: |
A plain `curl -fsSL https://www.nvidia.com/nemoclaw.sh | bash` (no flags) in non-TTY mode runs phases [1/3] Node.js + [2/3] NemoClaw CLI to completion before failing at the third-party-software acceptance step in phase [3/3]. The user is left in a half-installed state — the nemoclaw binary is on PATH and `--version` works, but ~/.nemoclaw/usage-notice.json was never created (license never accepted), and onboard cannot proceed. A retry hits the same failure because the license helper still has nothing to read from. The license gate already knew it could not succeed: in show_usage_notice() the failure is deterministic when stdin is not a TTY, /dev/tty is unopenable, NON_INTERACTIVE != 1, and ACCEPT_THIRD_PARTY_SOFTWARE != 1. Hoist that same check to the top of main(), right after flag parsing, so the install errors out BEFORE phases 1/2 leave artifacts on disk. Same friendly error message as before — just emitted before any state changes. Effect on the user-visible flow: - `curl|bash` (no flags) on a server with no /dev/tty: fail-fast, exit 1 with the existing TTY hint, no Node.js install attempted, no nemoclaw on PATH. Retry-after-fixing-the-flag is now clean. - `curl|bash --yes-i-accept-third-party-software` (post NVIDIA#2670): clear, install proceeds. - `curl|bash --non-interactive`: clear, install proceeds. - Interactive run on a desktop terminal: clear, prompt as before. - curl|bash piped from a desktop where /dev/tty IS openable: clear, show_usage_notice falls back to /dev/tty input as before. - `npx vitest run --project installer-integration -t "installer atomicity"` — 3 new sourced tests: - NVIDIA#2671: curl|bash with no flags exits 1 BEFORE phase 1 — stub node/npm/docker record every invocation; assertion that the log stays empty is the atomicity proof. - --yes-i-accept-third-party-software alone clears the gate (regression-guard against an over-aggressive check). - --non-interactive alone clears the gate (same). - `attempts nvm upgrade when system Node.js is below minimum version` needed `NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1` added to its env — the test exercises the Node-version path which is unrelated to license acceptance, and now (correctly) cannot reach Node detection without bypassing the license gate first. - Full installer-integration suite: 71/71 pass (prev. 70 + 3 new − 1 test re-bypassed = 71). Fixes NVIDIA#2671. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
CodeRabbit flagged that ACCEPT_THIRD_PARTY_SOFTWARE=1 alone bypassed the fail-fast preflight, but show_usage_notice is only one of two phase-3 steps that need a TTY or --non-interactive. run_onboard has the same gate, so a curl|bash run with --yes-i-accept-third-party-software but no --non-interactive could still partial-fail at run_onboard, leaving phases 1/2 on disk anyway. Treat the acceptance flag as non-interactive intent: when ACCEPT_THIRD_PARTY_SOFTWARE=1, set NON_INTERACTIVE=1 early in main() before the preflight check runs. The user already said "yes I accept" non-interactively; assuming they also want the rest of the install non-interactive matches the curl|bash use case the flag was named for. Tighten the preflight to no longer skip on the acceptance flag alone (it's now redundant since acceptance implies NON_INTERACTIVE=1). Also harden the bypass tests: assert phases !== "" in addition to absence of the TTY error message, so the tests can't false-pass if the install bailed for some other reason. - Re-ran installer-integration: 71/71 pass (4.5s atomicity tests). Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
af55822 to
0464769
Compare
ericksoa
approved these changes
May 4, 2026
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Reviewed against issue #2671 and duplicate PR #2694. This is the right fix shape: the license/TTY preflight stays shell-native and runs before Phase 1/2, explicit acceptance is normalized into non-interactive intent, and the added install-preflight coverage proves the no-flag curl-pipe path exits before Node/CLI while accepted and non-interactive paths still clear the preflight. CodeRabbit's earlier findings are addressed on the current head.
Contributor
|
/ok to test 0464769 |
12 tasks
miyoungc
added a commit
that referenced
this pull request
May 5, 2026
## Summary Catch up the docs for user-facing changes that landed over the weekend and today, so the published guidance matches current installer, onboarding, status, logs, local inference, rebuild backup behavior, the next docs version selector, and refreshed generated user skills. ## Related Issue None. ## Changes - Document WSL Windows-host Ollama onboarding actions, including use, start, restart, install, and `host.docker.internal` model pulls from #2800; clarify that onboard owns Ollama install and model pulls from #2952. - Document installer fail-fast behavior for non-TTY third-party software acceptance from #2706. - Update sandbox-name guidance and Brev deploy validation behavior from #2948. - Add `nemoclaw <name> logs --tail/--since` coverage from #2825. - Add global `nemoclaw status --json` coverage from #2822. - Document verified-gateway status behavior and non-zero degraded exits from #2884. - Clarify that rebuild stops before deleting the original sandbox when backup fails, including unreadable or root-owned state paths. - Bump docs switcher metadata from 0.0.33 to 0.0.34 without changing package versions or creating release tags. - Regenerate `.agents/skills/nemoclaw-user-*` from docs, including the new `nemoclaw-user-manage-sandboxes` generated skill and removal of the stale `nemoclaw-user-workspace` output. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Verification - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [x] `make docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> Made with [Cursor](https://cursor.com) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded Ollama/local inference guidance with detailed WSL and Windows-host workflows, proxy/token behavior, and onboarding options * Standardized sandbox name validation and updated troubleshooting, CLI, and deploy docs to surface the rules and validation timing * Added/rewrote Manage Sandboxes, policy management, backup/restore, messaging channels, workspace persistence, and CLI selection guides * Refreshed quickstart/Hermes guidance, skill mappings, and bumped docs version to 0.0.34 <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
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
3 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
curl -fsSL https://www.nvidia.com/nemoclaw.sh | bash(no flags) on a non-TTY host runs phases [1/3] Node.js + [2/3] NemoClaw CLI to completion before failing at the third-party-software notice in phase [3/3]. The user is left withnemoclaw --versionworking but~/.nemoclaw/usage-notice.jsonnever created — the license was never accepted, but the binary is on PATH. A retry hits the same wall and they have torm -rf ~/.nemoclaw ~/.local/bin/nemoclawto recover. Fixes #2671.Problem
The license check in
show_usage_noticeis deterministic: when stdin isn't a TTY,/dev/ttyisn't openable, and neither--non-interactivenor--yes-i-accept-third-party-softwareis set, it errors out. But that check runs as part of phase [3/3], afterinstall_nodejsandinstall_nemoclawhave already done their work. We knew up front the install couldn't finish, and we still ran two phases of disk changes before saying so.Fix
Run the same precondition check at the top of
main(), right after flag parsing. Same error message — just emitted before phase 1 instead of after phase 2. Skipped whenNON_INTERACTIVE=1,ACCEPT_THIRD_PARTY_SOFTWARE=1(the #2692 gate), stdin is a TTY, or/dev/ttyis openable.Stacks cleanly with #2692 — that PR makes
--yes-i-accept-third-party-softwarework on its own; this PR makes the no-flag case fail without leaving a half-install behind.Test plan
npx vitest run --project installer-integration -t "installer atomicity"— 3 new sourced tests:#2671: curl|bash with no flags exits 1 BEFORE phase 1— stubnode/npm/dockerwrite to a phase log on every invocation; assert the log is empty after the run, and that no[1/3]or[2/3]step output appeared.--yes-i-accept-third-party-software alone is sufficient to clear the fail-fast gate— guards against the new check over-firing and regressing fix(install): honor --yes-i-accept-third-party-software in non-TTY mode #2692.--non-interactive alone is sufficient to clear the fail-fast gate— same.attempts nvm upgrade when system Node.js is below minimum version, line 116) neededNEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE: "1"added to its env so it can reach the Node-version path. The test isn't about licensing, but with the new gate it can't get pastmain()'s entry without something to satisfy it. Inline comment explains why.installer-integrationsuite: 71/71 pass (66.7s).npm run typecheckclean.Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests