feat(onboard): add --name flag for explicit sandbox naming#2591
Merged
Conversation
## Summary `nemoclaw onboard` had no `--name` flag, so picking a sandbox name in non-interactive or no-TTY runs required the undiscoverable `NEMOCLAW_SANDBOX_NAME` env var, and `--from <Dockerfile>` without a TTY silently fell back to the `my-assistant` default — clobbering the default sandbox. Adds `--name <sandbox>` plumbed through `OnboardCommandOptions` and `OnboardOptions`. The flag wins over the env var. When prompting is impossible (no TTY or `--non-interactive`) the env var is now seeded into the same path the flag takes, so it actually flows through to `createSandbox` instead of being dropped. `--from` runs that cannot prompt and have no resolved name now error cleanly. Both `--name` and `NEMOCLAW_SANDBOX_NAME` go through the same `validateName` + reserved-name guard that the interactive prompt enforces, so a name like `status` is rejected before it can shadow a CLI command. ## Related issues Closes #2543 ## Changes - `src/lib/onboard-command.ts`: parse `--name <sandbox>`, validate the value, surface it on `OnboardCommandOptions.sandboxName`, and list it in the usage line. - `src/lib/onboard.ts`: add `sandboxName` to `OnboardOptions`; lift the reserved-name set out of `promptValidatedSandboxName` so the flag and env-var paths share it; resolve the requested sandbox name early (flag, then env var when prompting is impossible), validate it once, and seed it into the local `sandboxName` so `createSandbox` receives the override; reject `--from` runs that cannot prompt and have no resolved name; teach `getRequestedSandboxNameHint`, `getResumeSandboxConflict`, and `getResumeConfigConflicts` to prefer `opts.sandboxName` over `NEMOCLAW_SANDBOX_NAME`; thread it through the resume conflict caller. - `src/lib/onboard-command.test.ts`: positive test for `--name` parsing alongside `--from`, plus the missing-value and flag-as-value error paths. - `test/onboard.test.ts`: cover hint precedence (option over env) and the resume conflict produced when `--name` diverges from the recorded sandbox. - `test/onboard-name-flag.test.ts` (new): integration coverage via spawnSync — non-TTY without `--non-interactive`, `--non-interactive` with whitespace-only env var, accept paths for `--name` and the env var, reserved-name rejection from both the flag and the env-var no-TTY seed, and `validateName` rejection of malformed `--name`. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "nemoclaw CLI"
participant Parser as "onboard-command.parseOnboardArgs"
participant Onboard as "onboard.onboard()"
participant Session as "session/sandboxes.json"
User->>CLI: run `nemoclaw onboard --from <path> --name foo`
CLI->>Parser: parse args
Parser-->>CLI: return options { from: <path>, sandboxName: "foo", ... }
CLI->>Onboard: invoke onboard(options)
Onboard->>Onboard: validate sandboxName (RFC1123, reserved)
alt invalid or missing in non-interactive --from
Onboard->>CLI: print usage + error
Onboard->>CLI: process.exit(1)
else valid
Onboard->>Session: check resume / existing sandbox conflicts (include sandboxName)
Onboard-->>Session: register or resume sandbox (write)
Onboard-->>CLI: complete onboarding
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/onboard.ts`:
- Around line 1622-1630: The helper getRequestedSandboxNameHint currently reads
process.env.NEMOCLAW_SANDBOX_NAME unconditionally which can introduce spurious
interactive resume conflicts; change the call sites (e.g., where
onboard()/getResumeConfigConflicts() build resume conflict checks) to pass the
already-resolved requestedSandboxName into getResumeConfigConflicts instead of
letting getRequestedSandboxNameHint fall back to the env var, or alternatively
modify getRequestedSandboxNameHint to accept a cannotPrompt flag and only use
process.env.NEMOCLAW_SANDBOX_NAME when cannotPrompt is true; update references
to getRequestedSandboxNameHint and getResumeConfigConflicts accordingly so the
env var is only considered when prompting is impossible.
- Around line 6555-6564: The guard that rejects no-TTY '--from' runs needs to
also verify whether a resumed session actually has a recorded sandbox name;
change the condition that currently reads "if (cannotPrompt && !resume &&
requestedFromDockerfile && !requestedSandboxName)" to only skip the check for
resume when the loaded session carries a name (e.g. require that resume is false
OR session.sandboxName is missing). In other words, include session.sandboxName
(or the object representing the loaded session) in the condition so the block
still errors when cannotPrompt && requestedFromDockerfile &&
!requestedSandboxName and (not resume OR resume && !session.sandboxName). Update
references to resume, cannotPrompt, requestedFromDockerfile,
requestedSandboxName and session.sandboxName in the if expression to enforce
this.
In `@test/onboard-name-flag.test.ts`:
- Around line 19-52: runOnboard currently inherits the host toolchain and can
call real Docker/OpenShell; create a hermetic fake bin directory (e.g., under
tmpDir) containing stub executables named "docker" and "openshell" (simple no-op
scripts that exit 0) and make them executable, then prepend that fake bin
directory to the PATH you build for the child process before calling spawnSync
(the env object passed to spawnSync, using tmpDir and scriptPath to locate the
temp workspace). This ensures runOnboard's spawnSync child uses the stubbed
docker/openshell rather than the host binaries and keeps tests deterministic and
side-effect free.
- Around line 47-50: The test's spawnSync call constructs env using the ambient
NodeJS namespace type (NodeJS.ProcessEnv) which triggers an ESLint no-undef;
update the env typing to use a local type by replacing NodeJS.ProcessEnv with
typeof process.env in the object passed to spawnSync (the env variable used
alongside process.execPath and scriptPath) so the test file no longer references
the global NodeJS ambient type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8bd3edea-25bc-4506-bae1-a821b5e1db58
📒 Files selected for processing (5)
src/lib/onboard-command.test.tssrc/lib/onboard-command.tssrc/lib/onboard.tstest/onboard-name-flag.test.tstest/onboard.test.ts
The two "accept" cases let onboard() run past the new --name guards into heavy preflight (Docker/OpenShell), which timed out vitest's 5s per-test budget in CI. They were already weak: they only asserted the guard's error message did not appear, which is trivially true when the guard does not fire. The negative cases that exercise the guard itself remain. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/onboard-name-flag.test.ts (2)
47-50:⚠️ Potential issue | 🟠 MajorFix ESLint
no-undeffromNodeJS.ProcessEnvcast at Line 50.The ambient
NodeJSnamespace trips lint here; use a local type expression instead.Proposed fix
- env: env as NodeJS.ProcessEnv, + env: env as typeof process.env,#!/bin/bash # Verify the problematic cast and lint-rule context without modifying the repo. rg -n "env as NodeJS\\.ProcessEnv|env as typeof process\\.env" test/onboard-name-flag.test.ts rg -n --iglob "eslint.config.*" --iglob ".eslintrc*" "no-undef|globals"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard-name-flag.test.ts` around lines 47 - 50, Replace the problematic ambient namespace cast "env as NodeJS.ProcessEnv" with a local type expression to satisfy ESLint; in the spawnSync invocation (the env property inside the call that constructs out), change the cast to "env as typeof process.env" (or create a local const typed as typeof process.env and pass that) so the test uses a local type instead of the global NodeJS namespace.
23-52:⚠️ Potential issue | 🟠 MajorMake the spawn-based harness hermetic (stub
docker/openshell).
runOnboardcurrently inherits host binaries. If a guard regresses, tests can drift into real preflight/tooling and become flaky or side-effectful.Proposed fix
function runOnboard(spec: { opts: Record<string, unknown>; envOverrides?: Record<string, string | undefined>; }): RunResult { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-onboard-name-")); + const fakeBin = path.join(tmpDir, "fake-bin"); const scriptPath = path.join(tmpDir, "run.js"); const optsLiteral = JSON.stringify(spec.opts); + + fs.mkdirSync(fakeBin); + for (const bin of ["docker", "openshell"]) { + const stubPath = path.join(fakeBin, bin); + fs.writeFileSync(stubPath, "#!/bin/sh\nexit 0\n"); + fs.chmodSync(stubPath, 0o755); + } @@ const env: Record<string, string | undefined> = { ...process.env, HOME: tmpDir, + PATH: `${fakeBin}:${process.env.PATH || ""}`, ...(spec.envOverrides ?? {}), };As per coding guidelines, "Root-level integration tests in
test/directory should use ESM imports and mock external dependencies without calling real NVIDIA APIs". Based on learnings, prefer the POSIX:separator when composingPATHin this repo’s tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard-name-flag.test.ts` around lines 23 - 52, The test harness is inheriting host binaries (docker/openshell) causing non-hermetic runs; create a fake bin directory under tmpDir, write small executable stubs named "docker" and "openshell" (e.g., scripts that print a safe message or exit 0) and chmod +x them, then prepend that bin dir to PATH using the POSIX ':' separator before constructing the env passed to spawnSync (update the env object built next to tmpDir and before spawnSync); reference tmpDir, env, spawnSync, and scriptPath to locate where to add the stub creation and PATH prepend logic so the spawned process uses the stubs instead of host binaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/onboard-name-flag.test.ts`:
- Around line 47-50: Replace the problematic ambient namespace cast "env as
NodeJS.ProcessEnv" with a local type expression to satisfy ESLint; in the
spawnSync invocation (the env property inside the call that constructs out),
change the cast to "env as typeof process.env" (or create a local const typed as
typeof process.env and pass that) so the test uses a local type instead of the
global NodeJS namespace.
- Around line 23-52: The test harness is inheriting host binaries
(docker/openshell) causing non-hermetic runs; create a fake bin directory under
tmpDir, write small executable stubs named "docker" and "openshell" (e.g.,
scripts that print a safe message or exit 0) and chmod +x them, then prepend
that bin dir to PATH using the POSIX ':' separator before constructing the env
passed to spawnSync (update the env object built next to tmpDir and before
spawnSync); reference tmpDir, env, spawnSync, and scriptPath to locate where to
add the stub creation and PATH prepend logic so the spawned process uses the
stubs instead of host binaries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7820e9e9-92e4-4422-8e08-22c005282e43
📒 Files selected for processing (1)
test/onboard-name-flag.test.ts
## Summary Three follow-ups from review of #2543: 1. The early --from no-name guard skipped when --resume was set so the session's recorded sandbox name could fill in. But `Session.sandboxName` is `string | null`, so a partial resume (sandbox creation never completed) would still slip past and let the prompt path silently default to 'my-assistant'. Added a backstop guard right after the session is loaded that exits 1 when resume + cannotPrompt + --from + no resolved name + no recorded session sandbox name. 2. `getResumeSandboxConflict` was reading `NEMOCLAW_SANDBOX_NAME` directly via the hint helper, which fired spurious resume conflicts whenever a user happened to export the env var in their shell rc — interactive resume never consults the env var anyway, since sandbox creation is already complete in the session. The conflict detector now relies solely on the caller-resolved `requestedSandboxName` (which already obeys the "env-only-when-cannotPrompt" rule). 3. Replaced the ambient `NodeJS.ProcessEnv` cast in the new integration test with `typeof process.env`, sidestepping the no-undef tripwire on the global namespace. ## Related issues Refs #2543 ## Changes - `src/lib/onboard.ts`: add post-session-load backstop guard for the resume-with-null-sandboxName edge; thread the resolved `requestedSandboxName` into `getResumeConfigConflicts` instead of the raw `opts.sandboxName`; drop the env-var fallback inside `getResumeSandboxConflict` so it uses the caller-resolved value exclusively. - `test/onboard.test.ts`: update the pre-existing `getResumeSandboxConflict` test to pass the requested name via opts; add a regression test asserting the env var alone no longer fires a conflict. - `test/onboard-name-flag.test.ts`: new spawnSync regression for the resume-with-null-sandboxName guard; switch the env cast to `typeof process.env`. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
The spawnSync helper inherited the host PATH, leaving the door open for future tests in this file to invoke real docker/openshell. Adding a hermetic stub bin directory was over-scaled for what these tests actually exercise (early-exit guards that never reach preflight), so dropping the suite outright is the simpler resolution. Unit-level coverage in src/lib/onboard-command.test.ts (--name parsing, missing-value, flag-as-value) and test/onboard.test.ts (hint precedence, resume conflict via opts, no-conflict-from-env) remain. The runtime guards inside onboard() are exercised indirectly through those helpers. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
wscurran
added
NemoClaw CLI
cv
approved these changes
Apr 28, 2026
…-flag Signed-off-by: Tinson Lai <tinsonl@nvidia.com> # Conflicts: # src/lib/onboard-command.test.ts # src/lib/onboard-command.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 6979-6992: The guard that blocks non-interactive runs currently
checks requestedFromDockerfile and requestedSandboxName, letting cannotPrompt &&
!resolvedSandboxName fall through and call promptValidatedSandboxName(); change
the condition in the if that uses cannotPrompt to test for !resolvedSandboxName
(not just !requestedSandboxName) so non-interactive/no-TTY runs are rejected
unless a sandbox name is already resolved, and remove the tight coupling to
requestedFromDockerfile so the check covers all non-interactive cases (reference
cannotPrompt, resume, requestedFromDockerfile, requestedSandboxName,
resolvedSandboxName, and promptValidatedSandboxName).
- Line 7265: The session-sourced sandbox name (assigned to sandboxName via
session?.sandboxName) is not being revalidated against RESERVED_SANDBOX_NAMES,
so update the logic where sandboxName is derived to run the same reserved-name
guard/validation as requestedSandboxName and prompted names (e.g., call the
existing validateName() or the RESERVED_SANDBOX_NAMES check) before using it;
ensure createSandbox() still calls validateName(), but also re-check
session?.sandboxName when setting sandboxName so stale sessions like
sandboxName: "status" are blocked just like other paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7a7a290a-1f3a-44dc-a0f8-38a94b5221fe
📒 Files selected for processing (4)
src/lib/onboard-command.test.tssrc/lib/onboard-command.tssrc/lib/onboard.tstest/onboard.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/onboard-command.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard-command.ts
- test/onboard.test.ts
CodeRabbit minor on PR #2591: createSandbox() only calls validateName(), so a session recorded before the reserved-name guard existed could carry a sandboxName like 'status' that would silently slip past every entry point on resume. Re-check RESERVED_SANDBOX_NAMES right after the session-vs-requested resolution before any sandbox operation runs, and exit with the same message the early --name/env-var guard prints. Docs gap on the same PR: docs/reference/commands.md still omitted --name from the usage line, had no per-flag section, and the non-interactive --from example was now invalid because the new no-prompt guard requires --name or NEMOCLAW_SANDBOX_NAME. Add the flag to the usage line, add a --name section near --from, and update the env-var example to include NEMOCLAW_SANDBOX_NAME. The behavioural test for the resumed-name guard is a static-source match in the existing onboard.ts assertion bank rather than a real spawnSync onboard run — the dropped integration suite reached preflight (Docker/OpenShell) and timed out vitest's 5s budget, and re-introducing it would re-introduce the same flakiness for a defensive guard that only fires on stale pre-PR sessions. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
6986-6992:⚠️ Potential issue | 🟠 MajorReject unnamed no-prompt runs even when
--fromis absent.These guards still only fire when a Dockerfile path is involved. An unnamed
--non-interactiveor no-TTY run without--fromcan still reach Line 7339; that path either defaults tomy-assistantor callsprompt()on a non-TTY, which reintroduces the clobber/hang case this change is trying to eliminate.Suggested fix
- if (cannotPrompt && !resume && requestedFromDockerfile && !requestedSandboxName) { + if (cannotPrompt && !resume && !requestedSandboxName) { console.error( - " --from <Dockerfile> requires --name <sandbox> (or NEMOCLAW_SANDBOX_NAME) when running without a TTY or with --non-interactive.", + " A sandbox name is required via --name <sandbox> (or NEMOCLAW_SANDBOX_NAME) when running without a TTY or with --non-interactive.", ); console.error(" A sandbox name cannot be prompted for in this context."); process.exit(1); } ... - if ( - resume && - cannotPrompt && - fromDockerfile && - !requestedSandboxName && - !session?.sandboxName - ) { + if (resume && cannotPrompt && !requestedSandboxName && !session?.sandboxName) { console.error( - " --from <Dockerfile> requires --name <sandbox> (or NEMOCLAW_SANDBOX_NAME) when running without a TTY or with --non-interactive.", + " A sandbox name is required via --name <sandbox> (or NEMOCLAW_SANDBOX_NAME) when running without a TTY or with --non-interactive.", ); console.error( " The resumed session has no recorded sandbox name, so one cannot be inferred.", ); process.exit(1); }Also applies to: 7163-7177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 6986 - 6992, The current guard only rejects non-interactive/no-TTY runs when requestedFromDockerfile is set, allowing unnamed no-prompt runs to proceed and later call prompt() or default to "my-assistant"; update the logic so that any run where cannotPrompt && !resume && !requestedSandboxName is true is rejected (regardless of requestedFromDockerfile), and return the same error/exit behavior; apply this change both around the existing block using cannotPrompt/resume/requestedFromDockerfile/requestedSandboxName and the similar code region referenced near the other occurrence (the 7163-7177 block) to prevent prompting or defaulting in non-interactive contexts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/commands.md`:
- Line 174: The sentence "A sandbox name must also be supplied (via `--name
<sandbox>` or `NEMOCLAW_SANDBOX_NAME`) so a `--from` build cannot silently
clobber the default `my-assistant` sandbox:" should be rewritten in second
person and end with a period; change it to address the reader directly (use
"you") and replace the trailing colon with a period while keeping the same flags
and example (`--name <sandbox>`, `NEMOCLAW_SANDBOX_NAME`, `--from`,
`my-assistant`) so the meaning is preserved.
---
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 6986-6992: The current guard only rejects non-interactive/no-TTY
runs when requestedFromDockerfile is set, allowing unnamed no-prompt runs to
proceed and later call prompt() or default to "my-assistant"; update the logic
so that any run where cannotPrompt && !resume && !requestedSandboxName is true
is rejected (regardless of requestedFromDockerfile), and return the same
error/exit behavior; apply this change both around the existing block using
cannotPrompt/resume/requestedFromDockerfile/requestedSandboxName and the similar
code region referenced near the other occurrence (the 7163-7177 block) to
prevent prompting or defaulting in non-interactive contexts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5b64c08b-243b-4638-952a-affe4adec173
📒 Files selected for processing (3)
docs/reference/commands.mdsrc/lib/onboard.tstest/onboard.test.ts
… person CodeRabbit minor on PR #2591: the sentence added in the previous commit ended with a colon and was written in third-person declarative voice. Address the reader directly with "you" and terminate with a period to match the surrounding prose. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/onboard.test.ts (1)
2681-2691: Prefer behavior-level assertion over source-regex matching here.This regex is fairly brittle to internal refactors (variable renames/reordering/format changes) while behavior stays correct. Consider validating the reserved-name recheck via a helper-level or subprocess behavior test instead of matching source text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 2681 - 2691, The test is brittle because it asserts implementation via a source-regex instead of behavior; replace the regex assertion with a behavior-level test that simulates a resumed session containing a reserved name and asserts the process exits (or the helper returns an error) when onboard logic re-checks RESERVED_SANDBOX_NAMES. Specifically, in the test currently referencing sandboxName, session?.sandboxName, requestedSandboxName and process.exit(1), spawn or invoke the onboarding entry point (or the helper that performs the sandbox-name check) with a mocked/resumed session whose sandboxName is a member of RESERVED_SANDBOX_NAMES and assert the observable outcome (exit code, thrown error, or logged error) rather than matching source text.src/lib/onboard.ts (1)
1872-1879: AligngetRequestedSandboxNameHint()with the new resolution rules.
onboard()now only consultsNEMOCLAW_SANDBOX_NAMEwhen prompting is impossible, but this exported helper still reads it unconditionally. That leaves callers/tests with a stale model of the real CLI behavior. Prefer passing the already-resolved name around, or gate the env fallback on the samecannotPromptcondition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 1872 - 1879, The helper getRequestedSandboxNameHint currently always falls back to process.env.NEMOCLAW_SANDBOX_NAME; update its behavior to match onboard()’s new rules by either (A) changing callers to pass the already-resolved sandbox name into getRequestedSandboxNameHint (so the helper only inspects the provided opts.sandboxName) or (B) gate the env fallback inside getRequestedSandboxNameHint behind the same cannotPrompt condition used by onboard() (only read NEMOCLAW_SANDBOX_NAME when cannotPrompt is true). Locate getRequestedSandboxNameHint and adjust its logic or call sites accordingly so tests and callers receive the same resolution semantics as onboard() without unconditionally reading the env var.
🤖 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/onboard.ts`:
- Around line 3211-3228: createSandbox(sandboxNameOverride) currently only calls
validateName(), allowing callers to bypass the new global-CLI-name guard; update
createSandbox to also check against RESERVED_SANDBOX_NAMES (the same set used
for prompt/flag/env validation) and reject or throw when sandboxNameOverride is
in that set, and do the same wherever createSandbox override path is used
(ensure validateNamePlusReserved check is applied in createSandbox, and mirror
this enforcement in the other override/resume code paths referenced by
createSandbox and callers such as the resume/session handlers).
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 1872-1879: The helper getRequestedSandboxNameHint currently always
falls back to process.env.NEMOCLAW_SANDBOX_NAME; update its behavior to match
onboard()’s new rules by either (A) changing callers to pass the
already-resolved sandbox name into getRequestedSandboxNameHint (so the helper
only inspects the provided opts.sandboxName) or (B) gate the env fallback inside
getRequestedSandboxNameHint behind the same cannotPrompt condition used by
onboard() (only read NEMOCLAW_SANDBOX_NAME when cannotPrompt is true). Locate
getRequestedSandboxNameHint and adjust its logic or call sites accordingly so
tests and callers receive the same resolution semantics as onboard() without
unconditionally reading the env var.
In `@test/onboard.test.ts`:
- Around line 2681-2691: The test is brittle because it asserts implementation
via a source-regex instead of behavior; replace the regex assertion with a
behavior-level test that simulates a resumed session containing a reserved name
and asserts the process exits (or the helper returns an error) when onboard
logic re-checks RESERVED_SANDBOX_NAMES. Specifically, in the test currently
referencing sandboxName, session?.sandboxName, requestedSandboxName and
process.exit(1), spawn or invoke the onboarding entry point (or the helper that
performs the sandbox-name check) with a mocked/resumed session whose sandboxName
is a member of RESERVED_SANDBOX_NAMES and assert the observable outcome (exit
code, thrown error, or logged error) rather than matching source text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c188ae73-3f41-4b69-bf46-124b234bd847
📒 Files selected for processing (3)
docs/reference/commands.mdsrc/lib/onboard.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/reference/commands.md
4 tasks
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
## Summary `nemoclaw onboard` had no `--name` flag, so picking a sandbox name in non-interactive or no-TTY runs required the undiscoverable `NEMOCLAW_SANDBOX_NAME` env var, and `--from <Dockerfile>` without a TTY silently fell back to the `my-assistant` default — clobbering the default sandbox. This adds `--name <sandbox>`, plumbed through `OnboardCommandOptions` and `OnboardOptions`. The flag wins over the env var. When prompting is impossible (no TTY or `--non-interactive`) the env var is now seeded into the same path the flag takes, so it actually flows through to `createSandbox` instead of being dropped silently. `--from` runs that cannot prompt and have no resolved name now error cleanly. Both the flag and the env-var seed go through the same `validateName` + reserved-name guard the interactive prompt enforces, so a name like `status` is rejected before it can shadow a CLI command. ## Related Issues Fixes NVIDIA#2543 Fixes NVIDIA#2534 ## Changes - `src/lib/onboard-command.ts`: parse `--name <sandbox>`, validate the value, surface it on `OnboardCommandOptions.sandboxName`, list it in the usage line. - `src/lib/onboard.ts`: add `sandboxName` to `OnboardOptions`; lift `RESERVED_SANDBOX_NAMES` out of `promptValidatedSandboxName` so the flag and env-var paths share it; resolve the requested name early (flag, then env var when prompting is impossible), validate once, and seed it into the local `sandboxName` so `createSandbox` receives the override; reject `--from` runs that cannot prompt and have no resolved name; teach `getRequestedSandboxNameHint` / `getResumeSandboxConflict` / `getResumeConfigConflicts` to prefer `opts.sandboxName` over the env var; thread it through the resume-conflict caller. Re-check `RESERVED_SANDBOX_NAMES` against the resumed `session?.sandboxName` so a stale pre-PR session whose recorded name now collides with a CLI command (e.g. `status`) is rejected before any sandbox operation runs. - `src/lib/onboard-command.test.ts`: positive test for `--name` parsing alongside `--from`, plus the missing-value and flag-as-value error paths. - `test/onboard.test.ts`: cover hint precedence (option over env) and the resume conflict produced when `--name` diverges from the recorded sandbox; static-source assertion that the resumed-session reserved-name guard stays in place. - `docs/reference/commands.md`: add `--name <sandbox>` to the `onboard` usage line, document the flag with its precedence and reserved-name semantics next to `--from`, and update the non-interactive `--from` example to set `NEMOCLAW_SANDBOX_NAME` so the new no-prompt guard is satisfied. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ## AI Disclosure - [x] AI-assisted — tool: Claude Code, Codex --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a --name <sandbox> flag to specify sandbox names during onboarding; explicit name now takes precedence over env var. * **Bug Fixes** * Onboarding fails early with clear messaging and exit code when a sandbox name is required but missing or invalid (non-interactive/--from cases); reserved names are rejected. * **Tests** * Added tests for parsing, help text, validation, precedence, reserved-name checks, and non-interactive failure cases. * **Documentation** * Updated command reference to document the new flag, validation rules, precedence, and failure behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
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
nemoclaw onboardhad no--nameflag, so picking a sandbox name in non-interactive or no-TTY runs required the undiscoverableNEMOCLAW_SANDBOX_NAMEenv var, and--from <Dockerfile>without a TTY silently fell back to themy-assistantdefault — clobbering the default sandbox. This adds--name <sandbox>, plumbed throughOnboardCommandOptionsandOnboardOptions. The flag wins over the env var. When prompting is impossible (no TTY or--non-interactive) the env var is now seeded into the same path the flag takes, so it actually flows through tocreateSandboxinstead of being dropped silently.--fromruns that cannot prompt and have no resolved name now error cleanly. Both the flag and the env-var seed go through the samevalidateName+ reserved-name guard the interactive prompt enforces, so a name likestatusis rejected before it can shadow a CLI command.Related Issues
Fixes #2543
Fixes #2534
Changes
src/lib/onboard-command.ts: parse--name <sandbox>, validate the value, surface it onOnboardCommandOptions.sandboxName, list it in the usage line.src/lib/onboard.ts: addsandboxNametoOnboardOptions; liftRESERVED_SANDBOX_NAMESout ofpromptValidatedSandboxNameso the flag and env-var paths share it; resolve the requested name early (flag, then env var when prompting is impossible), validate once, and seed it into the localsandboxNamesocreateSandboxreceives the override; reject--fromruns that cannot prompt and have no resolved name; teachgetRequestedSandboxNameHint/getResumeSandboxConflict/getResumeConfigConflictsto preferopts.sandboxNameover the env var; thread it through the resume-conflict caller. Re-checkRESERVED_SANDBOX_NAMESagainst the resumedsession?.sandboxNameso a stale pre-PR session whose recorded name now collides with a CLI command (e.g.status) is rejected before any sandbox operation runs.src/lib/onboard-command.test.ts: positive test for--nameparsing alongside--from, plus the missing-value and flag-as-value error paths.test/onboard.test.ts: cover hint precedence (option over env) and the resume conflict produced when--namediverges from the recorded sandbox; static-source assertion that the resumed-session reserved-name guard stays in place.docs/reference/commands.md: add--name <sandbox>to theonboardusage line, document the flag with its precedence and reserved-name semantics next to--from, and update the non-interactive--fromexample to setNEMOCLAW_SANDBOX_NAMEso the new no-prompt guard is satisfied.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation