fix(installer): restore piped TTY notice prompt#3055
Merged
Conversation
ericksoa
changed the title
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 (3)
📝 WalkthroughWalkthroughThis PR introduces a third-party software usage notice acceptance workflow to the NemoClaw installer. The implementation adds shell functions for config resolution, state persistence, and interactive prompting, integrates a preflight acceptance check into the installer's startup sequence, documents the headless/CI usage pattern, and includes comprehensive test coverage for both interactive and non-interactive acceptance flows. ChangesUsage Notice Acceptance Workflow
Sequence DiagramsequenceDiagram
participant User
participant Installer as scripts/install.sh
participant Config as Config File
participant State as State File
participant TTY as /dev/tty
User->>Installer: Run piped installer or interactive
Installer->>Installer: main() starts
Installer->>Installer: preflight_usage_notice_prompt()
alt Environment: ACCEPT_THIRD_PARTY_SOFTWARE=1
Installer->>Installer: Skip prompt, continue
else Check State File
Installer->>State: usage_notice_accepted_shell(version)
State-->>Installer: Already accepted?
alt Version matches
Installer->>Installer: Continue to Phase 1
else Version mismatch or no state
alt NON_INTERACTIVE mode
Installer->>User: Error: acceptance required
Installer->>Installer: Exit fail-fast
else Interactive + TTY available
Installer->>Config: Load notice config
Config-->>Installer: title, body, version
alt stdin is piped
Installer->>TTY: Attach /dev/tty for prompting
else stdin is TTY
Installer->>User: Display notice + prompt
end
User->>Installer: Accept or reject
alt User accepts
Installer->>State: Save acceptance + version
Installer->>Installer: Continue to Phase 1
else User rejects
Installer->>Installer: Exit fail-fast before Phase 1
end
end
end
end
Installer->>Installer: Phase 1, Phase 2, etc.
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25398263536
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/get-started/quickstart.md`:
- Around line 49-51: Replace the triple-backtick bash block containing the curl
install command with a console block and add a leading "$ " prompt; specifically
change the code fence from ```bash to ```console and prefix the line "curl -fsSL
https://www.nvidia.com/nemoclaw.sh | NEMOCLAW_NON_INTERACTIVE=1
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 bash" with "$ " so the CLI snippet in
quickstart.md uses the required console language tag and prompt.
In `@scripts/install.sh`:
- Around line 678-691: preflight_usage_notice_prompt currently returns early
when NON_INTERACTIVE=1 which lets headless installs bypass the preflight; change
the logic so the function does NOT skip prompting merely because NON_INTERACTIVE
is set — only skip when an explicit acceptance/bypass is provided (e.g.
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 or the equivalent --yes-i-accept flag),
and keep the existing TTY check/third-party prompt using show_usage_notice_shell
on /dev/tty; update the condition at the top of preflight_usage_notice_prompt to
remove the NON_INTERACTIVE early-return and instead check the explicit
acceptance env/flag before returning.
In `@test/install-preflight.test.ts`:
- Around line 3193-3205: The test should explicitly assert the installer
succeeded and derive the expected notice version from bin/lib/usage-notice.json
instead of hard-coding it: after calling
runInstallerWithPipedStdinAndTty(checking result, phases, state), add an
assertion that the process exit code/status is zero (e.g. expect(result.status
or result.exitCode).toBe(0)) to ensure success, read/require
bin/lib/usage-notice.json to obtain the "version" (or equivalent field) and use
that value in the state assertion (replace the hard-coded "2026-04-01b" with the
derived version), while leaving the existing TTY/prompt/ordering assertions
intact; locate symbols runInstallerWithPipedStdinAndTty, result, phases, state
and update the test accordingly.
🪄 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: 5d3f4923-7533-4523-85ea-f1786013ce21
📒 Files selected for processing (5)
README.mddocs/get-started/quickstart.mddocs/reference/commands.mdscripts/install.shtest/install-preflight.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25398516876
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25398874036
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/install.sh (1)
623-675:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed if the shell path cannot render the notice body.
This path validates
version, but it never verifies thatprint_usage_notice_body_shell()actually produced any notice text. Ifusage-notice.jsonis missing/reformatted aroundbody, the installer can still collectyeswithout showing the notice content first.Suggested fix
show_usage_notice_shell() { - local notice_json version title prompt answer answer_lc + local notice_json version title prompt answer answer_lc rendered_body notice_json="$(usage_notice_config_path)" if [[ ! -f "$notice_json" ]]; then error "Third-party software notice configuration not found." @@ prompt="$(json_string_field "$notice_json" "interactivePrompt")" if [[ -z "$version" ]]; then error "Third-party software notice version not found." fi + rendered_body="$(print_usage_notice_body_shell "$notice_json")" + [[ -n "${rendered_body//[$'\t\r\n ']/}" ]] || error "Third-party software notice body not found." if usage_notice_accepted_shell "$version"; then return 0 fi printf "\n" printf " %s\n" "${title:-Third-Party Software Notice - NemoClaw Installer}" printf " ──────────────────────────────────────────────────\n" - print_usage_notice_body_shell "$notice_json" + printf "%s\n" "$rendered_body" printf "\n"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install.sh` around lines 623 - 675, In show_usage_notice_shell, ensure the installer fails closed if the notice body is missing by capturing the output of print_usage_notice_body_shell (call it e.g. notice_body="$(print_usage_notice_body_shell "$notice_json")"), checking that notice_body is non-empty before printing the prompt, and calling error/return 1 if empty; keep the existing validation of version and acceptance flow, and then print notice_body (instead of calling print_usage_notice_body_shell directly) so the absence or empty body from usage_notice_config_path/json_string_field is detected and aborts rather than allowing the prompt to proceed.
🧹 Nitpick comments (1)
test/install-preflight.test.ts (1)
3217-3255: ⚡ Quick winAdd the direct-TTY acceptance case too.
The new happy-path assertion only covers the
/dev/ttyfallback viarunInstallerWithPipedStdinAndTty("yes\n"). The[ -t 0 ]branch is only exercised for decline, so a success regression there would still slip through. A matchingrunInstallerWithInteractiveStdin("yes\n")case would keep both acceptance paths protected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/install-preflight.test.ts` around lines 3217 - 3255, Add a new test that mirrors the existing successful `/dev/tty` case but uses runInstallerWithInteractiveStdin("yes\n") to exercise the direct-TTY ([ -t 0 ]) branch: call runInstallerWithInteractiveStdin("yes\n"), combine result.stdout and result.stderr, load the usage-notice.json version, assert result.status is 0, assert the output contains the TTY prompt and "Third-Party Software Notice - NemoClaw Installer", assert the output does not contain "Interactive third-party software acceptance requires a TTY", assert the ordering of "Third-Party Software Notice - NemoClaw Installer" before "Node.js", assert phases is not empty, and assert state contains `"acceptedVersion": "<noticeVersion>"`; place this next to the other acceptance/decline tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@scripts/install.sh`:
- Around line 623-675: In show_usage_notice_shell, ensure the installer fails
closed if the notice body is missing by capturing the output of
print_usage_notice_body_shell (call it e.g.
notice_body="$(print_usage_notice_body_shell "$notice_json")"), checking that
notice_body is non-empty before printing the prompt, and calling error/return 1
if empty; keep the existing validation of version and acceptance flow, and then
print notice_body (instead of calling print_usage_notice_body_shell directly) so
the absence or empty body from usage_notice_config_path/json_string_field is
detected and aborts rather than allowing the prompt to proceed.
---
Nitpick comments:
In `@test/install-preflight.test.ts`:
- Around line 3217-3255: Add a new test that mirrors the existing successful
`/dev/tty` case but uses runInstallerWithInteractiveStdin("yes\n") to exercise
the direct-TTY ([ -t 0 ]) branch: call
runInstallerWithInteractiveStdin("yes\n"), combine result.stdout and
result.stderr, load the usage-notice.json version, assert result.status is 0,
assert the output contains the TTY prompt and "Third-Party Software Notice -
NemoClaw Installer", assert the output does not contain "Interactive third-party
software acceptance requires a TTY", assert the ordering of "Third-Party
Software Notice - NemoClaw Installer" before "Node.js", assert phases is not
empty, and assert state contains `"acceptedVersion": "<noticeVersion>"`; place
this next to the other acceptance/decline tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0a1bcfc1-68bd-4156-bd2a-32a3f02cc14f
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25400175221
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25400686425
|
cv
approved these changes
May 5, 2026
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25405102067
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25405701586
|
…eptance # Conflicts: # src/lib/onboard-cli-commands.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25408752042
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install.sh`:
- Around line 696-713: The new preflight blocks reject the existing FIFO-driven
smoke install path by requiring a TTY; update the logic so scripted/stdin-piped
installs are still supported: when exec 3</dev/tty fails, check whether stdin is
a pipe (i.e. [ -t 0 ] is false) and if so call show_usage_notice_shell reading
from stdin (or from the piped FD) instead of erroring; additionally avoid
conflating NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE with NON_INTERACTIVE in
main()—keep acceptance from the env/flag separate so the smoke harness can set
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 without the installer switching to
NON_INTERACTIVE and stopping further stdin-driven prompts (update the handling
in main() and retain show_usage_notice_shell usage for piped stdin).
- Around line 683-685: The early return when ACCEPT_THIRD_PARTY_SOFTWARE is set
prevents persisting acceptance, so modify the branch that checks the
ACCEPT_THIRD_PARTY_SOFTWARE / NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE flag to call
save_usage_notice_acceptance_shell() (or whatever persistence helper is used)
before returning; ensure the code path that exits early still invokes the same
persistence function used for interactive acceptance so
~/.nemoclaw/usage-notice.json is written and state is versioned.
🪄 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: f12ed924-bdc8-40d5-9560-21ed3ebbd901
📒 Files selected for processing (3)
scripts/install.shsrc/lib/onboard-cli-commands.tstest/install-preflight.test.ts
Comment on lines
+683
to
+685
| if [ "${ACCEPT_THIRD_PARTY_SOFTWARE:-}" = "1" ]; then | ||
| return 0 | ||
| fi |
Contributor
There was a problem hiding this comment.
Persist explicit acceptance as well.
Line 683 short-circuits before save_usage_notice_acceptance_shell(), so the documented headless path (NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 / --yes-i-accept-third-party-software) never writes ~/.nemoclaw/usage-notice.json. Users who accept via env/flag will be prompted again on the next run even though this flow is now versioned and stateful.
Suggested fix
preflight_usage_notice_prompt() {
if [ "${ACCEPT_THIRD_PARTY_SOFTWARE:-}" = "1" ]; then
+ local notice_json version
+ notice_json="$(usage_notice_config_path)"
+ if [[ -f "$notice_json" ]]; then
+ version="$(json_string_field "$notice_json" "version")"
+ if [[ -n "$version" ]]; then
+ save_usage_notice_acceptance_shell "$version"
+ fi
+ fi
return 0
fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install.sh` around lines 683 - 685, The early return when
ACCEPT_THIRD_PARTY_SOFTWARE is set prevents persisting acceptance, so modify the
branch that checks the ACCEPT_THIRD_PARTY_SOFTWARE /
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE flag to call
save_usage_notice_acceptance_shell() (or whatever persistence helper is used)
before returning; ensure the code path that exits early still invokes the same
persistence function used for interactive acceptance so
~/.nemoclaw/usage-notice.json is written and state is versioned.
Comment on lines
+696
to
+713
| if [ "${NON_INTERACTIVE:-}" = "1" ]; then | ||
| error "Non-interactive installation requires explicit third-party software acceptance. Re-run with --yes-i-accept-third-party-software or set NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1." | ||
| fi | ||
|
|
||
| if [ -t 0 ]; then | ||
| show_usage_notice_shell | ||
| return "$?" | ||
| fi | ||
|
|
||
| if { exec 3</dev/tty; } 2>/dev/null; then | ||
| info "Installer stdin is piped; prompting for the third-party software notice on /dev/tty before install." | ||
| local status=0 | ||
| show_usage_notice_shell <&3 || status=$? | ||
| exec 3<&- | ||
| return "$status" | ||
| fi | ||
|
|
||
| error "Interactive third-party software acceptance requires a TTY. Re-run in a terminal or pass --yes-i-accept-third-party-software (or set NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1)." |
Contributor
There was a problem hiding this comment.
This closes off the existing FIFO-driven smoke install path.
scripts/smoke-macos-install.sh:205-212 still drives install.sh with stdin from a named pipe and no controlling TTY. With this preflight, that path now exits before phase 1. The obvious workaround (NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1) still changes behavior, because main() later coerces explicit acceptance into NON_INTERACTIVE=1, so the smoke harness can no longer feed the remaining prompts from its answers pipe. Please update that harness in the same PR, or preserve a supported scripted-input path here.
Also applies to: 1701-1705
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install.sh` around lines 696 - 713, The new preflight blocks reject
the existing FIFO-driven smoke install path by requiring a TTY; update the logic
so scripted/stdin-piped installs are still supported: when exec 3</dev/tty
fails, check whether stdin is a pipe (i.e. [ -t 0 ] is false) and if so call
show_usage_notice_shell reading from stdin (or from the piped FD) instead of
erroring; additionally avoid conflating NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE
with NON_INTERACTIVE in main()—keep acceptance from the env/flag separate so the
smoke harness can set NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 without the
installer switching to NON_INTERACTIVE and stopping further stdin-driven prompts
(update the handling in main() and retain show_usage_notice_shell usage for
piped stdin).
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25410330520
|
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
/dev/ttybefore phase 1 when stdin is the installer pipe.curl | bashenv placement.Testing
npm ci --ignore-scriptsnpm run build:clinpm run typecheck:clinpm run source-shape:checkbash -n install.sh && bash -n scripts/install.shgit diff --checknpx vitest run test/install-preflight.test.tsSummary by CodeRabbit
New Features
Documentation