feat(cli): add --name <sandbox-name> flag to nemoclaw onboard#2618
Closed
latenighthackathon wants to merge 5 commits into
Closed
feat(cli): add --name <sandbox-name> flag to nemoclaw onboard#2618latenighthackathon wants to merge 5 commits into
latenighthackathon wants to merge 5 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
actor CLI as CLI User
participant Parser as parseOnboardArgs
participant Validator as Validation
participant Runner as runOnboardCommand
participant Env as Environment
end
CLI->>Parser: nemoclaw onboard --name "my-sandbox" --non-interactive
Parser->>Validator: Extract `--name` and validate value
alt invalid name (empty or flag-like)
Validator-->>Parser: Reject -> print usage & exit
Parser-->>CLI: Exit with error
else valid name
Validator-->>Parser: Return sandboxName
end
alt non-interactive and no `--name`
Parser->>Env: Check NEMOCLAW_SANDBOX_NAME
alt env empty or whitespace and not `--resume`
Env-->>Parser: Fail -> print usage & exit
Parser-->>CLI: Exit with error
else env present or `--resume`
Env-->>Parser: OK
end
end
Parser-->>Runner: options { sandboxName: "my-sandbox" }
Runner->>Env: Set NEMOCLAW_SANDBOX_NAME = "my-sandbox"
Runner->>Runner: Execute onboard flow targeting sandbox
Runner-->>CLI: Completion / status output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard-command.ts (1)
128-139:⚠️ Potential issue | 🟠 MajorEnforce sandbox name for
--non-interactivemode before returning parsed options.Line 129 currently allows
--non-interactivewithout--name(and without checkingdeps.env.NEMOCLAW_SANDBOX_NAME). That does not satisfy the linked objective requiring a clean error in non-interactive runs when name is missing.Proposed patch
const resume = parsedArgs.includes("--resume"); const fresh = parsedArgs.includes("--fresh"); + const nonInteractive = parsedArgs.includes("--non-interactive"); if (resume && fresh) { error(" --resume and --fresh are mutually exclusive."); printOnboardUsage(error, noticeAcceptFlag); exit(1); } + + if ( + nonInteractive && + !sandboxName && + !String(deps.env.NEMOCLAW_SANDBOX_NAME ?? "").trim() + ) { + error( + " --non-interactive requires --name <sandbox-name> (or NEMOCLAW_SANDBOX_NAME).", + ); + printOnboardUsage(error, noticeAcceptFlag); + exit(1); + } return { - nonInteractive: parsedArgs.includes("--non-interactive"), + nonInteractive, resume, fresh, recreateSandbox: parsedArgs.includes("--recreate-sandbox"), fromDockerfile,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-command.ts` around lines 128 - 139, The returned options allow nonInteractive=true without ensuring sandboxName is set; add a pre-return check in the same function to enforce that if nonInteractive (parsedArgs.includes("--non-interactive")) is true and sandboxName is falsy and deps.env[noticeAcceptEnv] (or specifically deps.env.NEMOCLAW_SANDBOX_NAME) is not provided, the function should throw or report a clear error and abort rather than returning options; place this check just before the current return block so nonInteractive, sandboxName, parsedArgs, and deps.env are validated and a clean error is produced when name is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/onboard-command.ts`:
- Around line 128-139: The returned options allow nonInteractive=true without
ensuring sandboxName is set; add a pre-return check in the same function to
enforce that if nonInteractive (parsedArgs.includes("--non-interactive")) is
true and sandboxName is falsy and deps.env[noticeAcceptEnv] (or specifically
deps.env.NEMOCLAW_SANDBOX_NAME) is not provided, the function should throw or
report a clear error and abort rather than returning options; place this check
just before the current return block so nonInteractive, sandboxName, parsedArgs,
and deps.env are validated and a clean error is produced when name is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e4a31bd8-d0ab-41ac-a930-2b2d20d2842d
📒 Files selected for processing (2)
src/lib/onboard-command.test.tssrc/lib/onboard-command.ts
latenighthackathon
added a commit
to latenighthackathon/NemoClaw
that referenced
this pull request
Apr 28, 2026
Without this guard, `nemoclaw onboard --non-interactive` silently fell through to the "my-assistant" default — which collides with the most common existing sandbox name and is exactly the surprise NVIDIA#2543 reported when trying to spin up a second sandbox via --from in non-interactive mode. CodeRabbit follow-up on NVIDIA#2618 asked us to fail-fast in this case. Reject when --non-interactive is set without --name and without NEMOCLAW_SANDBOX_NAME (whitespace-only env values count as missing). Resume flows are exempt — the sandbox name is recorded in the session and re-derived from there, so `--non-interactive --resume` keeps working unchanged. Test surfaces the new guard from four angles: - error when both --name and env are missing - accept when --name is provided - accept when NEMOCLAW_SANDBOX_NAME is set - whitespace-only env value still triggers the guard - --resume bypasses the guard Updated E2E callers that didn't set NEMOCLAW_SANDBOX_NAME so they keep working post-guard: - test-port8080-conflict.sh — added NEMOCLAW_SANDBOX_NAME="$SANDBOX_NAME" to both onboard invocations (preflight failure path is what the test exercises; the new guard would short-circuit before the port check) - test-inference-routing.sh — added explicit NEMOCLAW_SANDBOX_NAME for five test cases that didn't previously specify one (TC-INF-05, unreachable, OpenAI, Anthropic, custom endpoint) - brev-e2e.test.ts — added NEMOCLAW_SANDBOX_NAME="e2e-brev" to the Launchable nohup invocation Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/brev-e2e.test.ts (1)
510-519:⚠️ Potential issue | 🟠 MajorKeep the onboard sandbox name consistent with the rest of this suite.
This override makes
nemoclaw onboardcreatee2e-brev, but every downstream check here still targetse2e-teston Line 552, Line 591, Line 626, and Line 703. The launchable path will now poll for the wrong sandbox and fail even when onboard succeeds.Suggested fix
- `NEMOCLAW_SANDBOX_NAME="e2e-brev" nohup nemoclaw onboard --non-interactive </dev/null >/tmp/nemoclaw-onboard.log 2>&1 & disown`, + `nohup nemoclaw onboard --non-interactive </dev/null >/tmp/nemoclaw-onboard.log 2>&1 & disown`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/brev-e2e.test.ts` around lines 510 - 519, The sshEnv invocation launches nemoclaw with NEMOCLAW_SANDBOX_NAME="e2e-brev" which is inconsistent with the rest of the suite that expects "e2e-test"; update the command string in the sshEnv call (the array building the chained shell commands used to run nemoclaw onboard) to use NEMOCLAW_SANDBOX_NAME="e2e-test" so the onboarding target matches downstream checks (or alternatively change the downstream checks that reference "e2e-test" to "e2e-brev" if you prefer the new name) and ensure all references to the sandbox name (the NEMOCLAW_SANDBOX_NAME env var in the sshEnv command and test assertions that look for "e2e-test"/"e2e-brev") are consistent.test/e2e/test-inference-routing.sh (1)
204-209:⚠️ Potential issue | 🟠 MajorDon't override
NEMOCLAW_SANDBOX_NAMEa second time in these onboard calls.In bash, the later inline assignment wins. These new entries override the earlier
$SANDBOX_NAME/$sbx_namevalues, so onboard creates a different sandbox than the one the test later SSHes into, asserts on, and destroys.TC-INF-07only duplicates the same value, but the other four cases now target the wrong sandbox.Suggested fix
NEMOCLAW_SANDBOX_NAME="$SANDBOX_NAME" \ NEMOCLAW_NON_INTERACTIVE=1 \ NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 \ NEMOCLAW_POLICY_TIER="open" \ - NEMOCLAW_SANDBOX_NAME="e2e-inf-routing" \ nemoclaw onboard --non-interactive --yes-i-accept-third-party-software \output=$(NVIDIA_API_KEY="nvapi-valid-format-but-fake-key-1234567890" \ NEMOCLAW_NON_INTERACTIVE=1 \ NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 \ NEMOCLAW_SANDBOX_NAME="e2e-unreachable" \ NEMOCLAW_PROVIDER="custom" \ NEMOCLAW_ENDPOINT_URL="https://nemoclaw-e2e.invalid/v1" \ NEMOCLAW_MODEL="test-model" \ COMPATIBLE_API_KEY="fake-key-for-unreachable-test" \ - NEMOCLAW_SANDBOX_NAME="e2e-unreachable" \ run_with_timeout 120 nemoclaw onboard --non-interactive --yes-i-accept-third-party-software \NEMOCLAW_SANDBOX_NAME="$sbx_name" \ NEMOCLAW_NON_INTERACTIVE=1 \ NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 \ NEMOCLAW_POLICY_TIER="open" \ NEMOCLAW_PROVIDER="openai" \ NEMOCLAW_MODEL="$model" \ OPENAI_API_KEY="$api_key" \ - NEMOCLAW_SANDBOX_NAME="e2e-inf-openai" \ run_with_timeout 300 nemoclaw onboard --non-interactive --yes-i-accept-third-party-software \NEMOCLAW_SANDBOX_NAME="$sbx_name" \ NEMOCLAW_NON_INTERACTIVE=1 \ NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 \ NEMOCLAW_POLICY_TIER="open" \ NEMOCLAW_PROVIDER="anthropic" \ NEMOCLAW_MODEL="$model" \ ANTHROPIC_API_KEY="$api_key" \ - NEMOCLAW_SANDBOX_NAME="e2e-inf-anthropic" \ run_with_timeout 300 nemoclaw onboard --non-interactive --yes-i-accept-third-party-software \NEMOCLAW_SANDBOX_NAME="$sbx_name" \ NEMOCLAW_NON_INTERACTIVE=1 \ NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 \ NEMOCLAW_POLICY_TIER="open" \ NEMOCLAW_PROVIDER="custom" \ NEMOCLAW_ENDPOINT_URL="$endpoint_url" \ NEMOCLAW_MODEL="$endpoint_model" \ COMPATIBLE_API_KEY="$endpoint_key" \ - NEMOCLAW_SANDBOX_NAME="e2e-inf-custom" \ run_with_timeout 300 nemoclaw onboard --non-interactive --yes-i-accept-third-party-software \Also applies to: 362-371, 431-439, 506-514, 593-602
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-inference-routing.sh` around lines 204 - 209, The inline environment variable NEMOCLAW_SANDBOX_NAME is being set twice before the nemoclaw onboard invocation, which causes the later literal "e2e-inf-routing" to override the earlier $SANDBOX_NAME/$sbx_name; remove the duplicate NEMOCLAW_SANDBOX_NAME="e2e-inf-routing" assignment so the onboard call uses the intended $SANDBOX_NAME value, and apply the same removal for the other onboard blocks that duplicate NEMOCLAW_SANDBOX_NAME (the blocks around the nemoclaw onboard command at the other mentioned locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/e2e/brev-e2e.test.ts`:
- Around line 510-519: The sshEnv invocation launches nemoclaw with
NEMOCLAW_SANDBOX_NAME="e2e-brev" which is inconsistent with the rest of the
suite that expects "e2e-test"; update the command string in the sshEnv call (the
array building the chained shell commands used to run nemoclaw onboard) to use
NEMOCLAW_SANDBOX_NAME="e2e-test" so the onboarding target matches downstream
checks (or alternatively change the downstream checks that reference "e2e-test"
to "e2e-brev" if you prefer the new name) and ensure all references to the
sandbox name (the NEMOCLAW_SANDBOX_NAME env var in the sshEnv command and test
assertions that look for "e2e-test"/"e2e-brev") are consistent.
In `@test/e2e/test-inference-routing.sh`:
- Around line 204-209: The inline environment variable NEMOCLAW_SANDBOX_NAME is
being set twice before the nemoclaw onboard invocation, which causes the later
literal "e2e-inf-routing" to override the earlier $SANDBOX_NAME/$sbx_name;
remove the duplicate NEMOCLAW_SANDBOX_NAME="e2e-inf-routing" assignment so the
onboard call uses the intended $SANDBOX_NAME value, and apply the same removal
for the other onboard blocks that duplicate NEMOCLAW_SANDBOX_NAME (the blocks
around the nemoclaw onboard command at the other mentioned locations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9ba34616-1400-46c1-ac4d-520b8a5edba0
📒 Files selected for processing (5)
src/lib/onboard-command.test.tssrc/lib/onboard-command.tstest/e2e/brev-e2e.test.tstest/e2e/e2e-cloud-experimental/test-port8080-conflict.shtest/e2e/test-inference-routing.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard-command.ts
- src/lib/onboard-command.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/troubleshooting.md`:
- Around line 340-341: Update the documented non-interactive recovery examples
for the `nemoclaw onboard` command that currently show `--non-interactive` with
new `TELEGRAM_BOT_TOKEN`, `DISCORD_BOT_TOKEN`, or `SLACK_BOT_TOKEN` so they also
include acceptance of third‑party software; specifically, append
`--yes-i-accept-third-party-software` to each `nemoclaw onboard
--non-interactive` example (or show the equivalent
`NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1` prefix) so the commands run
successfully in non-interactive automation.
🪄 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: 6afa4972-85fb-41f4-8e1e-9fba5fdc008c
📒 Files selected for processing (5)
docs/deployment/set-up-telegram-bridge.mddocs/inference/use-local-inference.mddocs/reference/commands.mddocs/reference/network-policies.mddocs/reference/troubleshooting.md
✅ Files skipped from review due to trivial changes (3)
- docs/deployment/set-up-telegram-bridge.md
- docs/reference/network-policies.md
- docs/inference/use-local-inference.md
wscurran
added
NemoClaw CLI
Contributor
|
✨ Thanks for submitting this PR that proposes a new feature to add a --name flag to the nemoclaw onboard command. This change surfaces the existing env-var override as a discoverable flag, improving the usability of the sandbox resource. Related open issues: |
The wizard already honors NEMOCLAW_SANDBOX_NAME via promptValidatedSandboxName (non-interactive: used as the sandbox name; interactive: used as the prompt default), but the env var is undiscoverable from `nemoclaw onboard --help`. This blocks scripted/CI workflows that pass `--from <Dockerfile>` and need to spin up a second sandbox alongside an existing one — the only documented path is openshell sandbox create, which leaves the new sandbox unregistered in ~/.nemoclaw/sandboxes.json (invisible to nemoclaw status). Surface the existing env var as a discoverable CLI flag: - Add --name <sandbox-name> to the onboard parser, with fail-fast rejection for missing values or values that look like another flag (--name --from ...). - Detailed validation (lowercase, leading-letter, hyphen rules, reserved-name list) still happens in promptValidatedSandboxName, so error messages stay consistent across CLI-flag and env-var entry paths. - runOnboardCommand assigns deps.env.NEMOCLAW_SANDBOX_NAME from the parsed flag before calling runOnboard so the existing wizard pickup keeps working unchanged — no new code path on the inner onboard side. Tests cover: parsing, missing-value rejection, --name <flag> rejection, env propagation, and that --name absent leaves an existing NEMOCLAW_SANDBOX_NAME untouched. Closes NVIDIA#2543. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
Without this guard, `nemoclaw onboard --non-interactive` silently fell through to the "my-assistant" default — which collides with the most common existing sandbox name and is exactly the surprise NVIDIA#2543 reported when trying to spin up a second sandbox via --from in non-interactive mode. CodeRabbit follow-up on NVIDIA#2618 asked us to fail-fast in this case. Reject when --non-interactive is set without --name and without NEMOCLAW_SANDBOX_NAME (whitespace-only env values count as missing). Resume flows are exempt — the sandbox name is recorded in the session and re-derived from there, so `--non-interactive --resume` keeps working unchanged. Test surfaces the new guard from four angles: - error when both --name and env are missing - accept when --name is provided - accept when NEMOCLAW_SANDBOX_NAME is set - whitespace-only env value still triggers the guard - --resume bypasses the guard Updated E2E callers that didn't set NEMOCLAW_SANDBOX_NAME so they keep working post-guard: - test-port8080-conflict.sh — added NEMOCLAW_SANDBOX_NAME="$SANDBOX_NAME" to both onboard invocations (preflight failure path is what the test exercises; the new guard would short-circuit before the port check) - test-inference-routing.sh — added explicit NEMOCLAW_SANDBOX_NAME for five test cases that didn't previously specify one (TC-INF-05, unreachable, OpenAI, Anthropic, custom endpoint) - brev-e2e.test.ts — added NEMOCLAW_SANDBOX_NAME="e2e-brev" to the Launchable nohup invocation Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
The previous commit makes nemoclaw onboard --non-interactive (without --resume) reject when neither --name nor NEMOCLAW_SANDBOX_NAME is set, to close the silent fall-through to "my-assistant" that NVIDIA#2543 reported. Update the doc examples that copy verbatim into terminals so they keep working after the guard lands. - docs/reference/commands.md — usage line gets [--name <sandbox-name>]; six console examples get NEMOCLAW_SANDBOX_NAME=my-assistant prepended (or --name my-assistant inline) so each is runnable as-is. - docs/inference/use-local-inference.md — five non-interactive setup recipes (Ollama, llama.cpp, vLLM-OpenAI, vLLM-Anthropic, NIM) gain NEMOCLAW_SANDBOX_NAME=my-assistant alongside their existing env-arg block. The Docker-login retry prose mentions the new requirement. - docs/deployment/set-up-telegram-bridge.md — credential-rotation prose cites --name <sandbox-name> in its re-run example. - docs/reference/network-policies.md — restricted-tier example uses the env var. - docs/reference/troubleshooting.md — credential rotation prose + the bare $ console example get the env var / flag. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
- test/e2e/test-inference-routing.sh — drop the five NEMOCLAW_SANDBOX_NAME additions from the previous commit. Each block already had the right name set on an earlier line ($SANDBOX_NAME / $sbx_name); the redundant trailing assignments would have last-won under bash inline-assignment semantics and onboarded a different sandbox than the one the test then SSHes into and asserts on. - test/e2e/brev-e2e.test.ts — switch the sandbox name from "e2e-brev" back to "e2e-test" so the launchable onboard path keeps matching the downstream connectivity / health / destroy checks at lines 552, 591, 626, 703. - docs/reference/troubleshooting.md — credential rotation example appends --yes-i-accept-third-party-software so the copy-paste form is complete. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
latenighthackathon
force-pushed
the
fix/onboard-name-flag
branch
from
e84311f to
6ae8328
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/troubleshooting.md (1)
349-353:⚠️ Potential issue | 🟡 MinorShow the updated token in the rerun example.
The credential-rotation check only has something new to detect when the updated
TELEGRAM_BOT_TOKEN,DISCORD_BOT_TOKEN, orSLACK_BOT_TOKENis in the environment. As written, the sample command only re-targets the sandbox, so readers can follow it and still keep using the stale credential.Suggested doc fix
```console -$ NEMOCLAW_SANDBOX_NAME=my-assistant nemoclaw onboard --non-interactive --yes-i-accept-third-party-software +$ TELEGRAM_BOT_TOKEN=<new-token> NEMOCLAW_SANDBOX_NAME=<sandbox-name> nemoclaw onboard --non-interactive --yes-i-accept-third-party-software</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/reference/troubleshooting.mdaround lines 349 - 353, Update the re-run
example to export the updated token in the environment so the
credential-rotation check picks it up: include one of TELEGRAM_BOT_TOKEN,
DISCORD_BOT_TOKEN, or SLACK_BOT_TOKEN (depending on the integration) before the
NEMOCLAW_SANDBOX_NAME and call to the "nemoclaw onboard" command (refer to the
existing example line with "nemoclaw onboard"); replace the placeholder sandbox
name with and show TELEGRAM_BOT_TOKEN= (or the
appropriate token variable) so readers set the new token when re-running
onboarding.</details> </blockquote></details> </blockquote></details>🧹 Nitpick comments (1)
test/e2e/brev-e2e.test.ts (1)
514-514: Avoid adding another hard-coded sandbox-name literal here.
"e2e-test"is already duplicated across env setup, polling, manual registry writes, and assertions. This extra inline assignment makes future renames easy to miss; extracting a sharedE2E_SANDBOX_NAMEconstant would keep the background onboard path aligned with the rest of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/brev-e2e.test.ts` at line 514, Replace the hard-coded "e2e-test" literal with a shared constant (e.g., E2E_SANDBOX_NAME) and use that constant when building the background onboard command string in the test; locate the inline command string containing NEMOCLAW_SANDBOX_NAME="e2e-test" nohup ... in test/e2e/brev-e2e.test.ts, add a top-level const E2E_SANDBOX_NAME = 'e2e-test' (or reuse an existing shared constant), and update the command to interpolate the constant instead of the literal so the sandbox name is centralized and consistent with the other env setup, polling, manual registry writes, and assertions.🤖 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-command.ts`: - Around line 149-165: The new guard in onboard-command.ts blocks "nemoclaw onboard --non-interactive" unless nonInteractive, resume, sandboxName, or env NEMOCLAW_SANDBOX_NAME is set; update the failing test (test-gpu-double-onboard.sh) to either export NEMOCLAW_SANDBOX_NAME with the expected SANDBOX_NAME before re-invoking onboard or invoke the command with --name "${SANDBOX_NAME}" (i.e., replace the failing call to "nemoclaw onboard --non-interactive" with one of those two options so sandboxName is provided). --- Outside diff comments: In `@docs/reference/troubleshooting.md`: - Around line 349-353: Update the re-run example to export the updated token in the environment so the credential-rotation check picks it up: include one of TELEGRAM_BOT_TOKEN, DISCORD_BOT_TOKEN, or SLACK_BOT_TOKEN (depending on the integration) before the NEMOCLAW_SANDBOX_NAME and call to the "nemoclaw onboard" command (refer to the existing example line with "nemoclaw onboard"); replace the placeholder sandbox name with <sandbox-name> and show TELEGRAM_BOT_TOKEN=<new-token> (or the appropriate token variable) so readers set the new token when re-running onboarding. --- Nitpick comments: In `@test/e2e/brev-e2e.test.ts`: - Line 514: Replace the hard-coded "e2e-test" literal with a shared constant (e.g., E2E_SANDBOX_NAME) and use that constant when building the background onboard command string in the test; locate the inline command string containing NEMOCLAW_SANDBOX_NAME="e2e-test" nohup ... in test/e2e/brev-e2e.test.ts, add a top-level const E2E_SANDBOX_NAME = 'e2e-test' (or reuse an existing shared constant), and update the command to interpolate the constant instead of the literal so the sandbox name is centralized and consistent with the other env setup, polling, manual registry writes, and assertions.🪄 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:
163108ed-92be-4ca8-b831-8729ebe8b31a📒 Files selected for processing (9)
docs/deployment/set-up-telegram-bridge.mddocs/inference/use-local-inference.mddocs/reference/commands.mddocs/reference/network-policies.mddocs/reference/troubleshooting.mdsrc/lib/onboard-command.test.tssrc/lib/onboard-command.tstest/e2e/brev-e2e.test.tstest/e2e/e2e-cloud-experimental/test-port8080-conflict.sh✅ Files skipped from review due to trivial changes (2)
- docs/reference/network-policies.md
- docs/deployment/set-up-telegram-bridge.md
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/e2e-cloud-experimental/test-port8080-conflict.sh
- docs/inference/use-local-inference.md
- docs/reference/commands.md
- src/lib/onboard-command.test.ts
- docs/reference/troubleshooting.md — credential rotation example was re-targeting the sandbox without setting the new token, so the rotation check had nothing fresh to detect. Show TELEGRAM_BOT_TOKEN alongside NEMOCLAW_SANDBOX_NAME in the rerun command and use a <sandbox-name> placeholder so the example is generic across the three messaging providers (TELEGRAM_BOT_TOKEN, DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN) named in the surrounding prose. - test/e2e/brev-e2e.test.ts — extract E2E_SANDBOX_NAME constant and replace the 11 inline "e2e-test" literals across env exports, registry construction, polling, and assertions. The previous commit added another inline literal at line 514 to a file that already duplicated the name, which made future renames easy to miss. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
wscurran
added
the
status: rfr
Contributor
Author
|
Closing — #2591 by @laitingsheng landed the same |
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
Closes #2543.
The wizard already honors
NEMOCLAW_SANDBOX_NAMEviapromptValidatedSandboxName(non-interactive: used as the sandbox name; interactive: used as the prompt default), but the env var is undiscoverable fromnemoclaw onboard --help. This blocks scripted/CI workflows that pass--from <Dockerfile>and need to spin up a second sandbox alongside an existing one — the only documented path isopenshell sandbox create, which leaves the new sandbox unregistered in~/.nemoclaw/sandboxes.json(invisible tonemoclaw status).Problem
@mayank6136 hit this in #2543 while running multiple sandboxes for A/B testing. The fix is purely a CLI surface addition — surface the existing env-var override as a discoverable
--nameflag.What changed
--name <sandbox-name>with fail-fast rejection for missing values or values that look like another flag (--name --from ...). Detailed validation (lowercase, leading-letter, hyphen rules, reserved-name list) still happens later inpromptValidatedSandboxName, so error messages stay consistent across CLI-flag and env-var entry paths.runOnboardCommandassignsdeps.env.NEMOCLAW_SANDBOX_NAMEfrom the parsed flag before callingrunOnboard, so the existing wizard pickup keeps working unchanged — no new code path on the inner onboard side.--name <sandbox-name>between--from <Dockerfile>and--agent <name>.Test plan
--nameflag (parsing, missing-value rejection,--name --flagrejection, env propagation, no-touch when--nameabsent).npx vitest run --project cli --testTimeout=180000 test/onboard.test.ts src/lib/onboard-command.test.ts)..toEqualassertions extended to include the newsandboxName: nullfield.Workflow this unblocks
Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
New Features
Tests
Documentation