feat: add shell completion for nemoclaw CLI#160
Closed
vasanth53 wants to merge 5 commits intoNVIDIA:mainfrom
Closed
feat: add shell completion for nemoclaw CLI#160vasanth53 wants to merge 5 commits intoNVIDIA:mainfrom
vasanth53 wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
wscurran
added
the
enhancement: feature
Contributor
|
I've reviewed the changes and see that this PR introduces a completion command supporting bash, zsh, and fish shells, which should make it easier for users to interact with the nemoclaw CLI. |
wscurran
added
NemoClaw CLI
Add completion command supporting bash, zsh, and fish shells. Completes issue NVIDIA#155.
Add backup and restore functionality for sandboxes: - nemoclaw backups - list all backups - nemoclaw <name> export - export sandbox to backup file - nemoclaw import <path> - import sandbox from backup
vasanth53
force-pushed
the
feat/shell-completion
branch
from
ad1af8b to
6728e40
Compare
Contributor
📝 WalkthroughWalkthroughAdds a new JSON-backed sandbox backup module and integrates CLI commands to list, export, import, and delete backups, plus shell completion support; backups include metadata, sandbox registry fields, and optional policy text fetched/applied via openshell. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CLI as nemoclaw.js
participant Backup as bin/lib/backup.js
participant Registry as registry
participant OpenShell as openshell
participant FS as File System
User->>CLI: export <sandbox-name> [output-path]
CLI->>Backup: exportSandbox(sandboxName, outputPath)
Backup->>Registry: getSandbox(sandboxName)
Registry-->>Backup: sandbox object / null
alt Sandbox exists
Backup->>OpenShell: exec policy get <sandbox-name>
OpenShell-->>Backup: policy text / error
Backup->>FS: ensure backup dir, write JSON backup (metadata + policy)
FS-->>Backup: file written (path)
Backup-->>CLI: return path
CLI-->>User: success message
else Sandbox not found
Backup-->>CLI: null
CLI-->>User: error message
end
sequenceDiagram
participant User as User/CLI
participant CLI as nemoclaw.js
participant Backup as bin/lib/backup.js
participant FS as File System
participant OpenShell as openshell
participant Registry as registry
User->>CLI: import <backup-file> [new-name]
CLI->>Backup: importSandbox(backupPath, newName)
Backup->>FS: read & parse backup JSON
FS-->>Backup: backup data / error
alt Valid backup structure
Backup->>OpenShell: exec sandbox exists <target>
alt Sandbox doesn't exist
Backup->>Registry: register sandbox (fields from backup)
Registry-->>Backup: registered
alt policy present
Backup->>FS: write temp policy YAML file
Backup->>OpenShell: policy set --policy <file> --wait <sandbox>
OpenShell-->>Backup: applied / failed
Backup->>FS: cleanup temp file & dir
end
Backup-->>CLI: true
CLI-->>User: success message
else Sandbox exists
Backup-->>CLI: false
CLI-->>User: already exists error
end
else Invalid backup
Backup-->>CLI: false
CLI-->>User: validation error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🤖 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 63-67: The metadata block uses require("../package.json").version
which resolves incorrectly to bin/package.json and causes MODULE_NOT_FOUND;
update the import used for the nemoclawVersion field to point to the repo root
package.json (i.e. change the relative require path to go up one more directory)
so that metadata.nemoclawVersion correctly reads the root package.json version.
- Around line 53-56: The code is vulnerable because sandboxName is interpolated
into shell strings passed to execSync; update the top import to require
execFileSync (in addition to or instead of execSync), then replace the three
execSync usages that build commands with argument-array calls to execFileSync:
in exportSandbox(), importSandbox(), and the other function that invokes
`openshell policy get`/`set`/`import` (references: the functions named
exportSandbox and importSandbox and the variable sandboxName)—pass
["policy","get", sandboxName] (or the appropriate subcommand args) as the argv
parameter, keep the same options object (encoding, timeout) and redirect stderr
by using the stdio/options equivalent, and remove all string interpolation of
sandboxName so untrusted input is never executed by the shell.
- Around line 133-145: The current restore flow writes backup.policy to a
predictable file (policyPath) in os.tmpdir() with default perms; change it to
create a unique temp directory via fs.mkdtempSync (use path.join(os.tmpdir(),
'nemoclaw-restore-')), write the policy file inside that directory using
fs.writeFileSync with mode 0o600, run the execSync call (execSync(`openshell
policy set ... ${sandboxName}`)), and in the finally block ensure you securely
remove the temp file and directory (fs.unlinkSync for the file and fs.rmdirSync
or fs.rmSync for the dir) so no world-readable file remains; reference the
variables policyPath, backup.policy, sandboxName, and the
execSync/fs.writeFileSync/fs.unlinkSync calls when locating the change.
In `@bin/nemoclaw.js`:
- Line 410: globalCmds currently builds completions only from GLOBAL_COMMANDS
and therefore omits the top-level "import" command (which is handled elsewhere
by the import dispatch). Update the completion list to include "import" by
either adding "import" to the GLOBAL_COMMANDS set or appending it when building
globalCmds (e.g., after const globalCmds =
Array.from(GLOBAL_COMMANDS).filter(...), push or concat "import"); ensure the
symbol globalCmds and the dispatched import handler remain in sync so "import"
appears in top-level suggestions.
- Around line 457-467: The completion currently checks the command name instead
of the first argument; in the zsh function _nemoclaw (which builds
cmd=(\${words[1,CURRENT-1]}) and then tests cmd[1]) change the check to use
cmd[2] so it compares the first argument against sandbox_names when deciding
between sandbox_actions and global_cmds; similarly, in the fish completion path
(which uses commandline -opc | head -1) ensure you extract and test the second
token (the first argument) against sandbox_names rather than the command token
so sandbox completions trigger correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7846d8f0-f504-478a-9a16-ffc2cfce2fa3
📒 Files selected for processing (2)
bin/lib/backup.jsbin/nemoclaw.js
- Fix package.json path in backup.js (../../package.json) - Replace execSync with execFileSync to prevent shell injection - Secure temp file handling with mkdtempSync and proper permissions - Add 'import' to globalCmds completion list - Fix zsh completion to check cmd[2] instead of cmd[1] - Fix fish completion to check second token
Author
|
Thanks for the review! All feedback addressed in the latest commit. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/backup.js (1)
146-149: Consider usingfs.rmSyncfor more robust cleanup.If
fs.unlinkSyncthrows,fs.rmdirSyncwon't execute, potentially leaving the temp directory behind. Additionally,rmdirSyncis deprecated in favor ofrmSync. Using a single recursive removal is more robust.♻️ Proposed fix
} finally { - fs.unlinkSync(policyPath); - fs.rmdirSync(tmpDir); + fs.rmSync(tmpDir, { recursive: true, force: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/backup.js` around lines 146 - 149, Replace the brittle cleanup that calls fs.unlinkSync(policyPath) then fs.rmdirSync(tmpDir) with a single robust removal using fs.rmSync so failures removing the file won't prevent removing the temp directory; specifically, where policyPath and tmpDir are used in the finally block (currently calling fs.unlinkSync and fs.rmdirSync), call fs.rmSync(tmpDir, { recursive: true, force: true }) to remove the file and directory atomically and handle deprecated rmdirSync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/backup.js`:
- Around line 146-149: Replace the brittle cleanup that calls
fs.unlinkSync(policyPath) then fs.rmdirSync(tmpDir) with a single robust removal
using fs.rmSync so failures removing the file won't prevent removing the temp
directory; specifically, where policyPath and tmpDir are used in the finally
block (currently calling fs.unlinkSync and fs.rmdirSync), call fs.rmSync(tmpDir,
{ recursive: true, force: true }) to remove the file and directory atomically
and handle deprecated rmdirSync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ae59208-8db7-43a9-868a-ea6827c2b406
📒 Files selected for processing (2)
bin/lib/backup.jsbin/nemoclaw.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 441-457: The usage block for unsupported shells currently prints
to stdout and returns success; change it to emit the usage/diagnostic to stderr
(use console.error or process.stderr.write) and then exit with a non-zero status
(e.g., process.exit(1)) when shell is missing or not in SHELL_TYPES; update the
code path around the check that references shell and SHELL_TYPES (and the
printCompletion()/completion handling) so that failing cases send output to
stderr and call process.exit(1) instead of returning normally.
- Around line 472-527: The completion scripts currently only offer word lists;
update the bash function _nemoclaw_completions, the zsh function _nemoclaw, and
the fish complete calls to detect when the current position is an import path
(previous token == "import") or a sandbox export path (first token is a sandbox
name and the next token == "export") and fall back to file/path completion for
those positions: in bash (inside _nemoclaw_completions) check if prev ==
"import" or (COMP_WORDS[1] in sandboxNamesStr and prev == "export") and then
generate COMPREPLY using compgen -f -- "$cur" instead of the word list; in zsh
(inside _nemoclaw) detect the same conditions using ${words} and call _files or
_path_files for path completion rather than _describe; in fish add separate
complete entries for the import and sandbox export argument positions without
the -f flag (or with appropriate -f/-a to allow file completion) so filesystem
paths are offered for those argument slots while leaving other completions
unchanged (use globalCmds, SANDBOX_ACTIONS,
sandboxNamesStr/sandboxNamesZsh/sandboxNamesFish to identify commands and
sandbox names).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d439005-1ff3-4376-b446-163247a2aa0a
📒 Files selected for processing (1)
bin/nemoclaw.js
Comment on lines
+441
to
+457
| if (!shell || !SHELL_TYPES.includes(shell)) { | ||
| console.log(" Usage: nemoclaw completion <shell>"); | ||
| console.log(""); | ||
| console.log(" Generate shell completion scripts."); | ||
| console.log(""); | ||
| console.log(" Shells supported: bash, zsh, fish"); | ||
| console.log(""); | ||
| console.log(" Example:"); | ||
| console.log(" # Bash:"); | ||
| console.log(" nemoclaw completion bash >> ~/.bashrc"); | ||
| console.log(""); | ||
| console.log(" # Zsh:"); | ||
| console.log(" nemoclaw completion zsh >> ~/.zshrc"); | ||
| console.log(""); | ||
| console.log(" # Fish:"); | ||
| console.log(" nemoclaw completion fish > ~/.config/fish/completions/nemoclaw.fish"); | ||
| return; |
Contributor
There was a problem hiding this comment.
Return a failure status for unsupported shells.
printCompletion() prints usage and returns success here. nemoclaw completion foo > ~/.bashrc will therefore look successful and can write the usage text into a shell config file. Please send this path through stderr and exit non-zero.
💡 Suggested fix
function printCompletion(shell) {
if (!shell || !SHELL_TYPES.includes(shell)) {
- console.log(" Usage: nemoclaw completion <shell>");
- console.log("");
- console.log(" Generate shell completion scripts.");
- console.log("");
- console.log(" Shells supported: bash, zsh, fish");
- console.log("");
- console.log(" Example:");
- console.log(" # Bash:");
- console.log(" nemoclaw completion bash >> ~/.bashrc");
- console.log("");
- console.log(" # Zsh:");
- console.log(" nemoclaw completion zsh >> ~/.zshrc");
- console.log("");
- console.log(" # Fish:");
- console.log(" nemoclaw completion fish > ~/.config/fish/completions/nemoclaw.fish");
- return;
+ console.error(" Usage: nemoclaw completion <shell>");
+ console.error("");
+ console.error(" Generate shell completion scripts.");
+ console.error("");
+ console.error(" Shells supported: bash, zsh, fish");
+ console.error("");
+ console.error(" Example:");
+ console.error(" # Bash:");
+ console.error(" nemoclaw completion bash >> ~/.bashrc");
+ console.error("");
+ console.error(" # Zsh:");
+ console.error(" nemoclaw completion zsh >> ~/.zshrc");
+ console.error("");
+ console.error(" # Fish:");
+ console.error(" nemoclaw completion fish > ~/.config/fish/completions/nemoclaw.fish");
+ process.exit(1);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/nemoclaw.js` around lines 441 - 457, The usage block for unsupported
shells currently prints to stdout and returns success; change it to emit the
usage/diagnostic to stderr (use console.error or process.stderr.write) and then
exit with a non-zero status (e.g., process.exit(1)) when shell is missing or not
in SHELL_TYPES; update the code path around the check that references shell and
SHELL_TYPES (and the printCompletion()/completion handling) so that failing
cases send output to stderr and call process.exit(1) instead of returning
normally.
Comment on lines
+448
to
+470
| console.log(" Example:"); | ||
| console.log(" # Bash:"); | ||
| console.log(" nemoclaw completion bash >> ~/.bashrc"); | ||
| console.log(""); | ||
| console.log(" # Zsh:"); | ||
| console.log(" nemoclaw completion zsh >> ~/.zshrc"); | ||
| console.log(""); | ||
| console.log(" # Fish:"); | ||
| console.log(" nemoclaw completion fish > ~/.config/fish/completions/nemoclaw.fish"); | ||
| return; | ||
| } | ||
|
|
||
| const globalCmds = [...Array.from(GLOBAL_COMMANDS).filter(c => !c.startsWith("-")), "import"]; | ||
| let sandboxNames = []; | ||
| try { | ||
| sandboxNames = registry.listSandboxes().sandboxes.map(s => s.name); | ||
| } catch { | ||
| sandboxNames = []; | ||
| } | ||
|
|
||
| const sandboxNamesStr = sandboxNames.length > 0 ? sandboxNames.join(" ") : ""; | ||
| const sandboxNamesZsh = sandboxNames.length > 0 ? sandboxNames.map(s => `"${s}"`).join(" ") : ""; | ||
| const sandboxNamesFish = sandboxNames.length > 0 ? sandboxNames.map(s => `'${s}'`).join(" ") : ""; |
Contributor
There was a problem hiding this comment.
Sandbox name completion is still a snapshot.
sandboxNames is resolved once in Node and then inlined into the generated script. Since Line 450 through Line 456 tell users to append that output once to their shell config, completions go stale after any later onboard, import, or destroy. The emitted completion function needs a runtime lookup for current sandbox names instead of baking them into the file.
Comment on lines
+472
to
+527
| if (shell === "bash") { | ||
| console.log(`_nemoclaw_completions() { | ||
| local cur prev opts | ||
| COMPREPLY=() | ||
| cur="\${COMP_WORDS[COMP_CWORD]}" | ||
| prev="\${COMP_WORDS[COMP_CWORD-1]}" | ||
|
|
||
| # Global commands | ||
| opts="${globalCmds.join(" ")}" | ||
|
|
||
| # Sandbox names (if previous word is a known sandbox) | ||
| if [[ " ${sandboxNamesStr} " =~ " $prev " ]]; then | ||
| opts="${SANDBOX_ACTIONS.join(" ")}" | ||
| fi | ||
|
|
||
| # Also add sandbox names as possible first argument | ||
| opts="$opts ${sandboxNamesStr}" | ||
|
|
||
| COMPREPLY=(\$(compgen -W "\$opts" -- \$cur)) | ||
| return 0 | ||
| } | ||
|
|
||
| complete -F _nemoclaw_completions nemoclaw`); | ||
| } else if (shell === "zsh") { | ||
| console.log(`# nemoclaw zsh completion | ||
|
|
||
| local -a global_cmds | ||
| global_cmds=(${globalCmds.map(c => `"${c}"`).join(" ")}) | ||
|
|
||
| local -a sandbox_actions | ||
| sandbox_actions=(${SANDBOX_ACTIONS.map(a => `"${a}"`).join(" ")}) | ||
|
|
||
| local -a sandbox_names | ||
| sandbox_names=(${sandboxNamesZsh}) | ||
|
|
||
| _nemoclaw() { | ||
| local -a cmd | ||
| cmd=(\${words[1,CURRENT-1]}) | ||
|
|
||
| # Check if first argument is a sandbox name | ||
| if [[ " \${sandbox_names[@]} " =~ " \${cmd[2]} " ]]; then | ||
| _describe 'sandbox actions' sandbox_actions | ||
| else | ||
| _describe 'commands' global_cmds | ||
| _describe 'sandboxes' sandbox_names | ||
| fi | ||
| } | ||
|
|
||
| compdef _nemoclaw nemoclaw`); | ||
| } else if (shell === "fish") { | ||
| console.log(`# nemoclaw fish completion | ||
|
|
||
| complete -c nemoclaw -f -a "${globalCmds.join(" ")} ${sandboxNamesStr}" -n "test (count (commandline -opc)) -eq 1" | ||
|
|
||
| complete -c nemoclaw -f -a "${SANDBOX_ACTIONS.join(" ")}" -n "test (count (commandline -opc)) -ge 2; and contains (commandline -opc)[2] ${sandboxNamesFish}" | ||
| `); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n bin/nemoclaw.js | sed -n '472,527p'Repository: NVIDIA/NemoClaw
Length of output: 2033
🏁 Script executed:
rg -n "globalCmds|SANDBOX_ACTIONS" bin/nemoclaw.js -B 2 -A 2Repository: NVIDIA/NemoClaw
Length of output: 1179
🏁 Script executed:
rg -n "import|export" bin/nemoclaw.js | head -30Repository: NVIDIA/NemoClaw
Length of output: 914
Add path completion for import and export argument positions.
The completion scripts only return command/action word lists and suppress path completion for import <path> and <sandbox> export [path] arguments. In bash and zsh, these positions only complete against the configured word lists; in fish, the -f flag explicitly disables file completion. Users cannot tab-complete filesystem paths for backup files or export destinations.
Add conditional branches or fallback handlers to enable path completion after these commands. Quick validation: nemoclaw import ./<TAB> and nemoclaw my-sandbox export ./<TAB> should expand filesystem paths in bash, zsh, and fish.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/nemoclaw.js` around lines 472 - 527, The completion scripts currently
only offer word lists; update the bash function _nemoclaw_completions, the zsh
function _nemoclaw, and the fish complete calls to detect when the current
position is an import path (previous token == "import") or a sandbox export path
(first token is a sandbox name and the next token == "export") and fall back to
file/path completion for those positions: in bash (inside _nemoclaw_completions)
check if prev == "import" or (COMP_WORDS[1] in sandboxNamesStr and prev ==
"export") and then generate COMPREPLY using compgen -f -- "$cur" instead of the
word list; in zsh (inside _nemoclaw) detect the same conditions using ${words}
and call _files or _path_files for path completion rather than _describe; in
fish add separate complete entries for the import and sandbox export argument
positions without the -f flag (or with appropriate -f/-a to allow file
completion) so filesystem paths are offered for those argument slots while
leaving other completions unchanged (use globalCmds, SANDBOX_ACTIONS,
sandboxNamesStr/sandboxNamesZsh/sandboxNamesFish to identify commands and
sandbox names).
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: 1
♻️ Duplicate comments (3)
bin/nemoclaw.js (3)
457-475:⚠️ Potential issue | 🟡 MinorFail invalid
completioninvocations.Line 458 enters the usage branch, but Line 474 just returns.
nemoclaw completion foo > ~/.bashrctherefore exits successfully and appends the usage text to the target file. Send this path to stderr and exit non-zero.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 457 - 475, The printCompletion function currently prints usage for invalid shells but returns success; update the invalid-argument branch in printCompletion (the CHECK on shell and SHELL_TYPES) to write the usage/diagnostic text to stderr instead of stdout and then exit the process with a non-zero code (e.g., process.exit(1)); ensure you modify the branch guarded by the condition that checks SHELL_TYPES so invalid invocations like `nemoclaw completion foo` fail with a non-zero status.
477-487:⚠️ Potential issue | 🟠 MajorGenerated completions still capture a one-time sandbox list.
Lines 478-487 resolve sandbox names before emitting the script, so the installed completion file goes stale after any later
onboard,import, ordestroy. Move the sandbox-name lookup into the generated shell functions instead of embedding a snapshot here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 477 - 487, The current code captures sandbox names once by calling registry.listSandboxes() into sandboxNames/sandboxNamesStr/sandboxNamesZsh/sandboxNamesFish, producing a stale snapshot; instead, remove the precomputed sandboxNames variables and move the call to registry.listSandboxes() (or a small helper that returns current sandbox names) into the generated shell completion functions so the shell runs the lookup at completion time (refer to GLOBAL_COMMANDS and the existing registry.listSandboxes() usage) and format the results per-shell inside the functions rather than embedding sandboxNamesStr, sandboxNamesZsh, or sandboxNamesFish at emit-time.
489-544:⚠️ Potential issue | 🟠 MajorKeep file completion for backup paths.
These branches only emit word-list completions, and the fish rules explicitly disable filenames with
-f. That preventsnemoclaw import ./<TAB>andnemoclaw <sandbox> export ./<TAB>from completing filesystem paths. Add path-completion fallbacks for those argument positions instead of forcing command-only suggestions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 489 - 544, The completion scripts emitted for bash, zsh and fish only provide word lists (globalCmds, SANDBOX_ACTIONS, sandboxNamesStr/ sandboxNamesZsh/ sandboxNamesFish) and explicitly disable or omit filesystem path suggestions, which breaks completions for commands that accept file paths (e.g. import/export). Update the bash, zsh and fish branches in bin/nemoclaw.js to detect when the current/previous word is an action that expects a filesystem argument (check SANDBOX_ACTIONS entries like "import" and "export") and in those cases fall back to path completions: for bash use compgen -f (or compopt +o filenames) instead of only compgen -W, for zsh use _files or add a conditional _describe/_files call, and for fish add a separate complete rule that uses file path completion (or remove the -f flag and use __fish_complete_path) for the argument positions that follow those sandbox actions. Ensure other command positions still return the original word-list completions.
🤖 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 128-150: The code uses newName || backup.sandbox.name directly for
sandboxName and registers it via registry.registerSandbox without validating;
call the existing validateName(name) function on sandboxName (reject or
normalize when invalid) before running the openshell existence check and before
calling registry.registerSandbox, and handle validation failures by printing an
error and returning false so malformed backup names are not persisted.
---
Duplicate comments:
In `@bin/nemoclaw.js`:
- Around line 457-475: The printCompletion function currently prints usage for
invalid shells but returns success; update the invalid-argument branch in
printCompletion (the CHECK on shell and SHELL_TYPES) to write the
usage/diagnostic text to stderr instead of stdout and then exit the process with
a non-zero code (e.g., process.exit(1)); ensure you modify the branch guarded by
the condition that checks SHELL_TYPES so invalid invocations like `nemoclaw
completion foo` fail with a non-zero status.
- Around line 477-487: The current code captures sandbox names once by calling
registry.listSandboxes() into
sandboxNames/sandboxNamesStr/sandboxNamesZsh/sandboxNamesFish, producing a stale
snapshot; instead, remove the precomputed sandboxNames variables and move the
call to registry.listSandboxes() (or a small helper that returns current sandbox
names) into the generated shell completion functions so the shell runs the
lookup at completion time (refer to GLOBAL_COMMANDS and the existing
registry.listSandboxes() usage) and format the results per-shell inside the
functions rather than embedding sandboxNamesStr, sandboxNamesZsh, or
sandboxNamesFish at emit-time.
- Around line 489-544: The completion scripts emitted for bash, zsh and fish
only provide word lists (globalCmds, SANDBOX_ACTIONS, sandboxNamesStr/
sandboxNamesZsh/ sandboxNamesFish) and explicitly disable or omit filesystem
path suggestions, which breaks completions for commands that accept file paths
(e.g. import/export). Update the bash, zsh and fish branches in bin/nemoclaw.js
to detect when the current/previous word is an action that expects a filesystem
argument (check SANDBOX_ACTIONS entries like "import" and "export") and in those
cases fall back to path completions: for bash use compgen -f (or compopt +o
filenames) instead of only compgen -W, for zsh use _files or add a conditional
_describe/_files call, and for fish add a separate complete rule that uses file
path completion (or remove the -f flag and use __fish_complete_path) for the
argument positions that follow those sandbox actions. Ensure other command
positions still return the original word-list completions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0688e5c4-b8c9-4da8-b265-fa1098c049ae
📒 Files selected for processing (2)
bin/lib/backup.jsbin/nemoclaw.js
Comment on lines
+128
to
+150
| const sandboxName = newName || backup.sandbox.name; | ||
|
|
||
| console.log(` Creating sandbox '${sandboxName}' from backup...`); | ||
|
|
||
| try { | ||
| execFileSync("openshell", ["sandbox", "exists", sandboxName], { | ||
| encoding: "utf-8", | ||
| stdio: ["pipe", "pipe", "pipe"] | ||
| }); | ||
| console.error(` Sandbox '${sandboxName}' already exists. Use a different name or delete it first.`); | ||
| return false; | ||
| } catch {} | ||
|
|
||
| console.log(` Note: This only imports the registry config.`); | ||
| console.log(` You need to manually recreate the sandbox and apply policies.`); | ||
|
|
||
| registry.registerSandbox({ | ||
| name: sandboxName, | ||
| model: backup.sandbox.model, | ||
| provider: backup.sandbox.provider, | ||
| gpuEnabled: backup.sandbox.gpuEnabled, | ||
| policies: backup.sandbox.policies || [], | ||
| }); |
Contributor
There was a problem hiding this comment.
Validate imported sandbox names before persisting them.
Line 128 accepts newName or backup.sandbox.name verbatim, but normal CLI flows already gate sandbox names through validateName(). A malformed backup can therefore register an entry that later nemoclaw <name> ... paths reject. Validate the chosen name before the openshell existence check and before registry.registerSandbox().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/backup.js` around lines 128 - 150, The code uses newName ||
backup.sandbox.name directly for sandboxName and registers it via
registry.registerSandbox without validating; call the existing
validateName(name) function on sandboxName (reject or normalize when invalid)
before running the openshell existence check and before calling
registry.registerSandbox, and handle validation failures by printing an error
and returning false so malformed backup names are not persisted.
6 tasks
Contributor
|
Hey @vasanth53 — heads up that I also submitted #895 for the same issue (#155). Didn't realize yours was already in progress until after I'd pushed mine, sorry about the overlap! Just flagging so maintainers can compare both and pick whichever fits best. |
8 tasks
Contributor
|
Closing in favor of #909 — shell completion will be handled by oclif's autocomplete plugin as part of the CLI migration. |
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 completioncommand supporting bash, zsh, and fish shellsUsage
Testing
Summary by CodeRabbit