feat(cli): add shell completion for bash, zsh, and fish#895
feat(cli): add shell completion for bash, zsh, and fish#895prekshivyas wants to merge 12 commits intoNVIDIA:mainfrom
Conversation
Add a `nemoclaw completion [bash|zsh|fish]` subcommand that outputs shell-specific completion scripts. Completions cover global commands, dynamic sandbox names from the registry, sandbox-scoped actions, and per-command flags (--follow, --yes, --quick, etc.). Auto-detects the current shell when invoked without arguments. Includes a hidden --list-sandboxes flag used internally by the completion scripts for dynamic sandbox name completion at tab-time. Fixes NVIDIA#155 Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "nemoclaw CLI"
participant Completion as "completion module"
participant Registry as "registry"
User->>CLI: nemoclaw completion [shell] or --list-sandboxes
CLI->>Completion: run(args)
alt --list-sandboxes
Completion->>Registry: listSandboxes()
Registry-->>Completion: sandbox names
Completion-->>User: print sandbox names (one per line)
else normal completion request
Completion->>Registry: listSandboxes()
Registry-->>Completion: sandbox names
Completion->>Completion: assemble script (global cmds, sandbox actions, debug flags, sandbox names)
Completion-->>User: output completion script
end
alt unknown shell
Completion-->>User: stderr "Unknown shell" + supported shells
CLI-->>User: exit 1
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Fix ESLint no-undef (words) and no-useless-escape (\$) errors in the generated bash and zsh completion output. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/lib/completion.js`:
- Around line 47-54: The case statement that determines command-specific
completions is missing the setup-spark command, so when users type "nemoclaw
setup-spark " the generic sandbox completions are offered; update the
switch/case block (the pattern grouping that currently contains
"onboard|list|start|stop|status|help|completion|uninstall" and the separate
"deploy" branch) to include "setup-spark" (or add a dedicated "setup-spark)
return ;;" branch) so that the function returns early with no completions for
setup-spark and prevents the fallback sandbox actions from being suggested.
- Around line 77-144: In function zsh, update the case that runs when (( CURRENT
== 3 )) and switches on "${words[2]}" inside the zsh completion script to
include setup-spark alongside
onboard|list|start|stop|status|help|completion|uninstall so that setup-spark
returns early and does not fall through to the sandbox action suggestions;
modify the case pattern in the _nemoclaw function where "${words[2]}" is matched
to add setup-spark to the existing alternation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e259828-e2fe-4bbb-98d2-1872c35306f7
📒 Files selected for processing (3)
bin/lib/completion.jsbin/nemoclaw.jstest/completion.test.js
KimBioInfoStudio
left a comment
KimBioInfoStudio
left a comment
There was a problem hiding this comment.
Nice work — the hand-written approach is the right call for this CLI's architecture. A few suggestions after reading the full diff + nemoclaw.js:
1. Command/action lists are duplicated and already drifting
completion.js hardcodes its own GLOBAL_COMMANDS and SANDBOX_ACTIONS arrays, separate from nemoclaw.js. They're already out of sync — completion.js is missing setup (deprecated but still functional). This will get worse as commands are added.
Consider exporting the canonical lists from a shared module (or from nemoclaw.js itself) so completion stays in sync automatically.
2. Completion should include --help / -h / --version
completion.js drops --help, -h, and --version from the completable tokens, but nemoclaw.js handles all of them as global commands. In non-interactive contexts (e.g. an AI agent or script discovering CLI usage via nemoclaw --help), tab-completing --h → --help is expected behavior. These flags should be included in the first-position completions alongside the named commands.
3. bash: _init_completion may not exist on macOS
_init_completion || return_init_completion comes from the bash-completion package, which isn't installed by default on macOS. Users who haven't brew install bash-completion will get a silent failure. A fallback would fix this:
if type _init_completion &>/dev/null; then
_init_completion || return
else
cur="${COMP_WORDS[COMP_CWORD]}"
prev="${COMP_WORDS[COMP_CWORD-1]}"
cword=$COMP_CWORD
fi4. args.includes("--list-sandboxes") is too loose
if (args.includes("--list-sandboxes")) {If someone runs nemoclaw completion --list-sandboxes bash, the --list-sandboxes check swallows the request before bash is ever looked at. Checking args[0] === "--list-sandboxes" would be more precise and match the positional nature of the other arguments.
5. zsh template mixes JS interpolation and escaped shell variables
The zsh() function uses JS template literals with ${GLOBAL_COMMANDS.join(...)} for JS interpolation alongside \${(f)"$(...)"} for zsh syntax. It works, but it's easy to accidentally drop or add a backslash when editing. Minor, but worth a comment or consider using plain string concatenation for the shell-variable-heavy sections.
6. (Nit) debug flags in completion may drift from debug.sh
The completion hardcodes --quick --output --help for debug, but nemoclaw.js passes args straight through to debug.sh. If that script gains flags, completion won't know. Not a blocker — just something to keep in mind, or worth a code comment.
7. Looking ahead: CLI framework comparison for future refactoring
The hand-written dispatch in nemoclaw.js works today, but as the CLI grows (more commands, more flags, nested subcommands), maintaining the switch/case router + hand-written completion in parallel will become a drag. If a refactor is ever on the table, here's how the main Node.js CLI frameworks stack up against NemoClaw's specific needs:
Key requirements from current architecture
- Dynamic positional arguments:
nemoclaw <sandbox-name> <action>— first arg can be a command OR a dynamic entity name - Zero-config completion: shell completion should derive from command definitions, not a separate file
- Async command handlers: several commands (
onboard,deploy,destroy) are async - Minimal dependencies: current CLI is dependency-light by design
- Interactive prompts:
onboardandpolicy-adduse interactive input
Framework comparison
| yargs | commander | oclif | clipanion | citty | |
|---|---|---|---|---|---|
| Built-in completion | ✅ .completion() bash/zsh |
❌ needs plugin | ✅ plugin, bash/zsh/fish | ✅ built-in | ❌ |
| Dynamic positional args | .arguments() + manual |
✅ hooks + arg parser | ✅ rich routing | ||
<name> <action> pattern |
❌ awkward, needs catch-all | ❌ doesn't fit subcommand model | ✅ topic:command or custom | ✅ path-based routing fits well | ❌ |
| Async handlers | ✅ | ✅ | ✅ | ✅ | ✅ |
| Bundle size (approx) | ~180KB | ~60KB | ~2MB+ (framework) | ~40KB | ~15KB |
| Learning curve | Low | Low | High | Medium | Low |
| Interactive prompts | External (inquirer) | External | Built-in (@oclif/prompts) |
External | External |
| TypeScript-first | ❌ | ✅ | ✅ | ✅ |
Recommendation
If refactoring: clipanion is the best fit.
- Its path-based command routing naturally handles
nemoclaw <name> <action>— you define a catch-all command class with a positional arg that checks against the registry, exactly like the current dispatch logic but declarative - Built-in completion derives from command definitions — no separate
completion.jsto maintain - Smallest bundle size among frameworks that actually solve the completion problem (~40KB)
- Used in production by Yarn Berry, so it's battle-tested for CLI tools with complex routing
If staying hand-written (totally valid): the suggestions in points 1–6 above are the highest-value improvements for this PR.
- Add setup-spark and deploy to no-arg case in bash/zsh completion - Add --help, -h, --version, -v to first-position completions - Add _init_completion fallback for macOS without bash-completion - Use args[0] instead of args.includes for --list-sandboxes check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review! Pushed fixes for points 1–4. checking on 5, 6 |
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/lib/completion.js`:
- Around line 33-37: The fallback branch that sets cur, prev and cword when
_init_completion is not available fails to initialize the words array, causing
${words[2]} to be undefined; update the fallback to also initialize words (e.g.,
assign words from COMP_WORDS like words=( "${COMP_WORDS[@]}" ) or an equivalent
POSIX-safe array) so subsequent references to words (such as ${words[2]}) work
on systems without bash-completion; ensure the change is applied alongside the
existing cur/prev/cword assignments in the same fallback block.
- Around line 6-10: The GLOBAL_COMMANDS array is missing the legacy "setup"
command used by the CLI dispatcher; update the GLOBAL_COMMANDS constant to
include "setup" alongside "setup-spark" so shell autocompletion lists it (locate
the GLOBAL_COMMANDS declaration in completion.js and add the string "setup" into
that array).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bec56c08-c3d4-4751-8eea-9fcd0dda6092
📒 Files selected for processing (1)
bin/lib/completion.js
| const GLOBAL_COMMANDS = [ | ||
| "onboard", "list", "deploy", "setup-spark", | ||
| "start", "stop", "status", "debug", "uninstall", | ||
| "help", "completion", "--help", "-h", "--version", "-v", | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if 'setup' is a recognized command in the CLI dispatcher
rg -n "setup" bin/nemoclaw.js | head -20Repository: NVIDIA/NemoClaw
Length of output: 967
Add setup command to GLOBAL_COMMANDS.
The setup command is a valid CLI command (defined in bin/nemoclaw.js line 87) and is callable via the dispatcher, but it is missing from the GLOBAL_COMMANDS array. Although deprecated in favor of onboard, it remains functional for backwards compatibility and should be included in shell completion:
const GLOBAL_COMMANDS = [
"onboard", "list", "deploy", "setup", "setup-spark",
"start", "stop", "status", "debug", "uninstall",
"help", "completion", "--help", "-h", "--version", "-v",
];
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/completion.js` around lines 6 - 10, The GLOBAL_COMMANDS array is
missing the legacy "setup" command used by the CLI dispatcher; update the
GLOBAL_COMMANDS constant to include "setup" alongside "setup-spark" so shell
autocompletion lists it (locate the GLOBAL_COMMANDS declaration in completion.js
and add the string "setup" into that array).
There was a problem hiding this comment.
Intentionally omitted — setup is deprecated (prints a warning and redirects to onboard). Completion should guide users to onboard, not the legacy alias.
- Add setup-spark and deploy to no-arg case in bash/zsh completion - Add --help, -h, --version, -v to first-position completions - Add _init_completion fallback for macOS without bash-completion - Use args[0] instead of args.includes for --list-sandboxes check - Refactor zsh() to string concatenation to avoid JS/shell escaping bugs - Add missing --sandbox flag to debug completions (was already drifted) - Add test that asserts completion DEBUG_FLAGS match scripts/debug.sh - Export DEBUG_FLAGS constant for testability Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When _init_completion is unavailable (macOS without bash-completion),
the fallback now also sets words from COMP_WORDS so ${words[2]}
resolves correctly at cword==3.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous regex also matched help-text lines ending with ')'. Narrowed to only match bash case patterns like '--flag)' or '--flag | -f)'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
bin/lib/completion.js (2)
6-10:⚠️ Potential issue | 🟡 MinorKeep the shell command inventories aligned with the dispatcher.
bin/nemoclaw.jsstill acceptssetupas a top-level command, but it is missing from the shared list here, the zshglobal_cmds, and fish'sglobal_cmdsguard. That leavesnemoclaw setupunsuggested and can fall through to sandbox-action completion. Consider generating these shell-specific lists from one shared source so they stop drifting.Also applies to: 105-121, 184-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/completion.js` around lines 6 - 10, The global command list in completion generation is missing the "setup" command and is out of sync with the dispatcher in bin/nemoclaw.js; update the shared command source so all shells use the same inventory (or add "setup" to GLOBAL_COMMANDS and the corresponding zsh global_cmds and fish global_cmds guards) and refactor generation to derive those shell-specific arrays from that single list (look for the GLOBAL_COMMANDS constant and the zsh/fish global_cmds blocks to make the change).
39-46:⚠️ Potential issue | 🟠 MajorInitialize
wordsin the bash fallback path.When
_init_completionis unavailable, Line 74 still reads${words[2]}, but the fallback only setscur,prev, andcword. Third-position flag completion after sandbox actions will silently break in that path.Proposed fix
_nemoclaw() { - local cur prev words cword + local cur prev cword + local -a words if type _init_completion &>/dev/null; then _init_completion || return else cur="${COMP_WORDS[COMP_CWORD]}" prev="${COMP_WORDS[COMP_CWORD-1]}" cword=$COMP_CWORD + words=("${COMP_WORDS[@]}") fiIn bash programmable completion, if `_init_completion` is unavailable and a script falls back to `COMP_WORDS`/`COMP_CWORD`, does it need to initialize a local `words` array before using `${words[2]}`?Also applies to: 73-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/completion.js` around lines 39 - 46, The fallback branch that runs when _init_completion is unavailable declares but does not populate the words array, yet later code reads ${words[2]} causing silent breakage; in the else branch (where cur, prev, cword are set from COMP_WORDS/COMP_CWORD) initialize words from COMP_WORDS (e.g. assign words=("${COMP_WORDS[@]}") or equivalent) so subsequent uses of words[...] work correctly; update the else block that references cur, prev, cword, and words to populate words before any ${words[n]} reads.
🤖 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/completion.js`:
- Line 33: The issue is that the global no-argument command list includes
"completion", causing the CLI to return early instead of offering shell options;
update every place where GLOBAL_COMMANDS is filtered into noArgCmds (and similar
filters at the other occurrences) to exclude both "debug" and "completion" (or
build the no-arg list from an explicit allowed array) so that the "completion"
command is not treated as a no-arg command; update the expressions that create
noArgCmds and any matching checks that use that list (referencing noArgCmds,
GLOBAL_COMMANDS, and the "completion" command name) accordingly.
In `@test/completion.test.js`:
- Around line 83-88: The current regex used to build casePatterns is too
permissive and picks up usage lines; update the regex so it only matches
case-label lines that end with a closing parenthesis (and may contain
pipe-separated flags) — e.g. replace the pattern in the match call for
casePatterns with one that requires flags like "--flag" (optionally "|--other")
followed immediately by ")" such as /^\s+--[^\s|)]+(?:\|--[^\s|)]+)*\)/gm; keep
the subsequent mapping logic that trims, strips the trailing ")" and splits on
"|" for debugShFlags so only true case labels from debugSh are parsed.
---
Duplicate comments:
In `@bin/lib/completion.js`:
- Around line 6-10: The global command list in completion generation is missing
the "setup" command and is out of sync with the dispatcher in bin/nemoclaw.js;
update the shared command source so all shells use the same inventory (or add
"setup" to GLOBAL_COMMANDS and the corresponding zsh global_cmds and fish
global_cmds guards) and refactor generation to derive those shell-specific
arrays from that single list (look for the GLOBAL_COMMANDS constant and the
zsh/fish global_cmds blocks to make the change).
- Around line 39-46: The fallback branch that runs when _init_completion is
unavailable declares but does not populate the words array, yet later code reads
${words[2]} causing silent breakage; in the else branch (where cur, prev, cword
are set from COMP_WORDS/COMP_CWORD) initialize words from COMP_WORDS (e.g.
assign words=("${COMP_WORDS[@]}") or equivalent) so subsequent uses of
words[...] work correctly; update the else block that references cur, prev,
cword, and words to populate words before any ${words[n]} reads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 441ced39-829a-4ed4-a906-96ecdc5e3428
📒 Files selected for processing (2)
bin/lib/completion.jstest/completion.test.js
test/completion.test.js
Outdated
| const casePatterns = debugSh.match(/^\s+--[^\n]+\)/gm) || []; | ||
| const debugShFlags = casePatterns | ||
| .map((m) => m.trim().replace(/\)$/, "").split(/\s*\|\s*/)) | ||
| .flat() | ||
| .map((f) => f.trim()) | ||
| .filter((f) => f.startsWith("--")); |
There was a problem hiding this comment.
Parse only case labels from debug.sh.
Line 83 currently matches any indented --... ) line, so usage text like --sandbox NAME ... (default: ...) is being treated as a flag. That is why CI is failing at Line 97 even though DEBUG_FLAGS already contains --sandbox.
Proposed fix
- const casePatterns = debugSh.match(/^\s+--[^\n]+\)/gm) || [];
- const debugShFlags = casePatterns
- .map((m) => m.trim().replace(/\)$/, "").split(/\s*\|\s*/))
+ const casePatterns = Array.from(
+ debugSh.matchAll(
+ /^\s+((?:--[^\s|)]+|-[^\s|)]+)(?:\s*\|\s*(?:--[^\s|)]+|-[^\s|)]+)*)\)\s*$/gm,
+ ),
+ ([, pattern]) => pattern,
+ );
+ const debugShFlags = casePatterns
+ .map((pattern) => pattern.split(/\s*\|\s*/))
.flat()
.map((f) => f.trim())
.filter((f) => f.startsWith("--"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/completion.test.js` around lines 83 - 88, The current regex used to
build casePatterns is too permissive and picks up usage lines; update the regex
so it only matches case-label lines that end with a closing parenthesis (and may
contain pipe-separated flags) — e.g. replace the pattern in the match call for
casePatterns with one that requires flags like "--flag" (optionally "|--other")
followed immediately by ")" such as /^\s+--[^\s|)]+(?:\|--[^\s|)]+)*\)/gm; keep
the subsequent mapping logic that trims, strips the trailing ")" and splits on
"|" for debugShFlags so only true case labels from debugSh are parsed.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/completion.test.js (2)
48-53: Strengthen the auto-detect test assertion.At Line 52, checking only output length can pass with unrelated output. Assert at least one shell-specific completion marker.
Proposed test hardening
it("completion with no arg auto-detects shell", () => { const r = run("completion"); expect(r.code).toBe(0); - // Should output some completion script regardless of shell - expect(r.out.length).toBeGreaterThan(100); + // Should output a known completion marker for one of the supported shells. + expect( + r.out.includes("_nemoclaw") || + r.out.includes("#compdef nemoclaw") || + r.out.includes("complete -c nemoclaw"), + ).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/completion.test.js` around lines 48 - 53, The test "completion with no arg auto-detects shell" is too weak—after calling run("completion") and getting r.out you should assert for at least one shell-specific completion marker instead of only length; update the assertion on r.out to check that it contains one of known markers (e.g., "complete -C" or "complete -o", "compdef", or "autoload -U +X compinit") so that the test verifies an actual shell completion script was emitted; locate the test using run("completion") and replace or augment the expect(r.out.length).toBeGreaterThan(100) check with an assertion that r.out includes at least one of those markers.
65-66: Fix the HOME comment to match actual setup.Line 65 says “empty HOME”, but Line 16 sets HOME to an isolated temp path. Update wording to avoid confusion.
Comment-only fix
- // With empty HOME, no sandboxes expected — just no error + // With isolated HOME, no pre-existing sandboxes expected — just no error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/completion.test.js` around lines 65 - 66, Update the misleading test comment that says “empty HOME” to accurately reflect the test setup which sets HOME to an isolated temporary path (the test’s HOME isolation done earlier in the file), e.g. change it to “With isolated HOME (temporary path), no sandboxes expected — just no error” so the comment matches the actual setup used by the test case in test/completion.test.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/completion.test.js`:
- Around line 48-53: The test "completion with no arg auto-detects shell" is too
weak—after calling run("completion") and getting r.out you should assert for at
least one shell-specific completion marker instead of only length; update the
assertion on r.out to check that it contains one of known markers (e.g.,
"complete -C" or "complete -o", "compdef", or "autoload -U +X compinit") so that
the test verifies an actual shell completion script was emitted; locate the test
using run("completion") and replace or augment the
expect(r.out.length).toBeGreaterThan(100) check with an assertion that r.out
includes at least one of those markers.
- Around line 65-66: Update the misleading test comment that says “empty HOME”
to accurately reflect the test setup which sets HOME to an isolated temporary
path (the test’s HOME isolation done earlier in the file), e.g. change it to
“With isolated HOME (temporary path), no sandboxes expected — just no error” so
the comment matches the actual setup used by the test case in
test/completion.test.js.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67efb472-e2b8-4377-bc64-bc64d78c206c
📒 Files selected for processing (2)
bin/lib/completion.jstest/completion.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/lib/completion.js
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bin/lib/completion.js (1)
33-33:⚠️ Potential issue | 🟠 Major
completionstill short-circuits in bash and zsh.Lines 33 and 97 exclude only
debugfromnoArgCmds, sonemoclaw completion <TAB>still hits the early return on Lines 63-64 and 142-143 instead of offeringbash,zsh, andfish.Suggested fix
- const noArgCmds = GLOBAL_COMMANDS.filter((c) => c !== "debug").join("|"); + const noArgCmds = GLOBAL_COMMANDS + .filter((c) => !["debug", "completion"].includes(c)) + .join("|"); @@ case "$prev" in + completion) + COMPREPLY=($(compgen -W "bash zsh fish" -- "$cur")) + return ;; ${noArgCmds}) return ;; @@ - const noArgCmds = GLOBAL_COMMANDS.filter((c) => c !== "debug").join("|"); + const noArgCmds = GLOBAL_COMMANDS + .filter((c) => !["debug", "completion"].includes(c)) + .join("|"); @@ case "${words[2]}" in + completion) + _values 'shell' bash zsh fish + return ;; ${noArgCmds}) return ;;Also applies to: 62-68, 97-97, 141-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/completion.js` at line 33, The short-circuit logic builds noArgCmds by filtering out only "debug", which causes the "completion" command to be treated as a no-arg command and return early; update the filter used when building noArgCmds (and any duplicated instances where noArgCmds is computed) to also exclude "completion" (i.e., GLOBAL_COMMANDS.filter(c => c !== "debug" && c !== "completion") ), so that when the current command is "completion" the completion handler will proceed to offer "bash", "zsh", and "fish" instead of hitting the early return; apply this change to every place where noArgCmds (or the same filter logic) is computed in the file.
🤖 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/completion.js`:
- Around line 244-253: The auto-detection currently falls back to emitting
bash() for any unsupported or unset SHELL; update the no-arg branch that reads
envShell so it only emits zsh(), fish(), or bash() when envShell explicitly
matches those shells and otherwise writes an error to stderr (using
process.stderr.write or processLogger) and exits with a non-zero code; reference
the same symbols: shell, envShell, zsh(), fish(), bash() to locate and change
the logic so unknown/empty SHELL does not silently default to bash but fails
like the explicit-shell path.
- Around line 99-101: The generated zsh completion string currently invokes the
completer but does not register it; replace the invocation `_nemoclaw "$@"` with
a registration call `compdef _nemoclaw nemoclaw` so the function `_nemoclaw` is
registered as the completer for the `nemoclaw` command when `completion zsh`
output is eval'd; update the returned string in the completion builder (the
function that returns the zsh completion block) to include the `compdef
_nemoclaw nemoclaw` line instead of the one-shot `_nemoclaw "$@"` call.
---
Duplicate comments:
In `@bin/lib/completion.js`:
- Line 33: The short-circuit logic builds noArgCmds by filtering out only
"debug", which causes the "completion" command to be treated as a no-arg command
and return early; update the filter used when building noArgCmds (and any
duplicated instances where noArgCmds is computed) to also exclude "completion"
(i.e., GLOBAL_COMMANDS.filter(c => c !== "debug" && c !== "completion") ), so
that when the current command is "completion" the completion handler will
proceed to offer "bash", "zsh", and "fish" instead of hitting the early return;
apply this change to every place where noArgCmds (or the same filter logic) is
computed in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab258468-accf-48dd-9f3d-d1d502b9c612
📒 Files selected for processing (1)
bin/lib/completion.js
- Don't treat completion as no-arg: offer bash/zsh/fish as subcommands - Replace _nemoclaw "$@" with compdef _nemoclaw nemoclaw for zsh registration - Error on unknown/unset SHELL in auto-detect instead of defaulting to bash - Update tests for zsh compdef and explicit SHELL in auto-detect test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Update: 5 and 6 are addressed |
It is not, as evidenced by the mountain of issues found immediately following that post. We should be using a library that knows how to automatically generate completions for all 3 shells. @prekshivyas that would be a bit more foundational work. Try having a look into https://oclif.io and similar libs. |
|
Closing in favor of #909 — the handwritten completion approach is being replaced by an oclif migration that provides shell completions (bash/zsh) out of the box via |
3 participants
Summary
Add a
nemoclaw completion [bash|zsh|fish]subcommand that generates shell completion scripts. Auto-detects the current shell when run without arguments.Fixes #155
Changes
bin/lib/completion.js— generates bash, zsh, and fish completion scriptsbin/nemoclaw.js— registerscompletionas a global commandtest/completion.test.js— 7 test casesDetails
Completions cover:
Hidden
--list-sandboxesflag is used internally by completion scripts for dynamic sandbox name completion at tab-time.Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.Checklist
General
Code Changes
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
New Features
nemoclaw completion [bash|zsh|fish]; omitting the shell auto-detects it. Completions include global commands, sandbox names, actions, and relevant flags. Adds a mode to list sandbox names for integration.Tests