fix(onboard): detect Windows-host Ollama via process probe#3969
Open
laitingsheng wants to merge 2 commits into
Open
fix(onboard): detect Windows-host Ollama via process probe#3969laitingsheng wants to merge 2 commits into
laitingsheng wants to merge 2 commits into
Conversation
`hasWindowsOllama` was probed only with `powershell.exe Get-Command ollama.exe`, which returns the binary path only when ollama.exe is on the calling user's PATH. Service-style installs (and other installers that do not update the user PATH) leave ollama.exe undiscoverable by Get-Command even when the daemon is running. Under the WSL2 repro in #3949 — Windows-host Ollama listening on 127.0.0.1:11434 only, host.docker.internal unresolvable from WSL — the PATH miss caused `hasWindowsOllama` to be false, so the inference menu showed "Install Ollama on Windows host (recommended)" instead of the "Restart Ollama on Windows host with 0.0.0.0 binding" variant. Fall back to a `Get-Process ollama` probe when the PATH lookup misses: a live ollama process is sufficient proof of installation, and the existing `winOllamaLoopbackOnly` block already uses Get-Process for PID and Get-NetTCPConnection for the bind address, so the loopback- only menu variant fires correctly once `hasWindowsOllama` is true. Add a regression test that asserts setupNim with NEMOCLAW_PROVIDER=start-windows-ollama is satisfied — and the loopback restart path is exercised — when Get-Command returns empty but Get-Process reports a live PID. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
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 (3)
📝 WalkthroughWalkthroughReplaces inline WSL PowerShell probing with a new ChangesWindows Ollama Fallback Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Contributor
E2E Advisor RecommendationRequired E2E: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review is based on trusted PR metadata, linked issue content, provided diff, and read-only repository inspection; no scripts, tests, package-manager commands, or network operations were executed.; Issue #3949 has no issue comments in the provided context; acceptance coverage maps the issue body clauses only.; CI and E2E results were still pending for the current head SHA at the time of review.; The E2E Advisor comment was found, but the required wsl-e2e job had not passed for 41752fe.; Interactive menu label rendering was inferred from setupNim state and existing label logic; the added test exercises non-interactive provider selection rather than asserting the literal menu output. Full advisor summaryPR Review AdvisorBase: The code change is narrowly scoped and improves the WSL Windows-host Ollama detection path, but merge is blocked by pending CI, BLOCKED merge state, and required WSL E2E not yet passed for the current head SHA. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
![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: 1
🤖 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 `@src/lib/onboard.ts`:
- Around line 6100-6117: The new fallback probe that sets hasWindowsOllama using
runCapture and winOllamaPid should be extracted into a small helper function
(e.g., isWindowsOllamaRunning or probeWindowsOllama) placed under
src/lib/onboard/; replace the inline block in onboard.ts (the else branch that
calls runCapture and computes hasWindowsOllama) with a call to that helper so
onboard.ts has no net line growth. Ensure the helper exports a boolean-returning
function that performs the powershell Get-Process call (using runCapture) and is
imported/used by the existing code to assign hasWindowsOllama.
🪄 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: 7c204475-e79b-41fe-8af6-1af18325ca11
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard-selection.test.ts
Addresses three Codex findings on the original #3949 fix: * P1 (onboard-entrypoint-budget): move the WSL/Windows-host Ollama detection out of src/lib/onboard.ts and into a focused module at src/lib/onboard/windows-host-ollama.ts. setupNim now calls a single `detectWindowsHostOllama()` helper that returns `{ installed, installedPath, loopbackOnly }`. Net change to onboard.ts is now negative (-16), so the entrypoint budget check passes. * P2 (path recovery): when `Get-Command ollama.exe` misses, also capture the running ollama process's `.Path`, not just its PID. Recover both the install signal and the verified executable path from a single source. If `.Path` cannot be recovered (e.g. SYSTEM-owned service the user shell cannot inspect), the install is deliberately left flagged as not-installed so the restart path does not kill a daemon it cannot relaunch. * P2 (restart wiring): forward the recovered `installedPath` to `setupWindowsOllamaWith0000Binding` on the restart branch as well, not just on the install branch. This lets windows.ts target the verified binary directly via Start-Process instead of falling back to the calling shell's Windows PATH — which is the broken case #3949 surfaces. * Tighten the regression test to assert the exact shape of the setupCalls argument (announceStop:true + installedPath threaded through), and update the existing "uses the Windows-host start path when install-windows-ollama is requested but Ollama is already installed" assertion to match the new shape (installedPath now always plumbed when known). P3 (docs) is unaffected because the code keeps the "verified executable path" guarantee — the new fallback only adds a second source for that verification (Get-Process .Path) and refuses to treat the daemon as installed when neither source returns a path. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
12 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
1 participant
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
On WSL2,
hasWindowsOllamawas determined solely by runningpowershell.exe Get-Command ollama.exe, which only returns a path whenollama.exeis on the calling user's PATH. Service-style installs (and any installer that does not update the user PATH) leave the binary undiscoverable byGet-Commandeven while the Windows-host Ollama daemon is running on127.0.0.1:11434.In that state, the inference menu offered "Install Ollama on Windows host (recommended)" instead of the "Restart Ollama on Windows host with 0.0.0.0 binding" variant that the bug report and the WSL2 Ollama test expect.
Fall back to a
Get-Process ollamaprobe whenGet-Commandreturns empty: a live ollama process is sufficient proof of installation. The existingwinOllamaLoopbackOnlyblock already usesGet-Processfor the PID andGet-NetTCPConnectionfor the listen address, so oncehasWindowsOllamais true the loopback-only menu variant fires correctly.Related Issue
Fixes #3949
Changes
Get-Command ollama.exereturns empty, fall back toGet-Process ollamaand treat a live PID as proof of installation. Bracketed byif (isWsl())as before.NEMOCLAW_PROVIDER=start-windows-ollamais satisfied andsetupWindowsOllamaWith0000Bindingis invoked exactly once.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests