fix(cli): respect defaultValue in promptOrDefault interactive mode (Fixes #360)#631
Open
craigamcw wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix(cli): respect defaultValue in promptOrDefault interactive mode (Fixes #360)#631craigamcw wants to merge 1 commit intoNVIDIA:mainfrom
craigamcw wants to merge 1 commit intoNVIDIA:mainfrom
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:
📝 WalkthroughWalkthroughPrompt handling in onboarding was changed: non-interactive env values are read raw and trimmed (empty/whitespace treated as unset), interactive prompts route through an overrideable prompt, sandbox name is computed from the trimmed answer (no hardcoded "my-assistant" fallback), and test hooks plus Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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: 2
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
968-969: Coerce test toggle input to boolean.Line 969 currently accepts any value; passing
"false"would still enable non-interactive mode. Coercing avoids accidental test flakiness.💡 Proposed fix
-function _setNonInteractiveForTest(value) { NON_INTERACTIVE = value; } +function _setNonInteractiveForTest(value) { NON_INTERACTIVE = Boolean(value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 968 - 969, The _setNonInteractiveForTest helper currently assigns the input directly to NON_INTERACTIVE which treats truthy strings like "false" as true; change _setNonInteractiveForTest to coerce the incoming value to a boolean (e.g., use Boolean(value) or !!value) before assigning to NON_INTERACTIVE so only true-like values enable non-interactive mode; update the function named _setNonInteractiveForTest and ensure references to NON_INTERACTIVE remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/onboard.test.js`:
- Around line 35-85: The tests never exercise the interactive branch of
promptOrDefault; add a test seam so promptOrDefault can be invoked with an
injectable prompt function (or expose a setter like setPromptForTest) and then
write tests that disable non-interactive mode via
_setNonInteractiveForTest(false) and call promptOrDefault using the injected
prompt to simulate responses "", " ", and " custom " asserting that
empty/blank inputs fall back to default and trimmed inputs return the trimmed
custom value; update the existing test suite to restore non-interactive mode and
remove the injected prompt after each test to avoid cross-test leakage.
- Around line 40-43: In the after hook replace the unconditional delete of
process.env.TEST_PROMPT_CUSTOM with logic that restores whatever value existed
before the test: save the original value (e.g., in a local variable at top of
the test file or in the before hook), call _setNonInteractiveForTest(false) as
you do now, and then if the saved original is undefined delete
process.env.TEST_PROMPT_CUSTOM else set process.env.TEST_PROMPT_CUSTOM back to
the saved value so pre-existing environment state is preserved.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 968-969: The _setNonInteractiveForTest helper currently assigns
the input directly to NON_INTERACTIVE which treats truthy strings like "false"
as true; change _setNonInteractiveForTest to coerce the incoming value to a
boolean (e.g., use Boolean(value) or !!value) before assigning to
NON_INTERACTIVE so only true-like values enable non-interactive mode; update the
function named _setNonInteractiveForTest and ensure references to
NON_INTERACTIVE remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46e3e00d-1f57-48a8-bb17-81bdda1f307c
📒 Files selected for processing (2)
bin/lib/onboard.jstest/onboard.test.js
craigamcw
force-pushed
the
fix/prompt-or-default-fallback
branch
3 times, most recently
from
ab85d17 to
7b826ab
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/lib/onboard.js (2)
55-64: Normalize non-interactive prompt values consistently with interactive mode.
promptOrDefault()now trims/falls back in interactive mode, but non-interactive mode still uses raw env values. A whitespace-only env var can bypass defaulting and later fail sandbox-name validation. Consider applying the sametrim() + empty => defaultrule in both branches.Proposed diff
async function promptOrDefault(question, envVar, defaultValue) { if (isNonInteractive()) { - const val = envVar ? process.env[envVar] : null; - const result = val || defaultValue; + const val = envVar ? process.env[envVar] : ""; + const normalized = String(val || "").trim(); + const result = normalized || defaultValue; note(` [non-interactive] ${question.trim()} → ${result}`); return result; } const promptFn = _promptOverride || prompt; const answer = (await promptFn(question) || "").trim(); return answer || defaultValue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 55 - 64, The non-interactive branch of promptOrDefault uses raw env values and can return whitespace-only strings; update it to mirror the interactive branch by reading process.env[envVar], trimming the value, and treating empty/whitespace as absent so it falls back to defaultValue. In practice, inside promptOrDefault's isNonInteractive() branch (the code that reads process.env[envVar] into val/result), call .trim() on the env value, then use the trimmed value || defaultValue and log the trimmed result, ensuring the same behavior as the interactive path.
1060-1061: Optionally harden_setPromptForTestinput.A non-function truthy value will cause a runtime error when
promptFn(question)is called. A small type guard would make test failures clearer.Proposed diff
let _promptOverride = null; -function _setPromptForTest(fn) { _promptOverride = fn; } +function _setPromptForTest(fn) { + if (fn !== null && typeof fn !== "function") { + throw new TypeError("_setPromptForTest expects a function or null"); + } + _promptOverride = fn; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1060 - 1061, The test helper _setPromptForTest currently assigns any truthy value to _promptOverride which will later cause a runtime error when promptFn(question) is invoked; update the function _setPromptForTest to validate its input (accept only a function or null/undefined) and either coerce non-function falsy values to null or throw a clear TypeError for invalid types, e.g. check typeof fn === 'function' before assigning to _promptOverride and reject other truthy non-function values so failures are explicit.
🤖 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 55-64: The non-interactive branch of promptOrDefault uses raw env
values and can return whitespace-only strings; update it to mirror the
interactive branch by reading process.env[envVar], trimming the value, and
treating empty/whitespace as absent so it falls back to defaultValue. In
practice, inside promptOrDefault's isNonInteractive() branch (the code that
reads process.env[envVar] into val/result), call .trim() on the env value, then
use the trimmed value || defaultValue and log the trimmed result, ensuring the
same behavior as the interactive path.
- Around line 1060-1061: The test helper _setPromptForTest currently assigns any
truthy value to _promptOverride which will later cause a runtime error when
promptFn(question) is invoked; update the function _setPromptForTest to validate
its input (accept only a function or null/undefined) and either coerce
non-function falsy values to null or throw a clear TypeError for invalid types,
e.g. check typeof fn === 'function' before assigning to _promptOverride and
reject other truthy non-function values so failures are explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ba54f1e-2ddf-4db5-820d-89edaa615ac3
📒 Files selected for processing (2)
bin/lib/onboard.jstest/onboard.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/onboard.test.js
craigamcw
force-pushed
the
fix/prompt-or-default-fallback
branch
from
7b826ab to
c05689e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
483-483: Minor: Redundant.trim()call.Since
promptOrDefault(line 63) already trims the answer before returning, the.trim()here is redundant. The code works correctly as-is, but you could simplify to:- const sandboxName = nameAnswer.trim().toLowerCase(); + const sandboxName = nameAnswer.toLowerCase();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` at line 483, The assignment to sandboxName redundantly calls .trim() because promptOrDefault already returns a trimmed string; remove the extra .trim() from the sandboxName initialization (where sandboxName is set from nameAnswer) so it simply lowercases the returned value, and leave promptOrDefault (the function that trims input) unchanged.
🤖 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`:
- Line 483: The assignment to sandboxName redundantly calls .trim() because
promptOrDefault already returns a trimmed string; remove the extra .trim() from
the sandboxName initialization (where sandboxName is set from nameAnswer) so it
simply lowercases the returned value, and leave promptOrDefault (the function
that trims input) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c1094ff-5382-490a-8d28-839af303832c
📒 Files selected for processing (2)
bin/lib/onboard.jstest/onboard.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/onboard.test.js
Implemented feature with help from Claude Code promptOrDefault discarded defaultValue in interactive mode — the raw prompt() return was used directly, so pressing Enter or entering whitespace yielded an empty string instead of the default. The redundant `|| "my-assistant"` fallback in createSandbox masked the problem only for that one call site. - Capture and trim the interactive answer, falling back to defaultValue when empty. - Remove the redundant fallback in createSandbox (now handled upstream). - Export promptOrDefault and _setNonInteractiveForTest for testability. - Add _setPromptForTest to inject a mock prompt for interactive tests. - Add 7 non-interactive tests covering env var, empty/whitespace fallback, trim preservation, and RFC 1123 validation. - Add 5 interactive tests covering empty input, whitespace-only, padded input, clean input, and null prompt return. Fixes NVIDIA#360 Signed-off-by: Craig <craig@epic28.com>
craigamcw
force-pushed
the
fix/prompt-or-default-fallback
branch
from
c05689e to
cfaadff
Compare
craigamcw
changed the title
Open
craigamcw
pushed a commit
to craigamcw/NemoClaw
that referenced
this pull request
Mar 22, 2026
Upstream main migrated from node:test to vitest (NVIDIA#653). Update the promptOrDefault tests to use vitest globals (expect/toMatch/toBe) and beforeAll/afterAll instead of node:test assert + before/after. Add root vitest.config.ts with globals enabled. https://claude.ai/code/session_01JwUa2BdccTciPkfsSRtBsM
craigamcw
pushed a commit
to craigamcw/NemoClaw
that referenced
this pull request
Mar 22, 2026
Resolve merge conflict with upstream vitest migration (NVIDIA#653). Rebase PR NVIDIA#631 onto current upstream main so the test file uses vitest globals (expect/toMatch/toBe, beforeAll/afterAll) instead of the removed node:test + node:assert/strict APIs. Fixes NVIDIA#360 https://claude.ai/code/session_01JwUa2BdccTciPkfsSRtBsM
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.
Implemented feature with help from Claude Code
promptOrDefault discarded defaultValue in interactive mode — the raw prompt() return was used directly, so pressing Enter or entering whitespace yielded an empty string instead of the default. The redundant
|| "my-assistant"fallback in createSandbox masked the problem only for that one call site.Summary
Fix
promptOrDefaultto respectdefaultValuein interactive mode. Previously, pressing Enter or entering whitespace returned an empty string instead of the default, because the rawprompt()return was used directly. The redundant|| "my-assistant"fallback increateSandboxmasked the bug for that one call site only.Related Issue
Fixes #360
Changes
promptOrDefault, falling back todefaultValuewhen the result is empty.|| "my-assistant"fallback increateSandbox(now handled upstream inpromptOrDefault).promptOrDefaultand_setNonInteractiveForTestfor testability.Type of Change
Testing
npm testpasses (174/174, 7 new tests added).make checkpasses — 4 pre-existing TypeScript lint errors in unrelated files (fetch.ts,logs.ts,status.test.ts); identical onmainbranch, not introduced by this PR.make docsbuilds without warnings. (for doc-only changes)Checklist
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
Bug Fixes
Tests
Chores