fix(onboard): detect installed Windows Ollama when daemon is stopped#4089
fix(onboard): detect installed Windows Ollama when daemon is stopped#4089Thabhelo wants to merge 1 commit into
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.
1 participant
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