refactor(cli): extract model prompt helpers from onboard.js - #1515
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracts model-selection prompt logic into a new module ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (5)
src/lib/model-prompts.ts (2)
131-137: Null credential passed to validator may cause unclear errors.When
getCredentialFn("NVIDIA_API_KEY")returnsnull, the fallback|| ""passes an empty string tovalidateNvidiaEndpointModelFn. This will likely cause an authentication failure during validation, which might surface as a confusing error message rather than guiding the user to set their API key first.Consider whether this edge case should be handled explicitly with a clearer prompt or error message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/model-prompts.ts` around lines 131 - 137, The current call to promptManualModelId passes deps.validateNvidiaEndpointModelFn(model, deps.getCredentialFn("NVIDIA_API_KEY") || "") which masks a missing API key by sending an empty string to validateNvidiaEndpointModelFn; update the flow to explicitly check deps.getCredentialFn("NVIDIA_API_KEY") for null/undefined before calling promptManualModelId (or call the validator) and, if missing, return or surface a clear error/prompt telling the user to set the NVIDIA_API_KEY; reference the functions promptManualModelId, validateNvidiaEndpointModelFn and getCredentialFn("NVIDIA_API_KEY") when making this change so the validator never receives a silent empty credential.
126-129: Consider handlingNaNfromparseIntexplicitly.If the user enters non-numeric text (e.g., "abc"),
parseIntreturnsNaN, andNaN - 1yieldsNaN. The conditionindex >= 0 && index < deps.cloudModelOptions.lengthevaluates tofalseforNaN, which correctly falls through to manual entry. However, this relies on implicit behavior that could confuse future maintainers.The same pattern appears in
promptRemoteModelat line 167.♻️ Optional: Make NaN handling explicit
const index = parseInt(choice || "1", 10) - 1; - if (index >= 0 && index < deps.cloudModelOptions.length) { + if (Number.isFinite(index) && index >= 0 && index < deps.cloudModelOptions.length) { return deps.cloudModelOptions[index].id; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/model-prompts.ts` around lines 126 - 129, The parseInt result for choice in the index computation can be NaN; update the logic in the function containing "const index = parseInt(choice || "1", 10) - 1;" (and the similar code in promptRemoteModel) to explicitly detect NaN (e.g., using Number.isNaN) before comparing ranges, so that non-numeric input is handled clearly and falls through to manual entry; ensure you only use deps.cloudModelOptions[index].id when the parsed index is a valid integer within [0, deps.cloudModelOptions.length - 1].src/lib/provider-models.ts (2)
37-52:parseModelIdscan throw on malformed JSON; callers catch this but behavior is implicit.
JSON.parsewill throw ifbodyis not valid JSON. The calling functions (fetchNvidiaEndpointModels, etc.) wrap this in try-catch, so runtime errors are handled. However, the flow is implicit—a malformed response results in a generic error message rather than indicating a parse failure.This is acceptable since the outer catch handles it, but documenting this behavior or returning an empty array on parse failure would make the intent clearer.
♻️ Optional: Handle parse errors gracefully within parseModelIds
function parseModelIds(body: string, itemKeys: string[] = ["id"]): string[] { + let parsed: { data?: Array<Record<string, unknown> | null> }; + try { + parsed = JSON.parse(body) as { data?: Array<Record<string, unknown> | null> }; + } catch { + return []; + } - const parsed = JSON.parse(body) as { data?: Array<Record<string, unknown> | null> }; if (!Array.isArray(parsed?.data)) return []; // ... rest unchanged }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/provider-models.ts` around lines 37 - 52, The parseModelIds function can throw on invalid JSON; wrap the JSON.parse call inside parseModelIds (function parseModelIds) with a try-catch and return an empty string[] on parse failure so callers like fetchNvidiaEndpointModels don’t rely on implicit external try-catch for JSON errors; ensure the function still validates parsed?.data as an array and preserves the current mapping/filtering behavior when parse succeeds.
147-150: Hardcoded Anthropic API version may become stale.The
anthropic-version: 2023-06-01header is hardcoded and duplicated across multiple files (src/lib/provider-models.ts and bin/lib/onboard.js). While this is currently the latest version, making it configurable viaProviderModelOptionsor extracting it to a constant would improve maintainability and reduce the need for coordinated updates across files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/provider-models.ts` around lines 147 - 150, The anthropic-version header is hardcoded; extract it into a single configurable constant (e.g., ANTHROPIC_API_VERSION) or add it to ProviderModelOptions so callers can override, then replace the literal "anthropic-version: 2023-06-01" in the header array used with normalizeCredentialValue(apiKey) with a reference to that constant/option; ensure all other places that set the same header (e.g., onboarding logic) read the same constant/option so the version is centralized and not duplicated.src/lib/validation-recovery.ts (1)
72-86: Consider caching classification results to avoid repeated iterations.
classifyValidationFailureis called multiple times on the same failures across.some(),.find(), and the fallback logic. For small failure arrays this is fine, but it could be optimized by pre-classifying once.♻️ Optional: Pre-classify failures to avoid repeated work
export function getProbeRecovery( probe: ProbeLike, options: ProbeRecoveryOptions = {}, ): ProbeRecovery { const allowModelRetry = options.allowModelRetry === true; const failures = Array.isArray(probe?.failures) ? probe.failures : []; if (failures.length === 0) { return { kind: "unknown", retry: "selection" }; } + const classified = failures.map((f) => ({ failure: f, classification: classifyValidationFailure(f) })); - if (failures.some((failure) => classifyValidationFailure(failure).kind === "credential")) { + if (classified.some((c) => c.classification.kind === "credential")) { return { kind: "credential", retry: "credential" }; } - const transportFailure = failures.find( - (failure) => classifyValidationFailure(failure).kind === "transport", - ); + const transportEntry = classified.find((c) => c.classification.kind === "transport"); + const transportFailure = transportEntry?.failure; if (transportFailure) { return { kind: "transport", retry: "retry", failure: transportFailure }; } - if (allowModelRetry && failures.some((failure) => classifyValidationFailure(failure).kind === "model")) { + if (allowModelRetry && classified.some((c) => c.classification.kind === "model")) { return { kind: "model", retry: "model" }; } - if (failures.some((failure) => classifyValidationFailure(failure).kind === "endpoint")) { + if (classified.some((c) => c.classification.kind === "endpoint")) { return { kind: "endpoint", retry: "selection" }; } - const fallback = classifyValidationFailure(failures[0]); + const fallback = classified[0].classification; if (!allowModelRetry && fallback.kind === "model") { return { kind: "unknown", retry: "selection" }; } return fallback; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/validation-recovery.ts` around lines 72 - 86, Pre-classify the failures once instead of calling classifyValidationFailure repeatedly: create an array (e.g., const classified = failures.map(f => ({ failure: f, kind: classifyValidationFailure(f).kind })) or a Map) and then use that classified collection for the subsequent checks (replace calls in the .some() and .find() checks and the allowModelRetry branch), updating transportFailure to be taken from classified (e.g., classified.find(c => c.kind === "transport")?.failure) so all branches reference the pre-computed kinds (referencing classifyValidationFailure, failures, allowModelRetry, and transportFailure).
🤖 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 1367-1368: The Ollama branches call promptOllamaModel() and pass
its result straight into prepareOllamaModel(), but
promptManualModelId()/promptOllamaModel() can return the BACK_TO_SELECTION
sentinel (e.g. "__NEMOCLAW_BACK_TO_SELECTION__"); update the Ollama flow so
after calling promptOllamaModel() you check if the return equals the
BACK_TO_SELECTION sentinel and if so return/propagate that sentinel (or re-enter
provider selection) instead of calling prepareOllamaModel(); adjust the branches
that currently call prepareOllamaModel() directly (the Ollama branch that
invokes promptOllamaModel and then prepareOllamaModel) to handle this early
return.
In `@src/lib/http-probe.ts`:
- Around line 103-114: The Node process timeout passed to spawnSyncImpl
(currently 30_000) can fire before curl's own --max-time (60s) used by
getCurlTimingArgs(), so update the call that invokes spawnSyncImpl with the curl
args (the block constructing [...args, "-o", bodyFile, "-w", "%{http_code}",
String(url || "")]) to compute a timeout that is at least as long as any
--max-time provided by the args (convert seconds to ms) plus a small buffer
(e.g., 1-2s); implement this by scanning the args array for a "--max-time"
value, using Math.max(currentTimeout, parsedMaxTime*1000 + buffer) when building
the options object (cwd/encoding/env remain unchanged) so Node won't kill the
process before curl's timeout and error 28 is preserved.
---
Nitpick comments:
In `@src/lib/model-prompts.ts`:
- Around line 131-137: The current call to promptManualModelId passes
deps.validateNvidiaEndpointModelFn(model, deps.getCredentialFn("NVIDIA_API_KEY")
|| "") which masks a missing API key by sending an empty string to
validateNvidiaEndpointModelFn; update the flow to explicitly check
deps.getCredentialFn("NVIDIA_API_KEY") for null/undefined before calling
promptManualModelId (or call the validator) and, if missing, return or surface a
clear error/prompt telling the user to set the NVIDIA_API_KEY; reference the
functions promptManualModelId, validateNvidiaEndpointModelFn and
getCredentialFn("NVIDIA_API_KEY") when making this change so the validator never
receives a silent empty credential.
- Around line 126-129: The parseInt result for choice in the index computation
can be NaN; update the logic in the function containing "const index =
parseInt(choice || "1", 10) - 1;" (and the similar code in promptRemoteModel) to
explicitly detect NaN (e.g., using Number.isNaN) before comparing ranges, so
that non-numeric input is handled clearly and falls through to manual entry;
ensure you only use deps.cloudModelOptions[index].id when the parsed index is a
valid integer within [0, deps.cloudModelOptions.length - 1].
In `@src/lib/provider-models.ts`:
- Around line 37-52: The parseModelIds function can throw on invalid JSON; wrap
the JSON.parse call inside parseModelIds (function parseModelIds) with a
try-catch and return an empty string[] on parse failure so callers like
fetchNvidiaEndpointModels don’t rely on implicit external try-catch for JSON
errors; ensure the function still validates parsed?.data as an array and
preserves the current mapping/filtering behavior when parse succeeds.
- Around line 147-150: The anthropic-version header is hardcoded; extract it
into a single configurable constant (e.g., ANTHROPIC_API_VERSION) or add it to
ProviderModelOptions so callers can override, then replace the literal
"anthropic-version: 2023-06-01" in the header array used with
normalizeCredentialValue(apiKey) with a reference to that constant/option;
ensure all other places that set the same header (e.g., onboarding logic) read
the same constant/option so the version is centralized and not duplicated.
In `@src/lib/validation-recovery.ts`:
- Around line 72-86: Pre-classify the failures once instead of calling
classifyValidationFailure repeatedly: create an array (e.g., const classified =
failures.map(f => ({ failure: f, kind: classifyValidationFailure(f).kind })) or
a Map) and then use that classified collection for the subsequent checks
(replace calls in the .some() and .find() checks and the allowModelRetry
branch), updating transportFailure to be taken from classified (e.g.,
classified.find(c => c.kind === "transport")?.failure) so all branches reference
the pre-computed kinds (referencing classifyValidationFailure, failures,
allowModelRetry, and transportFailure).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c1cca71-f5b5-4377-8f2a-b24c7f5cd5d3
📒 Files selected for processing (11)
bin/lib/onboard.jssrc/lib/http-probe.test.tssrc/lib/http-probe.tssrc/lib/model-prompts.test.tssrc/lib/model-prompts.tssrc/lib/provider-models.test.tssrc/lib/provider-models.tssrc/lib/validation-recovery.test.tssrc/lib/validation-recovery.tstest/credential-exposure.test.jstest/onboard.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
1365-1366: ImportBACK_TO_SELECTIONfrommodel-promptsas well.Line 99 still hardcodes the sentinel while this import already pulls the rest of the prompt contract from
modelPrompts. Keeping the sentinel duplicated in two modules is easy to desync in follow-up extractions and would silently break the new back-navigation checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1365 - 1366, The code imports several symbols from modelPrompts but omits the BACK_TO_SELECTION sentinel; update the destructuring import (where promptManualModelId, promptCloudModel, promptRemoteModel, promptInputModel are imported from modelPrompts) to also include BACK_TO_SELECTION, then replace any hardcoded sentinel usage (the one referenced as the duplicated constant) with the imported BACK_TO_SELECTION to keep the prompt contract centralized and avoid desync between modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/model-prompts.ts`:
- Around line 96-101: The manual model-id prompt currently treats all
validator() failures as invalid IDs and loops, which swallows transient
auth/transport errors when promptCloudModel() passes a live /models validator;
change promptManualModelId (and the similar block at the other occurrence) to
distinguish validation failures from transient errors by having the validator
return or throw a distinct transient/retry signal (e.g., a {retry: true} result
or a specific Error subtype) instead of only ok/message, and when that transient
signal is seen call deps.errorLine with the error message and propagate a retry
indication back to the caller (do not continue the input loop); update
promptCloudModel to use that contract for the validator so auth/transport
failures bubble up to the caller’s recovery flow rather than being treated as
bad model ids.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 1365-1366: The code imports several symbols from modelPrompts but
omits the BACK_TO_SELECTION sentinel; update the destructuring import (where
promptManualModelId, promptCloudModel, promptRemoteModel, promptInputModel are
imported from modelPrompts) to also include BACK_TO_SELECTION, then replace any
hardcoded sentinel usage (the one referenced as the duplicated constant) with
the imported BACK_TO_SELECTION to keep the prompt contract centralized and avoid
desync between modules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72384c62-751d-478f-97f3-92b241c48871
📒 Files selected for processing (4)
bin/lib/onboard.jssrc/lib/model-prompts.test.tssrc/lib/model-prompts.tstest/onboard-selection.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/model-prompts.test.ts
brandonpelfrey
approved these changes
Apr 6, 2026
tranzmatt
pushed a commit
to tranzmatt/NemoClaw
that referenced
this pull request
Apr 6, 2026
) ## Summary - move cloud/remote/manual model prompt helpers into `src/lib/model-prompts.ts` - keep `onboard.js` focused on inference-selection orchestration - add focused tests for back-navigation, default selection, missing-key handling, and validator retry behavior ## Notes - merged `main` into this branch in `acafdcc`, so the PR now contains only the model-prompts extraction - explicitly handle missing `NVIDIA_API_KEY` before validating custom NVIDIA Endpoints models - handle the `BACK_TO_SELECTION` sentinel in both Ollama setup paths before attempting model preparation ## Testing - `npm run build:cli` - `npx vitest run --project cli src/lib/model-prompts.test.ts test/onboard-selection.test.js test/onboard.test.js test/credential-exposure.test.js` - `npm test` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed back navigation during local model entry—users can return to provider selection cleanly and see a confirmation log. * **Refactor** * Consolidated model selection and prompt flows into a shared module for more consistent behavior across cloud, remote, and manual model choices. * **Tests** * Added comprehensive tests covering cloud, remote, manual entry, navigation, validation, and retry scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
) ## Summary - move cloud/remote/manual model prompt helpers into `src/lib/model-prompts.ts` - keep `onboard.js` focused on inference-selection orchestration - add focused tests for back-navigation, default selection, missing-key handling, and validator retry behavior ## Notes - merged `main` into this branch in `acafdcc`, so the PR now contains only the model-prompts extraction - explicitly handle missing `NVIDIA_API_KEY` before validating custom NVIDIA Endpoints models - handle the `BACK_TO_SELECTION` sentinel in both Ollama setup paths before attempting model preparation ## Testing - `npm run build:cli` - `npx vitest run --project cli src/lib/model-prompts.test.ts test/onboard-selection.test.js test/onboard.test.js test/credential-exposure.test.js` - `npm test` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed back navigation during local model entry—users can return to provider selection cleanly and see a confirmation log. * **Refactor** * Consolidated model selection and prompt flows into a shared module for more consistent behavior across cloud, remote, and manual model choices. * **Tests** * Added comprehensive tests covering cloud, remote, manual entry, navigation, validation, and retry scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
21 tasks
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
3 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
src/lib/model-prompts.tsonboard.jsfocused on inference-selection orchestrationNotes
maininto this branch inacafdcc, so the PR now contains only the model-prompts extractionNVIDIA_API_KEYbefore validating custom NVIDIA Endpoints modelsBACK_TO_SELECTIONsentinel in both Ollama setup paths before attempting model preparationTesting
npm run build:clinpx vitest run --project cli src/lib/model-prompts.test.ts test/onboard-selection.test.js test/onboard.test.js test/credential-exposure.test.jsnpm testSummary by CodeRabbit
Bug Fixes
Refactor
Tests