fix(onboard): offer Ollama upgrade when host version too old#4186
Merged
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds MIN_OLLAMA_VERSION and utilities to detect/compare Ollama CLI and daemon versions, surfaces Install/Upgrade menu entries, integrates macOS/Linux installers with upgrade semantics and verification, updates onboarding wiring and docs, and adds unit and integration tests for these flows. ChangesOllama version detection and upgrade flow
Sequence DiagramsequenceDiagram
participant User as User/NemoClaw CLI
participant Resolver as resolveOllamaInstallMenuEntry
participant RunCapture as runCapture (shell)
participant Installer as Installer (brew / install.sh)
participant Daemon as Ollama daemon
User->>Resolver: probe installed and daemon versions
Resolver->>RunCapture: ["ollama","--version"]
RunCapture-->>Resolver: binary version stdout
Resolver->>RunCapture: ["curl","http://127.0.0.1:11434/api/version"]
RunCapture-->>Resolver: daemon JSON {version}
Resolver->>User: show Install/Upgrade menu entry when below MIN_OLLAMA_VERSION
User->>Installer: run installer (brew / install.sh) with isUpgrade flag
Installer->>Daemon: start/restart daemon
User->>RunCapture: ["curl","http://127.0.0.1:11434/api/version"]
RunCapture-->>User: daemon version JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
Contributor
PR Review AdvisorFindings: 0 needs attention, 2 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. |
![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
🧹 Nitpick comments (2)
src/lib/onboard.ts (2)
4225-4232: Run the onboarding E2E set for this change before merge.These updates touch non-interactive failure control flow and provider-option generation in core onboarding; I’d run the listed onboarding-related E2Es (or the selective
nightly-e2e.yamljobs command) for regression coverage.As per coding guidelines:
src/lib/onboard.ts: “This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow.” and includes the explicit E2E test recommendation list.Also applies to: 4274-4414
🤖 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 4225 - 4232, The change alters non-interactive failure control flow around the validation check (the block using validation.ok and isNonInteractive()) and other onboarding provider-option generation (areas around lines covering the onboarding flow, e.g., code referenced between 4274-4414); before merging, run the onboarding end-to-end tests (the listed onboarding-related E2E jobs or the selective nightly-e2e.yaml job) to exercise non-interactive paths and provider-option generation, reproduce the failure scenario where validation fails in non-interactive mode, and fix any regressions found (restore expected behavior for isNonInteractive() exit path and provider option outputs) before approving the PR.
4400-4406: ⚡ Quick winAlign macOS “Upgrade Ollama” labeling with Homebrew upgrade behavior
labelPrefixbecomes “Upgrade Ollama” whenhasUpgradableOllamais true, but the macOS handler still runsbrew install ollama. Homebrew typically upgrades an already-installed outdated formula viabrew install(unlessHOMEBREW_NO_INSTALL_UPGRADEis set), so this usually won’t leave Ollama outdated—however switching tobrew upgrademakes the behavior match the UI and is more robust to that env var.💡 Proposed fix
} else if (selected.key === "install-ollama") { if (!checkOllamaPortsOrWarn()) continue selectionLoop; if (process.platform === "darwin") { - console.log(" Installing Ollama via Homebrew..."); - run(["brew", "install", "ollama"], { ignoreError: true }); + if (hasUpgradableOllama) { + console.log(" Upgrading Ollama via Homebrew..."); + run(["brew", "upgrade", "ollama"], { ignoreError: true }); + } else { + console.log(" Installing Ollama via Homebrew..."); + run(["brew", "install", "ollama"], { ignoreError: true }); + } // brew install doesn't auto-start a service; launch directly. // Shell required: backgrounding (&), env var prefix, output redirection. console.log(" Starting Ollama...");🤖 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 4400 - 4406, The macOS branch builds a menu item using labelPrefix/upgradeSuffix when hasUpgradableOllama is true but still invokes `brew install ollama`; update the macOS handler (the branch where process.platform === "darwin" and the options entry with key "install-ollama") so that when hasUpgradableOllama is true it runs `brew upgrade ollama` (or calls the upgrade flow) instead of `brew install ollama`, keeping the labelPrefix/upgradeSuffix logic intact so UI and action match; ensure the existing installedOllamaVersion and MIN_OLLAMA_VERSION logic are preserved when choosing the command.
🤖 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 4274-4276: The new Ollama version/menu logic
(installedOllamaVersion, hasUpgradableOllama) should be extracted out of the
large onboard entrypoint into a small helper module: create a new helper (e.g.,
an ollama utility) that exports a function like getOllamaStatus() which
internally calls getInstalledOllamaVersion(), isOllamaVersionAtLeast(), and
references MIN_OLLAMA_VERSION to compute and return { installedOllamaVersion,
hasUpgradableOllama }; then replace the inline logic in onboard.ts with a single
import and call to getOllamaStatus() so the entrypoint stays thin and the heavy
logic moves behind the helper.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 4225-4232: The change alters non-interactive failure control flow
around the validation check (the block using validation.ok and
isNonInteractive()) and other onboarding provider-option generation (areas
around lines covering the onboarding flow, e.g., code referenced between
4274-4414); before merging, run the onboarding end-to-end tests (the listed
onboarding-related E2E jobs or the selective nightly-e2e.yaml job) to exercise
non-interactive paths and provider-option generation, reproduce the failure
scenario where validation fails in non-interactive mode, and fix any regressions
found (restore expected behavior for isNonInteractive() exit path and provider
option outputs) before approving the PR.
- Around line 4400-4406: The macOS branch builds a menu item using
labelPrefix/upgradeSuffix when hasUpgradableOllama is true but still invokes
`brew install ollama`; update the macOS handler (the branch where
process.platform === "darwin" and the options entry with key "install-ollama")
so that when hasUpgradableOllama is true it runs `brew upgrade ollama` (or calls
the upgrade flow) instead of `brew install ollama`, keeping the
labelPrefix/upgradeSuffix logic intact so UI and action match; ensure the
existing installedOllamaVersion and MIN_OLLAMA_VERSION logic are preserved when
choosing the command.
🪄 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: 08557d2b-a65d-4f08-8d77-77f8f8ffcad2
📒 Files selected for processing (3)
src/lib/inference/local.test.tssrc/lib/inference/local.tssrc/lib/onboard.ts
…budget Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4186.docs.buildwithfern.com/nemoclaw |
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/inference/ollama-version.ts (1)
12-15: ⚡ Quick winType signature mismatch with actual
runCaptureimplementation.The
OllamaVersionRunCapturetype acceptsstring | string[], but the actualrunCapturefunction fromrunner.ts(lines 214-218) only acceptsreadonly string[]and explicitly throws on strings with the error: "runCapture no longer accepts shell strings; pass an argv array instead".Additionally,
local.tsalready definesRunCaptureFnat line 79 with an identical signature, creating duplication.Consider either:
- Reusing the existing
RunCaptureFntype instead of defining a duplicate, or- Narrowing this type to match the actual implementation:
(cmd: readonly string[], opts?: { ignoreError?: boolean }) => string♻️ Proposed fix: Reuse existing type
-export type OllamaVersionRunCapture = ( - cmd: string | string[], - opts?: { ignoreError?: boolean }, -) => string; +import type { RunCaptureFn } from "./local";Then update the function signature at line 26:
export function getInstalledOllamaVersion( - runCaptureImpl?: OllamaVersionRunCapture, + runCaptureImpl?: RunCaptureFn, ): string | null {🤖 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/inference/ollama-version.ts` around lines 12 - 15, The OllamaVersionRunCapture type is incompatible with the real runCapture (which only accepts readonly string[] and throws on shell strings) and duplicates RunCaptureFn from local.ts; replace the new type by reusing the existing RunCaptureFn (or if you prefer, change OllamaVersionRunCapture to (cmd: readonly string[], opts?: { ignoreError?: boolean }) => string) so signatures match runCapture, and remove the redundant duplicate type; update any references to use RunCaptureFn (or the narrowed signature) to ensure callers and the implementation align.
🤖 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 5268-5270: The brew upgrade path currently swallows errors (see
brewAction, ollamaInstallMenu.hasUpgradableOllama and run([...], { ignoreError:
true })); change the logic so that when brewAction === "upgrade" you do not
ignore errors: run the upgrade and check its exit status/output, and if it
fails, log the failure (including the error), then attempt a fallback (e.g.,
run(["brew","install","ollama"]) without ignoreError) or abort/exit with a clear
error so we don't continue with an unknown Ollama state; keep the install path
behavior unchanged.
---
Nitpick comments:
In `@src/lib/inference/ollama-version.ts`:
- Around line 12-15: The OllamaVersionRunCapture type is incompatible with the
real runCapture (which only accepts readonly string[] and throws on shell
strings) and duplicates RunCaptureFn from local.ts; replace the new type by
reusing the existing RunCaptureFn (or if you prefer, change
OllamaVersionRunCapture to (cmd: readonly string[], opts?: { ignoreError?:
boolean }) => string) so signatures match runCapture, and remove the redundant
duplicate type; update any references to use RunCaptureFn (or the narrowed
signature) to ensure callers and the implementation align.
🪄 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: 27936163-876d-498d-8b54-ba2bd6f25e26
📒 Files selected for processing (8)
docs/inference/use-local-inference.mdxsrc/lib/inference/local.tssrc/lib/inference/ollama-version.test.tssrc/lib/inference/ollama-version.tssrc/lib/onboard.tssrc/lib/onboard/ollama-install-menu.test.tssrc/lib/onboard/ollama-install-menu.tstest/onboard-selection.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/inference/ollama-version.test.ts
- docs/inference/use-local-inference.mdx
…stall Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/install-ollama-linux.ts (1)
113-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock
NEMOCLAW_OLLAMA_INSTALL_MODE=userduring upgrades.
isUpgradeonly takes effect after the env override path, soNEMOCLAW_OLLAMA_INSTALL_MODE=userstill selectsuser-localhere. That leaves the old system daemon on:11434, which defeats the upgrade flow this PR is adding. Please reject that combination explicitly and add the matching regression test.Proposed fix
export function decideInstallOllamaLinuxMode( opts: InstallOllamaLinuxOptions, ): InstallOllamaLinuxMode { if (opts.modeOverride) return opts.modeOverride; const explicit = String(process.env[INSTALL_MODE_ENV] || "").trim().toLowerCase(); + if (opts.isUpgrade && explicit === "user") { + const errorLog = opts.errorLog ?? ((m: string) => console.error(m)); + errorLog( + ` ${INSTALL_MODE_ENV}=user is not supported while upgrading Ollama because it would leave the stale system daemon on :${OLLAMA_PORT}.`, + ); + process.exit(1); + } if (explicit === "user") return "user-local"; if (explicit === "system") return "system";Also applies to: 127-142
🧹 Nitpick comments (1)
docs/inference/use-local-inference.mdx (1)
34-34: ⚡ Quick winSplit this long paragraph into one sentence per source line.
Line 34 currently packs several sentences onto one line, which violates the docs diff/readability rule.
As per coding guidelines: "One sentence per line in source (makes diffs readable)."
🤖 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 `@docs/inference/use-local-inference.mdx` at line 34, The long paragraph describing the Ollama upgrade flow should be split so each sentence is on its own source line: break the single combined paragraph into separate sentences (mentioning the "Upgrade Ollama" provider entry, the express setup resolving to that entry, the platform install/upgrade commands like `brew upgrade ollama` and `https://ollama.com/install.sh`, the Linux sudo behavior and fallback, and the final `ollama --version` re-probe) and place each sentence on its own line in the MDX source so diffs/readability rules are satisfied.
🤖 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/install-ollama-macos.ts`:
- Around line 54-56: The returned binPath currently hardcodes
/opt/homebrew/bin/ollama which fails on Intel macs; update the logic in the
function that returns { ok: ..., binPath: ... } (in
src/lib/onboard/install-ollama-macos.ts) to compute the binary path dynamically:
first try to resolve the binary with a platform-safe lookup (e.g.,
spawnSync('which'/'command -v') or fs.existsSync checks), fallback to
architecture-based defaults (use /opt/homebrew/bin/ollama for arm64/Apple
Silicon and /usr/local/bin/ollama for x86_64), and return the resolved path (or
null/empty) rather than always /opt/homebrew/bin/ollama so Intel installs are
supported. Ensure the function uses process.arch or uname detection and checks
existence before returning the path.
---
Nitpick comments:
In `@docs/inference/use-local-inference.mdx`:
- Line 34: The long paragraph describing the Ollama upgrade flow should be split
so each sentence is on its own source line: break the single combined paragraph
into separate sentences (mentioning the "Upgrade Ollama" provider entry, the
express setup resolving to that entry, the platform install/upgrade commands
like `brew upgrade ollama` and `https://ollama.com/install.sh`, the Linux sudo
behavior and fallback, and the final `ollama --version` re-probe) and place each
sentence on its own line in the MDX source so diffs/readability rules are
satisfied.
🪄 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: f3c8284b-074f-452e-8478-d2d95fb81319
📒 Files selected for processing (8)
docs/inference/use-local-inference.mdxsrc/lib/inference/ollama-version.tssrc/lib/onboard.tssrc/lib/onboard/install-ollama-linux.test.tssrc/lib/onboard/install-ollama-linux.tssrc/lib/onboard/install-ollama-macos.tssrc/lib/onboard/ollama-install-menu.test.tssrc/lib/onboard/ollama-install-menu.ts
… override Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
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/inference/ollama-version.ts`:
- Around line 44-47: The new helper getRunningOllamaDaemonVersion declared in
src/lib/inference/ollama-version.ts is not exported from the local inference
facade; update src/lib/inference/local.ts to re-export
getRunningOllamaDaemonVersion alongside the existing Ollama version helpers (the
same place where other functions from ollama-version are re-exported) so the
public API remains consistent; locate the export block that currently re-exports
other Ollama helpers and add getRunningOllamaDaemonVersion to that list.
🪄 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: 6c581f66-7b17-42ae-b2a4-594df0122b07
📒 Files selected for processing (7)
src/lib/inference/ollama-version.test.tssrc/lib/inference/ollama-version.tssrc/lib/onboard/install-ollama-linux.test.tssrc/lib/onboard/install-ollama-linux.tssrc/lib/onboard/ollama-install-menu.test.tssrc/lib/onboard/ollama-install-menu.tstest/onboard-selection.test.ts
…grade menu Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
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/ollama-install-menu.ts`:
- Around line 86-90: The upgrade suffix text currently always says "installed
..." even though reportedVersion may come from runningOllamaVersion; change it
to reflect the actual source: compute a sourceLabel (e.g., sourceLabel =
runningOllamaVersion != null ? "running" : "installed") and use that in the
template for upgradeSuffix, e.g., ` — upgrade ${sourceLabel} ${reportedVersion
?? "unknown"} to ≥ ${MIN_OLLAMA_VERSION}`; update the code paths around
reportedVersion, runningOllamaVersion, installedOllamaVersion,
hasUpgradableOllama and upgradeSuffix so the label matches the chosen version
source and still handles null reportedVersion.
🪄 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: e125f4ea-0ee5-4d82-82c8-583b092c3205
📒 Files selected for processing (5)
docs/inference/use-local-inference.mdxsrc/lib/inference/local.tssrc/lib/onboard/install-ollama-macos.tssrc/lib/onboard/ollama-install-menu.test.tssrc/lib/onboard/ollama-install-menu.ts
…el stale source Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-double-onboard-e2e | ⏭️ skipped |
| gpu-e2e | ⏭️ skipped |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/onboard/ollama-install-menu.ts (1)
112-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a source-aware suffix instead of always saying “installed”.
Line 118 always renders
upgrade installed ..., but Lines 112-116 can intentionally report the daemon as the stale source. This makes the prompt misleading.💡 Minimal fix
- const reportedVersion = daemonNeedsUpgrade + const reportedSource = daemonNeedsUpgrade + ? "daemon" + : binaryNeedsUpgrade + ? "binary" + : "installed"; + const reportedVersion = daemonNeedsUpgrade ? (runningOllamaVersion ?? installedOllamaVersion) : binaryNeedsUpgrade ? (installedOllamaVersion ?? runningOllamaVersion) : null; const upgradeSuffix = hasUpgradableOllama - ? ` — upgrade installed ${reportedVersion ?? "unknown"} to ≥ ${MIN_OLLAMA_VERSION}` + ? ` — upgrade ${reportedSource} ${reportedVersion ?? "unknown"} to ≥ ${MIN_OLLAMA_VERSION}` : "";🤖 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/ollama-install-menu.ts` around lines 112 - 119, The prompt always says "upgrade installed ..." even when the stale source is the running daemon; update the logic that builds upgradeSuffix to choose a source-aware label based on which source produced reportedVersion (use daemonNeedsUpgrade to label as "running daemon", binaryNeedsUpgrade to label as "installed binary", fall back to "source" or "unknown" when needed). Modify the upgradeSuffix construction (and if helpful add a small helper variable like sourceLabel) so it reads e.g. `— upgrade [installed binary|running daemon|source unknown] ${reportedVersion ?? "unknown"} to ≥ ${MIN_OLLAMA_VERSION}` and still only shows when hasUpgradableOllama is true, keeping the existing reportedVersion and MIN_OLLAMA_VERSION symbols.
🤖 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.
Duplicate comments:
In `@src/lib/onboard/ollama-install-menu.ts`:
- Around line 112-119: The prompt always says "upgrade installed ..." even when
the stale source is the running daemon; update the logic that builds
upgradeSuffix to choose a source-aware label based on which source produced
reportedVersion (use daemonNeedsUpgrade to label as "running daemon",
binaryNeedsUpgrade to label as "installed binary", fall back to "source" or
"unknown" when needed). Modify the upgradeSuffix construction (and if helpful
add a small helper variable like sourceLabel) so it reads e.g. `— upgrade
[installed binary|running daemon|source unknown] ${reportedVersion ?? "unknown"}
to ≥ ${MIN_OLLAMA_VERSION}` and still only shows when hasUpgradableOllama is
true, keeping the existing reportedVersion and MIN_OLLAMA_VERSION symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7bd4c503-a7be-49a9-98e6-221297d27b03
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/ollama-install-menu.test.tssrc/lib/onboard/ollama-install-menu.ts
…source Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/inference/use-local-inference.mdx (1)
35-35: ⚡ Quick winSplit into separate lines and use active voice.
This line contains two sentences on the same line, which makes diffs harder to read.
Additionally, "is intentionally excluded" uses passive voice.As per coding guidelines: "One sentence per line in source (makes diffs readable)" and "Active voice required. Flag passive constructions."
Suggested rewrite
-The wizard inspects both the CLI binary (`ollama --version`) and the locally running daemon (`/api/version` on `:11434`) so the upgrade entry still appears when only one side is stale, for example a fresh user-local binary paired with the original system daemon. Windows-host Ollama reached from WSL via `host.docker.internal` is intentionally excluded from this gate — that case is handled by the separate **Use / Start / Install Ollama on Windows host** entries, which run their own actions on the Windows side. +The wizard inspects both the CLI binary (`ollama --version`) and the locally running daemon (`/api/version` on `:11434`) so the upgrade entry still appears when only one side is stale, for example a fresh user-local binary paired with the original system daemon. +The wizard intentionally excludes Windows-host Ollama reached from WSL via `host.docker.internal` from this gate — that case is handled by the separate **Use / Start / Install Ollama on Windows host** entries, which run their own actions on the Windows side.🤖 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 `@docs/inference/use-local-inference.mdx` at line 35, Split the long sentence into multiple lines so each sentence occupies its own source line and rewrite the passive phrase "is intentionally excluded" into active voice; specifically, break the current line describing the checks of the CLI binary (`ollama --version`) and the locally running daemon (`/api/version` on `:11434`) into separate sentences/lines, and change the Windows-host case sentence to an active form that names the actor (e.g., "We/The system exclude Windows-host Ollama reached from WSL via `host.docker.internal` from this gate — the separate Use / Start / Install Ollama on Windows host entries handle that case by running their own actions on the Windows side.").
🤖 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/install-ollama-linux.ts`:
- Around line 442-445: The code currently pins the resolved host to 127.0.0.1
immediately after calling installOllamaUserLocal/installOllamaSystem, which can
overwrite a previously discovered host when installOllamaSystem returns ok true
via the overrideState === "not-applicable" path (which uses
findReachableOllamaHostImpl); instead, change the install paths to return the
actually validated host (e.g., include validatedHost on the result of
installOllamaUserLocal/installOllamaSystem) or only call
setResolvedOllamaHost("127.0.0.1") inside the branches within
installOllamaUserLocal/installOllamaSystem that explicitly start or verify a
loopback daemon (the branches that confirm loopback connectivity), ensuring
setResolvedOllamaHost is invoked with the real validated host rather than
unconditionally after the install call.
---
Nitpick comments:
In `@docs/inference/use-local-inference.mdx`:
- Line 35: Split the long sentence into multiple lines so each sentence occupies
its own source line and rewrite the passive phrase "is intentionally excluded"
into active voice; specifically, break the current line describing the checks of
the CLI binary (`ollama --version`) and the locally running daemon
(`/api/version` on `:11434`) into separate sentences/lines, and change the
Windows-host case sentence to an active form that names the actor (e.g., "We/The
system exclude Windows-host Ollama reached from WSL via `host.docker.internal`
from this gate — the separate Use / Start / Install Ollama on Windows host
entries handle that case by running their own actions on the Windows side.").
🪄 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: cf2800ec-0d92-435f-9941-7ad6a7664915
📒 Files selected for processing (6)
docs/inference/use-local-inference.mdxsrc/lib/onboard/install-ollama-linux.test.tssrc/lib/onboard/install-ollama-linux.tssrc/lib/onboard/install-ollama-macos.tssrc/lib/onboard/ollama-install-menu.test.tssrc/lib/onboard/ollama-install-menu.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26408223904
|
…responded Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/onboard/install-ollama-linux.ts (1)
418-425:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-probe loopback before treating the install as ready.
findReachableOllamaHost()is cached for the rest of the onboard run, so this can reuse an earlier127.0.0.1success instead of the daemon state after the install. On the non-systemd upgrade path that means we can skipollama serve, then Line 453 pins validation to loopback while the old daemon is still the one answering there. Use a fresh loopback probe/restart here instead of relying on the cached host string.🤖 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/install-ollama-linux.ts` around lines 418 - 425, The code currently relies on the cached findReachableOllamaHost() result; instead, re-run an uncached probe here before deciding the install is ready by calling the lower-level probe function findReachableOllamaHostImpl() (or otherwise performing a fresh loopback probe) and compute localDaemonReachable from that fresh result (check against "127.0.0.1" / "localhost"); if the fresh probe is not loopback, proceed with the start/restart path rather than treating overrideState === "not-applicable" as ready. Ensure you replace any use of the cached findReachableOllamaHost() for this readiness decision and keep references to overrideState and localDaemonReachable.
🤖 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.
Duplicate comments:
In `@src/lib/onboard/install-ollama-linux.ts`:
- Around line 418-425: The code currently relies on the cached
findReachableOllamaHost() result; instead, re-run an uncached probe here before
deciding the install is ready by calling the lower-level probe function
findReachableOllamaHostImpl() (or otherwise performing a fresh loopback probe)
and compute localDaemonReachable from that fresh result (check against
"127.0.0.1" / "localhost"); if the fresh probe is not loopback, proceed with the
start/restart path rather than treating overrideState === "not-applicable" as
ready. Ensure you replace any use of the cached findReachableOllamaHost() for
this readiness decision and keep references to overrideState and
localDaemonReachable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2517848b-f066-49b7-94e9-e9739c22ad14
📒 Files selected for processing (3)
docs/inference/use-local-inference.mdxsrc/lib/onboard/install-ollama-linux.test.tssrc/lib/onboard/install-ollama-linux.ts
✅ Files skipped from review due to trivial changes (1)
- docs/inference/use-local-inference.mdx
…lver result Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
Contributor
There was a problem hiding this comment.
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/install-ollama-linux.ts (1)
114-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
NEMOCLAW_OLLAMA_INSTALL_MODE=systemupgrades the same as other headless upgrades.If
isUpgradeis true, Line 129 returns"system"before the non-interactive/no-sudo guard at Lines 145-153. In a headless run withNEMOCLAW_OLLAMA_INSTALL_MODE=systemand no passwordless sudo, we still fall into the sudo-driven installer instead of failing with the actionable upgrade message this PR is trying to guarantee.Suggested fix
export function decideInstallOllamaLinuxMode( opts: InstallOllamaLinuxOptions, ): InstallOllamaLinuxMode { if (opts.modeOverride) return opts.modeOverride; const explicit = String(process.env[INSTALL_MODE_ENV] || "").trim().toLowerCase(); + const isTty = opts.isTty ?? (() => Boolean(process.stdin.isTTY)); + const exitIfHeadlessUpgradeWithoutSudo = () => { + if (!opts.isUpgrade || canRunSudoNonInteractive(opts)) return; + if (!opts.isNonInteractive() && isTty()) return; + const errorLog = opts.errorLog ?? ((m: string) => console.error(m)); + errorLog( + " Upgrading the system Ollama requires sudo, which is not available in this non-interactive run.", + ); + errorLog( + " Run interactively, configure passwordless sudo, or upgrade manually: curl -fsSL https://ollama.com/install.sh | sh", + ); + process.exit(1); + }; if (opts.isUpgrade && explicit === "user") { const errorLog = opts.errorLog ?? ((m: string) => console.error(m)); errorLog( ` ${INSTALL_MODE_ENV}=user is incompatible with the Ollama upgrade path:`, ); @@ } if (explicit === "user") return "user-local"; - if (explicit === "system") return "system"; + if (explicit === "system") { + exitIfHeadlessUpgradeWithoutSudo(); + return "system"; + } if (explicit) { const errorLog = opts.errorLog ?? ((m: string) => console.error(m)); errorLog( ` Unsupported ${INSTALL_MODE_ENV} value: ${explicit}. Use 'system', 'user', or leave it unset.`, ); @@ const getEuid = opts.getEuid ?? (() => process.getuid?.()); if (getEuid() === 0) return "system"; if (canRunSudoNonInteractive(opts)) return "system"; - const isTty = opts.isTty ?? (() => Boolean(process.stdin.isTTY)); if (opts.isUpgrade) { - if (opts.isNonInteractive() || !isTty()) { - const errorLog = opts.errorLog ?? ((m: string) => console.error(m)); - errorLog( - " Upgrading the system Ollama requires sudo, which is not available in this non-interactive run.", - ); - errorLog( - " Run interactively, configure passwordless sudo, or upgrade manually: curl -fsSL https://ollama.com/install.sh | sh", - ); - process.exit(1); - } + exitIfHeadlessUpgradeWithoutSudo(); return "system"; }Also applies to: 141-156
🤖 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/install-ollama-linux.ts` around lines 114 - 129, The current logic returns "system" when INSTALL_MODE_ENV is set to "system" before the headless/no-sudo upgrade guard runs, so a headless upgrade with no passwordless sudo incorrectly proceeds to the sudo-driven installer; update the branch handling the explicit variable so that when opts.isUpgrade is true and explicit === "system" you treat it like other headless upgrades (emit the same actionable error via opts.errorLog and call process.exit(1)) instead of immediately returning "system"; locate the check using INSTALL_MODE_ENV, opts.isUpgrade and the explicit variable in install-ollama-linux.ts and add the isUpgrade + explicit==="system" guard (mirroring the existing opts.isUpgrade + explicit==="user" behavior) before returning "system".
🤖 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/install-ollama-linux.ts`:
- Around line 114-129: The current logic returns "system" when INSTALL_MODE_ENV
is set to "system" before the headless/no-sudo upgrade guard runs, so a headless
upgrade with no passwordless sudo incorrectly proceeds to the sudo-driven
installer; update the branch handling the explicit variable so that when
opts.isUpgrade is true and explicit === "system" you treat it like other
headless upgrades (emit the same actionable error via opts.errorLog and call
process.exit(1)) instead of immediately returning "system"; locate the check
using INSTALL_MODE_ENV, opts.isUpgrade and the explicit variable in
install-ollama-linux.ts and add the isUpgrade + explicit==="system" guard
(mirroring the existing opts.isUpgrade + explicit==="user" behavior) before
returning "system".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 22cb5bfa-9285-4bbf-ba55-76ed17de23c8
📒 Files selected for processing (2)
src/lib/onboard/install-ollama-linux.test.tssrc/lib/onboard/install-ollama-linux.ts
…ck test fake-curl Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
|
One macOS upgrade edge case looks unresolved: If the old daemon is still bound to the port, the new |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/install-ollama-linux.ts (1)
411-412: 💤 Low valueRemove unused
findReachableOllamaHostImplvariable.This variable is defined but never used in
installOllamaSystem—the implementation now useswaitForHttpImpldirectly for the fresh loopback probe. Consider also removing the corresponding option fromInstallOllamaLinuxOptions(lines 73-74) and the import since it's no longer needed as a test seam here.♻️ Proposed cleanup
function installOllamaSystem(opts: InstallOllamaLinuxOptions): InstallOllamaLinuxResult { const log = opts.log ?? ((m: string) => console.log(m)); const errorLog = opts.errorLog ?? ((m: string) => console.error(m)); const runShellImpl = opts.runShellImpl ?? runShell; const sleepSecondsImpl = opts.sleepSecondsImpl ?? sleepSeconds; const waitForHttpImpl = opts.waitForHttpImpl ?? waitForHttp; - const findReachableOllamaHostImpl = - opts.findReachableOllamaHostImpl ?? findReachableOllamaHost; const ensureOverrideImpl =🤖 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/install-ollama-linux.ts` around lines 411 - 412, Remove the unused findReachableOllamaHostImpl binding and its test seam: delete the const findReachableOllamaHostImpl = opts.findReachableOllamaHostImpl ?? findReachableOllamaHost; from installOllamaSystem, remove the corresponding optional property (findReachableOllamaHostImpl) from the InstallOllamaLinuxOptions type, and drop the related import for findReachableOllamaHost since installOllamaSystem now uses waitForHttpImpl directly for the loopback probe; ensure no other code in this module references findReachableOllamaHostImpl before committing.
🤖 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/install-ollama-linux.ts`:
- Around line 411-412: Remove the unused findReachableOllamaHostImpl binding and
its test seam: delete the const findReachableOllamaHostImpl =
opts.findReachableOllamaHostImpl ?? findReachableOllamaHost; from
installOllamaSystem, remove the corresponding optional property
(findReachableOllamaHostImpl) from the InstallOllamaLinuxOptions type, and drop
the related import for findReachableOllamaHost since installOllamaSystem now
uses waitForHttpImpl directly for the loopback probe; ensure no other code in
this module references findReachableOllamaHostImpl before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 29aa2444-4402-4da7-a1e7-8c3a136531ac
📒 Files selected for processing (4)
docs/inference/use-local-inference.mdxsrc/lib/onboard/install-ollama-linux.test.tssrc/lib/onboard/install-ollama-linux.tstest/onboard-selection.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/inference/use-local-inference.mdx
…Linux probe seam Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
Contributor
There was a problem hiding this comment.
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/install-ollama-linux.ts (1)
423-438:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestart the daemon explicitly in the non-systemd upgrade path.
When
overrideState === "not-applicable", Line 428 treats any loopback responder as success. On upgrade runs that can still be the pre-upgradeollama serveprocess, so the new binary never takes ownership of:11434and the post-upgrade version check will keep seeing the stale daemon. The macOS path already handles this by stopping the old process before relaunch; Linux needs the same step here.🔧 Proposed fix
if (overrideState === "not-applicable") { + if (opts.isUpgrade) { + log(" Stopping stale Ollama daemon before relaunching..."); + runShellImpl("sudo pkill -x ollama || pkill -x ollama", { + ignoreError: true, + }); + sleepSecondsImpl(1); + } + // Re-probe loopback fresh here so this decision reflects the // post-install daemon state. The earlier `findReachableOllamaHost` // shortcut was cached for the rest of the onboard run and could echo🤖 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/install-ollama-linux.ts` around lines 423 - 438, When overrideState === "not-applicable" the code assumes any loopback responder is the new daemon; change the non-systemd path to explicitly stop any pre-upgrade ollama before relaunching: use runShellImpl to detect/terminate the old process (e.g., pkill -f "ollama serve" or use lsof/netstat to find the PID and kill it) prior to calling runShellImpl(`OLLAMA_HOST=127.0.0.1:${OLLAMA_PORT} ollama serve ...`); keep the existing waitForHttpImpl checks (first probe, then probe again after restart) and preserve the existing timeout/errorLog and return behavior if the new daemon does not become ready. Ensure these changes touch the block that references overrideState, waitForHttpImpl, runShellImpl, OLLAMA_PORT, and errorLog.
🧹 Nitpick comments (1)
docs/reference/commands.mdx (1)
1259-1259: ⚡ Quick winRewrite in active voice and adjust punctuation.
The description contains passive constructions and a colon that does not introduce a list:
Passive voice (flag as suggestion per style guide):
- "
useris refused" → use active voice: "the installer refusesuser" or "NemoClaw refusesuser"- "
systemis also refused" → "the installer also refusessystem" or "NemoClaw also refusessystem"Colon usage: The colon after "Upgrades:" does not introduce a list. As per coding guidelines, colons should only introduce lists, not general explanations. Replace the colon with a period or integrate the label differently.
Bold label (possible LLM pattern): The "Upgrades:" bold label may be unnecessary emphasis. Bold is reserved for UI labels, parameter names, and genuine warnings. Consider whether this label is essential or if the upgrade constraints can be integrated into the description without the bold label.
As per coding guidelines, active voice is required for all documentation, colons should only introduce lists, and bold should be reserved for specific use cases.
🤖 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 `@docs/reference/commands.mdx` at line 1259, Rewrite the NEMOCLAW_OLLAMA_INSTALL_MODE description in active voice and remove the colon after "Upgrades" (or drop the bold label), e.g. say that NemoClaw or the installer "refuses `user`" or "refuses `system`" rather than using passive phrasing; keep the behavior details for `system`, `user`, and empty/unset values, clarify auto-detection rules (root or passwordless sudo selects system; non-interactive without passwordless sudo selects user; interactive shell falls back to system so the installer can prompt), and integrate the upgrade constraints into the sentence (explain that the installer refuses `user` for upgrades because a user-local install cannot replace the system daemon on :11434, and that NemoClaw/installer also refuses `system` when NEMOCLAW_NON_INTERACTIVE=1 without passwordless sudo to avoid a hanging sudo prompt) while removing unnecessary bolding.
🤖 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/install-ollama-linux.ts`:
- Around line 423-438: When overrideState === "not-applicable" the code assumes
any loopback responder is the new daemon; change the non-systemd path to
explicitly stop any pre-upgrade ollama before relaunching: use runShellImpl to
detect/terminate the old process (e.g., pkill -f "ollama serve" or use
lsof/netstat to find the PID and kill it) prior to calling
runShellImpl(`OLLAMA_HOST=127.0.0.1:${OLLAMA_PORT} ollama serve ...`); keep the
existing waitForHttpImpl checks (first probe, then probe again after restart)
and preserve the existing timeout/errorLog and return behavior if the new daemon
does not become ready. Ensure these changes touch the block that references
overrideState, waitForHttpImpl, runShellImpl, OLLAMA_PORT, and errorLog.
---
Nitpick comments:
In `@docs/reference/commands.mdx`:
- Line 1259: Rewrite the NEMOCLAW_OLLAMA_INSTALL_MODE description in active
voice and remove the colon after "Upgrades" (or drop the bold label), e.g. say
that NemoClaw or the installer "refuses `user`" or "refuses `system`" rather
than using passive phrasing; keep the behavior details for `system`, `user`, and
empty/unset values, clarify auto-detection rules (root or passwordless sudo
selects system; non-interactive without passwordless sudo selects user;
interactive shell falls back to system so the installer can prompt), and
integrate the upgrade constraints into the sentence (explain that the installer
refuses `user` for upgrades because a user-local install cannot replace the
system daemon on :11434, and that NemoClaw/installer also refuses `system` when
NEMOCLAW_NON_INTERACTIVE=1 without passwordless sudo to avoid a hanging sudo
prompt) while removing unnecessary bolding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 89cd6c44-4c10-437b-a12a-e882bf537fcb
📒 Files selected for processing (4)
docs/reference/commands.mdxsrc/lib/onboard/install-ollama-linux.test.tssrc/lib/onboard/install-ollama-linux.tssrc/lib/onboard/install-ollama-macos.ts
💤 Files with no reviewable changes (1)
- src/lib/onboard/install-ollama-linux.test.ts
…ystemd upgrade Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/reference/commands.mdx (1)
1259-1259: ⚡ Quick winSplit this table-cell text to keep one sentence per source line.
This updated row packs multiple sentences into one source line, which hurts diff readability under the docs style rules.
As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 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 `@docs/reference/commands.mdx` at line 1259, The table cell for NEMOCLAW_OLLAMA_INSTALL_MODE currently contains multiple sentences on one source line; split that cell's content so each sentence is on its own source line to improve diff readability. Edit the row text for the `NEMOCLAW_OLLAMA_INSTALL_MODE` entry in docs/reference/commands.mdx (the table cell describing `system`, `user`, auto-detect rules, and upgrade/upgrade-path constraints) and break it into separate lines such that each sentence is a distinct source line while preserving the original wording and punctuation.
🤖 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 `@docs/reference/commands.mdx`:
- Line 1259: The table cell for NEMOCLAW_OLLAMA_INSTALL_MODE currently contains
multiple sentences on one source line; split that cell's content so each
sentence is on its own source line to improve diff readability. Edit the row
text for the `NEMOCLAW_OLLAMA_INSTALL_MODE` entry in docs/reference/commands.mdx
(the table cell describing `system`, `user`, auto-detect rules, and
upgrade/upgrade-path constraints) and break it into separate lines such that
each sentence is a distinct source line while preserving the original wording
and punctuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f71d3cb8-8bc2-41a8-8413-a032af507eb2
📒 Files selected for processing (4)
docs/reference/commands.mdxsrc/lib/onboard/install-ollama-linux.test.tssrc/lib/onboard/install-ollama-linux.tssrc/lib/onboard/install-ollama-macos.test.ts
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/inference/use-local-inference.mdx (1)
40-40: ⚡ Quick winFormat
sudoas inline code for command consistency.Use
`sudo`in this sentence to match CLI-token formatting used elsewhere on the page.As per coding guidelines: "CLI commands, file paths, flags, parameter names, and values must use inline
codeformatting."🤖 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 `@docs/inference/use-local-inference.mdx` at line 40, Wrap the word sudo in inline code formatting in the sentence "If sudo is not available in a non-interactive run, NemoClaw refuses to silently downgrade the path and asks you to rerun interactively or upgrade Ollama manually." so it reads with `sudo` as a CLI token; update the Markdown/MDX content where that exact sentence appears (search for the string starting "If sudo is not available...") to use backticks around sudo to match CLI-token formatting used elsewhere.
🤖 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 `@docs/inference/use-local-inference.mdx`:
- Line 40: Wrap the word sudo in inline code formatting in the sentence "If sudo
is not available in a non-interactive run, NemoClaw refuses to silently
downgrade the path and asks you to rerun interactively or upgrade Ollama
manually." so it reads with `sudo` as a CLI token; update the Markdown/MDX
content where that exact sentence appears (search for the string starting "If
sudo is not available...") to use backticks around sudo to match CLI-token
formatting used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c4056db7-2226-4e28-8a75-c9172d29f55e
📒 Files selected for processing (7)
docs/inference/use-local-inference.mdxdocs/reference/commands.mdxsrc/lib/onboard.tssrc/lib/onboard/install-ollama-linux-mode.tssrc/lib/onboard/install-ollama-linux-upgrade.test.tssrc/lib/onboard/install-ollama-linux.test.tssrc/lib/onboard/install-ollama-linux.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/commands.mdx
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26414889786
|
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
cv
approved these changes
May 25, 2026
12 tasks
miyoungc
added a commit
that referenced
this pull request
May 26, 2026
## Summary Refresh NemoClaw documentation and generated user skills for the v0.0.50 and v0.0.51 release-prep window. Remove obsolete legacy docs version metadata now that Fern docs no longer use `docs/project.json`, `docs/versions1.json`, or the legacy Sphinx config. ## Source summary - #1757 -> `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Document Slack channel allowlisting with `SLACK_ALLOWED_CHANNELS`. - #4134 -> `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Document Cloudflare named tunnel support through `CLOUDFLARE_TUNNEL_TOKEN`. - #4186 and #4135 -> `docs/inference/use-local-inference.mdx`, `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Document Ollama upgrade and user-local install behavior. - #4185 -> `docs/network-policy/integration-policy-examples.mdx`, `docs/about/release-notes.mdx`: Clarify Jira policy validation probes. - Release cleanup -> `.claude/skills/nemoclaw-contributor-update-docs/SKILL.md`, `docs/CONTRIBUTING.md`, `.github/PULL_REQUEST_TEMPLATE.md`, `scripts/bump-version.ts`: Stop using legacy docs version JSON files and align docs verification on `npm run docs`. ## Changes - Add v0.0.50 and v0.0.51 release notes. - Regenerate NemoClaw user skills from the current Fern docs. - Remove obsolete `docs/conf.py`, `docs/project.json`, and `docs/versions1.json`. - Update docs workflow guidance and PR templates to use `npm run docs` instead of `make docs`. - Remove release-version JSON handling from `scripts/bump-version.ts`. ## 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 - [ ] `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) Additional verification: - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `npm run build:cli` - `npx tsc --noEmit --allowSyntheticDefaultImports --module NodeNext --moduleResolution NodeNext --target ES2022 --types node scripts/bump-version.ts` - `ReadLints` on touched docs, skills, template, and script files - Searched for stale `versions1.json`, `project.json`, and `make docs` references Known gaps: - `npm run docs` was not rerun after cleanup because the earlier Fern CLI fetch failed with npm registry `403 Forbidden` in this environment. - A broad `npm run typecheck -- --noEmit` hit an unrelated existing `scripts/dev-tier-selector.js` type error. --- Signed-off-by: Miyoung Cho <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added resource profiling with CPU/RAM configuration controls for sandboxes * Enhanced local Ollama inference with automatic GPU memory-aware model fallback * Added `nemoclaw resources` command to display host hardware inventory * Enabled Cloudflare named tunnel support via environment configuration * **Documentation** * Improved setup guides for local inference, sandbox hardening, and policy validation * Enhanced troubleshooting for messaging delivery and host service routing * Added release notes for v0.0.50 and v0.0.51 * **Chores** * Updated build documentation commands from `make docs` to `npm run docs` <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4262?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 -->
](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4262?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
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
4 participants
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
DGX Spark express setup pins
NEMOCLAW_PROVIDER=install-ollama+NEMOCLAW_MODEL=qwen3.6:35b. With Ollama already on the host, the onboard menu dropped theinstall-ollamaentry, the provider-key fallback rewrote the request to plainollama, and the stale 0.6.2 daemon was reused — crashing the runner onqwen3.6:35band looping the validation prompt. This PR detects both the host CLI binary version and the locally running daemon's/api/version, keepsinstall-ollamain the menu as an explicit "Upgrade Ollama" entry when either is belowMIN_OLLAMA_VERSION(0.7.0), routes the install/upgrade through the right per-platform path, pins the resolved host to local loopback so a cachedhost.docker.internalfrom an earlier WSL probe cannot misroute validation, re-probes the daemon after the upgrade command to fail the run if the upgrade did not take effect, and exits cleanly when validation fails underisNonInteractive()instead of looping.Related Issue
Fixes #4178
Changes
src/lib/inference/ollama-version.ts(new) —MIN_OLLAMA_VERSION(0.7.0),getInstalledOllamaVersionparsingollama --version,getRunningOllamaDaemonVersionparsing/api/versionon a chosen endpoint,isOllamaVersionAtLeastcomponent comparator,OllamaVersionRunCapturetyped asreadonly string[].inference/local.tsre-exports.src/lib/onboard/ollama-install-menu.ts(new) —resolveOllamaInstallMenuEntryprobes both the CLI binary and the local daemon (skipping the daemon side whenollamaHost === host.docker.internal), labels the stale source ("running daemon" or "installed binary"), and returns ahasUpgradableOllamaflag.assertOllamaUpgradeAppliedre-probes the daemon after an upgrade (skipped for fresh installs) to catch silent failures.src/lib/onboard/install-ollama-linux.ts— newisUpgrade?: booleanoption.decideInstallOllamaLinuxModeforces the sudo system path during upgrades, refusesNEMOCLAW_OLLAMA_INSTALL_MODE=userfor upgrades, and exits with an actionable error when sudo is unavailable in non-interactive mode (includingNEMOCLAW_OLLAMA_INSTALL_MODE=system).installOllamaSystem's non-systemd fallback (WSL / minimal containers) re-probes loopback with a freshwaitForHttpinstead of trusting the cachedfindReachableOllamaHostresolver. On the upgrade path it stops the stale daemon (pkill -x ollama+ brief sleep) and always relaunchesollama serve, so the freshly installed binary owns:11434before the post-upgrade version probe runs.installOllamaOnLinuxpinssetResolvedOllamaHost("127.0.0.1")on success.src/lib/onboard/install-ollama-macos.ts(new) — mirrors the Linux contract: runsbrew upgrade ollamaon upgrades (withoutignoreError),brew install ollamaon fresh, starts the daemon on loopback, waits for:11434, and pins the resolved host to loopback on success.src/lib/onboard.ts— replace the inline install-ollama macOS + Linux blocks with a single dispatcher that picksinstallOllamaOnMacOSorinstallOllamaOnLinuxand callsassertOllamaUpgradeAppliedon success. AddisNonInteractive() process.exit(1)at the!validation.okbranch ofselectAndValidateOllamaModelso non-interactive runs cannot loop on a probe their pinned model can never pass. Net-negative line count to keeponboard-entrypoint-budgetgreen.src/lib/inference/ollama-version.test.ts,src/lib/onboard/ollama-install-menu.test.ts,src/lib/onboard/install-ollama-linux.test.ts(env-override refusal, sudo-required gate, host pin, WSLhost.docker.internalcarve-out). Branch-level test intest/onboard-selection.test.tsmocks an installed 0.6.2 host, drivesNEMOCLAW_PROVIDER=install-ollama, and verifies the official installer runs.docs/inference/use-local-inference.mdx— describe the dual binary/daemon gate, the Windows-host carve-out, the platform install/upgrade path, the sudo-required Linux upgrade constraint, and the post-upgrade daemon re-probe.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
New Features
Bug Fixes
Documentation
Tests