Open
Conversation
|
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:
📝 WalkthroughWalkthroughReplaces hardcoded NVIDIA inference defaults with provider-agnostic onboarding and routing. Onboard collects/validates provider and model, injects provider/model into the sandbox image at build time, stores credentials on host, and configures OpenShell gateway to route sandbox requests to the chosen provider. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as nemoclaw CLI
participant Onboard as Onboard Handler
participant Validator as Provider Validator
participant Docker as Docker Build
participant OpenShell as OpenShell Gateway
participant Sandbox as Sandbox Container
participant Registry as Registry
User->>CLI: nemoclaw onboard
CLI->>Onboard: start onboarding
Onboard->>User: prompt provider & model
User-->>Onboard: select provider, model, credentials
Onboard->>Validator: probe provider endpoints (e.g., /responses, /chat/completions, /v1/messages)
Validator-->>Onboard: validation result
Onboard->>Docker: patch staged Dockerfile (inject provider/model args)
Docker->>Docker: generate openclaw.json with provider map
Docker-->>Onboard: image built
Onboard->>OpenShell: create sandbox from image
OpenShell->>Sandbox: launch container (sandbox)
Onboard->>OpenShell: upsert provider route (gateway)
OpenShell->>Validator: verify inference route
Validator-->>Sandbox: routed inference verified
Onboard->>Registry: save sandbox metadata (provider, model, nimContainer)
Onboard-->>User: onboarding complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/credentials.js (1)
172-179:⚠️ Potential issue | 🔴 Critical
execSyncis used but not imported — runtimeReferenceError.The functions
isRepoPrivate()(line 174) andensureGithubToken()(line 189) both callexecSync, but this function is not imported fromchild_process. This will cause aReferenceErrorwhen either function is executed.Proposed fix: restore the execSync import
const fs = require("fs"); const path = require("path"); const readline = require("readline"); +const { execSync } = require("child_process");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/credentials.js` around lines 172 - 179, The file uses execSync in functions isRepoPrivate and ensureGithubToken but never imports it; add an import for execSync from the child_process module at the top of the file so both functions can call execSync without causing a ReferenceError (locate the top-level imports and add the execSync import alongside them so isRepoPrivate and ensureGithubToken work correctly).
🧹 Nitpick comments (2)
docs/inference/switch-inference-providers.md (1)
94-94: Keep one sentence per source line here.This bullet packs two sentences onto one source line. Split it so diffs stay readable.
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 the current code and only fix it if needed. In `@docs/inference/switch-inference-providers.md` at line 94, The bullet "Runtime switching changes the OpenShell route. It does not rewrite your stored credentials." contains two sentences on one source line; split it into two source lines so each sentence is on its own line (e.g., place "Runtime switching changes the OpenShell route." and "It does not rewrite your stored credentials." on separate lines) to satisfy the "one sentence per source line" guideline and keep diffs readable.docs/reference/inference-profiles.md (1)
6-6: UseNemoClawcasing in thekeywordsentries.Both new keywords use
nemoclaw, which breaks the project word list and leaks inconsistent casing into search metadata.As per coding guidelines, “NemoClaw, OpenClaw, and OpenShell must use correct casing.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/inference-profiles.md` at line 6, The keywords array uses incorrect lowercase "nemoclaw" tokens which violate the casing rule; update the two entries in the keywords array to use the exact "NemoClaw" casing (e.g., change "nemoclaw inference profiles" -> "NemoClaw inference profiles" and "nemoclaw provider routing" -> "NemoClaw provider routing") so the keywords list and search metadata follow the required NemoClaw capitalization.
🤖 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 1347-1356: The setupNim(gpu) function currently assumes
sandboxName exists which causes a ReferenceError; move any code that starts or
registers a NIM container (calls to nim.startNimContainer(...) and
registry.updateSandbox(...)) out of setupNim and defer it until after
createSandbox(...) returns in onboard(), so that you call
nim.startNimContainer(sandboxName, model) and then
registry.updateSandbox(sandboxName, { nimContainer }) using the real
sandboxName; keep setupNim responsible only for selecting
model/provider/preferredInferenceApi and return a flag or the model info (e.g.,
nimPending or model) to onboard(), then perform the actual container creation
and sandbox update there.
In `@docs/reference/inference-profiles.md`:
- Around line 41-50: The docs incorrectly mark Local Ollama as experimental and
omit it from the non-experimental provider table; update
docs/reference/inference-profiles.md to add a row for "Local Ollama" in the
provider table (e.g., Provider: Local Ollama, Endpoint Type: OpenAI-compatible
or appropriate type, Notes: local Ollama runtime) and remove or adjust the text
that wraps Local Ollama behind NEMOCLAW_EXPERIMENTAL (the gating mentioned later
in the file) so Local Ollama is listed alongside the other non-experimental
providers (ensure references to NEMOCLAW_EXPERIMENTAL only apply to local NVIDIA
NIM and local vLLM as intended).
In `@README.md`:
- Around line 184-193: The README's "Supported non-experimental onboarding
paths" table omits "Local Ollama" while the surrounding text still labels Ollama
as experimental; update the table row entries to include a "Local Ollama"
provider (with a short note like "Local Ollama — local model serving via
Ollama") or alternatively narrow the heading to explicitly state which providers
are listed (e.g., "Supported non-experimental onboarding paths (cloud
providers)"). Ensure the change aligns with the PR that moves Ollama into the
standard flow and update any adjacent note that still calls Ollama experimental
so wording is consistent.
In `@test/onboard.test.js`:
- Around line 196-215: The test fixture uses the old provider id "nvidia-nim" in
both the mocked inference get output and the setup call; update the mock
response lines that include "Provider: nvidia-nim" and the setupInference
invocation that passes "nvidia-nim" to instead use "nvidia-prod" so the test
aligns with the rest of the PR (look for the mocked block that returns "Gateway
inference" and the call to setupInference("test-box", ..., "nvidia-nim") and
replace the provider string in both places).
---
Outside diff comments:
In `@bin/lib/credentials.js`:
- Around line 172-179: The file uses execSync in functions isRepoPrivate and
ensureGithubToken but never imports it; add an import for execSync from the
child_process module at the top of the file so both functions can call execSync
without causing a ReferenceError (locate the top-level imports and add the
execSync import alongside them so isRepoPrivate and ensureGithubToken work
correctly).
---
Nitpick comments:
In `@docs/inference/switch-inference-providers.md`:
- Line 94: The bullet "Runtime switching changes the OpenShell route. It does
not rewrite your stored credentials." contains two sentences on one source line;
split it into two source lines so each sentence is on its own line (e.g., place
"Runtime switching changes the OpenShell route." and "It does not rewrite your
stored credentials." on separate lines) to satisfy the "one sentence per source
line" guideline and keep diffs readable.
In `@docs/reference/inference-profiles.md`:
- Line 6: The keywords array uses incorrect lowercase "nemoclaw" tokens which
violate the casing rule; update the two entries in the keywords array to use the
exact "NemoClaw" casing (e.g., change "nemoclaw inference profiles" -> "NemoClaw
inference profiles" and "nemoclaw provider routing" -> "NemoClaw provider
routing") so the keywords list and search metadata follow the required NemoClaw
capitalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99f3efbe-a2ad-41d0-85ad-76ed5ea3acbd
📒 Files selected for processing (15)
DockerfileREADME.mdbin/lib/credentials.jsbin/lib/inference-config.jsbin/lib/local-inference.jsbin/lib/onboard.jsdocs/about/how-it-works.mddocs/inference/switch-inference-providers.mddocs/reference/commands.mddocs/reference/inference-profiles.mddocs/reference/troubleshooting.mdnemoclaw/src/commands/migration-state.test.tstest/inference-config.test.jstest/onboard-selection.test.jstest/onboard.test.js
kjw3
requested review from
cv,
drobison00,
ericksoa,
jayavenkatesh19,
liveaverage and
miyoungc
kjw3
added
the
enhancement: provider
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/onboard.js (1)
1220-1233:⚠️ Potential issue | 🟠 MajorReusing an existing sandbox can leave the baked model identity stale.
By this point the gateway has already been switched to the newly selected provider/model, but this early return skips Dockerfile patching and rebuild. If the user changed either value, the existing sandbox keeps the old baked OpenClaw identity while the host route now points somewhere else.
Also applies to: 1988-1990
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1220 - 1233, The early return after the user chooses to "Keep existing sandbox" (`if (recreate.toLowerCase() !== "y") { ... return sandboxName; }`) causes Dockerfile patching and rebuild to be skipped, leaving a stale baked model identity; instead, change the flow so that choosing to keep the existing sandbox does NOT return early — run the subsequent Dockerfile patching and rebuild steps (the code that updates the baked OpenClaw identity) unless you can detect that neither provider nor model changed; modify the branch around liveExists / prompt (symbols: liveExists, isNonInteractive(), prompt(), sandboxName) to only short-circuit when it is safe (e.g., provider/model unchanged), and apply the same fix at the other occurrence mentioned (the logic around lines 1988-1990) so both code paths perform Dockerfile patching/rebuild when necessary.
♻️ Duplicate comments (1)
bin/lib/onboard.js (1)
1779-1780:⚠️ Potential issue | 🟠 MajorDon't write sandbox metadata under
GATEWAY_NAME.
setupInference()is invoked with"nemoclaw"beforecreateSandbox()returns the real sandbox name, but it still updates the registry withmodel/provider. That either creates a straynemoclawentry or leaves the real sandbox showingunknowninnemoclaw list/status.Suggested fix
- await setupInference(GATEWAY_NAME, model, provider, endpointUrl, credentialEnv); + await setupInference(model, provider, endpointUrl, credentialEnv); const sandboxName = await createSandbox(gpu, model, provider, preferredInferenceApi); - if (nimContainer) { - registry.updateSandbox(sandboxName, { nimContainer }); - } + registry.updateSandbox(sandboxName, { + model, + provider, + ...(nimContainer ? { nimContainer } : {}), + });and remove the registry write from
setupInference(), since that step is gateway-scoped, not sandbox-scoped.Also applies to: 1986-1993
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1779 - 1780, The code writes sandbox metadata under the GATEWAY_NAME because setupInference() (called with "nemoclaw") calls registry.updateSandbox(sandboxName, { model, provider }) before the real sandbox name is known; remove the registry.updateSandbox call from setupInference() so gateway-scoped setup doesn't write sandbox-scoped metadata, and instead perform registry.updateSandbox(sandboxName, { model, provider }) only after createSandbox() returns the actual sandboxName (or in the function that has the real sandboxName), keeping verifyInferenceRoute(...) and provider/model handling in setupInference() but not the registry write.
🤖 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 531-552: The non-interactive validation path is using process.env
directly and can miss credentials; update validateOpenAiLikeSelection to resolve
credentials via getCredential(credentialEnv) (not raw process.env lookup) before
calling probeOpenAiLikeEndpoint so the probe gets the same credential resolution
as interactive flows (use the same pattern for other validators mentioned);
specifically, ensure the apiKey is derived via getCredential when credentialEnv
is provided, pass that apiKey into probeOpenAiLikeEndpoint, and replicate this
fix for the other functions that call probeOpenAiLikeEndpoint/Anthropic probes
so non-interactive CI runs use the resolved credential.
- Around line 1265-1275: The code is appending secrets (e.g., NVIDIA_API_KEY,
DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN) into envArgs which get serialized into the
openshell sandbox create command and exposed on process argv; instead stop
adding secret values via formatEnvAssignment into envArgs and pass them via a
secure channel: set these values on the spawned process environment
(child_process.spawn/exec options.env) or deliver via stdin/temporary file with
restrictive permissions before calling runOpenshell, updating the logic around
envArgs, formatEnvAssignment and the call site that invokes runOpenshell so
non-secret flags remain in envArgs while actual token values are injected into
the child process env (or a secure file) rather than the command line.
- Around line 436-445: The curl invocations built into the cmd array (the code
that constructs cmd using probe.body and probe.url and then calls
spawnSync("bash", ["-c", cmd])) need connection and overall timeouts added so a
stalled endpoint doesn't hang onboarding; modify the cmd construction to include
--connect-timeout 5 and --max-time 15 (or use configurable constants) before the
URL and data args, and apply the same change to the other identical curl
builders (the other cmd/spawnSync occurrences) so all synchronous probes time
out reliably.
- Around line 1351-1355: When the nim-local fallback path triggers inside
selectionLoop it only logs and breaks without choosing cloud inference, leaving
model and preferredInferenceApi unset; change that flow to assign the cloud
provider values before breaking: set provider, endpointUrl, credentialEnv from
the remote/cloud configuration (e.g., REMOTE_PROVIDER_CONFIG.build or the
appropriate cloud entry), and set preferredInferenceApi to the cloud inference
API name so later inference setup/Dockerfile patching has valid routing data;
update the branch that currently only logs the fallback to perform these
assignments (affecting variables provider, endpointUrl, credentialEnv,
preferredInferenceApi and ensuring model is set/cleared appropriately) before
exiting selectionLoop.
---
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 1220-1233: The early return after the user chooses to "Keep
existing sandbox" (`if (recreate.toLowerCase() !== "y") { ... return
sandboxName; }`) causes Dockerfile patching and rebuild to be skipped, leaving a
stale baked model identity; instead, change the flow so that choosing to keep
the existing sandbox does NOT return early — run the subsequent Dockerfile
patching and rebuild steps (the code that updates the baked OpenClaw identity)
unless you can detect that neither provider nor model changed; modify the branch
around liveExists / prompt (symbols: liveExists, isNonInteractive(), prompt(),
sandboxName) to only short-circuit when it is safe (e.g., provider/model
unchanged), and apply the same fix at the other occurrence mentioned (the logic
around lines 1988-1990) so both code paths perform Dockerfile patching/rebuild
when necessary.
---
Duplicate comments:
In `@bin/lib/onboard.js`:
- Around line 1779-1780: The code writes sandbox metadata under the GATEWAY_NAME
because setupInference() (called with "nemoclaw") calls
registry.updateSandbox(sandboxName, { model, provider }) before the real sandbox
name is known; remove the registry.updateSandbox call from setupInference() so
gateway-scoped setup doesn't write sandbox-scoped metadata, and instead perform
registry.updateSandbox(sandboxName, { model, provider }) only after
createSandbox() returns the actual sandboxName (or in the function that has the
real sandboxName), keeping verifyInferenceRoute(...) and provider/model handling
in setupInference() but not the registry write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1468b6ba-e6ed-433c-9846-91527e84a444
📒 Files selected for processing (6)
README.mdbin/lib/nim.jsbin/lib/onboard.jsbin/nemoclaw.jstest/e2e/test-full-e2e.shtest/local-inference.test.js
✅ Files skipped from review due to trivial changes (1)
- README.md
Comment on lines
+436
to
+445
| const cmd = [ | ||
| "curl -sS", | ||
| `-o ${shellQuote(bodyFile)}`, | ||
| "-w '%{http_code}'", | ||
| "-H 'Content-Type: application/json'", | ||
| ...(apiKey ? ['-H "Authorization: Bearer $NEMOCLAW_PROBE_API_KEY"'] : []), | ||
| `-d ${shellQuote(probe.body)}`, | ||
| shellQuote(probe.url), | ||
| ].join(" "); | ||
| const result = spawnSync("bash", ["-c", cmd], { |
There was a problem hiding this comment.
Add time limits to the live validation curls.
All of these probes are synchronous spawnSync(...curl...) calls without --connect-timeout or --max-time. A blackholed hostname or stalled endpoint will hang onboarding indefinitely instead of reaching the retry/reprompt flow.
Minimal hardening
- "curl -sS",
+ "curl -sS --connect-timeout 5 --max-time 15",Also applies to: 479-493, 641-649, 694-701, 729-737
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 436 - 445, The curl invocations built into
the cmd array (the code that constructs cmd using probe.body and probe.url and
then calls spawnSync("bash", ["-c", cmd])) need connection and overall timeouts
added so a stalled endpoint doesn't hang onboarding; modify the cmd construction
to include --connect-timeout 5 and --max-time 15 (or use configurable constants)
before the URL and data args, and apply the same change to the other identical
curl builders (the other cmd/spawnSync occurrences) so all synchronous probes
time out reliably.
Comment on lines
1265
to
1275
| if (process.env.NVIDIA_API_KEY) { | ||
| envArgs.push(`NVIDIA_API_KEY=${shellQuote(process.env.NVIDIA_API_KEY)}`); | ||
| envArgs.push(formatEnvAssignment("NVIDIA_API_KEY", process.env.NVIDIA_API_KEY)); | ||
| } | ||
| const discordToken = getCredential("DISCORD_BOT_TOKEN") || process.env.DISCORD_BOT_TOKEN; | ||
| if (discordToken) { | ||
| envArgs.push(`DISCORD_BOT_TOKEN=${shellQuote(discordToken)}`); | ||
| envArgs.push(formatEnvAssignment("DISCORD_BOT_TOKEN", discordToken)); | ||
| } | ||
| const slackToken = getCredential("SLACK_BOT_TOKEN") || process.env.SLACK_BOT_TOKEN; | ||
| if (slackToken) { | ||
| envArgs.push(`SLACK_BOT_TOKEN=${shellQuote(slackToken)}`); | ||
| envArgs.push(formatEnvAssignment("SLACK_BOT_TOKEN", slackToken)); | ||
| } |
There was a problem hiding this comment.
NVIDIA_API_KEY still leaks through the sandbox-create argv.
envArgs is serialized into openshell sandbox create ... -- env ..., and runOpenshell() shells that through bash -c, so the provider key is still exposed in process arguments during onboarding. The same argv path also affects any other tokens appended here.
Also applies to: 1281-1284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 1265 - 1275, The code is appending secrets
(e.g., NVIDIA_API_KEY, DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN) into envArgs which
get serialized into the openshell sandbox create command and exposed on process
argv; instead stop adding secret values via formatEnvAssignment into envArgs and
pass them via a secure channel: set these values on the spawned process
environment (child_process.spawn/exec options.env) or deliver via
stdin/temporary file with restrictive permissions before calling runOpenshell,
updating the logic around envArgs, formatEnvAssignment and the call site that
invokes runOpenshell so non-secret flags remain in envArgs while actual token
values are injected into the child process env (or a secure file) rather than
the command line.
Contributor
Author
|
Reviewer summary for this PR: This branch changes NemoClaw from a mostly NVIDIA-hosted onboarding flow into a provider-first routed inference flow. What changed
Validation behavior
Notable compatibility work
Docs and tests
Validation completed on this branch
Still intentionally experimental
If you are reviewing behavior, the highest-value things to check are:
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
bin/lib/onboard.js (3)
1264-1275:⚠️ Potential issue | 🟠 MajorSecrets still passed through sandbox-create argv.
NVIDIA_API_KEY,DISCORD_BOT_TOKEN, andSLACK_BOT_TOKENvalues are serialized intoenvArgsand passed torunOpenshell(["sandbox", "create", ...]). This exposes secrets in process arguments during onboarding.Consider passing secrets via the spawned process's environment or a temporary file with restricted permissions instead of command-line arguments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1264 - 1275, The diff is serializing secrets into envArgs (via formatEnvAssignment) which are passed to runOpenshell(["sandbox", "create", ...]) exposing sensitive values; change the logic so NVIDIA_API_KEY, DISCORD_BOT_TOKEN and SLACK_BOT_TOKEN are NOT appended to envArgs but are injected via the child process environment or a secure temporary file instead: detect values using getCredential or process.env as now, remove any pushes to envArgs for those keys, and when invoking runOpenshell("sandbox", "create", ...) pass an env object that includes these secrets (or pass a path to a file created with restrictive permissions) so secrets never appear in the command-line; update any callers that build envArgs/formatEnvAssignment and ensure runOpenshell accepts and forwards the env map to the spawned process.
436-452:⚠️ Potential issue | 🟠 MajorAdd timeouts to synchronous curl probes.
These
spawnSynccurl calls lack--connect-timeoutand--max-timeflags. A blackholed or stalled endpoint will hang onboarding indefinitely instead of reaching the retry/reprompt flow.Suggested fix
const cmd = [ - "curl -sS", + "curl -sS --connect-timeout 5 --max-time 30", `-o ${shellQuote(bodyFile)}`,Apply this pattern to all curl invocations: lines 436, 479, 641, 694, and 729.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 436 - 452, The curl command built into the cmd array before the spawnSync call (where variables like probe, bodyFile, shellQuote, and ROOT are used) must include connection and total timeouts to avoid hanging; add --connect-timeout and --max-time flags with appropriate values to the array construction that creates cmd so the spawned bash curl call receives them, and apply the same change to the other identical curl invocations in this file (the other places that construct a curl cmd and call spawnSync with the same pattern).
531-552:⚠️ Potential issue | 🟡 MinorCredential resolution works correctly for interactive flows.
The
validateOpenAiLikeSelectionfunction properly usesgetCredential(credentialEnv)to resolve credentials. This addresses the concern about credential lookup for validation.However, for non-interactive mode, if credentials are only in
process.envand not stored viasaveCredential, there's a gap sincegetCredentialreads from the credentials file, notprocess.env.Suggested fix to support env-only credentials
async function validateOpenAiLikeSelection( label, endpointUrl, model, credentialEnv = null, retryMessage = "Please choose a provider/model again." ) { - const apiKey = credentialEnv ? getCredential(credentialEnv) : ""; + const apiKey = credentialEnv ? (getCredential(credentialEnv) || process.env[credentialEnv] || "") : ""; const probe = probeOpenAiLikeEndpoint(endpointUrl, model, apiKey);Apply the same pattern to
validateAnthropicSelection(line 555),validateAnthropicSelectionWithRetryMessage(line 578),validateCustomOpenAiLikeSelection(line 595), andvalidateCustomAnthropicSelection(line 617).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 531 - 552, The non-interactive validation functions (validateOpenAiLikeSelection, validateAnthropicSelection, validateAnthropicSelectionWithRetryMessage, validateCustomOpenAiLikeSelection, validateCustomAnthropicSelection) currently only use getCredential(credentialEnv) which reads the credentials file and thus misses credentials provided solely via process.env; update each function to resolve apiKey by falling back to process.env when getCredential returns falsy (e.g. const apiKey = credentialEnv ? (getCredential(credentialEnv) || process.env[credentialEnv] || "") : "";), then pass that apiKey into the existing probe* calls so non-interactive flows accept env-only credentials.
🧹 Nitpick comments (3)
bin/lib/onboard.js (2)
1615-1643: Duplicate Ollama validation logic.The Ollama model selection and validation loop appears twice: once in the
ollamabranch (lines 1615-1643) and again in theinstall-ollamabranch (lines 1655-1683). Consider extracting this into a helper function.Extract shared Ollama validation loop
+async function selectAndValidateOllamaModel(gpu, requestedModel) { + while (true) { + const installedModels = getOllamaModelOptions(runCapture); + let model; + if (isNonInteractive()) { + model = requestedModel || getDefaultOllamaModel(runCapture, gpu); + } else { + model = await promptOllamaModel(gpu); + } + const probe = prepareOllamaModel(model, installedModels); + if (!probe.ok) { + console.error(` ${probe.message}`); + if (isNonInteractive()) process.exit(1); + console.log(" Choose a different Ollama model or select Other."); + console.log(""); + continue; + } + const api = await validateOpenAiLikeSelection( + "Local Ollama", + getLocalProviderValidationBaseUrl("ollama-local"), + model, + null, + "Choose a different Ollama model or select Other." + ); + if (!api) continue; + return { model, api }; + } +}Also applies to: 1655-1683
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1615 - 1643, The Ollama model selection/validation loop (using getOllamaModelOptions, isNonInteractive, getDefaultOllamaModel, promptOllamaModel, prepareOllamaModel, validateOpenAiLikeSelection, getLocalProviderValidationBaseUrl, model, preferredInferenceApi) is duplicated; extract it into a single helper function (for example selectAndValidateOllamaModel(runCapture, provider, gpu, requestedModel)) that encapsulates the while(true) loop and returns the validated preferredInferenceApi (and selected model if needed), then replace both duplicated blocks in the ollama and install-ollama branches by calling this helper and handling its return value.
1568-1570: Cloud fallback relies on implicit defaults—consider making this explicit.When no NIM models fit GPU VRAM, the code logs "Falling back to cloud API" but then falls through without explicitly setting cloud provider values. This works because the defaults at lines 1351-1355 are already set to NVIDIA Endpoints (
nvidia-prod), but the implicit behavior is subtle.Make the fallback explicit
if (models.length === 0) { console.log(" No NIM models fit your GPU VRAM. Falling back to cloud API."); + // Defaults already set to NVIDIA Endpoints (nvidia-prod) + break; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1568 - 1570, The branch that handles models.length === 0 only logs a message but relies on earlier implicit defaults for the cloud fallback; explicitly set the cloud fallback variables when no local NIM models fit GPU VRAM by assigning the cloud provider/endpoint and any flag that selects cloud inference (e.g., set provider/endpoint to the NVIDIA production endpoint value used as the default and ensure the cloud selection flag is true). Update the models.length === 0 block to explicitly set the same provider/endpoint variables used elsewhere (the variables that previously defaulted to "nvidia-prod"/NVIDIA) so the fallback behavior is clear and not reliant on implicit earlier defaults.test/onboard.test.js (1)
174-177: Subprocess tests don't clean up temporary directories.The subprocess tests create temporary directories (e.g., line 176) but don't clean them up after completion, unlike the earlier tests that use
try/finallywithfs.rmSync. Consider adding cleanup or using a shared test fixture helper.Add cleanup to subprocess tests
+ try { const result = spawnSync(process.execPath, [scriptPath], { // ... }); // assertions... + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 174 - 177, The subprocess test starting with the "passes credential names to openshell without embedding secret values in argv" it-block creates temporary directories (tmpDir, fakeBin) but never removes them; wrap the test body in a try/finally (or add an afterEach/fixture) and in the finally call fs.rmSync(tmpDir, { recursive: true, force: true }) to ensure tmpDir and fakeBin are removed, referencing tmpDir/fakeBin variables in test/onboard.test.js to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bin/lib/onboard.js`:
- Around line 1264-1275: The diff is serializing secrets into envArgs (via
formatEnvAssignment) which are passed to runOpenshell(["sandbox", "create",
...]) exposing sensitive values; change the logic so NVIDIA_API_KEY,
DISCORD_BOT_TOKEN and SLACK_BOT_TOKEN are NOT appended to envArgs but are
injected via the child process environment or a secure temporary file instead:
detect values using getCredential or process.env as now, remove any pushes to
envArgs for those keys, and when invoking runOpenshell("sandbox", "create", ...)
pass an env object that includes these secrets (or pass a path to a file created
with restrictive permissions) so secrets never appear in the command-line;
update any callers that build envArgs/formatEnvAssignment and ensure
runOpenshell accepts and forwards the env map to the spawned process.
- Around line 436-452: The curl command built into the cmd array before the
spawnSync call (where variables like probe, bodyFile, shellQuote, and ROOT are
used) must include connection and total timeouts to avoid hanging; add
--connect-timeout and --max-time flags with appropriate values to the array
construction that creates cmd so the spawned bash curl call receives them, and
apply the same change to the other identical curl invocations in this file (the
other places that construct a curl cmd and call spawnSync with the same
pattern).
- Around line 531-552: The non-interactive validation functions
(validateOpenAiLikeSelection, validateAnthropicSelection,
validateAnthropicSelectionWithRetryMessage, validateCustomOpenAiLikeSelection,
validateCustomAnthropicSelection) currently only use
getCredential(credentialEnv) which reads the credentials file and thus misses
credentials provided solely via process.env; update each function to resolve
apiKey by falling back to process.env when getCredential returns falsy (e.g.
const apiKey = credentialEnv ? (getCredential(credentialEnv) ||
process.env[credentialEnv] || "") : "";), then pass that apiKey into the
existing probe* calls so non-interactive flows accept env-only credentials.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 1615-1643: The Ollama model selection/validation loop (using
getOllamaModelOptions, isNonInteractive, getDefaultOllamaModel,
promptOllamaModel, prepareOllamaModel, validateOpenAiLikeSelection,
getLocalProviderValidationBaseUrl, model, preferredInferenceApi) is duplicated;
extract it into a single helper function (for example
selectAndValidateOllamaModel(runCapture, provider, gpu, requestedModel)) that
encapsulates the while(true) loop and returns the validated
preferredInferenceApi (and selected model if needed), then replace both
duplicated blocks in the ollama and install-ollama branches by calling this
helper and handling its return value.
- Around line 1568-1570: The branch that handles models.length === 0 only logs a
message but relies on earlier implicit defaults for the cloud fallback;
explicitly set the cloud fallback variables when no local NIM models fit GPU
VRAM by assigning the cloud provider/endpoint and any flag that selects cloud
inference (e.g., set provider/endpoint to the NVIDIA production endpoint value
used as the default and ensure the cloud selection flag is true). Update the
models.length === 0 block to explicitly set the same provider/endpoint variables
used elsewhere (the variables that previously defaulted to "nvidia-prod"/NVIDIA)
so the fallback behavior is clear and not reliant on implicit earlier defaults.
In `@test/onboard.test.js`:
- Around line 174-177: The subprocess test starting with the "passes credential
names to openshell without embedding secret values in argv" it-block creates
temporary directories (tmpDir, fakeBin) but never removes them; wrap the test
body in a try/finally (or add an afterEach/fixture) and in the finally call
fs.rmSync(tmpDir, { recursive: true, force: true }) to ensure tmpDir and fakeBin
are removed, referencing tmpDir/fakeBin variables in test/onboard.test.js to
locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e2c3775-838b-4bd9-84f5-170136252f4e
📒 Files selected for processing (8)
README.mdbin/lib/credentials.jsbin/lib/onboard.jsdocs/inference/switch-inference-providers.mddocs/reference/inference-profiles.mdnemoclaw/src/commands/migration-state.test.tsnemoclaw/src/commands/migration-state.tstest/onboard.test.js
✅ Files skipped from review due to trivial changes (1)
- nemoclaw/src/commands/migration-state.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- nemoclaw/src/commands/migration-state.test.ts
- README.md
- bin/lib/credentials.js
kjw3
force-pushed
the
feat/openai-anthropic-gemini-providers
branch
from
69adc37 to
cca052e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/onboard.js (1)
1565-1604:⚠️ Potential issue | 🟠 Major
nim-localfallback leavesmodelunset when no NIM models fit GPU.If a user selects
nim-localbut no models fit their GPU VRAM (line 1568), the code logs a fallback message and breaks. However,modelremainsnullfrom line 1350, causing downstreamcreateSandboxandsetupInferenceto receive invalid data.Since this path is already gated by
NEMOCLAW_EXPERIMENTAL=1, consider prompting the user to select a different provider instead of silently continuing with null values.Suggested fix
if (models.length === 0) { - console.log(" No NIM models fit your GPU VRAM. Falling back to cloud API."); + console.log(" No NIM models fit your GPU VRAM. Please choose a different provider."); + continue selectionLoop; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1565 - 1604, When the "nim-local" branch finds no compatible models (models.length === 0), the code currently logs a fallback and breaks while leaving the shared variable model null, causing downstream callers like createSandbox and setupInference to receive invalid input; update this branch so that instead of just logging and breaking it prompts the user to choose a different provider (or programmatically falls back to a cloud provider) and sets provider and model to valid values before continuing, or exits with an explicit error—modify the nim-local handling around the models.length === 0 check to either re-run the provider selection flow (so model gets assigned) or set provider to the cloud fallback and assign a sane default to model, while preserving the NEMOCLAW_EXPERIMENTAL gate.
♻️ Duplicate comments (2)
bin/lib/onboard.js (2)
531-552:⚠️ Potential issue | 🟠 MajorNon-interactive validation may fail due to credential resolution mismatch.
In non-interactive mode, lines 1460-1477 check
process.env[credentialEnv]to verify credentials are present, but thenvalidateOpenAiLikeSelectionand similar functions resolve credentials viagetCredential()(line 538). IfgetCredential()reads from~/.nemoclaw/credentials.jsonrather thanprocess.env, the validation probe will run without the API key even though the env var was provided.Suggested fix
+function resolveCredentialValue(envName) { + return envName ? (process.env[envName] || getCredential(envName) || "") : ""; +} + async function validateOpenAiLikeSelection( label, endpointUrl, model, credentialEnv = null, retryMessage = "Please choose a provider/model again." ) { - const apiKey = credentialEnv ? getCredential(credentialEnv) : ""; + const apiKey = resolveCredentialValue(credentialEnv);Apply the same pattern to
validateAnthropicSelection,validateAnthropicSelectionWithRetryMessage,validateCustomOpenAiLikeSelection, andvalidateCustomAnthropicSelection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 531 - 552, The validation functions (validateOpenAiLikeSelection, validateAnthropicSelection, validateAnthropicSelectionWithRetryMessage, validateCustomOpenAiLikeSelection, validateCustomAnthropicSelection) resolve credentials via getCredential(), which can read from the credentials file and thus miss an API key provided in process.env[credentialEnv]; update each function so when credentialEnv is passed it first checks process.env[credentialEnv] for the API key and uses that value if present, otherwise fall back to getCredential(credentialEnv), then pass that resolved apiKey into probeOpenAiLikeEndpoint / probeAnthropicEndpoint calls to ensure non-interactive validation uses environment-provided secrets.
436-445:⚠️ Potential issue | 🟠 MajorAdd timeouts to validation curl commands.
These synchronous
spawnSync(...curl...)calls lack--connect-timeoutand--max-time. A blackholed hostname or stalled endpoint will hang onboarding indefinitely instead of reaching the retry/reprompt flow.Suggested fix
const cmd = [ - "curl -sS", + "curl -sS --connect-timeout 5 --max-time 30", `-o ${shellQuote(bodyFile)}`,Apply the same fix to the other curl invocations at lines 480, 642, 695, and 730.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 436 - 445, The curl invocations built into the cmd array (used with spawnSync in the onboarding flow) lack timeouts and can hang; update the cmd construction to include safe timeout flags (e.g., add '--connect-timeout 5' and '--max-time 30' as separate elements in the array before the URL) while preserving existing headers and shellQuote usage (refer to the cmd, probe.body, probe.url, shellQuote, and spawnSync occurrences); apply the same change to the other curl command constructions/spawnSync calls in this file (the other cmd/spawnSync blocks) so every synchronous curl includes the connect and overall timeouts.
🧹 Nitpick comments (2)
docs/reference/inference-profiles.md (1)
23-25: Split compound sentence for clarity.Line 24 is a long compound sentence. Consider splitting for readability and to follow the one-sentence-per-line guideline.
Suggested fix
NemoClaw configures inference through the OpenShell gateway. -The agent inside the sandbox talks to `inference.local`, and OpenShell routes that traffic to the provider you selected during onboarding. +The agent inside the sandbox talks to `inference.local`. +OpenShell routes that traffic to the provider you selected during onboarding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/inference-profiles.md` around lines 23 - 25, The sentence "The agent inside the sandbox talks to `inference.local`, and OpenShell routes that traffic to the provider you selected during onboarding." is a long compound sentence — split it into two clearer sentences; for example, keep "The agent inside the sandbox talks to `inference.local`." as one sentence and make "OpenShell routes that traffic to the provider you selected during onboarding." a separate sentence so the document follows the one-sentence-per-line guideline and improves readability.docs/inference/switch-inference-providers.md (1)
90-95: Use a MyST admonition for the Notes section.The "Notes" section uses a plain H2 header with bullet points. Consider using a
:::{note}admonition for callout content, which better signals informational context to readers. As per coding guidelines, "Use MyST admonitions (:::{tip}, :::{note}, :::{warning}) for callouts, not bold text or blockquotes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/inference/switch-inference-providers.md` around lines 90 - 95, Replace the plain H2 "## Notes" and its bullet list with a MyST admonition block by converting the header into a `:::{note}` block, placing the four bullet lines ("- The host keeps provider credentials.", "- The sandbox continues to use `inference.local`.", "- Runtime switching changes the OpenShell route.", "- It does not rewrite your stored credentials.") inside the admonition, and closing it with `:::` so the content becomes a proper MyST note callout instead of a normal section.
🤖 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/credentials.js`:
- Around line 110-124: The outer Promise in prompt() only accepts resolve so the
silent branch's catch never settles callers; update the Promise constructor to
(resolve, reject) and in the promptSecret().catch handler call reject(err) for
both error paths instead of throwing (and for the SIGINT path call reject(err)
then process.kill(process.pid, "SIGINT") to ensure the Promise is settled before
exiting). This change should be made in the prompt function and touches the
promptSecret error handling block so callers awaiting prompt() won't hang.
In `@bin/lib/onboard.js`:
- Around line 1264-1275: The envArgs array currently includes sensitive values
via formatEnvAssignment for NVIDIA_API_KEY, DISCORD_BOT_TOKEN, and
SLACK_BOT_TOKEN which causes secret leakage in command-line arguments; change
the onboarding flow so these keys are not appended to envArgs and instead inject
them into the spawned process environment (opts.env) or send them over stdin/a
securely permissioned temp file when calling the sandbox create command;
specifically, stop pushing formatEnvAssignment("NVIDIA_API_KEY", ...),
formatEnvAssignment("DISCORD_BOT_TOKEN", ...), and
formatEnvAssignment("SLACK_BOT_TOKEN", ...) into envArgs and set process spawn
options.env (or write/read from stdin/temp file) so the secrets are available to
the child process without appearing in the argv.
---
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 1565-1604: When the "nim-local" branch finds no compatible models
(models.length === 0), the code currently logs a fallback and breaks while
leaving the shared variable model null, causing downstream callers like
createSandbox and setupInference to receive invalid input; update this branch so
that instead of just logging and breaking it prompts the user to choose a
different provider (or programmatically falls back to a cloud provider) and sets
provider and model to valid values before continuing, or exits with an explicit
error—modify the nim-local handling around the models.length === 0 check to
either re-run the provider selection flow (so model gets assigned) or set
provider to the cloud fallback and assign a sane default to model, while
preserving the NEMOCLAW_EXPERIMENTAL gate.
---
Duplicate comments:
In `@bin/lib/onboard.js`:
- Around line 531-552: The validation functions (validateOpenAiLikeSelection,
validateAnthropicSelection, validateAnthropicSelectionWithRetryMessage,
validateCustomOpenAiLikeSelection, validateCustomAnthropicSelection) resolve
credentials via getCredential(), which can read from the credentials file and
thus miss an API key provided in process.env[credentialEnv]; update each
function so when credentialEnv is passed it first checks
process.env[credentialEnv] for the API key and uses that value if present,
otherwise fall back to getCredential(credentialEnv), then pass that resolved
apiKey into probeOpenAiLikeEndpoint / probeAnthropicEndpoint calls to ensure
non-interactive validation uses environment-provided secrets.
- Around line 436-445: The curl invocations built into the cmd array (used with
spawnSync in the onboarding flow) lack timeouts and can hang; update the cmd
construction to include safe timeout flags (e.g., add '--connect-timeout 5' and
'--max-time 30' as separate elements in the array before the URL) while
preserving existing headers and shellQuote usage (refer to the cmd, probe.body,
probe.url, shellQuote, and spawnSync occurrences); apply the same change to the
other curl command constructions/spawnSync calls in this file (the other
cmd/spawnSync blocks) so every synchronous curl includes the connect and overall
timeouts.
---
Nitpick comments:
In `@docs/inference/switch-inference-providers.md`:
- Around line 90-95: Replace the plain H2 "## Notes" and its bullet list with a
MyST admonition block by converting the header into a `:::{note}` block, placing
the four bullet lines ("- The host keeps provider credentials.", "- The sandbox
continues to use `inference.local`.", "- Runtime switching changes the OpenShell
route.", "- It does not rewrite your stored credentials.") inside the
admonition, and closing it with `:::` so the content becomes a proper MyST note
callout instead of a normal section.
In `@docs/reference/inference-profiles.md`:
- Around line 23-25: The sentence "The agent inside the sandbox talks to
`inference.local`, and OpenShell routes that traffic to the provider you
selected during onboarding." is a long compound sentence — split it into two
clearer sentences; for example, keep "The agent inside the sandbox talks to
`inference.local`." as one sentence and make "OpenShell routes that traffic to
the provider you selected during onboarding." a separate sentence so the
document follows the one-sentence-per-line guideline and improves readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12d15cc3-576c-4c90-99bf-e5ba1679442a
📒 Files selected for processing (18)
DockerfileREADME.mdbin/lib/credentials.jsbin/lib/inference-config.jsbin/lib/local-inference.jsbin/lib/nim.jsbin/lib/onboard.jsbin/nemoclaw.jsdocs/about/how-it-works.mddocs/inference/switch-inference-providers.mddocs/reference/commands.mddocs/reference/inference-profiles.mddocs/reference/troubleshooting.mdtest/e2e/test-full-e2e.shtest/inference-config.test.jstest/local-inference.test.jstest/onboard-selection.test.jstest/onboard.test.js
✅ Files skipped from review due to trivial changes (3)
- docs/reference/troubleshooting.md
- docs/reference/commands.md
- test/onboard.test.js
🚧 Files skipped from review as they are similar to previous changes (6)
- test/local-inference.test.js
- docs/about/how-it-works.md
- test/inference-config.test.js
- bin/lib/nim.js
- README.md
- test/onboard-selection.test.js
Comment on lines
+110
to
+124
| function prompt(question, opts = {}) { | ||
| return new Promise((resolve) => { | ||
| const silent = opts.secret === true && process.stdin.isTTY && process.stderr.isTTY; | ||
| if (silent) { | ||
| promptSecret(question) | ||
| .then(resolve) | ||
| .catch((err) => { | ||
| if (err && err.code === "SIGINT") { | ||
| process.kill(process.pid, "SIGINT"); | ||
| return; | ||
| } | ||
| throw err; | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unresolved Promise paths in prompt() silent branch (read-only).
awk 'NR>=110 && NR<=124 {printf "%4d %s\n", NR, $0}' bin/lib/credentials.js
rg -nP 'new Promise\(\(resolve\)\s*=>\s*\{' bin/lib/credentials.js -n -A12 -B2Repository: NVIDIA/NemoClaw
Length of output: 1081
🏁 Script executed:
sed -n '110,200p' bin/lib/credentials.js | cat -nRepository: NVIDIA/NemoClaw
Length of output: 3624
Settle the outer prompt() Promise on silent-branch failures.
The Promise at line 111 only accepts resolve, but the silent branch (lines 114–123) has error paths that never settle it. When promptSecret() rejects with SIGINT (line 118), process.kill() executes and returns, leaving the outer Promise unresolved. For other errors (line 121), throw creates an unhandled rejection while the outer Promise still hangs. Callers awaiting prompt() can hang indefinitely.
Add reject to the Promise constructor and call reject(err) in both error cases:
Suggested fix
-function prompt(question, opts = {}) {
- return new Promise((resolve) => {
+function prompt(question, opts = {}) {
+ return new Promise((resolve, reject) => {
const silent = opts.secret === true && process.stdin.isTTY && process.stderr.isTTY;
if (silent) {
promptSecret(question)
.then(resolve)
.catch((err) => {
if (err && err.code === "SIGINT") {
process.kill(process.pid, "SIGINT");
- return;
+ reject(err);
+ return;
}
- throw err;
+ reject(err);
});
return;
}📝 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
| function prompt(question, opts = {}) { | |
| return new Promise((resolve) => { | |
| const silent = opts.secret === true && process.stdin.isTTY && process.stderr.isTTY; | |
| if (silent) { | |
| promptSecret(question) | |
| .then(resolve) | |
| .catch((err) => { | |
| if (err && err.code === "SIGINT") { | |
| process.kill(process.pid, "SIGINT"); | |
| return; | |
| } | |
| throw err; | |
| }); | |
| return; | |
| } | |
| function prompt(question, opts = {}) { | |
| return new Promise((resolve, reject) => { | |
| const silent = opts.secret === true && process.stdin.isTTY && process.stderr.isTTY; | |
| if (silent) { | |
| promptSecret(question) | |
| .then(resolve) | |
| .catch((err) => { | |
| if (err && err.code === "SIGINT") { | |
| process.kill(process.pid, "SIGINT"); | |
| reject(err); | |
| return; | |
| } | |
| reject(err); | |
| }); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/credentials.js` around lines 110 - 124, The outer Promise in prompt()
only accepts resolve so the silent branch's catch never settles callers; update
the Promise constructor to (resolve, reject) and in the promptSecret().catch
handler call reject(err) for both error paths instead of throwing (and for the
SIGINT path call reject(err) then process.kill(process.pid, "SIGINT") to ensure
the Promise is settled before exiting). This change should be made in the prompt
function and touches the promptSecret error handling block so callers awaiting
prompt() won't hang.
Comment on lines
+1264
to
1275
| const envArgs = [formatEnvAssignment("CHAT_UI_URL", chatUiUrl)]; | ||
| if (process.env.NVIDIA_API_KEY) { | ||
| envArgs.push(`NVIDIA_API_KEY=${shellQuote(process.env.NVIDIA_API_KEY)}`); | ||
| envArgs.push(formatEnvAssignment("NVIDIA_API_KEY", process.env.NVIDIA_API_KEY)); | ||
| } | ||
| const discordToken = getCredential("DISCORD_BOT_TOKEN") || process.env.DISCORD_BOT_TOKEN; | ||
| if (discordToken) { | ||
| envArgs.push(`DISCORD_BOT_TOKEN=${shellQuote(discordToken)}`); | ||
| envArgs.push(formatEnvAssignment("DISCORD_BOT_TOKEN", discordToken)); | ||
| } | ||
| const slackToken = getCredential("SLACK_BOT_TOKEN") || process.env.SLACK_BOT_TOKEN; | ||
| if (slackToken) { | ||
| envArgs.push(`SLACK_BOT_TOKEN=${shellQuote(slackToken)}`); | ||
| envArgs.push(formatEnvAssignment("SLACK_BOT_TOKEN", slackToken)); | ||
| } |
There was a problem hiding this comment.
Secrets still leak through sandbox-create command arguments.
NVIDIA_API_KEY, DISCORD_BOT_TOKEN, and SLACK_BOT_TOKEN are serialized into envArgs and passed as command-line arguments to openshell sandbox create ... -- env NVIDIA_API_KEY=.... These values are visible in process listings (ps aux).
The PR objectives state "provider secrets are not passed in openshell argv during onboarding/provider creation," but this code path still exposes them.
Consider passing secrets via the spawned process's environment (opts.env) or via stdin/temporary file with restrictive permissions instead of command-line arguments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 1264 - 1275, The envArgs array currently
includes sensitive values via formatEnvAssignment for NVIDIA_API_KEY,
DISCORD_BOT_TOKEN, and SLACK_BOT_TOKEN which causes secret leakage in
command-line arguments; change the onboarding flow so these keys are not
appended to envArgs and instead inject them into the spawned process environment
(opts.env) or send them over stdin/a securely permissioned temp file when
calling the sandbox create command; specifically, stop pushing
formatEnvAssignment("NVIDIA_API_KEY", ...),
formatEnvAssignment("DISCORD_BOT_TOKEN", ...), and
formatEnvAssignment("SLACK_BOT_TOKEN", ...) into envArgs and set process spawn
options.env (or write/read from stdin/temp file) so the secrets are available to
the child process without appearing in the argv.
|
❯ @kjw3 Does baking the model into openclaw.json matter given the gateway rewrites the model in flight? As the openshell inference set handles routing regardless of what openclaw.json says, even random model names get rewritten. Since the .nemoclaw/config.json actually have the inference model, why not make that override openclaw.json and be controlled by the user on the host via ~/.nemoclaw/config.json in my current sandbox |
kjw3
force-pushed
the
feat/openai-anthropic-gemini-providers
branch
from
cca052e to
c6cfccc
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
bin/lib/credentials.js (1)
110-124:⚠️ Potential issue | 🔴 CriticalThe outer
prompt()Promise remains unsettled on silent-branch failures.The Promise at line 111 only accepts
resolve, but the silent branch (lines 114–123) has error paths that never settle it. WhenpromptSecret()rejects with SIGINT (line 117-119),process.kill()executes and returns, leaving the outer Promise unresolved. For other errors (line 121),throwcreates an unhandled rejection while the outer Promise still hangs. Callers awaitingprompt()can hang indefinitely.Suggested fix
function prompt(question, opts = {}) { - return new Promise((resolve) => { + return new Promise((resolve, reject) => { const silent = opts.secret === true && process.stdin.isTTY && process.stderr.isTTY; if (silent) { promptSecret(question) .then(resolve) .catch((err) => { if (err && err.code === "SIGINT") { + reject(err); process.kill(process.pid, "SIGINT"); return; } - throw err; + reject(err); }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/credentials.js` around lines 110 - 124, The outer Promise returned by prompt(...) only accepts resolve, so the silent branch using promptSecret(...) can reject without settling the outer Promise; modify the Promise executor to accept (resolve, reject) and in the promptSecret(...).catch handler call reject(err) instead of throwing (and for the SIGINT branch, call reject(err) after invoking process.kill or simply reject with a descriptive error before/after killing) so all error paths settle the outer Promise; update references in this logic around the prompt function and its promptSecret call accordingly.bin/lib/onboard.js (2)
436-445:⚠️ Potential issue | 🟠 MajorAdd time limits to the live validation curls.
These probes use synchronous
spawnSync(...curl...)calls without--connect-timeoutor--max-time. A blackholed hostname or stalled endpoint will hang onboarding indefinitely instead of reaching the retry/reprompt flow.Suggested fix
const cmd = [ - "curl -sS", + "curl -sS --connect-timeout 5 --max-time 15", `-o ${shellQuote(bodyFile)}`,Apply the same hardening to all
curlinvocations inprobeAnthropicEndpoint,fetchNvidiaEndpointModels,fetchOpenAiLikeModels, andfetchAnthropicModels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 436 - 445, The curl-based synchronous probes build a cmd array and call spawnSync("bash", ["-c", cmd]) without time limits causing potential hangs; update the cmd construction in the probe functions (probeAnthropicEndpoint, fetchNvidiaEndpointModels, fetchOpenAiLikeModels, fetchAnthropicModels) to include robust curl timing flags such as --connect-timeout <seconds> and --max-time <seconds> (e.g., small sensible defaults) as additional elements in the cmd array before spawning, so every curl invocation uses fixed connection and total timeouts and still preserves the existing headers/body/url handling.
1264-1275:⚠️ Potential issue | 🟠 MajorSecrets still leak through sandbox-create command arguments.
NVIDIA_API_KEY,DISCORD_BOT_TOKEN, andSLACK_BOT_TOKENare serialized intoenvArgsand passed as command-line arguments toopenshell sandbox create ... -- env NVIDIA_API_KEY=.... These values are visible in process listings (ps aux).The PR objectives state "provider secrets are not passed in openshell argv during onboarding/provider creation." While the provider creation flow now correctly uses
--credential ENV_NAME, the sandbox startup still embeds actual secret values.Consider passing secrets via a temporary env file with restrictive permissions or through the spawned process's environment rather than the command line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1264 - 1275, The envArgs array in onboard.js (built via formatEnvAssignment and populated from process.env.NVIDIA_API_KEY and getCredential("DISCORD_BOT_TOKEN") / getCredential("SLACK_BOT_TOKEN")) is leaking secrets on the command line; stop adding provider secrets to envArgs and instead deliver them via a secure channel (e.g., create a temporary file with restrictive permissions and write NVIDIA_API_KEY/ DISCORD_BOT_TOKEN/ SLACK_BOT_TOKEN as KEY=VALUE, or set them on the child process environment when spawning the sandbox) and update the sandbox creation invocation to reference that secure source rather than embedding values in envArgs. Ensure the temp file is created with mode 0600 and removed after use, or use spawn/child_process.env to pass secrets, and keep formatEnvAssignment usage only for non-secret vars like CHAT_UI_URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bin/lib/credentials.js`:
- Around line 110-124: The outer Promise returned by prompt(...) only accepts
resolve, so the silent branch using promptSecret(...) can reject without
settling the outer Promise; modify the Promise executor to accept (resolve,
reject) and in the promptSecret(...).catch handler call reject(err) instead of
throwing (and for the SIGINT branch, call reject(err) after invoking
process.kill or simply reject with a descriptive error before/after killing) so
all error paths settle the outer Promise; update references in this logic around
the prompt function and its promptSecret call accordingly.
In `@bin/lib/onboard.js`:
- Around line 436-445: The curl-based synchronous probes build a cmd array and
call spawnSync("bash", ["-c", cmd]) without time limits causing potential hangs;
update the cmd construction in the probe functions (probeAnthropicEndpoint,
fetchNvidiaEndpointModels, fetchOpenAiLikeModels, fetchAnthropicModels) to
include robust curl timing flags such as --connect-timeout <seconds> and
--max-time <seconds> (e.g., small sensible defaults) as additional elements in
the cmd array before spawning, so every curl invocation uses fixed connection
and total timeouts and still preserves the existing headers/body/url handling.
- Around line 1264-1275: The envArgs array in onboard.js (built via
formatEnvAssignment and populated from process.env.NVIDIA_API_KEY and
getCredential("DISCORD_BOT_TOKEN") / getCredential("SLACK_BOT_TOKEN")) is
leaking secrets on the command line; stop adding provider secrets to envArgs and
instead deliver them via a secure channel (e.g., create a temporary file with
restrictive permissions and write NVIDIA_API_KEY/ DISCORD_BOT_TOKEN/
SLACK_BOT_TOKEN as KEY=VALUE, or set them on the child process environment when
spawning the sandbox) and update the sandbox creation invocation to reference
that secure source rather than embedding values in envArgs. Ensure the temp file
is created with mode 0600 and removed after use, or use spawn/child_process.env
to pass secrets, and keep formatEnvAssignment usage only for non-secret vars
like CHAT_UI_URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eea6ce79-73cb-432d-b62f-7762f11c91bc
📒 Files selected for processing (18)
DockerfileREADME.mdbin/lib/credentials.jsbin/lib/inference-config.jsbin/lib/local-inference.jsbin/lib/nim.jsbin/lib/onboard.jsbin/nemoclaw.jsdocs/about/how-it-works.mddocs/inference/switch-inference-providers.mddocs/reference/commands.mddocs/reference/inference-profiles.mddocs/reference/troubleshooting.mdtest/e2e/test-full-e2e.shtest/inference-config.test.jstest/local-inference.test.jstest/onboard-selection.test.jstest/onboard.test.js
✅ Files skipped from review due to trivial changes (1)
- docs/reference/commands.md
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/reference/troubleshooting.md
- test/local-inference.test.js
- test/inference-config.test.js
- bin/lib/local-inference.js
- README.md
- test/e2e/test-full-e2e.sh
- bin/lib/nim.js
- bin/lib/inference-config.js
- test/onboard-selection.test.js
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
2 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
Expand
nemoclaw onboardfrom an NVIDIA-only path into provider-first onboarding with live validation for hosted and local inference runtimes. This makes the sandbox model identity match the selected provider/model, adds compatible endpoint support, and updates the docs to reflect the current host-routed inference architecture.Gemini note: the compatible-endpoint path here was informed by the debugging thread in #368. Credit to @andy-ratsirarson for surfacing the
supportsStore: falsedirection that unblocked Gemini, and to @alannguyen1 for adjacent investigation that helped narrow the fix path.Related Issue
Closes #24
Closes #567
Closes #628
Related:
openshellargv during onboarding/provider setupOther OpenAI-compatible endpointChanges
NVIDIA Endpoints,OpenAI,Other OpenAI-compatible endpoint,Anthropic,Other Anthropic-compatible endpoint,Google Gemini, andLocal Ollama, with experimental localNIM/vLLMstill gated separately./responsesfirst and fall back to/chat/completions/v1/messagesOther...flows with retry logic:/v1/models/modelsinference.localwith the correct selected model identity so in-sandbox reporting matches the active provider instead of a baked Nemotron default.openshellargv during onboarding/provider creation.main.Type of Change
Testing
make checkpasses.npm testpasses.make docsbuilds without warnings. (for doc-only changes)Additional validation:
cd nemoclaw && npm run buildnode --test test/onboard-selection.test.js test/onboard.test.js test/inference-config.test.jsChecklist
General
Code Changes
make formatapplied (TypeScript and Python).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Summary by CodeRabbit
New Features
Documentation
Tests