fix(cli): surface copy-paste recovery hints in onboard error paths#4426
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
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)
📝 WalkthroughWalkthroughAdds a slug suggestion helper and integrates it into sandbox name validation to emit "Try: " guidance; introduces a missing-NVIDIA-API-key helper used in non-interactive onboarding to print copy-paste ChangesCopy-paste recovery commands for onboard errors
Sequence Diagram(s)sequenceDiagram
participant UserCLI
participant setupNim
participant CredentialResolver
participant logMissingNvidiaApiKeyHelp
participant ProcessExit
UserCLI->>setupNim: run non-interactive onboard
setupNim->>CredentialResolver: resolve NVIDIA_API_KEY
CredentialResolver-->>setupNim: null (missing)
setupNim->>logMissingNvidiaApiKeyHelp: call with provider helpUrl
logMissingNvidiaApiKeyHelp-->>UserCLI: stderr lines (requirement + export example + helpUrl)
setupNim->>ProcessExit: exit(1)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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 There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 0 worth checking, 0 nice ideas This is an automated advisory review. A human maintainer must make the final merge decision. |
Selective E2E Results — ✅ All requested jobs passedRun: 26568327452
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Actionable comments posted: 0 |
Selective E2E Results — ✅ All requested jobs passedRun: 26569135616
|
…ery hint Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-4426.docs.buildwithfern.com/nemoclaw |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/reference/commands.mdx (1)
160-160: ⚡ Quick winUse active voice and split into separate sentences.
This line has two style guide violations:
- Passive voice: "Names that do not match these rules are rejected" should use active voice.
- Multiple clauses on one line: The two clauses separated by a semicolon should be split into separate lines for better diff readability.
📝 Suggested rewrite
-Names that do not match these rules are rejected; the CLI prints a `Try: <suggested-slug>` recovery line whenever it can derive a valid lowercase, hyphen-separated form from the input (for example, `MyAssistant` produces `Try: myassistant`). +The CLI rejects names that do not match these rules. +The CLI prints a `Try: <suggested-slug>` recovery line whenever it can derive a valid lowercase, hyphen-separated form from the input (for example, `MyAssistant` produces `Try: myassistant`).As per coding guidelines, active voice is required, and one sentence per line in source makes diffs readable.
🤖 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 `@docs/reference/commands.mdx` at line 160, Rewrite the passive sentence "Names that do not match these rules are rejected; the CLI prints a `Try: <suggested-slug>` recovery line whenever it can derive a valid lowercase, hyphen-separated form from the input (for example, `MyAssistant` produces `Try: myassistant`)." into active voice and split it into two separate sentences/lines: one that states the action (e.g., "The CLI rejects names that do not match these rules.") and a second that explains the recovery behavior and example using the existing `Try: <suggested-slug>` text and `MyAssistant` -> `myassistant` example.
🤖 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.
Nitpick comments:
In `@docs/reference/commands.mdx`:
- Line 160: Rewrite the passive sentence "Names that do not match these rules
are rejected; the CLI prints a `Try: <suggested-slug>` recovery line whenever it
can derive a valid lowercase, hyphen-separated form from the input (for example,
`MyAssistant` produces `Try: myassistant`)." into active voice and split it into
two separate sentences/lines: one that states the action (e.g., "The CLI rejects
names that do not match these rules.") and a second that explains the recovery
behavior and example using the existing `Try: <suggested-slug>` text and
`MyAssistant` -> `myassistant` example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3218a6fd-5f09-4be2-b77c-3bedad626de5
📒 Files selected for processing (2)
docs/reference/commands.mdxdocs/reference/troubleshooting.mdx
✅ Files skipped from review due to trivial changes (1)
- docs/reference/troubleshooting.mdx
Selective E2E Results — ✅ All requested jobs passedRun: 26571628162
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Actionable comments posted: 0 |
Selective E2E Results — ✅ All requested jobs passedRun: 26572266320
|
…dy-valid input as no-op Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Actionable comments posted: 0 |
1 similar comment
|
Actionable comments posted: 0 |
Selective E2E Results — ❌ Some jobs failedRun: 26572952389
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Actionable comments posted: 0 |
Selective E2E Results — ✅ All requested jobs passedRun: 26573530924
|
Selective E2E Results — ✅ All requested jobs passedRun: 26587915047
|
cjagwani
left a comment
There was a problem hiding this comment.
lgtm. fixes #4425, scope is appropriately minimal:
suggestNameSlug()covers every degenerate input (already-valid→null, empty→null, leading non-letter prefixeds-, truncation re-trims trailing hyphen)logMissingNvidiaApiKeyHelp()is 13 lines, just emits the recovery block- 235 lines of test coverage, both extractions covered
- no scope creep into adjacent error paths
dismissed — posted without maintainer authorization
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed adversarially against the linked recovery-hint issue and the current PR head.
This keeps the fix narrowly scoped: name validation remains strict while adding actionable Try: guidance, the non-interactive NVIDIA credential path still exits fail-closed while adding a copy-paste recovery command, and the tests cover both helper behavior and CLI-boundary failure paths. Focused local validation passed (build:cli, typecheck:cli, targeted vitest, docs strict, diff check), and the live checks/advisors are green on this head.
## Summary Refreshes the NemoClaw documentation for the v0.0.54 release and regenerates user skills from the Fern MDX source. Also keeps the Fern CLI pin current so local docs checks use the upgraded Fern version. ## Related Issue <!-- No single related issue. This is release-prep documentation catch-up. --> ## Changes - #4403 -> `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Document Telegram, Discord, and Slack post-rebuild bridge verification and summarize channel activation fixes. - #4222 -> `docs/about/release-notes.mdx`: Include Slack generated channel enablement in the v0.0.54 messaging summary. - #4346 -> `docs/get-started/windows-preparation.mdx`, `docs/about/release-notes.mdx`: Document safer Windows bootstrap behavior for Ubuntu first-run and Docker Desktop WSL integration. - #4416 -> `docs/inference/use-local-inference.mdx`, `docs/about/release-notes.mdx`: Document the Docker Desktop WSL requirement for Windows-host Ollama. - #4442 -> `docs/about/release-notes.mdx`: Summarize the optional NemoHermes native web dashboard and related environment variables. - #4426 -> `docs/about/release-notes.mdx`: Summarize copy-paste recovery hints for invalid sandbox names and missing NVIDIA API keys. - #4459 -> `docs/about/release-notes.mdx`: Summarize the Linuxbrew prefix fix for sandbox Homebrew usage. - #4450 -> `docs/about/release-notes.mdx`: Summarize `/nemoclaw` slash command startup activation. - #4468 -> `docs/about/release-notes.mdx`: Summarize scope-upgrade approval recovery. - #4325 -> `docs/about/release-notes.mdx`: Summarize the narrowed `web_fetch` host-gateway allowance. - #4474 -> `docs/about/release-notes.mdx`: Summarize Hermes Provider smoke-check behavior for OAuth versus Nous API key setup. - Refresh generated `.agents/skills/nemoclaw-user-*` references from `docs/` and update `fern/fern.config.json` to Fern `5.41.2`. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [ ] `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 - [ ] `npm run 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> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Optional NemoHermes native web dashboard (configurable port and TUI) * GPU memory cleanup now unloads Ollama models when switching providers or stopping services * **Bug Fixes** * Improved sandbox name validation with suggested slug recovery * Windows-host Ollama now requires Docker Desktop WSL integration and exits with remediation guidance when unsupported * **Documentation** * Clarified quickstart/onboard flow, installer TTY/non‑TTY guidance, Hermes Docker prerequisites, sandbox hardening, and channels add rebuild checks <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4539?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Two
nemoclaw onboarderror paths named the root cause and exited non-zero, but neither emitted the copy-paste-ready recovery command the T5987921 spec requires. Sandbox-name validation now appends aTry: <derived-slug>line whenever a recoverable slug differs from the rejected input, and the non-interactiveNVIDIA_API_KEYrequirement now prints the concreteexport NVIDIA_API_KEY=nvapi-...line plus the existing build-provider help URL.Related Issue
Fixes #4425.
Changes
src/lib/name-validation.ts: add and exportsuggestNameSlug(value)which derives a copy-paste-ready RFC 1123 label from arbitrary user input (lowercase, replace illegal characters with-, collapse runs of-, trim terminal-, prefix a leading non-letter withs-, truncate toNAME_MAX_LENGTH) and returnsnullwhen no recoverable slug exists or the input is already valid.getNameValidationGuidance(label, value, opts)now appends aTry: <slug>line wheneversuggestNameSlug(value)yields a non-null suggestion, so every CLI surface that already prints guidance (onboard,sandbox-agent,deploy) picks it up automatically.src/lib/onboard/missing-credential-hints.ts: new helper module exposinglogMissingNvidiaApiKeyHelp(helpUrl). The helper emits a three-line recovery block before the caller exits non-zero — first the requirement line (NVIDIA_API_KEY (or NEMOCLAW_PROVIDER_KEY) is required for NVIDIA Endpoints in non-interactive mode.), then theSet with:label on its own line followed by the standaloneexport NVIDIA_API_KEY=nvapi-...command so the user can copy-paste it directly, and finally the existingGet a key from <helpUrl>line when a help URL is configured.src/lib/onboard.ts: the non-interactiveNVIDIA_API_KEY"is required" branch in the build-provider configuration step delegates tologMissingNvidiaApiKeyHelpand then exits non-zero, replacing the previous inlineconsole.errorblock. This keeps the top-level entrypoint net-smaller and satisfies thecodebase-growth-guardrailspolicy.src/lib/runner.tsandsrc/lib/name-validation.ts: the RFC 1123 label regex now lives once inname-validation.tsas the exportedNAME_VALID_PATTERN, andrunner.ts:validateName()imports it instead of duplicating the literal — so the slug helper and the hard validator cannot drift.test/onboard-sandbox-name.test.ts: update the existing equality assertions forgetNameValidationGuidanceto expect the newTry: <slug>line where the input is recoverable, and add a dedicatedsuggestNameSlugdescribe block covering: mixed-case (MyAssistant→myassistant), illegal characters mapped to hyphen (bad name,My Project Sandbox,agent_007), collapsed and trimmed hyphen runs that produce a different slug (--legacy--,foo bar), digit-leading inputs that gain thes-prefix (123-leading,9lives), length truncation (a × 80→a × 63), the already-valid no-op cases including internal hyphen runs (my-assistant,openclaw,a---b), and the unrecoverable cases that returnnull(empty string,---,!!!). A subprocess test drives the onboard entrypoint with{ sandboxName: "MyAssistant", nonInteractive: true }and asserts exit code 1, theInvalid sandbox name: 'MyAssistant'.error line, and the standaloneTry: myassistantrecovery line so the CLI wiring cannot regress while the helper-level tests pass.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests