fix: [Bug] Ollama reasoning models return empty content — agent responses are blank#277
Closed
kagura-agent wants to merge 5 commits intoNVIDIA:mainfrom
Closed
Conversation
…iant Ollama reasoning models (e.g., nemotron-3-nano, deepseek-r1, qwq) put their output into a 'reasoning' field instead of 'content' in the OpenAI-compatible API response. OpenClaw reads 'content' only, so the agent's response appears blank. This commit: 1. Adds reasoning model detection during Ollama onboarding (both the legacy JS wizard and the new TS onboard command). Known reasoning model patterns are checked against the selected model name. 2. When a reasoning model is detected, the onboarding flow automatically creates a non-reasoning chat variant using 'ollama create' with a simple Modelfile wrapper. This variant writes to 'content' normally. 3. Lists locally available Ollama models during onboarding instead of hardcoding 'nemotron-3-nano', letting users pick from their pulled models and see which ones are reasoning models. 4. Documents the limitation and workaround in the inference provider switching guide. Fixes NVIDIA#246
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects local Ollama models during onboarding, flags models that output to a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NemoclawCLI as "nemoclaw onboard"
participant OllamaAPI as "Local Ollama /v1/models"
participant Shell as "shell / filesystem (ollama create, Modelfile)"
User->>NemoclawCLI: start onboarding (choose provider=ollama)
NemoclawCLI->>OllamaAPI: list models
OllamaAPI-->>NemoclawCLI: model list
NemoclawCLI->>User: present model choices (mark reasoning models)
User->>NemoclawCLI: select model
alt selected is reasoning model
NemoclawCLI->>User: offer to create chat variant
opt user accepts (or non-interactive)
NemoclawCLI->>Shell: write temporary Modelfile
NemoclawCLI->>Shell: run `ollama create` (create variant)
Shell-->>NemoclawCLI: success/failure
alt success
NemoclawCLI->>NemoclawCLI: switch to created chat variant
else failure
NemoclawCLI->>User: warn and provide workaround commands
end
end
end
NemoclawCLI->>User: finish onboarding with chosen model/settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![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: 4
🧹 Nitpick comments (1)
nemoclaw/src/commands/onboard.ts (1)
36-48: Consider usingfetchor a native HTTP client instead of shelling out tocurl.Shelling out to
curlintroduces an external dependency and is less idiomatic in TypeScript/Node.js. Since Node.js 18+ has nativefetch, or you could use the built-inhttpmodule, this would be more robust and eliminate the dependency oncurlbeing installed.That said, the current implementation works and matches the JS counterpart. The timeout and error handling are reasonable.
♻️ Optional: Use native fetch instead of curl
async function listOllamaModels(): Promise<string[]> { try { const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 5000); const response = await fetch("http://localhost:11434/api/tags", { signal: controller.signal, }); clearTimeout(timeout); if (!response.ok) return []; const data = (await response.json()) as { models?: Array<{ name: string }> }; return (data.models ?? []).map((m) => m.name); } catch { return []; } }Note: This would require making the function async and updating call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/onboard.ts` around lines 36 - 48, The function listOllamaModels currently shells out to curl via execSync; replace it with an async implementation using the native fetch API (or Node's http client) to avoid external dependencies: convert listOllamaModels to async, use an AbortController and a 5s timeout, call fetch("http://localhost:11434/api/tags"), check response.ok, parse JSON as { models?: { name: string }[] } and return (data.models ?? []).map(m => m.name), catch errors and return []; update all call sites that use listOllamaModels to await the Promise (or provide a sync wrapper if necessary) and keep the same error/empty-array behavior as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 55-69: The createChatVariant function currently interpolates
baseModel into a shell command passed to execSync which allows shell
metacharacter injection; replace the execSync call that runs `echo ... | ollama
create ${variantName}` with a safe child_process.execFileSync (or spawnSync)
invocation that runs the executable "ollama" with args ["create", variantName]
and pass the modelfile string as the stdin/input option, preserving encoding,
stdio and timeout options; keep the rest of the function (modelfile creation,
variantName derivation, error handling) the same but remove any direct shell
string interpolation of baseModel to eliminate shell injection risk.
- Around line 426-428: The default suggestion "nemotron-3-nano" currently gets
set without the reasoning-model handling used elsewhere; update this
fresh-install branch to detect reasoning models with isReasoningModel(model) (or
the KNOWN_REASONING_MODEL_PATTERNS check) and, if true, call
createChatVariant(model) (as done in the other branches) to produce/use the chat
variant (or fallback to a non-reasoning default); ensure you update the local
model variable to the variant returned and adjust the console suggestion to
reference the variant name and handle errors the same way the other paths do.
In `@docs/inference/switch-inference-providers.md`:
- Around line 84-85: The example model name is inconsistent with the code's
tag-stripping logic and regex; update the example to match
baseModel.replace(/:.*$/, "") + "-chat" and the regex /^nemotron.*nano/i by
changing `nemotron-nano-chat` to `nemotron-3-nano-chat` (or otherwise remove the
:tag from the base model before appending "-chat") so the example reflects the
actual behavior of the pattern and the model-name generation logic.
In `@nemoclaw/src/commands/onboard.ts`:
- Around line 57-74: In createOllamaChatVariant, avoid shell interpolation of
baseModel to prevent shell injection: write the Modelfile content (`FROM
${baseModel}`) to a securely-created temporary file (use fs.mkdtempSync or temp
file creation), call execFileSync (or spawnSync) with the ollama executable and
args ["create", variantName, "-f", tempFilePath] instead of embedding via a
shell string, ensure stdio/timeout are preserved, and remove the temporary file
after the call; also validate or sanitize baseModel minimally (e.g., trim)
before writing to the file to be defensive.
---
Nitpick comments:
In `@nemoclaw/src/commands/onboard.ts`:
- Around line 36-48: The function listOllamaModels currently shells out to curl
via execSync; replace it with an async implementation using the native fetch API
(or Node's http client) to avoid external dependencies: convert listOllamaModels
to async, use an AbortController and a 5s timeout, call
fetch("http://localhost:11434/api/tags"), check response.ok, parse JSON as {
models?: { name: string }[] } and return (data.models ?? []).map(m => m.name),
catch errors and return []; update all call sites that use listOllamaModels to
await the Promise (or provide a sync wrapper if necessary) and keep the same
error/empty-array behavior as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30eab969-b3b1-46b6-b5bc-d573e2911246
📒 Files selected for processing (3)
bin/lib/onboard.jsdocs/inference/switch-inference-providers.mdnemoclaw/src/commands/onboard.ts
Comment on lines
+426
to
+428
| // Fresh install — no models available yet, suggest pulling one | ||
| model = "nemotron-3-nano"; | ||
| console.log(" Note: You'll need to pull a model. Run: ollama pull nemotron-3-nano"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the default model name matches reasoning model patterns
node -e "
const patterns = [/^nemotron.*nano/i, /^deepseek-r1/i, /^qwq/i];
const model = 'nemotron-3-nano';
const isReasoning = patterns.some(p => p.test(model));
console.log('Model:', model);
console.log('Is reasoning model:', isReasoning);
"Repository: NVIDIA/NemoClaw
Length of output: 106
🏁 Script executed:
cat -n bin/lib/onboard.js | sed -n '300,320p'Repository: NVIDIA/NemoClaw
Length of output: 1011
🏁 Script executed:
cat -n bin/lib/onboard.js | sed -n '405,435p'Repository: NVIDIA/NemoClaw
Length of output: 1674
🏁 Script executed:
rg "nemotron.*nano|reasoning.*model|variant" bin/lib/onboard.js -B 3 -A 3Repository: NVIDIA/NemoClaw
Length of output: 4314
nemotron-3-nano as default will be detected as a reasoning model, but no variant creation attempted.
Lines 426–428 set nemotron-3-nano as the default model suggestion for fresh Ollama installs, but unlike the code paths at lines 290–314 and lines 380–417, this path does not call isReasoningModel() or createChatVariant(). Since nemotron-3-nano matches the pattern /^nemotron.*nano/i in KNOWN_REASONING_MODEL_PATTERNS, users who follow the suggestion and pull this model will get a reasoning model that may produce blank responses (as documented in issue #246). Either use a non-reasoning default model for fresh installs, or add the same reasoning model detection and variant creation logic here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 426 - 428, The default suggestion
"nemotron-3-nano" currently gets set without the reasoning-model handling used
elsewhere; update this fresh-install branch to detect reasoning models with
isReasoningModel(model) (or the KNOWN_REASONING_MODEL_PATTERNS check) and, if
true, call createChatVariant(model) (as done in the other branches) to
produce/use the chat variant (or fallback to a non-reasoning default); ensure
you update the local model variable to the variant returned and adjust the
console suggestion to reference the variant name and handle errors the same way
the other paths do.
… model, align docs
- Replace shell-interpolated `echo | ollama create` with temp Modelfile +
`execFileSync('ollama', ['create', name, '-f', tmpFile])` in both JS and TS
to prevent command injection via crafted model names
- Change default Ollama model from `nemotron-3-nano` (matches reasoning
patterns) to `nemotron-3-nano-chat` variant with setup instructions
- Fix Ollama-with-no-models fallback in TS: suggest pulling local models
instead of offering cloud model IDs that won't work with local Ollama
- Align docs variant name to `nemotron-3-nano-chat` (matches onboarding's
`<base>-chat` naming convention)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 317-322: In the experimental auto-select fallback branch that
assigns model = "nemotron-3-nano-chat", add a concise user-facing guidance
message (e.g., console.warn/processLogger.warn) immediately after the assignment
informing the user that this is only a temporary fallback and they must pull or
create the "nemotron-3-nano" base model and/or the "nemotron-3-nano-chat"
variant before running inference (include a short hint about the CLI or config
command to pull/create the model if applicable); update any onboarding summary
output (the function that performs final onboarding messages) to include the
same note so onboarding doesn't appear complete while the model is missing.
- Around line 19-30: The reasoning-detection currently treats chat variants like
"nemotron-3-nano-chat" as reasoning models; update
KNOWN_REASONING_MODEL_PATTERNS so isReasoningModel ignores any model names that
include the "-chat" suffix (e.g., adjust the nemotron pattern to exclude matches
containing "-chat" or add a negative check for "-chat"), ensuring
isReasoningModel only returns true for non-chat variants (refer to
KNOWN_REASONING_MODEL_PATTERNS and function isReasoningModel when making the
change).
In `@nemoclaw/src/commands/onboard.ts`:
- Around line 21-34: The current KNOWN_REASONING_MODEL_PATTERNS match variants
like "nemotron-3-nano-chat"; update the regexes in
KNOWN_REASONING_MODEL_PATTERNS (used by isReasoningModel) so they do not match
when the model name has a "-chat" suffix—for example add a negative lookahead
after the key token (e.g., change /^nemotron.*nano/i to
/^nemotron.*nano(?!-chat)/i or otherwise ensure the pattern requires no
immediate "-chat" after the matched segment) and apply the same adjustment to
the /^deepseek-r1/i and /^qwq/i entries so isReasoningModel will return false
for generated "-chat" variants.
- Around line 458-465: When auto-creating a non-interactive chat variant fails
in the non-interactive branch, there is no warning so the reasoning model may be
kept silently; update the logic around createOllamaChatVariant(...) so that when
it returns falsy you emit a warning via logger.warn indicating the auto-creation
failed and include a brief workaround hint (e.g., "responses may be blank; run
in interactive mode or specify a non-reasoning model explicitly") so operators
can diagnose the issue instead of silently using the original model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 240cd305-fb10-4b64-8cb7-de72b20969d3
📒 Files selected for processing (3)
bin/lib/onboard.jsdocs/inference/switch-inference-providers.mdnemoclaw/src/commands/onboard.ts
…lback warnings - Add early return in isReasoningModel() to skip names ending with '-chat' (e.g., nemotron-3-nano-chat), preventing false-positive reasoning detection on variants we explicitly created as non-reasoning - Add user guidance in experimental auto-select no-models fallback path - Add warning log when non-interactive variant creation fails instead of silently continuing with a broken reasoning model
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/lib/onboard.js (1)
19-23: Consider extracting shared patterns to avoid drift.
KNOWN_REASONING_MODEL_PATTERNSis duplicated in bothnemoclaw/src/commands/onboard.ts(lines 21-25) and this file (lines 19-23). If the pattern list changes, both files must be updated. Consider extracting these patterns to a shared configuration file or generating one from the other to prevent drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 19 - 23, The KNOWN_REASONING_MODEL_PATTERNS array is duplicated; extract it into a single shared export (e.g., export const KNOWN_REASONING_MODEL_PATTERNS) in a new shared module and replace the inline arrays in both places with imports of that symbol (update references in functions that use it, e.g., any onboard logic that tests model names). Ensure the new module is imported where the constant was previously defined and run existing tests/linters to confirm no breaking changes.nemoclaw/src/commands/onboard.ts (1)
418-423: Inconsistent fallback model between TypeScript and JavaScript implementations.When no Ollama models are found, this TypeScript implementation defaults to
llama3.2, while the JavaScript implementation inbin/lib/onboard.js(lines 323-327, 431-434, 445-447) defaults tonemotron-3-nano-chat. Consider aligning the fallback behavior for consistency.Additionally, there's no reasoning model check or variant creation for the default model here. If a user later pulls
nemotron-3-nanoinstead ofllama3.2, they'll still encounter blank responses until re-onboarding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/onboard.ts` around lines 418 - 423, The TypeScript fallback in commands/onboard.ts currently sets model = "llama3.2" causing inconsistency with bin/lib/onboard.js and missing reasoning-variant setup; change the fallback to match the JS default (use "nemotron-3-nano-chat" or the same canonical string used in bin/lib/onboard.js) and add the same reasoning model checks and variant creation logic that exists for other models so a reasoning variant is created if necessary (update the same block that logs "No models found..." and assigns model), ensuring the onboarding path calls the functions/methods that create/check reasoning variants for the chosen default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 19-23: The KNOWN_REASONING_MODEL_PATTERNS array is duplicated;
extract it into a single shared export (e.g., export const
KNOWN_REASONING_MODEL_PATTERNS) in a new shared module and replace the inline
arrays in both places with imports of that symbol (update references in
functions that use it, e.g., any onboard logic that tests model names). Ensure
the new module is imported where the constant was previously defined and run
existing tests/linters to confirm no breaking changes.
In `@nemoclaw/src/commands/onboard.ts`:
- Around line 418-423: The TypeScript fallback in commands/onboard.ts currently
sets model = "llama3.2" causing inconsistency with bin/lib/onboard.js and
missing reasoning-variant setup; change the fallback to match the JS default
(use "nemotron-3-nano-chat" or the same canonical string used in
bin/lib/onboard.js) and add the same reasoning model checks and variant creation
logic that exists for other models so a reasoning variant is created if
necessary (update the same block that logs "No models found..." and assigns
model), ensuring the onboarding path calls the functions/methods that
create/check reasoning variants for the chosen default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa76c875-c38c-411b-9749-971034a3024e
📒 Files selected for processing (2)
bin/lib/onboard.jsnemoclaw/src/commands/onboard.ts
…istOllamaModels
- Change empty-models fallback from llama3.2 to nemotron-3-nano-chat for
consistency with bin/lib/onboard.js (prevents reasoning-model blank responses)
- Add step-by-step guidance for pulling/creating the variant after onboarding
- Replace execSync('curl ...') with execFileSync('curl', [...]) in
listOllamaModels to avoid shell interpolation (consistent with
createOllamaChatVariant pattern)
Addresses CodeRabbit review comments on PR NVIDIA#277.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nemoclaw/src/commands/onboard.ts (2)
67-68:⚠️ Potential issue | 🟠 MajorUse a securely created temp path for the Modelfile.
join(tmpdir(), \nemoclaw-modelfile-${Date.now()}`)is predictable and can collide across concurrent runs; on shared systems it also leaves a symlink/TOCTOU window beforewriteFileSync. Please switch tomkdtempSync` (or another exclusive-create temp file strategy) and clean up the temp directory afterward.Proposed fix
-import { writeFileSync, unlinkSync } from "node:fs"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; ... - const tmpFile = join(tmpdir(), `nemoclaw-modelfile-${Date.now()}`); - writeFileSync(tmpFile, modelfile, "utf-8"); + const tmpDir = mkdtempSync(join(tmpdir(), "nemoclaw-modelfile-")); + const tmpFile = join(tmpDir, "Modelfile"); + writeFileSync(tmpFile, modelfile, { encoding: "utf-8", flag: "wx" }); try { execFileSync("ollama", ["create", variantName, "-f", tmpFile], { encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"], timeout: 120000, }); } finally { - try { - unlinkSync(tmpFile); - } catch { - // ignore cleanup errors - } + rmSync(tmpDir, { recursive: true, force: true }); }Also applies to: 75-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/onboard.ts` around lines 67 - 68, The temp path for writing the Modelfile is created predictably with join(tmpdir(), `nemoclaw-modelfile-${Date.now()}`) which is vulnerable to collisions/TOCTOU; replace that logic in the onboarding flow (the tmpFile creation and subsequent writeFileSync of modelfile) to use an exclusive temp directory or file API such as fs.mkdtempSync (or fs.promises.mkdtemp) to create a unique temp dir, then write the modelfile into that directory (use a deterministic filename inside the new dir), and ensure you remove the temp dir/file after use (cleanup on both success and error). Apply the same change to the other similar block referenced (lines 75-77) so all temporary file creation for Modelfile uses mkdtempSync and proper cleanup.
424-426:⚠️ Potential issue | 🟠 MajorThe recovery hint still uses the stdin form of
ollama create.These warnings tell users to
echo 'FROM ...' | ollama create ..., but the helper above had to switch to a real Modelfile path and-f. If auto-creation fails, this fallback can send users into another dead end. Reuse the same file-based instructions here so the documented workaround matches the code path that works.Does the Ollama CLI officially support piping a Modelfile to `ollama create` on stdin, or does `-f` need a file path?Also applies to: 459-460, 472-472
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/onboard.ts` around lines 424 - 426, The recovery hints still suggest piping a Modelfile via stdin but the working fallback uses a real Modelfile path and -f; update the logger.warn messages in onboard.ts (the lines that call logger.warn with "echo 'FROM ...' | ollama create ...") to instead show the file-based form (e.g. ollama create -f <Modelfile> <name>) so the user-facing instructions match the actual helper/code path used for auto-creation; change all occurrences referenced (the logger.warn calls around the onboarding completion and the other spots noted) to use the -f file path form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/commands/onboard.ts`:
- Around line 422-423: The code currently forces model = "nemotron-3-nano-chat"
and logs logger.warn then continues to create providers and save config even
when the model is unusable; update the onboarding flow in onboard (the branch
that sets model and the branch that handles variant creation failure) to abort
early instead of proceeding: when no local models are found (the branch that
sets model and calls logger.warn) throw or return an error from the onboard flow
and do not proceed to provider creation or config save, and likewise when
variant creation/validation fails (the branch that currently keeps the reasoning
model after variant creation fails) exit the function with an error/throw before
reaching provider creation and the config save path so a broken model is never
persisted. Ensure you update the surrounding control flow where variable model
is used (and any function that creates variants) to respect the early
return/throw.
---
Duplicate comments:
In `@nemoclaw/src/commands/onboard.ts`:
- Around line 67-68: The temp path for writing the Modelfile is created
predictably with join(tmpdir(), `nemoclaw-modelfile-${Date.now()}`) which is
vulnerable to collisions/TOCTOU; replace that logic in the onboarding flow (the
tmpFile creation and subsequent writeFileSync of modelfile) to use an exclusive
temp directory or file API such as fs.mkdtempSync (or fs.promises.mkdtemp) to
create a unique temp dir, then write the modelfile into that directory (use a
deterministic filename inside the new dir), and ensure you remove the temp
dir/file after use (cleanup on both success and error). Apply the same change to
the other similar block referenced (lines 75-77) so all temporary file creation
for Modelfile uses mkdtempSync and proper cleanup.
- Around line 424-426: The recovery hints still suggest piping a Modelfile via
stdin but the working fallback uses a real Modelfile path and -f; update the
logger.warn messages in onboard.ts (the lines that call logger.warn with "echo
'FROM ...' | ollama create ...") to instead show the file-based form (e.g.
ollama create -f <Modelfile> <name>) so the user-facing instructions match the
actual helper/code path used for auto-creation; change all occurrences
referenced (the logger.warn calls around the onboarding completion and the other
spots noted) to use the -f file path form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90867217-0d75-42b0-b100-9065aeb1c689
📒 Files selected for processing (1)
nemoclaw/src/commands/onboard.ts
Comment on lines
+422
to
+423
| model = "nemotron-3-nano-chat"; | ||
| logger.warn("No models found in local Ollama. Defaulting to nemotron-3-nano-chat."); |
Contributor
There was a problem hiding this comment.
Abort instead of saving a model we already know is unusable.
One branch hard-codes nemotron-3-nano-chat right after finding no local models, and the other keeps the reasoning model after variant creation fails even though blank responses are expected. Both paths still fall through to provider creation and config save, so onboard can exit successfully with a broken route.
Also applies to: 465-471
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/commands/onboard.ts` around lines 422 - 423, The code currently
forces model = "nemotron-3-nano-chat" and logs logger.warn then continues to
create providers and save config even when the model is unusable; update the
onboarding flow in onboard (the branch that sets model and the branch that
handles variant creation failure) to abort early instead of proceeding: when no
local models are found (the branch that sets model and calls logger.warn) throw
or return an error from the onboard flow and do not proceed to provider creation
or config save, and likewise when variant creation/validation fails (the branch
that currently keeps the reasoning model after variant creation fails) exit the
function with an error/throw before reaching provider creation and the config
save path so a broken model is never persisted. Ensure you update the
surrounding control flow where variable model is used (and any function that
creates variants) to respect the early return/throw.
- Use mkdtempSync for cryptographically random temp directories instead of predictable Date.now()-based paths (TOCTOU fix) - Prompt user for model ID when no local Ollama models found instead of silently defaulting to potentially non-existent model - Default to llama3.2 (non-reasoning) instead of nemotron-3-nano-chat which requires a base model that may not be pulled
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 429-434: The else branch that sets the fallback model currently
assigns model = "nemotron-3-nano-chat" and outputs instructions for
pulling/creating that model; change this to default to "llama3.2" instead so it
matches the EXPERIMENTAL auto-select behavior, update the console messages to
recommend pulling "llama3.2" (e.g., "ollama pull llama3.2") and remove the
manual "create chat variant" suggestion for nemotron; ensure the variable model
and its related log lines in this else block are updated accordingly so defaults
are consistent with the EXPERIMENTAL path.
- Around line 443-446: The fresh-install branch sets model =
"nemotron-3-nano-chat" and prints instructions for pulling/creating that model,
but it should default to "llama3.2"; change the default assignment of the
variable model from "nemotron-3-nano-chat" to "llama3.2" and update the
displayed console messages to instruct pulling/creating the llama3.2 chat
variant (adjust the pull/create commands and any referenced model names in the
two console.log calls so they reference llama3.2 and its chat variant instead of
nemotron-3-nano).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c21b915-09e1-4cc0-936b-fe6b698a13ab
📒 Files selected for processing (2)
bin/lib/onboard.jsnemoclaw/src/commands/onboard.ts
Comment on lines
+429
to
+434
| } else { | ||
| model = "nemotron-3-nano-chat"; | ||
| console.log(" No models found locally. Defaulting to nemotron-3-nano-chat."); | ||
| console.log(" Pull the base model with: ollama pull nemotron-3-nano"); | ||
| console.log(" Then create the chat variant: echo 'FROM nemotron-3-nano' | ollama create nemotron-3-nano-chat"); | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent default model selection.
This path defaults to nemotron-3-nano-chat (line 430), but the EXPERIMENTAL auto-select path at line 323 defaults to llama3.2. Per the commit message, the intent was to change to llama3.2 to avoid reasoning-only models by default. Additionally, nemotron-3-nano-chat won't exist until the user follows the manual steps.
🔧 Proposed fix for consistency
} else {
- model = "nemotron-3-nano-chat";
- console.log(" No models found locally. Defaulting to nemotron-3-nano-chat.");
- console.log(" Pull the base model with: ollama pull nemotron-3-nano");
- console.log(" Then create the chat variant: echo 'FROM nemotron-3-nano' | ollama create nemotron-3-nano-chat");
+ model = "llama3.2";
+ console.log(" No models found locally. Defaulting to llama3.2.");
+ console.log(" After onboarding, pull the model:");
+ console.log(" ollama pull llama3.2");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else { | |
| model = "nemotron-3-nano-chat"; | |
| console.log(" No models found locally. Defaulting to nemotron-3-nano-chat."); | |
| console.log(" Pull the base model with: ollama pull nemotron-3-nano"); | |
| console.log(" Then create the chat variant: echo 'FROM nemotron-3-nano' | ollama create nemotron-3-nano-chat"); | |
| } | |
| } else { | |
| model = "llama3.2"; | |
| console.log(" No models found locally. Defaulting to llama3.2."); | |
| console.log(" After onboarding, pull the model:"); | |
| console.log(" ollama pull llama3.2"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 429 - 434, The else branch that sets the
fallback model currently assigns model = "nemotron-3-nano-chat" and outputs
instructions for pulling/creating that model; change this to default to
"llama3.2" instead so it matches the EXPERIMENTAL auto-select behavior, update
the console messages to recommend pulling "llama3.2" (e.g., "ollama pull
llama3.2") and remove the manual "create chat variant" suggestion for nemotron;
ensure the variable model and its related log lines in this else block are
updated accordingly so defaults are consistent with the EXPERIMENTAL path.
Comment on lines
+443
to
+446
| // Fresh install — no models available yet, suggest pulling one | ||
| model = "nemotron-3-nano-chat"; | ||
| console.log(" Note: You'll need to pull a model. Run: ollama pull nemotron-3-nano"); | ||
| console.log(" Then create a chat variant: echo 'FROM nemotron-3-nano' | ollama create nemotron-3-nano-chat"); |
Contributor
There was a problem hiding this comment.
Same inconsistency: should default to llama3.2.
This fresh install path also defaults to nemotron-3-nano-chat instead of llama3.2. This duplicates the issue at lines 429-434.
🔧 Proposed fix
// Fresh install — no models available yet, suggest pulling one
- model = "nemotron-3-nano-chat";
- console.log(" Note: You'll need to pull a model. Run: ollama pull nemotron-3-nano");
- console.log(" Then create a chat variant: echo 'FROM nemotron-3-nano' | ollama create nemotron-3-nano-chat");
+ model = "llama3.2";
+ console.log(" Note: You'll need to pull a model. Run: ollama pull llama3.2");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fresh install — no models available yet, suggest pulling one | |
| model = "nemotron-3-nano-chat"; | |
| console.log(" Note: You'll need to pull a model. Run: ollama pull nemotron-3-nano"); | |
| console.log(" Then create a chat variant: echo 'FROM nemotron-3-nano' | ollama create nemotron-3-nano-chat"); | |
| // Fresh install — no models available yet, suggest pulling one | |
| model = "llama3.2"; | |
| console.log(" Note: You'll need to pull a model. Run: ollama pull llama3.2"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 443 - 446, The fresh-install branch sets
model = "nemotron-3-nano-chat" and prints instructions for pulling/creating that
model, but it should default to "llama3.2"; change the default assignment of the
variable model from "nemotron-3-nano-chat" to "llama3.2" and update the
displayed console messages to instruct pulling/creating the llama3.2 chat
variant (adjust the pull/create commands and any referenced model names in the
two console.log calls so they reference llama3.2 and its chat variant instead of
nemotron-3-nano).
Contributor
Author
|
Closing — this PR bundles too many changes (Ollama reasoning fix + onboard changes + docs). I'll split the Ollama reasoning field fix into a focused PR once I verify the approach against the latest codebase. The test coverage is already addressed in #279. |
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.
Fixes #246
Automated PR via GoGetAJob
Summary by CodeRabbit
New Features
Documentation