fix(onboard): gate Windows-host Ollama on Docker Desktop WSL#4416
Conversation
Signed-off-by: zyang-dev <267119621+zyang-dev@users.noreply.github.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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughComputes a Windows-host Ollama Docker requirement from container runtime, adds host-aware running-Ollama menu entry and port validation, renames the provider fallback flag to ChangesWindows-host Ollama Docker Requirement Gating
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 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: 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: 1 needs attention, 3 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/onboard/local-inference-topology.ts (1)
67-131: Run the targeted onboarding E2E subset before merge.Given this changes WSL runtime gating for provider selection, run the recommended onboarding/cloud lifecycle suite to validate end-to-end behavior under real topology transitions.
As per coding guidelines: "E2E test recommendation: cloud-e2e, sandbox-operations-e2e, rebuild-openclaw-e2e, channels-stop-start-e2e, messaging-compatible-endpoint-e2e, bedrock-runtime-compatible-anthropic-e2e, hermes-discord-e2e, hermes-slack-e2e, openshell-gateway-upgrade-e2e, issue-3600-gpu-proof-optional-e2e".
🤖 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/local-inference-topology.ts` around lines 67 - 131, Change to WSL runtime gating in getWindowsHostOllamaDockerRequirement affects provider selection; before merging, run the targeted onboarding E2E subset to validate real topology transitions and attach results. Execute the recommended suites: cloud-e2e, sandbox-operations-e2e, rebuild-openclaw-e2e, channels-stop-start-e2e, messaging-compatible-endpoint-e2e, bedrock-runtime-compatible-anthropic-e2e, hermes-discord-e2e, hermes-slack-e2e, openshell-gateway-upgrade-e2e, and issue-3600-gpu-proof-optional-e2e against this branch (or in your CI job) and ensure all pass for changes in getWindowsHostOllamaDockerRequirement; if any fail, reproduce locally, fix the gating logic or messaging, and re-run until green, then upload logs/results to the PR.src/lib/onboard.ts (1)
4896-4906: Run the recommended onboarding E2E suite before merge.This guardrail touches core provider-selection flow and non-interactive rejection paths, so run the selective nightly E2E job set for onboard/rebuild/channel/provider regressions:
gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=cloud-e2e,sandbox-operations-e2e,rebuild-openclaw-e2e,channels-stop-start-e2e,messaging-compatible-endpoint-e2e,bedrock-runtime-compatible-anthropic-e2e,hermes-discord-e2e,hermes-slack-e2e,openshell-gateway-upgrade-e2e,issue-3600-gpu-proof-optional-e2eAs per coding guidelines:
src/lib/onboard.tsis core onboarding logic and includes an explicit E2E test recommendation list for this area.🤖 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.ts` around lines 4896 - 4906, This change affects the provider-selection/non-interactive rejection flow around windowsHostOllamaDockerRequirement (used with selected.key, reject, isNonInteractive, abortNonInteractive and selectionLoop); before merging, run the recommended selective nightly E2E suite listed in the review (jobs: cloud-e2e,sandbox-operations-e2e,rebuild-openclaw-e2e,channels-stop-start-e2e,messaging-compatible-endpoint-e2e,bedrock-runtime-compatible-anthropic-e2e,hermes-discord-e2e,hermes-slack-e2e,openshell-gateway-upgrade-e2e,issue-3600-gpu-proof-optional-e2e) against this branch to verify provider selection and non-interactive rejection behavior, and only merge if those E2E runs pass.
🤖 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 `@src/lib/onboard.ts`:
- Around line 4896-4906: This change affects the
provider-selection/non-interactive rejection flow around
windowsHostOllamaDockerRequirement (used with selected.key, reject,
isNonInteractive, abortNonInteractive and selectionLoop); before merging, run
the recommended selective nightly E2E suite listed in the review (jobs:
cloud-e2e,sandbox-operations-e2e,rebuild-openclaw-e2e,channels-stop-start-e2e,messaging-compatible-endpoint-e2e,bedrock-runtime-compatible-anthropic-e2e,hermes-discord-e2e,hermes-slack-e2e,openshell-gateway-upgrade-e2e,issue-3600-gpu-proof-optional-e2e)
against this branch to verify provider selection and non-interactive rejection
behavior, and only merge if those E2E runs pass.
In `@src/lib/onboard/local-inference-topology.ts`:
- Around line 67-131: Change to WSL runtime gating in
getWindowsHostOllamaDockerRequirement affects provider selection; before
merging, run the targeted onboarding E2E subset to validate real topology
transitions and attach results. Execute the recommended suites: cloud-e2e,
sandbox-operations-e2e, rebuild-openclaw-e2e, channels-stop-start-e2e,
messaging-compatible-endpoint-e2e, bedrock-runtime-compatible-anthropic-e2e,
hermes-discord-e2e, hermes-slack-e2e, openshell-gateway-upgrade-e2e, and
issue-3600-gpu-proof-optional-e2e against this branch (or in your CI job) and
ensure all pass for changes in getWindowsHostOllamaDockerRequirement; if any
fail, reproduce locally, fix the gating logic or messaging, and re-run until
green, then upload logs/results to the PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9d233917-c4d8-4b92-b238-1f475db88af8
📒 Files selected for processing (4)
src/lib/onboard.tssrc/lib/onboard/local-inference-topology.tssrc/lib/onboard/ollama-install-menu.tstest/onboard-selection.test.ts
…untimes Signed-off-by: zyang-dev <267119621+zyang-dev@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
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.ts (1)
4160-4166:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the Windows-host hint behind supported topology.
This branch now tells users to rerun with
NEMOCLAW_PROVIDER=ollama, but theollamapath is rejected later whenwindowsHostOllamaDockerRequirement.supportedis false. In the native-Docker-in-WSL case, that turns this recovery hint into another immediate abort. Only show that hint when the Windows-host path is actually supported; otherwise surface the same unsupported-topology guidance used by the new guardrail.🤖 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.ts` around lines 4160 - 4166, In the branch starting with if (isWsl() && recordedProvider === "ollama-local" && isWindowsHostOllama) adjust the recovery hint so it is only shown when the Windows-host path is actually supported: check windowsHostOllamaDockerRequirement.supported and, if true, log the "re-run with NEMOCLAW_PROVIDER=ollama" hint; otherwise log the same unsupported-topology guidance used by the guardrail (the message shown when the topology is unsupported). This ensures the suggestion to use ollama on the Windows host is only displayed when windowsHostOllamaDockerRequirement.supported is true.
🤖 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.
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 4160-4166: In the branch starting with if (isWsl() &&
recordedProvider === "ollama-local" && isWindowsHostOllama) adjust the recovery
hint so it is only shown when the Windows-host path is actually supported: check
windowsHostOllamaDockerRequirement.supported and, if true, log the "re-run with
NEMOCLAW_PROVIDER=ollama" hint; otherwise log the same unsupported-topology
guidance used by the guardrail (the message shown when the topology is
unsupported). This ensures the suggestion to use ollama on the Windows host is
only displayed when windowsHostOllamaDockerRequirement.supported is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aa4d47a3-02ce-468c-9f8e-6f48c6513932
📒 Files selected for processing (7)
src/lib/onboard.tssrc/lib/onboard/local-inference-topology.tssrc/lib/onboard/ollama-install-menu.test.tssrc/lib/onboard/ollama-install-menu.tssrc/lib/onboard/provider-key-fallback.test.tssrc/lib/onboard/provider-key-fallback.tstest/onboard-selection.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/ollama-install-menu.ts
- src/lib/onboard/local-inference-topology.ts
dismissed — posted without maintainer authorization
dismissed — approval body referenced local test verification, but I'm on macOS and unit-tested with mocked WSL topology, so real-world WSL/native-docker behavior is not actually verified. need WSL repro before approving.
|
Actionable comments posted: 0 |
|
Clean extraction — No Windows host to verify the three Fixes-linked reproducers (#4208, #4421, #4257) end-to-end — flagging that as a QA spot-check before merge, not a code-review concern. |
cjagwani
left a comment
There was a problem hiding this comment.
Approving on the strength of @zifu Yang's WSL2 + native-Docker-in-WSL verification: option 7 now correctly carries the "(requires Docker Desktop WSL integration)" label suffix, and selecting it produces the actionable detected native Docker Engine in WSL abort instead of continuing into the broken step-[4/8] state. That's #4421's exact reproducer landing on the new clean reject path as designed.
Code review + unit tests pass on macOS; my own platform constraint is now covered by Zifu's real-Windows run.
## 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
This PR keeps Windows-host Ollama options visible during WSL onboarding, but clearly marks them as requiring Docker Desktop WSL integration when native Docker Engine is detected. Selecting those providers in an unsupported native Docker-in-WSL topology now fails early with an actionable message instead of continuing into a confusing install failure.
Related Issue
Fixes #4208, Fixes #4421, Fixes #4257
Changes
start-windows-ollamaandinstall-windows-ollamabefore launching or installing Ollama in unsupported native Docker-in-WSL setups.onboard.ts.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: zyang-dev 267119621+zyang-dev@users.noreply.github.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests