feat: add sandbox export/import backup commands#472
Open
vasanth53 wants to merge 8 commits intoNVIDIA:mainfrom
Open
feat: add sandbox export/import backup commands#472vasanth53 wants to merge 8 commits intoNVIDIA:mainfrom
vasanth53 wants to merge 8 commits intoNVIDIA:mainfrom
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 new sandbox backup module ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "nemoclaw (bin/nemoclaw.js)"
participant Backup as "Backup Module (bin/lib/backup.js)"
participant Registry as "Registry (./registry)"
participant Shell as "openshell"
participant FS as "Filesystem"
User->>CLI: nemoclaw sandbox export <name> [--path]
CLI->>Registry: get sandbox config
Registry-->>CLI: sandbox data
CLI->>Backup: exportSandbox(name, path)
Backup->>Shell: openshell policy get <sandbox> (optional)
Shell-->>Backup: policy text
Backup->>FS: write JSON backup (metadata + policy)
FS-->>Backup: file written
Backup-->>CLI: return path
CLI-->>User: export complete
User->>CLI: nemoclaw import <backup.json> [--name]
CLI->>Backup: importSandbox(backupPath, newName)
Backup->>FS: read backup JSON
FS-->>Backup: backup content
Backup->>Shell: openshell sandbox exists <name>?
Shell-->>Backup: exists status
alt sandbox does not exist
Backup->>Registry: register sandbox config
Registry-->>Backup: registered
end
alt policy present
Backup->>FS: write temp YAML
Backup->>Shell: openshell policy set --wait <temp.yml>
Shell-->>Backup: apply status
Backup->>FS: delete temp YAML
end
Backup-->>CLI: import complete
CLI-->>User: sandbox imported
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 4
🧹 Nitpick comments (2)
bin/lib/backup.js (2)
12-12: Preferos.homedir()overprocess.env.HOME.
os.homedir()is more robust cross-platform—it handles Windows (USERPROFILE) and has built-in fallbacks, whereasprocess.env.HOMEmay be undefined on some systems.♻️ Suggested fix
-const BACKUP_DIR = path.join(process.env.HOME || "/tmp", ".nemoclaw", "backups"); +const BACKUP_DIR = path.join(os.homedir(), ".nemoclaw", "backups");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/backup.js` at line 12, Replace use of process.env.HOME when constructing BACKUP_DIR with os.homedir() to ensure cross-platform correctness: import or require the built-in os module (e.g., const os = require('os')) and change the BACKUP_DIR definition (the BACKUP_DIR constant) to use os.homedir() instead of process.env.HOME (keep the rest of the path construction the same).
133-134: Temp policy file should be created with restricted permissions.The backup file write at line 83 uses
mode: 0o600. For consistency and defense in depth, the temp policy file should also be created with restricted permissions.♻️ Suggested fix
- fs.writeFileSync(policyPath, backup.policy); + fs.writeFileSync(policyPath, backup.policy, { mode: 0o600 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/backup.js` around lines 133 - 134, The temp policy file created at policyPath using fs.writeFileSync(path.join(os.tmpdir(), `nemoclaw-restore-${Date.now()}.yaml`), backup.policy) lacks explicit file permissions; change the write to set restrictive permissions (mode 0o600) when creating the file so the policy is only owner-readable/writable — e.g., call fs.writeFileSync with an options object that includes { mode: 0o600 } (or create the file via fs.openSync with that mode then write) to mirror the backup file behavior and ensure defense in depth.
🤖 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/backup.js`:
- Around line 53-56: The execSync call that builds a shell string with
sandboxName (assigning to policyContent) introduces command injection; change
this to either validate sandboxName against a strict whitelist regex (e.g. only
allow [A-Za-z0-9._-]) before use, and throw/skip on invalid values, or replace
execSync with child_process.spawnSync/execFileSync using an argument array (call
openshell with args like ["policy","get", sandboxName]) with shell disabled so
the name is never interpreted by a shell; apply the same validation or
spawn-sync replacement for the other uses of execSync that interpolate
sandboxName in this file (the other execSync invocations creating policy/backup
commands).
- Around line 23-33: The current for-loop that reads backup files swallows all
errors with an empty catch, hiding parse errors or missing metadata; update the
catch in the block that reads/parses files (the JSON.parse and metadata access
around backups.push) to log a clear warning including the filename and the
caught error, and also validate that content.metadata.name and
content.metadata.createdAt exist before pushing to backups (log a warning and
skip if they are missing) so malformed or corrupted backup files are visible to
users for diagnosis.
In `@bin/nemoclaw.js`:
- Around line 354-356: The listBackups() wrapper currently discards the array
returned by backup.listBackups(); fix it by capturing that return value and
printing it to the user — e.g. call const backups = backup.listBackups(); then
output either a formatted JSON (JSON.stringify(backups, null, 2)) or iterate and
console.log each backup object; update the function listBackups() to ensure
backup.listBackups()’s result is displayed to stdout.
- Around line 448-451: The Fish completion's conditional is checking the wrong
token — replace the contains check that uses "(commandline -opc | head -1)" with
an expression that indexes the second token of commandline, e.g. use
"(commandline -opc)[2]" so the contains test compares the first argument (second
token) against ${sandboxNamesFish}; update the completion line that sets -n
"test (count (commandline -opc)) -ge 2; and contains (commandline -opc | head
-1) ${sandboxNamesFish}" to use the indexed form and keep the existing count
check (references: SANDBOX_ACTIONS, sandboxNamesFish).
---
Nitpick comments:
In `@bin/lib/backup.js`:
- Line 12: Replace use of process.env.HOME when constructing BACKUP_DIR with
os.homedir() to ensure cross-platform correctness: import or require the
built-in os module (e.g., const os = require('os')) and change the BACKUP_DIR
definition (the BACKUP_DIR constant) to use os.homedir() instead of
process.env.HOME (keep the rest of the path construction the same).
- Around line 133-134: The temp policy file created at policyPath using
fs.writeFileSync(path.join(os.tmpdir(), `nemoclaw-restore-${Date.now()}.yaml`),
backup.policy) lacks explicit file permissions; change the write to set
restrictive permissions (mode 0o600) when creating the file so the policy is
only owner-readable/writable — e.g., call fs.writeFileSync with an options
object that includes { mode: 0o600 } (or create the file via fs.openSync with
that mode then write) to mirror the backup file behavior and ensure defense in
depth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65389c9e-7731-42df-b69a-8069d95daa62
📒 Files selected for processing (2)
bin/lib/backup.jsbin/nemoclaw.js
wscurran
added
the
enhancement: feature
Contributor
|
Thanks for adding sandbox export/import backup commands and shell completion, this will make it easier for users to manage their sandboxes. |
Author
|
Added docstrings to all functions in backup.js and nemoclaw.js to address the coverage warning. |
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 `@bin/nemoclaw.js`:
- Around line 422-424: The importBackup function lacks validation for the
required backupPath parameter; update importBackup (and its use of
backup.importSandbox) to check that backupPath is provided and non-empty, and if
not print a clear usage/error message (e.g., console.error with expected usage
for `nemoclaw import`) and exit early with a non-zero status (process.exit(1))
instead of calling backup.importSandbox(undefined); keep newName handling
unchanged.
- Around line 499-510: The completion function _nemoclaw is checking the wrong
index for the first argument: after cmd=(\${words[1,CURRENT-1]}) the command
name is in cmd[1] and the first user argument is in cmd[2], so change the
sandbox-name containment check to use cmd[2] (and ensure you reference
sandbox_names and sandbox_actions as before); update the condition that
currently tests [[ " \${sandbox_names[@]} " =~ " \${cmd[1]} " ]] to test against
\${cmd[2]} so sandbox_actions are offered correctly, leaving the _describe calls
for global_cmds and sandbox_names intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5624e10-53cf-4a03-a471-587aa1b29abd
📒 Files selected for processing (2)
bin/lib/backup.jsbin/nemoclaw.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/lib/backup.js
8 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
bin/nemoclaw.js (2)
728-730:⚠️ Potential issue | 🔴 CriticalActually print the backups list.
Line 729 discards the array returned by
backup.listBackups(), sonemoclaw backupsproduces no output even when backups exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 728 - 730, The function listBackups currently calls backup.listBackups() and discards its returned array, so nothing is printed; update listBackups to capture the result of backup.listBackups() and output it (e.g., stringify/format and console.log each entry or the full array) so running the CLI command actually displays the backups list.
818-820:⚠️ Potential issue | 🟠 MajorZsh/Fish action completion is checking the wrong token.
In the generated zsh script,
cmd[1]is the command name. In fish,commandline -opc | head -1is also the command name.nemoclaw <sandbox> <TAB>will not offer sandbox actions in either shell until these checks use the first user argument instead (cmd[2]/(commandline -opc)[2]).Also applies to: 833-833
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 818 - 820, The zsh/fish completion generation is checking the command name token instead of the first user argument; update the checks in the generated script so they use the first user argument (change zsh usage from cmd[1] to cmd[2] and fish usage from (commandline -opc | head -1) / (commandline -opc)[1] to the second token (commandline -opc)[2] or equivalent), and apply the same fix for the other occurrence referenced (around the sandbox actions check at the other location). Ensure you update both places that build the zsh/fish conditional (the sandbox name membership checks) so completion triggers when the user types `nemoclaw <sandbox> <TAB>`.bin/lib/backup.js (2)
69-72:⚠️ Potential issue | 🔴 CriticalDo not interpolate sandbox names into
execSyncshell strings.Line 131 derives
sandboxNamefromnewNameorbackup.sandbox.name, then Lines 136 and 161 feed it into shell commands. A crafted backup can inject arbitrary shell syntax during import, and the same pattern on Line 69 makes later exports unsafe for poisoned names already written to the registry. UsespawnSync/execFileSyncwith argument arrays and validate the name before any subprocess or registry write.Also applies to: 131-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/backup.js` around lines 69 - 72, The execSync call that interpolates sandboxName (variable sandboxName) into a shell string is unsafe; validate sandboxName against a strict whitelist/regex (e.g., allow only alphanumerics, dashes/underscores) before using it, and replace string-interpolated execSync usages (the call assigning _policyContent and the other places that run openshell with sandboxName) with child_process.spawnSync or execFileSync using an argument array (e.g., spawnSync("openshell", ["policy", "get", sandboxName"], {encoding, timeout})), so no shell interpolation occurs and poisoned names cannot execute arbitrary commands. Ensure validation runs before any registry write or subprocess invocation.
112-116:⚠️ Potential issue | 🟠 MajorReject a missing backup path before
fs.existsSync().If
backupPathis missing, Line 113 callsfs.existsSync(undefined)and Node throwsERR_INVALID_ARG_TYPE. Once the dispatch bug is fixed,nemoclaw importwith no path will crash instead of returning a clean error.Proposed fix
function importSandbox(backupPath, newName) { + if (!backupPath || typeof backupPath !== "string") { + console.error(" Backup path is required."); + return false; + } if (!fs.existsSync(backupPath)) { console.error(` Backup file not found: ${backupPath}`); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/backup.js` around lines 112 - 116, The importSandbox function calls fs.existsSync(backupPath) without validating backupPath, causing ERR_INVALID_ARG_TYPE when backupPath is undefined; before invoking fs.existsSync in importSandbox, validate that backupPath is provided and is a non-empty string (e.g., if (!backupPath) or typeof check), log a clear error like "Backup path required" and return false so the command fails gracefully instead of throwing.
🤖 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 737-739: The importBackup wrapper currently ignores the boolean
result of backup.importSandbox; update the async function
importBackup(backupPath, newName) to capture the return value from
backup.importSandbox(backupPath, newName) and if it returns false either throw
an Error or return a non-zero process exit (so the CLI exits with failure). Make
this change inside importBackup so callers see failures (reference: importBackup
and backup.importSandbox).
- Around line 40-45: GLOBAL_COMMANDS currently omits the "import" command so the
global-command branch and generated completions never include it; add "import"
to the GLOBAL_COMMANDS Set literal (the same Set used to derive globalCmds) so
that the early global-command switch (which checks membership in
GLOBAL_COMMANDS) will handle "import" and completions generation will include it
as well.
- Line 797: The generated Bash completion assigns compgen output to COMPREPLY as
a scalar and uses single quotes so $opts and $cur are not expanded; change the
assignment to populate the indexed array and allow variable expansion by using
array assignment with proper quoting, e.g. use an array-style assignment that
captures compgen output into COMPREPLY and expand $opts and $cur (reference
COMPREPLY, compgen, and the variables $opts and $cur).
---
Duplicate comments:
In `@bin/lib/backup.js`:
- Around line 69-72: The execSync call that interpolates sandboxName (variable
sandboxName) into a shell string is unsafe; validate sandboxName against a
strict whitelist/regex (e.g., allow only alphanumerics, dashes/underscores)
before using it, and replace string-interpolated execSync usages (the call
assigning _policyContent and the other places that run openshell with
sandboxName) with child_process.spawnSync or execFileSync using an argument
array (e.g., spawnSync("openshell", ["policy", "get", sandboxName"], {encoding,
timeout})), so no shell interpolation occurs and poisoned names cannot execute
arbitrary commands. Ensure validation runs before any registry write or
subprocess invocation.
- Around line 112-116: The importSandbox function calls
fs.existsSync(backupPath) without validating backupPath, causing
ERR_INVALID_ARG_TYPE when backupPath is undefined; before invoking fs.existsSync
in importSandbox, validate that backupPath is provided and is a non-empty string
(e.g., if (!backupPath) or typeof check), log a clear error like "Backup path
required" and return false so the command fails gracefully instead of throwing.
In `@bin/nemoclaw.js`:
- Around line 728-730: The function listBackups currently calls
backup.listBackups() and discards its returned array, so nothing is printed;
update listBackups to capture the result of backup.listBackups() and output it
(e.g., stringify/format and console.log each entry or the full array) so running
the CLI command actually displays the backups list.
- Around line 818-820: The zsh/fish completion generation is checking the
command name token instead of the first user argument; update the checks in the
generated script so they use the first user argument (change zsh usage from
cmd[1] to cmd[2] and fish usage from (commandline -opc | head -1) / (commandline
-opc)[1] to the second token (commandline -opc)[2] or equivalent), and apply the
same fix for the other occurrence referenced (around the sandbox actions check
at the other location). Ensure you update both places that build the zsh/fish
conditional (the sandbox name membership checks) so completion triggers when the
user types `nemoclaw <sandbox> <TAB>`.
🪄 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: 7d6392a9-fb4b-4b0e-9683-8c8a13c77931
📒 Files selected for processing (2)
bin/lib/backup.jsbin/nemoclaw.js
- Add warning logging for malformed backup files - Use spawnSync with args array to prevent command injection - Fix listBackups() to display backups to user - Fix Fish completion to check correct token [(commandline -opc)[2]] - Fix Zsh completion to check cmd[2] instead of cmd[1] - Register 'import' as global command - Propagate import failures to CLI exit code - Fix Bash completion COMPREPLY to use array syntax
Contributor
|
Thanks for adding sandbox export/import and shell completion — both are useful additions. The CLI has been refactored to TypeScript since March. Could you rebase against main and verify the implementation still aligns with the current command structure? Happy to review once it's updated. |
wscurran
added
the
status: rebase
vasanth53
changed the title
vasanth53
force-pushed
the
feat/sandbox-backup-clean
branch
from
59b7903 to
9d8d5b3
Compare
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
nemoclaw backups,nemoclaw <name> export,nemoclaw importNew Commands
List Backups
Export Sandbox
Import Sandbox
Summary by CodeRabbit