fix(onboard): clarify Ollama model selection - #6874
Conversation
|
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:
📝 WalkthroughWalkthroughOllama onboarding now logs shared gateway route guidance during interactive setup, keeps that guidance silent in non-interactive mode, verifies both behaviors, and documents using another gateway port to select a different model. ChangesOllama onboarding route guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Ho Lim <subhoya@gmail.com>
3e64250 to
a2cee5e
Compare
Signed-off-by: Ho Lim <subhoya@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard/setup-nim-ollama.test.ts (1)
66-106: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore console spies even when a test fails.
log.mockRestore()runs only after successful assertions. Wrap each test intry/finally, or use anafterEachcleanup, so a failed test cannot leak the mocked globalconsole.loginto subsequent tests.As per coding guidelines, deterministic tests must restore spies and reset test globals reliably.
Also applies to: 109-135
🤖 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 `@src/lib/onboard/setup-nim-ollama.test.ts` around lines 66 - 106, Ensure the console.log spy created in the setup Ollama tests is always restored, including when assertions or the handler fail. Add shared afterEach cleanup or wrap each affected test, including the additional test around lines 109-135, in try/finally so log.mockRestore() executes on every exit path.Source: Coding guidelines
🤖 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/inference/set-up-ollama.mdx`:
- Around line 186-190: Update the onboarding guidance around the
NEMOCLAW_GATEWAY_PORT example to explicitly state that 8990 is only an example
and must be an unused port; retain the command while clarifying that users
should choose another unused port if necessary.
---
Outside diff comments:
In `@src/lib/onboard/setup-nim-ollama.test.ts`:
- Around line 66-106: Ensure the console.log spy created in the setup Ollama
tests is always restored, including when assertions or the handler fail. Add
shared afterEach cleanup or wrap each affected test, including the additional
test around lines 109-135, in try/finally so log.mockRestore() executes on every
exit path.
🪄 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: ee2d97a2-3367-495b-9238-3e93e6db5677
📒 Files selected for processing (3)
docs/inference/set-up-ollama.mdxsrc/lib/onboard/setup-nim-ollama.test.tssrc/lib/onboard/setup-nim-ollama.ts
|
Addressed the Advisor warning in d634d4f: non-interactive onboarding now has explicit coverage proving shared-route guidance stays silent while the required model is still forwarded as |
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
|
Addressed the remaining CodeRabbit outside-diff cleanup finding in e34dcad: the test file now restores all Vitest spies in afterEach, so console.log cannot leak when a handler or assertion fails. Validation: npx vitest run src/lib/onboard/setup-nim-ollama.test.ts (9 passed), npm run typecheck:cli, and npm run check:diff passed. |
wscurran
added
area: onboarding
|
✨ Thanks for the fix, @HOYALIM. Clarifying the Ollama model selection constraint and separate-gateway path should resolve the onboarding confusion. Ready for maintainer review. Related open issues: |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed the shared-gateway Ollama guidance end to end. It reports the route-locked model only in interactive mode, preserves non-interactive behavior, documents the separate-port escape hatch, and passes focused tests, CLI build/typecheck, and docs-route validation.
|
@cv Exact-head fork E2E skip approval is ready for |
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical dated changelog entry for NemoClaw `v0.0.89` before the release plan captures the tag commit. The entry summarizes the user-visible Station preparation, inference, recovery, policy-disclosure, and CLI-containment changes merged since `v0.0.88`. ## Changes - Add `docs/changelog/2026-07-20.mdx` with the exact `## v0.0.89` release heading, parser-safe SPDX comment, summary, and detailed bullets. - Link each shipped theme to the most specific published OpenClaw documentation routes. - Keep internal E2E, advisory-registry, and review-workflow refactors out of the user-facing release summary. Source summary: - #7214, #7241, #7237, #7223, #7204, #7202, #7183, and #7090 -> `docs/changelog/2026-07-20.mdx`: Summarize qualified DGX Station identity, package-state, PackageKit, DKMS, and reboot-handoff fixes. - #7242, #7221, #7186, #7164, and #6874 -> `docs/changelog/2026-07-20.mdx`: Summarize inference endpoint provenance, provider attachment, managed vLLM cleanup and progress, and Ollama selection guidance. - #7225, #7216, #7192, #7136, #7096, and #6910 -> `docs/changelog/2026-07-20.mdx`: Summarize sandbox readiness, recovery guidance, rebuilt model routing, durable Hermes state, and gateway restart behavior. - #7187 -> `docs/changelog/2026-07-20.mdx`: Summarize complete effective messaging-preset egress disclosure before policy mutation. - #7218, #7165, and #7184 -> `docs/changelog/2026-07-20.mdx`: Summarize structured output containment, terminal-safe route display, and Hermes forward cleanup. ## 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) ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This PR adds release-history prose only; the focused changelog contract test validates its required structure and routes. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `mise exec node@22.23.1 -- npx vitest run test/changelog-docs.test.ts` (6 passed). - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not applicable to this doc-only release entry. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — `mise exec node@22.23.1 -- npm run docs` completed with 0 errors and 2 existing site-wide warnings. - [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) — the native changelog entry uses the required parser-safe MDX SPDX comment and does not use frontmatter. --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Expanded DGX Station installation support for qualified GB300 and OTA-upgraded environments. * Preserved selected inference providers, endpoints, model pins, and tuning settings during sandbox creation and rebuilds. * Improved sandbox recovery by validating availability and stability before restarting services. * Added clearer policy output showing the complete effective messaging egress configuration. * **Bug Fixes** * Hardened status and inference route displays by sanitizing terminal control characters. * Improved Hermes uninstall behavior by stopping detached dashboard forwarding. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Summary
Interactive Ollama onboarding can reuse a same-gateway model without explaining why the model menu was skipped. This change makes the existing one-route-per-gateway constraint visible and gives the exact separate-gateway path for choosing a different model.
Related Issue
Fixes #6758
Changes
NEMOCLAW_GATEWAY_PORT.NEMOCLAW_MODEL.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/onboard/setup-nim-ollama.test.ts(8 passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not applicable; this is a focused diagnostic message guarded by the existing route-compatibility test surface.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Ho Lim subhoya@gmail.com
Summary by CodeRabbit