[codex] add first-run onboarding usage notice#1222
Open
13ernkastel wants to merge 8 commits intoNVIDIA:mainfrom
Open
[codex] add first-run onboarding usage notice#122213ernkastel wants to merge 8 commits intoNVIDIA:mainfrom
13ernkastel wants to merge 8 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a configurable "Usage Notice" to onboarding: a bundled JSON payload, a notice-management module with state persistence and display/acknowledgement logic, integration into the onboard flow before provisioning, tests, and documentation. Supports interactive and non-interactive modes and re-displays when the notice version changes. Changes
Sequence DiagramsequenceDiagram
participant User
participant Onboard as onboard.js
participant Notice as onboard-notice.js
participant ConfigFS as Config File System
participant StateFS as State File System
User->>Onboard: Start onboarding
Onboard->>Onboard: Mark preflight complete
Onboard->>Notice: showOnboardNoticeIfNeeded(options)
Notice->>ConfigFS: Load notice config (env override or default)
ConfigFS-->>Notice: return config
Notice->>Notice: Validate config
Notice->>StateFS: Load last-seen state
StateFS-->>Notice: return lastSeenVersion / null
Notice->>Notice: Compare versions
alt Should show notice
Notice->>Notice: Render notice lines
Notice->>User: Write lines to stderr / writeLine
alt Interactive
Notice->>User: promptFn(prompt)
User-->>Notice: acknowledgement
else Non-interactive
Notice->>User: log continuation message
end
Notice->>StateFS: Atomically save new state (version + timestamp)
StateFS-->>Notice: save result
end
Notice-->>Onboard: Return shown status & version
Onboard->>Onboard: Continue onboarding (gateway checks/start)
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 unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
test/onboard.test.js (1)
1827-1835: Tighten this ordering assertion around the gateway boundary.This regex only proves the notice call appears somewhere after
markStepComplete("preflight"). If the call moved below gateway startup, the test would still pass. Please anchor the match to the first gateway logic (for exampleconst gatewayStatus =orstartGateway() so the test protects the contract this PR is adding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 1827 - 1835, The test's regex currently only ensures showOnboardNoticeIfNeeded appears after onboardSession.markStepComplete("preflight") but not that it occurs before gateway startup; update the assertion in onboard.test.js so the match anchors the sequence to the first gateway boundary (e.g., require showOnboardNoticeIfNeeded\({ before either the first occurrence of "const gatewayStatus =" or "startGateway("); locate the existing symbols onboardSession.markStepComplete("preflight") and showOnboardNoticeIfNeeded and modify the regex to assert they appear in that order and precede the gateway marker (const gatewayStatus or startGateway) so the test fails if the notice call moves below gateway startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard-notice.js`:
- Around line 122-123: The call to saveOnboardNoticeState should be made
best-effort so that write failures do not abort onboarding: wrap the
saveOnboardNoticeState(config.version, statePath) call in a try/catch, and on
error call the existing logger (or console.warn) to emit a warning that includes
the error and the statePath, but do not rethrow; always return { shown: true,
version: config.version } regardless of persistence success so
non-interactive/onboard flow continues.
In `@docs/reference/usage-notice.md`:
- Around line 37-47: Add a required "Next Steps" section to the bottom of this
new page (the "Operator Override" section that documents
NEMOCLAW_ONBOARD_NOTICE_CONFIG and the JSON keys) that links to related docs
such as the CLI reference, configuration guide, and onboarding/installation
pages; ensure the section title is "Next Steps" and include short bulleted links
to the CLI usage, configuration file format, and onboarding guides so reviewers
see related navigation from this page.
- Around line 26-35: Replace the generic `{admonition}` block used for "Notice
and Disclaimer" with a MyST warning admonition (use the ::: warning ... :::
syntax) so the page follows the docs style guide; locate the existing
`{admonition}` token and the "Notice and Disclaimer" title and convert that
block to a MyST warning callout containing the same text content.
---
Nitpick comments:
In `@test/onboard.test.js`:
- Around line 1827-1835: The test's regex currently only ensures
showOnboardNoticeIfNeeded appears after
onboardSession.markStepComplete("preflight") but not that it occurs before
gateway startup; update the assertion in onboard.test.js so the match anchors
the sequence to the first gateway boundary (e.g., require
showOnboardNoticeIfNeeded\({ before either the first occurrence of "const
gatewayStatus =" or "startGateway("); locate the existing symbols
onboardSession.markStepComplete("preflight") and showOnboardNoticeIfNeeded and
modify the regex to assert they appear in that order and precede the gateway
marker (const gatewayStatus or startGateway) so the test fails if the notice
call moves below gateway startup.
🪄 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: c0d944eb-d74e-4baa-97a7-5af1e09801b3
📒 Files selected for processing (8)
bin/config/onboard-notice.jsonbin/lib/onboard-notice.jsbin/lib/onboard.jsdocs/index.mddocs/reference/commands.mddocs/reference/usage-notice.mdtest/onboard-notice.test.jstest/onboard.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard-notice.js (1)
66-83: Optional: clean up temp file if atomic rename fails.If
fs.renameSyncfails (e.g., permissions/race), the temp file may be left behind. A small cleanup path would avoid stale files in the state directory.♻️ Suggested hardening for temp-file cleanup
function saveOnboardNoticeState(version, statePath = getOnboardNoticeStatePath()) { const dir = path.dirname(statePath); const tmpPath = path.join( dir, `.onboard-notice.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`, ); fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); const payload = JSON.stringify( { lastSeenVersion: version, lastSeenAt: new Date().toISOString(), }, null, 2, ); - fs.writeFileSync(tmpPath, payload, { mode: 0o600 }); - fs.renameSync(tmpPath, statePath); + try { + fs.writeFileSync(tmpPath, payload, { mode: 0o600 }); + fs.renameSync(tmpPath, statePath); + } catch (error) { + try { + fs.unlinkSync(tmpPath); + } catch { + // ignore cleanup failure + } + throw error; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard-notice.js` around lines 66 - 83, The temp file created in saveOnboardNoticeState (tmpPath) can be left behind if fs.renameSync fails; wrap the rename in a try/catch so that on error you attempt to remove the tmpPath (fs.unlinkSync(tmpPath)) before rethrowing the original error, and ensure any unlink errors are swallowed/logged but don't mask the rename error; keep the existing write-to-temp logic and rethrow the original exception after cleanup so callers still see the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/onboard-notice.js`:
- Around line 66-83: The temp file created in saveOnboardNoticeState (tmpPath)
can be left behind if fs.renameSync fails; wrap the rename in a try/catch so
that on error you attempt to remove the tmpPath (fs.unlinkSync(tmpPath)) before
rethrowing the original error, and ensure any unlink errors are swallowed/logged
but don't mask the rename error; keep the existing write-to-temp logic and
rethrow the original exception after cleanup so callers still see the failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cde6e627-1646-412e-b0c9-e5541fe1367b
📒 Files selected for processing (3)
bin/lib/onboard-notice.jsdocs/reference/usage-notice.mdtest/onboard-notice.test.js
✅ Files skipped from review due to trivial changes (1)
- docs/reference/usage-notice.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/onboard-notice.test.js
wscurran
added
status: triage
Contributor
|
✨ Thanks for submitting this pull request, which proposes a way to add a first-run onboarding usage notice. Possibly related open issues: #1213, #1248, #1249. Possibly related open issues:
|
wscurran
added
the
Getting Started
kjw3
added a commit
that referenced
this pull request
Apr 2, 2026
…ing (#1290) ## Summary - show a third-party software notice before `install.sh` launches onboarding and before direct `nemoclaw onboard` - require explicit `yes` in interactive flows and fail immediately on any other answer - require explicit non-interactive acceptance via `--yes-i-accept-third-party-software` or `NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1` - link users to the official OpenClaw security guidance before continuing - render the security URL as a clickable terminal hyperlink where OSC 8 is supported, while still printing the raw URL - implement the new notice capability in TypeScript under `src/lib` ## Details This is aimed at issue #1213, but the acceptance behavior is stricter than the original issue text. The notice is specifically about third-party software liability and OpenClaw usage, not NemoClaw terms. The acceptance flag and env var are named accordingly. The current notice is intentionally formatted for standard terminal width and uses the approved wording in short, non-wrapping lines. Reference shown to users: - https://docs.openclaw.ai/gateway/security Credit: - builds on prior overlapping notice work in #1222 by @13ernkastel, but extends it to the current acceptance model, non-interactive enforcement, and the shared `install.sh` + `nemoclaw onboard` flow ## Validation - `npm run build:cli` - `npx vitest run test/usage-notice.test.js test/cli.test.js test/install-preflight.test.js` - `npx eslint bin/lib/usage-notice.js test/usage-notice.test.js` - `npx tsc -p jsconfig.json --noEmit` ## Note The full local git hook suite is currently blocked in this environment by an unrelated `src/lib/preflight.test.ts` localhost listen failure (`EPERM` on `127.0.0.1`). I did not change that area in this branch. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Onboarding and installer now require explicit acceptance of third‑party software terms before proceeding; onboarding exits early if consent is not given. * Interactive mode displays a bundled usage notice and prompts users to type "yes"; acceptance is persisted per user. * Non‑interactive installs allow scripted acceptance via a new explicit acceptance mechanism. * **Documentation** * CLI help, installer messages, and docs updated to document the new non‑interactive acceptance option and TTY guidance. * **Tests** * Added and updated tests and e2e scripts to cover acceptance flows and enforcement. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
cjagwani
pushed a commit
to fdzdev/NemoClaw
that referenced
this pull request
Apr 3, 2026
…ing (NVIDIA#1290) ## Summary - show a third-party software notice before `install.sh` launches onboarding and before direct `nemoclaw onboard` - require explicit `yes` in interactive flows and fail immediately on any other answer - require explicit non-interactive acceptance via `--yes-i-accept-third-party-software` or `NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1` - link users to the official OpenClaw security guidance before continuing - render the security URL as a clickable terminal hyperlink where OSC 8 is supported, while still printing the raw URL - implement the new notice capability in TypeScript under `src/lib` ## Details This is aimed at issue NVIDIA#1213, but the acceptance behavior is stricter than the original issue text. The notice is specifically about third-party software liability and OpenClaw usage, not NemoClaw terms. The acceptance flag and env var are named accordingly. The current notice is intentionally formatted for standard terminal width and uses the approved wording in short, non-wrapping lines. Reference shown to users: - https://docs.openclaw.ai/gateway/security Credit: - builds on prior overlapping notice work in NVIDIA#1222 by @13ernkastel, but extends it to the current acceptance model, non-interactive enforcement, and the shared `install.sh` + `nemoclaw onboard` flow ## Validation - `npm run build:cli` - `npx vitest run test/usage-notice.test.js test/cli.test.js test/install-preflight.test.js` - `npx eslint bin/lib/usage-notice.js test/usage-notice.test.js` - `npx tsc -p jsconfig.json --noEmit` ## Note The full local git hook suite is currently blocked in this environment by an unrelated `src/lib/preflight.test.ts` localhost listen failure (`EPERM` on `127.0.0.1`). I did not change that area in this branch. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Onboarding and installer now require explicit acceptance of third‑party software terms before proceeding; onboarding exits early if consent is not given. * Interactive mode displays a bundled usage notice and prompts users to type "yes"; acceptance is persisted per user. * Non‑interactive installs allow scripted acceptance via a new explicit acceptance mechanism. * **Documentation** * CLI help, installer messages, and docs updated to document the new non‑interactive acceptance option and TTY guidance. * **Tests** * Added and updated tests and e2e scripts to cover acceptance flows and enforcement. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
…ing (NVIDIA#1290) ## Summary - show a third-party software notice before `install.sh` launches onboarding and before direct `nemoclaw onboard` - require explicit `yes` in interactive flows and fail immediately on any other answer - require explicit non-interactive acceptance via `--yes-i-accept-third-party-software` or `NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1` - link users to the official OpenClaw security guidance before continuing - render the security URL as a clickable terminal hyperlink where OSC 8 is supported, while still printing the raw URL - implement the new notice capability in TypeScript under `src/lib` ## Details This is aimed at issue NVIDIA#1213, but the acceptance behavior is stricter than the original issue text. The notice is specifically about third-party software liability and OpenClaw usage, not NemoClaw terms. The acceptance flag and env var are named accordingly. The current notice is intentionally formatted for standard terminal width and uses the approved wording in short, non-wrapping lines. Reference shown to users: - https://docs.openclaw.ai/gateway/security Credit: - builds on prior overlapping notice work in NVIDIA#1222 by @13ernkastel, but extends it to the current acceptance model, non-interactive enforcement, and the shared `install.sh` + `nemoclaw onboard` flow ## Validation - `npm run build:cli` - `npx vitest run test/usage-notice.test.js test/cli.test.js test/install-preflight.test.js` - `npx eslint bin/lib/usage-notice.js test/usage-notice.test.js` - `npx tsc -p jsconfig.json --noEmit` ## Note The full local git hook suite is currently blocked in this environment by an unrelated `src/lib/preflight.test.ts` localhost listen failure (`EPERM` on `127.0.0.1`). I did not change that area in this branch. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Onboarding and installer now require explicit acceptance of third‑party software terms before proceeding; onboarding exits early if consent is not given. * Interactive mode displays a bundled usage notice and prompts users to type "yes"; acceptance is persisted per user. * Non‑interactive installs allow scripted acceptance via a new explicit acceptance mechanism. * **Documentation** * CLI help, installer messages, and docs updated to document the new non‑interactive acceptance option and TTY guidance. * **Tests** * Added and updated tests and e2e scripts to cover acceptance flows and enforcement. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 tasks
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
NEMOCLAW_ONBOARD_NOTICE_CONFIGRoot Cause
The onboarding flow provisioned resources immediately after preflight with no configurable notice surface. A simple hardcoded banner would not satisfy the issue because legal text changes would either require JS edits or remain hidden behind a one-time boolean.
Impact
Validation
./node_modules/.bin/eslint bin/lib/onboard-notice.js bin/lib/onboard.js test/onboard-notice.test.js test/onboard.test.js./node_modules/.bin/vitest run test/onboard-notice.test.js test/onboard.test.jsCloses #1213
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes