fix(install): honor --yes-i-accept-third-party-software in non-TTY mode#2692
Conversation
|
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 due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughInstaller now honors ChangesInstaller acceptance change
Sequence Diagram(s)sequenceDiagram
participant Runner as Runner / CI
participant Install as install.sh
participant Usage as usage-notice.js
participant TTY as TTY / Env
Runner->>Install: invoke script with flags/env
Install->>TTY: detect TTY presence
alt ACCEPT flag set (env/flag)
Install->>Usage: exec node usage-notice.js --non-interactive --yes-i-accept-third-party-software
Usage-->>Install: write usage-notice.json (accepted)
Install->>Runner: continue install
else NON_INTERACTIVE set only
Install->>Usage: exec node usage-notice.js --non-interactive
Usage-->>Install: write usage-notice.json or prompt depending on env
Install->>Runner: continue or exit
else neither set & no TTY
Install->>Runner: emit error: interactive acceptance requires a TTY (hint: use --yes-i-accept-third-party-software or --non-interactive)
Install-->>Runner: exit non-zero
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
✨ Thanks for submitting this PR that fixes a bug where the --yes-i-accept-third-party-software flag is ignored in non-TTY mode. Related open issues: |
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 `@test/install-preflight.test.ts`:
- Around line 2571-2577: The test currently only asserts the friendly hint but
doesn't assert that any /dev/tty leakage is suppressed; update the test that
calls callShowUsageNotice({}) (the "errors with the friendly hint when neither
flag is set in non-TTY mode" case) to also assert the combined output variable
(output) does NOT contain /dev/tty or the specific shell noise (e.g., "exec
3</dev/tty"); add an expectation like expect(output).not.toMatch(/\/dev\/tty/)
or a negative match for the exact "exec 3</dev/tty" string to ensure the
regression is covered.
- Around line 2498-2544: callShowUsageNotice() currently composes the child
process environment in a way that lets ambient runner vars (e.g.
NON_INTERACTIVE, ACCEPT_THIRD_PARTY_SOFTWARE) alter test behavior; to fix it,
build a clean env object for the spawnSync call (instead of merging global
process.env) and explicitly remove or unset NON_INTERACTIVE and
ACCEPT_THIRD_PARTY_SOFTWARE before passing it to spawnSync (refer to
callShowUsageNotice, the env object passed to spawnSync and
NEMOCLAW_SOURCE_ROOT/TEST_SYSTEM_PATH/PATH symbols so you can locate where to
modify the environment).
🪄 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: 23e827c4-c589-4d83-ac23-13860993282d
📒 Files selected for processing (3)
install.shscripts/install.shtest/install-preflight.test.ts
✅ Files skipped from review due to trivial changes (2)
- install.sh
- scripts/install.sh
`bash install.sh --help` lists `--yes-i-accept-third-party-software` as a
top-level flag for skipping the third-party-software notice, but in
curl|bash mode (no TTY) the flag was silently ignored unless
`--non-interactive` (or `NEMOCLAW_NON_INTERACTIVE=1`) was also passed —
the install would still try to read `/dev/tty` and fail with
"Interactive third-party software acceptance requires a TTY".
The error message did point users at the workaround, but a flag whose
name is "yes-i-accept" should clear the notice on its own. Hidden
gating on a separate flag is exactly the worst-of-both: the parser
accepts the flag, the installer rejects the install.
`scripts/install.sh::show_usage_notice` now treats
`ACCEPT_THIRD_PARTY_SOFTWARE=1` as sufficient to run the notice helper
non-interactively, regardless of `NON_INTERACTIVE` — same envelope the
notice helper already accepted when both flags were set, just with the
gating relaxed. Subsequent `run_onboard` still requires
`--non-interactive` for full curl|bash automation; that gate is
unchanged and its error message already names both flags.
Help text in `install.sh` and `scripts/install.sh` updated from
"in non-interactive mode" to "without prompting (works in any mode)"
so the contract matches the runtime behavior.
- `npx vitest run --project installer-integration -t "installer license acceptance"`
— 3 new sourced tests:
- NVIDIA#2670: ACCEPT_THIRD_PARTY_SOFTWARE=1 alone in non-TTY mode now
clears the notice and passes both flags through to the helper
- NON_INTERACTIVE=1 alone keeps existing prompt-driven behavior
(the new branch must not regress the historical path)
- neither flag in non-TTY mode still errors with the friendly hint
- Full installer-integration suite: 70/70 pass.
Fixes NVIDIA#2670.
Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
The "errors with the friendly hint when neither flag is set" test failed in WSL CI: even with stdin redirected to /dev/null, the WSL runner's child process kept a controlling pty so `(: </dev/tty)` succeeded inside show_usage_notice and the wizard took its /dev/tty-fallback branch instead of the `else error` path the test means to exercise. Wrap the bash invocation in `setsid` so the child runs in a fresh session with no controlling terminal. /dev/tty is then deterministically unopenable across Linux, WSL, and macOS CI environments. - Re-ran all 3 `installer license acceptance` tests locally: pass. - Full installer-integration suite still passes. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
When stdin is piped and /dev/tty is unopenable (e.g. headless CI shells,
setsid-wrapped invocations), bash printed the raw redirect error before
show_usage_notice and the install.sh onboard fallback could emit their
friendly hint:
/app/scripts/install.sh: line 536: /dev/tty: No such device or address
Wrap the exec redirection in a brace group so the failed-redirect
diagnostic is scoped to /dev/null while the fd assignment still applies
to the current shell on success. Same shape applied to the onboard
fallback at line 1584.
Test changes (test/install-preflight.test.ts):
- Skip setsid on darwin. macOS does not ship util-linux setsid, so
spawnSync silently returned status: null with stdout/stderr undefined,
surfacing as the macos-e2e regression on this PR. Headless GitHub-hosted
macOS runners have no controlling TTY in the first place, so plain bash
is sufficient there.
- Assert the /dev/tty diagnostic does not leak into the friendly-hint
output (catches the regression this commit fixes in scripts/install.sh).
- Comment that the env literal is intentionally constructed without merging
process.env so ambient runner vars (NON_INTERACTIVE,
ACCEPT_THIRD_PARTY_SOFTWARE) cannot leak in.
Re-ran installer license acceptance (3 tests pass) and the full
install-preflight suite (75/75).
Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
c38299a to
df389bb
Compare
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`:
- Line 487: The help text for --yes-i-accept-third-party-software incorrectly
states "works in any mode" even though run_onboard() still requires
NON_INTERACTIVE=1 for noninteractive installs; update the behavior or the copy:
either have the flag set or export NON_INTERACTIVE=1 before invoking
run_onboard() when the flag is passed (ensure flag parsing in the script
propagates to the onboarding branch and to run_onboard()), or change the printed
description to limit it to clearing only the third‑party notice (remove "works
in any mode"); make the same change for the duplicated help text block around
the 1584-1591 area so both places are consistent.
🪄 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: 1660163e-5ba7-46e9-9dc6-1f0c561fb84f
📒 Files selected for processing (3)
install.shscripts/install.shtest/install-preflight.test.ts
✅ Files skipped from review due to trivial changes (2)
- install.sh
- test/install-preflight.test.ts
…actual effect CodeRabbit flagged the "(works in any mode)" parenthetical as overpromising: the flag clears the third-party notice in any mode, but run_onboard() still requires NON_INTERACTIVE=1 for headless onboarding. The base description "Accept the third-party software notice without prompting" already states what the flag does without overstating scope. Updated both copies — install.sh:110 and scripts/install.sh:487. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
|
@coderabbitai — fair point, scoped the help text to its actual effect (drop |
|
Tip For best results, initiate chat on the files or code changes.
|
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed the installer acceptance path and targeted tests. This keeps --yes-i-accept-third-party-software scoped to clearing the third-party notice without broadening full non-interactive onboarding, suppresses raw /dev/tty noise, and adds focused coverage for the non-TTY acceptance cases.\n\nLocal validation passed: npm run build:cli; npm run typecheck:cli; npx vitest run test/install-preflight.test.ts; git diff --check on the PR diff.
ericksoa
left a comment
There was a problem hiding this comment.
Second, more adversarial pass found a blocker against the linked issue contract.\n\nThe exact #2670 no-TTY, flag-only path still exits non-zero. This PR now lets show_usage_notice accept --yes-i-accept-third-party-software and suppresses the raw /dev/tty diagnostic, but run_onboard still only enters the automated branch when NON_INTERACTIVE=1. With only --yes-i-accept-third-party-software in a no-TTY invocation, the installer writes ~/.nemoclaw/usage-notice.json and then fails at onboarding with:\n\n[ERROR] Interactive onboarding requires a TTY. Re-run in a terminal or set NEMOCLAW_NON_INTERACTIVE=1 with --yes-i-accept-third-party-software.\n\nThat means the reproducer from #2670 still does not proceed; it just fails one gate later. Since this PR says Fixes #2670, I think it needs one of two changes before merge: either make flag-only headless installs actually proceed by having the acceptance flag imply/pass through the non-interactive onboarding path, or narrow the PR/user-facing contract so it does not claim to fix the flag-only no-TTY install case and explicitly documents that full headless installs still require --non-interactive.\n\nAdversarial local smoke results in a stubbed full installer path:\n- bash install.sh --yes-i-accept-third-party-software with no TTY: exit 1, usage-notice.json written, onboard not invoked.\n- bash install.sh --non-interactive --yes-i-accept-third-party-software in the same harness: exit 0, invoked onboard --non-interactive --yes-i-accept-third-party-software.
Dismissed after maintainer discussion: the PR is a net improvement and the remaining behavior is accepted as scoped to notice acceptance.
ericksoa
left a comment
There was a problem hiding this comment.
Approved after follow-up review. The change is scoped to the third-party notice acceptance path, and it is a net improvement over current behavior: the acceptance flag now clears the notice without prompting, the help text no longer overclaims full non-interactive onboarding, and the no-TTY failure path avoids raw /dev/tty noise. Full headless onboarding still correctly requires --non-interactive.
…#2706) ## 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 with `nemoclaw --version` working but `~/.nemoclaw/usage-notice.json` never created — the license was never accepted, but the binary is on PATH. A retry hits the same wall and they have to `rm -rf ~/.nemoclaw ~/.local/bin/nemoclaw` to recover. Fixes #2671. ## Problem The license check in `show_usage_notice` is deterministic: when stdin isn't a TTY, `/dev/tty` isn't openable, and neither `--non-interactive` nor `--yes-i-accept-third-party-software` is set, it errors out. But that check runs as part of phase [3/3], after `install_nodejs` and `install_nemoclaw` have 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 when `NON_INTERACTIVE=1`, `ACCEPT_THIRD_PARTY_SOFTWARE=1` (the [#2692](#2692) gate), stdin is a TTY, or `/dev/tty` is openable. Stacks cleanly with #2692 — that PR makes `--yes-i-accept-third-party-software` work on its own; this PR makes the no-flag case fail without leaving a half-install behind. ## Test plan - [x] `npx vitest run --project installer-integration -t "installer atomicity"` — 3 new sourced tests: - `#2671: curl|bash with no flags exits 1 BEFORE phase 1` — stub `node` / `npm` / `docker` write 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 #2692. - `--non-interactive alone is sufficient to clear the fail-fast gate` — same. - [x] One existing test (`attempts nvm upgrade when system Node.js is below minimum version`, line 116) needed `NEMOCLAW_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 past `main()`'s entry without something to satisfy it. Inline comment explains why. - [x] Full `installer-integration` suite: 71/71 pass (66.7s). - [x] `npm run typecheck` clean. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Installer performs an early preflight check and aborts quickly when interactive license acceptance requires a TTY, preventing partial runs. * **Bug Fixes** * Explicit third‑party acceptance now forces non‑interactive behavior so gating is consistent across install phases. * **Tests** * Added tests verifying installer behavior for TTY vs non‑TTY and for acceptance vs non‑acceptance paths. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
Summary
bash install.sh --helplists--yes-i-accept-third-party-softwareas a top-level flag for skipping the third-party-software notice, but incurl | bashmode (no TTY) the flag is silently ignored unless--non-interactive(orNEMOCLAW_NON_INTERACTIVE=1) is also passed — the install still tries to read/dev/ttyand fails with[ERROR] Interactive third-party software acceptance requires a TTY.The error message points at the workaround, but a flag whose name is "yes-i-accept" should clear the notice on its own. The current behavior is the worst-of-both: the parser accepts the flag, the installer rejects the install. Fixes #2670.Problem
scripts/install.sh::show_usage_noticeonly honoredACCEPT_THIRD_PARTY_SOFTWARE=1inside the[ "${NON_INTERACTIVE:-}" = "1" ]branch. In curl|bash mode (-t 0false,/dev/ttynot openable,NON_INTERACTIVEunset), control fell through past every interactive branch to the friendlyerrormessage — even though the user had explicitly accepted the licence on the command line. Help text ininstall.shandscripts/install.shfurther misled users by claiming the flag worked "in non-interactive mode".The
run_onboardstep has the same flag-name on a separate gate, but onboard genuinely needs a TTY or--non-interactiveto drive the wizard, and its error message already names both flags. That gate is unchanged here — this PR scopes itself to the licence-acceptance step the flag is named for.Fix
Relax the gate in
show_usage_noticesoACCEPT_THIRD_PARTY_SOFTWARE=1alone routes through the same notice-helper invocation thatNON_INTERACTIVE=1 + ACCEPT_THIRD_PARTY_SOFTWARE=1already used. The notice helper itself was already designed to accept that combo; only the installer's gate was wrong. Help text updated from "in non-interactive mode" to "without prompting (works in any mode)" so the contract matches the runtime behavior.Test plan
npx vitest run --project installer-integration -t "installer license acceptance"— 3 new sourced tests (8.7s):#2670: ACCEPT_THIRD_PARTY_SOFTWARE=1 alone clears the notice in non-TTY mode — stubsusage-notice.jsto record argv, sourcesscripts/install.sh, runsshow_usage_notice </dev/nullwith onlyACCEPT_THIRD_PARTY_SOFTWARE=1set; asserts exit 0 and that both--non-interactiveand--yes-i-accept-third-party-softwareflow through.NON_INTERACTIVE=1alone keeps the notice prompt-driven (regression) — same harness with onlyNON_INTERACTIVE=1; asserts existing behavior (helper gets--non-interactivebut not the licence flag) is preserved.installer-integrationsuite: 70/70 pass (72.9s) — no regressions in the adjacent license/path-resolution tests.npm run typecheckclean.Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests