fix(onboard): reuse a Windows-host Ollama that WSL mirrored networking exposes on loopback - #9348
Conversation
|
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 (10)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughNemoClaw validates Windows-host Ollama responses, detects mirrored WSL loopback access, suppresses incompatible Linux upgrades, reuses the Windows daemon during onboarding, and skips incompatible Linux systemd repair. ChangesMirrored WSL Ollama handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to This change alters onboarding behavior for Windows-hosted Ollama reuse. Targeted tests are reported as passing, but the applicable broad validation gate is still unrecorded, so merge readiness remains pending that check or explicit acceptance. Sequence Diagram(s)sequenceDiagram
participant WSL
participant ProviderHostState
participant OllamaInstallMenu
participant SetupNimFlow
participant LocalInferenceRepair
WSL->>ProviderHostState: probe Windows-host Ollama
ProviderHostState->>ProviderHostState: detect mirrored loopback topology
ProviderHostState->>OllamaInstallMenu: pass windowsDaemonOnWslLoopback
OllamaInstallMenu-->>SetupNimFlow: suppress Linux upgrade handling
SetupNimFlow->>SetupNimFlow: reuse running Windows daemon
SetupNimFlow->>LocalInferenceRepair: pass topology detector
LocalInferenceRepair->>LocalInferenceRepair: skip Linux systemd repair
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
…g exposes on loopback Mirrored WSL networking shares the loopback interface with Windows, so the Windows host's Ollama daemon answers on `127.0.0.1` instead of `host.docker.internal`. Onboarding recognized a Windows-host daemon only by that hostname, so it read the Windows daemon's version as a local Linux one, found it below the required minimum, and drove two Linux paths that cannot apply to a Windows install: the install menu routed onboarding into the sudo-bound Linux installer, and the reuse handler applied the Linux loopback systemd override. A non-interactive run stopped before creating the sandbox. `detectInferenceProviderHostState` already decides this exact question when it suppresses the duplicate-daemon warning: WSL, a daemon on `127.0.0.1`, a reachable Windows daemon, and `wslinfo --networking-mode` reporting `mirrored` mean one daemon, not two. That result was computed and discarded. It is now a field on the provider host state, so both paths read it. The daemon stays reused, and a requested `install-ollama` falls back to the reuse entry through the existing provider-key fallback. The binary gate is held off with the daemon gate because the `ollama` the WSL PATH exposes is the Windows client reached through interop, which the Linux installer cannot replace either. The Windows reachability probe now requires the Ollama wire format rather than any non-empty body, applying the reasoning already used for the loopback probe (NVIDIA#4275), because its result now decides whether a version gate runs. Refs: NVIDIA#9300 Signed-off-by: harjoth <harjoth.khara@gmail.com>
89af93a to
9d73ca9
Compare
… daemon on resume Resume repair applied the Linux loopback systemd override to every recorded `ollama-local` route. A route recorded against the Windows daemon that mirrored WSL networking exposes on loopback has no Linux service to repair, so a residual `ollama.service` could be started or modified, asking for sudo and taking `:11434` from the route resume then validates. A recorded route carries no topology, so resume re-detects it. `detectWindowsDaemonOnWslLoopback` resolves the same fact as provider discovery, from the same predicate, for callers that hold no host snapshot. Each probe short-circuits, so a non-WSL host costs one `isWsl` check. Model warm-up and context validation still run; only Linux service management is skipped. The predicate moves to `local-inference-topology.ts` so both the discovery snapshot and the resume path read one definition. `onboard.ts` stays pure wiring and `provider-inference.ts` composes the option, keeping the onboarding entry composition boundary at its recorded provider-decision count. Reported by the PR review advisor as PRA-1 on NVIDIA#9348. Refs: NVIDIA#9300 Signed-off-by: harjoth <harjoth.khara@gmail.com>
|
Looks like this was already addressed by #9345, but feel free to reopen if there's more to do. |
|
@cv Thanks. #9345 covers the install menu and fresh onboarding. Three items from this PR did not land:
Also stale: the I can reopen this rebased to points 1 and 2, reusing #9345's classification, or open a fresh follow-up. |
The Windows-host Ollama reachability probe trusted any non-empty Docker capture as proof the daemon is live. A captive proxy, a stale listener, or a stub on host.docker.internal can answer with an arbitrary 2xx body that then fed isWindowsHostOllama and the install menu. Validate the /api/tags body against the Ollama wire format instead. Refs: #9348 <!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 plain sentences: what changes and why. Describe before-and-after behavior when it applies. Follow the NemoClaw Writing Guide: https://github.com/NVIDIA/NemoClaw/blob/main/WRITING.md. Do not add unrelated prose cleanup. --> ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> ## Changes <!-- List concrete changes. If this adds an abstraction, configuration, fallback, migration, or compatibility path, name its current requirement and consumer, explain why a direct change is insufficient, and identify the test that protects it. --> ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check one tests line. Check other lines when applicable. Add every requested justification or approval reference. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests 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: ## DGX Station Hardware Evidence <!-- Required only when scripts/prepare-dgx-station-host.sh changes. Maintainers must review the linked evidence before approving or merging. This is human-reviewed evidence, not authenticated hardware provenance. Exceptional bypasses use existing repository governance and must be documented on the PR. --> - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [ ] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [ ] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [ ] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [ ] Quality Gates section completed with required justifications or waivers - [ ] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] 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 is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Your Name <your-email@example.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved Windows Ollama detection by verifying that the host response is a valid Ollama response. - Prevents unrelated services from being incorrectly recognized as reachable Ollama installations. - Preserves the local installation option when a valid Ollama service cannot be detected. - **Tests** - Added coverage for valid Ollama responses and rejected non-Ollama responses. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Kuangyicheng <c8688rickowens@outlook.com>
<!-- markdownlint-disable MD041 --> ## Summary The Windows-host Ollama probe introduced in #9482 accepted a `models` array even when its entries were `null`, primitive values, or nested arrays. The shared response validator now requires every model entry to be a non-null, non-array object while preserving valid empty model lists. ## Related Issue Refs #9348. Follow-up to #9482. ## Changes - Harden `isValidOllamaTagsResponseBody` so malformed model entries fail closed. - Add public onboarding-boundary regression coverage for `null`, primitive, and array entries. This change adds no abstraction, configuration, fallback, migration, or compatibility path. It tightens the response contract already used by the Windows-host reachability probe. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: security review passes because the change only tightens untrusted response parsing; it adds no credential, authentication, command, filesystem, permission, dependency, or network-destination behavior. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence Not applicable. `scripts/prepare-dgx-station-host.sh` is unchanged. ## 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 validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project cli src/lib/onboard/provider-host-state.test.ts src/lib/inference/local.test.ts`: 110 passed; `npx vitest run --project integration test/growth-guardrails.test.ts`: 32 passed; `npm run typecheck:cli`: passed. - [x] Applicable broad gate passed — the focused consumer and shared-validator suites cover this bounded two-file response-validation change. - [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) - [ ] 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) --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved validation of local Ollama and vLLM responses. * Malformed responses—including HTML, null values, numbers, arrays, error objects, and invalid model entries—are now rejected. * Prevented incorrect detection of Ollama availability while preserving the local Ollama installation option. * **Tests** * Expanded regression coverage for malformed responses from Windows-hosted Ollama installations, backend health checks, and authentication proxies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Summary
Mirrored WSL networking shares the loopback interface with Windows, so the Windows host's Ollama daemon answers on
127.0.0.1instead ofhost.docker.internal. Onboarding recognized a Windows-host daemon only by that hostname, so it read the Windows daemon's version as a local Linux one, found it below0.32.9, and drove two Linux paths that cannot apply to a Windows install: the install menu routed onboarding into the sudo-bound Linux installer, and the reuse handler applied the Linux loopback systemd override. A non-interactive run stopped before creating the sandbox. NemoClaw now reuses that daemon and takes neither Linux path.Related Issue
Refs: #9300
Changes
detectInferenceProviderHostStatealready answers this exact question when it suppresses the duplicate-daemon warning: WSL, a daemon on127.0.0.1, a reachable Windows daemon, andwslinfo --networking-modereportingmirroredmean one daemon, not two. That result was computed and thrown away. This makes it a state field so every consumer of the decision reads it.src/lib/onboard/provider-host-state.ts: extractisWindowsDaemonOnWslLoopbackfrommaybeWarnAboutDuplicateOllamaDaemons, compute it once, and expose it aswindowsDaemonOnWslLoopbackon the provider host state.src/lib/onboard/ollama-install-menu.ts: add the optionalwindowsDaemonOnWslLoopbackinput. When set, hold off both the daemon version gate and the binary version gate.src/lib/onboard/setup-nim-flow.ts: pass the fact to the reuse handler's Windows-host argument, which is what decides whether Linux systemd management runs.isWindowsHostOllamaitself is deliberately not widened —provider-selection.ts:152uses it to fail sandbox recovery withwsl-recorded-ollama-windows-host, and that behavior should not change here.src/lib/onboard/provider-host-state.ts+src/lib/inference/local.ts: the Windows reachability probe now requires the Ollama wire format ({"models":[...]}) instead of any non-empty body, reusing the exportedisValidOllamaTagsResponseBody. The repository already applies this reasoning to the loopback probe ([All Platforms][CLI&UX] nemoclaw status reports Local Ollama backend and auth proxy as "healthy" after backend is stopped (stale health for Scenario A) #4275); it matters more now that this result decides whether a version gate runs.src/lib/onboard/local-inference-topology.ts+machine/handlers/provider-inference.ts: resume re-detects the topology and skips Linux service repair for the same daemon (advisor finding PRA-1).onboard.tsstays pure wiring so the onboarding entry composition boundary keeps its recorded provider-decision count; the handler composes the option.docs/inference/set-up-ollama.mdx: name the exception to the "Upgrade Ollama entry appears when either side is below0.32.9" rule, scoped to the topology the code actually confirms.No new configuration, fallback, or compatibility layer. The menu input is one optional boolean with a single production caller, derived from a predicate the repository already trusts, protected by the tests below.
Judgment calls a reviewer should check
ollamaon the WSL PATH in this topology is the Windows client reached through interop, so the Linux installer cannot replace it either. Even where a genuine WSL-local binary coexists, the Windows daemon owns:11434under mirrored networking, so installing over it would start a service that cannot bind. The code does not separately prove CLI origin, only daemon origin.:11434, which is why the existing duplicate-daemon warning already draws this conclusion. Asystemctl is-active ollamaprobe would give positive origin evidence; that seemed like more mechanism than this defect earns, but it is the maintainers' call.test/onboard-selection.test.ts, whichTest file size budgetthen rejects at 4179 lines against a 4178 legacy budget.OllamaInstallMenuResult.binaryNeedsUpgrade?: booleanuses the same shape.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Advisor finding PRA-1
The PR review advisor's primary lane flagged that resume repair still applied the Linux systemd override to a recorded route pointing at the Windows daemon. Commit
c9eda3f3cfixes it:repairLocalInferenceSystemdOverrideOrExitnow takes a topology detector,detectWindowsDaemonOnWslLoopbackresolves the same fact from the same predicate for callers with no host snapshot, and a resume test assertsensureOllamaLoopbackSystemdOverrideis not called while model warm-up and validation still run. Verified by mutation: forcing the guard open fails that test withexpected "vi.fn()" to not be called at all, but actually been called 1 times.Two existing handler tests asserted the exact repair options object and were updated to include the new field. They encoded the old call shape, not old behavior.
Negative control
All six new tests, run against
git checkout origin/main --of the four changed source files with the new tests kept:With the change, the same command is
40 passed (40). The other 34 pass either way, so the change moves only the cases it claims to.The routing test was also checked by mutation: reverting just
args.isWindowsHostOllama || args.windowsDaemonOnWslLoopbacktoargs.isWindowsHostOllamafails it withexpected false to be true, so it is pinned to the systemd decision and not to incidental state.Blast radius
Every file in the repository matching
resolveOllamaInstallMenuEntry,OllamaInstallMenuInput,detectInferenceProviderHostState,InferenceProviderHostState,ollamaInstallMenu,windowsOllamaReachable,windowsDaemonOnWslLoopback,binaryNeedsUpgrade, orisValidOllamaTagsResponseBody, run in both lanes —integrationis not covered bynpm run test:changed:All 37 files under
src/lib/onboard/machine/handlers/also pass (486 tests).npm run typecheck:cli,npm run checks:repository,npx oxfmt --check,npx oxlint, andnpm run docsall pass onorigin/mainate746431d2.No existing test needed rewriting: the mirrored topology was untested, so nothing encoded the old behavior. The three Windows-host reachability mocks in
provider-host-state.test.tschanged from"{}"to'{"models":[]}'— the real/api/tagsshape, and the one the issue reports.The new tests live in
src/lib/onboard/setup-nim-flow-ollama-topology.test.tsrather thansetup-nim-flow.test.tsbecause that file is at the 1500-line budget;Test file size budgetrejected the commit until the test moved to a focused file.What this does not cover
detectInferenceProviderHostState,resolveOllamaInstallMenuEntry, andcreateSetupNimwith only I/O seams injected, but the end-to-end claim in [WSL2][Onboard] local Ollama onboard aborts after requesting a privileged reinstall when no models are present #9300 — that onboarding then pullsqwen2.5:1.5band reaches a Ready Hermes sandbox — is not something this machine can run. HenceRefs:rather thanCloses:. The pull itself already exists on the reuse path (src/lib/onboard.tsunderNEMOCLAW_YES=1, andsrc/lib/inference/ollama/proxy.tsover HTTP when no Linux CLI exists, [WSL2][Install] express install fails with "Install did not produce ollama.exe on PATH" when Ollama already installed #7472).windowsOllamaReachable, which requires Docker Desktop WSL integration. Widening it there would be wrong, not merely incomplete:local-inference-topology.tsstates that native Docker-in-WSL "cannot reliably route sandbox containers to Windows-host Ollama", so reusing the Windows daemon would hand the sandbox an endpoint it cannot reach.does not treat a loopback daemon as the Windows host without Docker Desktop routing (#9300)locks that boundary. Closing it properly needs a routing answer for those runtimes, which is a separate issue.0.32.9tool-call floor. That matches existing Windows-host behavior — NemoClaw does not offer to upgrade a Windows Ollama from WSL today either — but model validation may still fail against a sufficiently old daemon. Upgrading Ollama on the Windows side in this topology is not addressed here.Local Ollama (WSL:11434).resolveRunningOllamaMenuEntry(src/lib/onboard/ollama-install-menu.ts) branches onollamaHost === "host.docker.internal", which this change does not alter. It is the same misclassification in a display path; left out to keep the fix to the two paths that actually fail, and worth a follow-up.install-ollamaprovider is unaffected:resolveProviderKeyFallbackalready collapses it to the reuseollamaentry (src/lib/onboard/provider-key-fallback.ts), andscripts/install.sh:5093-5104only emitsinstall-ollamaon WSL when Docker Desktop is absent — the case this predicate cannot fire in.Two review suggestions deliberately not taken
/api/tagsbodies insidefindReachableOllamaHost. ApplyingisValidOllamaTagsResponseBodyat host discovery would harden every platform, not just this topology, and it changes which host onboarding selects everywhere.src/lib/inference/local.test.tscurrently asserts a non-JSON body is accepted there, so that is a standing contract worth its own issue rather than a rider on this fix. The Windows-side probe, which this change made load-bearing for a version-gate decision, is validated.Signed-off-by: harjoth harjoth.khara@gmail.com
Summary by CodeRabbit
New Features
Documentation