fix: parameterize sandbox name in setup.sh instead of hardcoding#340
fix: parameterize sandbox name in setup.sh instead of hardcoding#340jacobtomlinson merged 2 commits intomainfrom
Conversation
Closes #197. setup.sh hardcoded --name nemoclaw for sandbox creation, ignoring the custom name chosen during onboard. This caused a split-brain: the NemoClaw registry stored the custom name but OpenShell had a sandbox named "nemoclaw", so connect/policy/bridge commands failed. setup.sh now accepts the sandbox name as $1 (default: nemoclaw). The legacy `nemoclaw setup` command passes the default sandbox name from the registry. Gateway name stays hardcoded to "nemoclaw" since gateway and sandbox names are independent.
7 tests verifying sandbox operations use $SANDBOX_NAME while gateway stays hardcoded, plus bash arg parsing tests. Refs: #197
📝 WalkthroughWalkthroughThe pull request parameterizes the sandbox name in the setup process. The main entry point now extracts the default sandbox name from the registry and passes it to setup.sh if it matches a valid format. The setup script then accepts this as a parameter and uses it instead of a hardcoded "nemoclaw" value throughout sandbox operations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
📝 Coding Plan
Comment |
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 `@bin/nemoclaw.js`:
- Around line 50-52: The current validation for defaultSandbox in the
bin/nemoclaw.js block uses /^[a-z0-9][a-z0-9-]*[a-z0-9]$/ which rejects
single-character names and is inconsistent with onboarding; change the check
that assigns safeName (the expression using registry.listSandboxes() and the
regex) to use the same pattern as onboarding (/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/)
so single-character lowercase alphanumeric sandboxes are accepted, keeping the
rest of the call to run(`bash "${SCRIPTS}/setup.sh" ${safeName}`) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4414f5c-c116-4598-9aab-028c3ed29e7c
📒 Files selected for processing (3)
bin/nemoclaw.jsscripts/setup.shtest/setup-sandbox-name.test.js
| const { defaultSandbox } = registry.listSandboxes(); | ||
| const safeName = defaultSandbox && /^[a-z0-9][a-z0-9-]*[a-z0-9]$/.test(defaultSandbox) ? defaultSandbox : ""; | ||
| run(`bash "${SCRIPTS}/setup.sh" ${safeName}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find sandbox naming patterns in tests and config to assess impact
# Check test fixtures for sandbox names
rg -n "sandbox.*name|defaultSandbox" --type=js -C2
# Check if single-char sandbox names are used anywhere
rg -nP '"[a-z]".*sandbox|sandbox.*"[a-z]"' --type=jsRepository: NVIDIA/NemoClaw
Length of output: 13252
Regex validation differs from other functions and inconsistently rejects single-char names.
The regex /^[a-z0-9][a-z0-9-]*[a-z0-9]$/ requires at least 2 characters, but users can create single-character sandbox names during onboarding (see bin/lib/onboard.js line 254, which uses /^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/). Additionally, start() at line 148 uses /^[a-zA-Z0-9._-]+$/, allowing uppercase, dots, and underscores—creating inconsistent behavior across the CLI.
A single-character name like "a" would silently fall back to an empty string during setup, whereas it would have passed validation during sandbox creation.
♻️ Suggested fix: Align with onboard validation
- const safeName = defaultSandbox && /^[a-z0-9][a-z0-9-]*[a-z0-9]$/.test(defaultSandbox) ? defaultSandbox : "";
+ const safeName = defaultSandbox && /^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(defaultSandbox) ? defaultSandbox : "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { defaultSandbox } = registry.listSandboxes(); | |
| const safeName = defaultSandbox && /^[a-z0-9][a-z0-9-]*[a-z0-9]$/.test(defaultSandbox) ? defaultSandbox : ""; | |
| run(`bash "${SCRIPTS}/setup.sh" ${safeName}`); | |
| const { defaultSandbox } = registry.listSandboxes(); | |
| const safeName = defaultSandbox && /^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(defaultSandbox) ? defaultSandbox : ""; | |
| run(`bash "${SCRIPTS}/setup.sh" ${safeName}`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/nemoclaw.js` around lines 50 - 52, The current validation for
defaultSandbox in the bin/nemoclaw.js block uses /^[a-z0-9][a-z0-9-]*[a-z0-9]$/
which rejects single-character names and is inconsistent with onboarding; change
the check that assigns safeName (the expression using registry.listSandboxes()
and the regex) to use the same pattern as onboarding
(/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/) so single-character lowercase alphanumeric
sandboxes are accepted, keeping the rest of the call to run(`bash
"${SCRIPTS}/setup.sh" ${safeName}`) unchanged.
…DIA#340) * fix: parameterize sandbox name in setup.sh instead of hardcoding Closes NVIDIA#197. setup.sh hardcoded --name nemoclaw for sandbox creation, ignoring the custom name chosen during onboard. This caused a split-brain: the NemoClaw registry stored the custom name but OpenShell had a sandbox named "nemoclaw", so connect/policy/bridge commands failed. setup.sh now accepts the sandbox name as $1 (default: nemoclaw). The legacy `nemoclaw setup` command passes the default sandbox name from the registry. Gateway name stays hardcoded to "nemoclaw" since gateway and sandbox names are independent. * test: add regression tests for setup.sh sandbox name parameterization 7 tests verifying sandbox operations use $SANDBOX_NAME while gateway stays hardcoded, plus bash arg parsing tests. Refs: NVIDIA#197
…DIA#340) * fix: parameterize sandbox name in setup.sh instead of hardcoding Closes NVIDIA#197. setup.sh hardcoded --name nemoclaw for sandbox creation, ignoring the custom name chosen during onboard. This caused a split-brain: the NemoClaw registry stored the custom name but OpenShell had a sandbox named "nemoclaw", so connect/policy/bridge commands failed. setup.sh now accepts the sandbox name as $1 (default: nemoclaw). The legacy `nemoclaw setup` command passes the default sandbox name from the registry. Gateway name stays hardcoded to "nemoclaw" since gateway and sandbox names are independent. * test: add regression tests for setup.sh sandbox name parameterization 7 tests verifying sandbox operations use $SANDBOX_NAME while gateway stays hardcoded, plus bash arg parsing tests. Refs: NVIDIA#197
Closes #197.
Summary
setup.shaccepts sandbox name as$1(default:nemoclaw)create,delete,get,listgrep) use$SANDBOX_NAMEnemoclaw(gateway and sandbox names are independent)nemoclaw setup(legacy command) passes the default sandbox name from the registryProblem
The onboard wizard lets users choose a custom sandbox name stored in
~/.nemoclaw/sandboxes.json, butsetup.shhardcoded--name nemoclaweverywhere. This created a split-brain: NemoClaw's registry had the custom name, OpenShell had "nemoclaw", and every subsequent command (connect, policy, bridge) failed with "sandbox not found."Test plan
bash scripts/setup.sh test-197creates sandbox namedtest-197bash scripts/setup.shcreates sandbox namednemoclawnemoclawin all gateway operationsSummary by CodeRabbit
New Features
Tests