fix(onboard): detect installed Windows Ollama when daemon is stopped#4089
Conversation
Probe known install paths even when Ollama is not on PATH and not running, so WSL onboarding offers Start instead of Install. Fixes NVIDIA#4066.
📝 WalkthroughWalkthroughThis PR fixes detection of installed-but-not-running Ollama on Windows by removing a PID-visibility gate that was preventing fallback to known installer paths, and adds test coverage validating the corrected behavior. ChangesWindows Ollama Detection Fallback
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~15 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 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 |
![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
🤖 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/windows-host-ollama.test.ts`:
- Around line 6-10: The mock for runCapture has the wrong signature; update the
mock declaration and implementation to match the real function signature
runCapture(cmd: readonly string[], opts: CaptureOptions = {}): string by
changing the vi.fn to accept two parameters (cmd: readonly string[], opts?:
CaptureOptions) and updating the vi.mock implementation to forward both
arguments (command and opts) to the runCapture mock; import or reference the
CaptureOptions type if needed so TypeScript types line up with the real
runCapture used by the tests.
🪄 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: 1b2ee34a-c5d9-49ef-8684-74a3bfd69b7e
📒 Files selected for processing (2)
src/lib/onboard/windows-host-ollama.test.tssrc/lib/onboard/windows-host-ollama.ts
| const runCapture = vi.fn<(command: string | string[]) => string>(() => ""); | ||
|
|
||
| vi.mock("../runner", () => ({ | ||
| runCapture: (command: string | string[]) => runCapture(command), | ||
| })); |
There was a problem hiding this comment.
Fix mock signature to match the real runCapture signature.
The mock signature doesn't match the actual runCapture function. Looking at src/lib/runner.ts:216-261, the real signature is:
function runCapture(cmd: readonly string[], opts: CaptureOptions = {}): stringBut the mock only declares one parameter. This may cause TypeScript compilation errors when the code under test calls runCapture([...], { ignoreError: true }).
🔧 Proposed fix to match the real signature
-const runCapture = vi.fn<(command: string | string[]) => string>(() => "");
+const runCapture = vi.fn<(cmd: readonly string[], opts?: { ignoreError?: boolean }) => string>(() => "");
vi.mock("../runner", () => ({
- runCapture: (command: string | string[]) => runCapture(command),
+ runCapture: (cmd: readonly string[], opts?: { ignoreError?: boolean }) => runCapture(cmd, opts),
}));Then update the mock implementation to use the correct parameter name:
- runCapture.mockImplementation((command) => {
- const cmd = Array.isArray(command) ? command.join(" ") : command;
+ runCapture.mockImplementation((cmd) => {
+ const cmdStr = cmd.join(" ");
- if (cmd.includes("Get-Command ollama.exe")) return "";
+ if (cmdStr.includes("Get-Command ollama.exe")) return "";
// ... update other checks similarly
});🤖 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/windows-host-ollama.test.ts` around lines 6 - 10, The mock
for runCapture has the wrong signature; update the mock declaration and
implementation to match the real function signature runCapture(cmd: readonly
string[], opts: CaptureOptions = {}): string by changing the vi.fn to accept two
parameters (cmd: readonly string[], opts?: CaptureOptions) and updating the
vi.mock implementation to forward both arguments (command and opts) to the
runCapture mock; import or reference the CaptureOptions type if needed so
TypeScript types line up with the real runCapture used by the tests.
|
✨ Thanks for submitting this detailed PR about detecting installed Windows Ollama when the daemon is stopped. This proposes a fix where the onboarding process checks known Windows Ollama install paths even when the daemon is not running and Related open issues: |
wscurran
added
area: cli
|
Code change is right — gating the known-install-path probe on a live PID was the bug. Please address the CodeRabbit nit on |
|
Two non-blocking notes for the record:
|
Address review feedback on NVIDIA#4089.
|
Updated the runCapture mock signature to match runner.ts — thanks for the review. |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
LGTM — root-cause fix for #4066 (drop the PID guard so probeInstalledPath reaches the known-install-path fallback on installed-but-not-running Ollama).
CR mock-signature nit addressed in cc0e54f — mock now uses readonly string[] matching runner.ts:215. Substantive CI verified on cc0e54f: 10 success / 0 failures across CodeQL, ShellCheck, dco-check, commit-lint, etc. The pending jobs on the latest merge SHA are concurrency replays of doc-only main-rollforward — no code change between cc0e54f and 9370710 beyond .agents/skills/* and docs/*.mdx from main.
The earlier non-blocking suggestion about negative-path tests stands as optional follow-up; not gating approval.
|
Cool, I will take care of those optional minor changes you highlighted after this gets merged. |
## Summary Follow-up to #4089: add negative-path tests for `detectWindowsHostOllama` and clarify that `installed` reflects on-disk binary presence while onboard still gates Start/Restart on reachability (#3949). ## Related Issue Follow-up to #4089 (closed #4066). ## Changes - Add tests for `!isWsl()` early return and all-probes-miss → `installed: false` - Document consumer-side Start/Restart gating in `windows-host-ollama.ts` ## 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) ## Verification - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make 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: Thabhelo <50872400+Thabhelo@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved test coverage for Ollama detection on Windows. * **Documentation** * Added internal clarifications for detection behavior and system integration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 participants
Summary
Probe known Windows Ollama install paths even when the daemon is not running and
ollama.exeis missing from PATH, so WSL onboarding offers Start Ollama on Windows host instead of Install.Related Issue
Fixes #4066
Changes
GET_KNOWN_OLLAMA_INSTALL_PATHafter PATH/process probes inprobeInstalledPath()src/lib/onboard/windows-host-ollama.test.tsType of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Thabhelo 50872400+Thabhelo@users.noreply.github.com
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests