refactor(cli): split share into oclif subcommands - #2911
Merged
Conversation
This reverts commit 4ebeae4.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds short-form flag aliases ( ChangesFlag aliases and CLI metadata
Share command refactor (independent DAG)
Sequence DiagramsequenceDiagram
participant User as User / CLI
participant Dispatch as Dispatch Layer
participant Oclif as Oclif Command
participant Runner as Runner Function
participant Impl as Implementation
User->>Dispatch: "alpha share mount <args>"
Note over Dispatch: resolveSandboxOclifDispatch maps action -> sandbox:share:mount
Dispatch-->>User: DispatchResult { kind: 'oclif', commandId: 'sandbox:share:mount' }
User->>Oclif: ShareMountCommand.run()
Oclif->>Oclif: parse args (name, remotePath, localMount)
Oclif->>Runner: runShareMount({sandboxName,...})
Runner->>Impl: ensureLive(sandbox) / get ssh config
Impl-->>Runner: ssh config
Runner->>Impl: check sshfs, create temp ssh config, spawn sshfs
Impl-->>Runner: success / error
Runner->>Runner: cleanup temp files
Runner-->>Oclif: exit code / messages
Oclif-->>User: exit(0) or exit(1)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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)
Comment |
This was referenced May 4, 2026
cv
added a commit
that referenced
this pull request
May 5, 2026
## Summary Add consistent `-y` confirmation aliases and examples across lifecycle-oriented oclif commands. This keeps confirmation semantics unchanged while documenting the parser-owned aliases in command help and root help metadata. ## Stack Navigation - Position: 18 of 60 - Previous PR: [#2909 — refactor(cli): require skill install path in oclif](#2909) - Next PR: [#2911 — refactor(cli): split share into oclif subcommands](#2911) ## Changes - Added `-y` aliases for `destroy`, `rebuild`, `upgrade-sandboxes`, and `gc` confirmation flags. - Added examples for `destroy` and `rebuild` command metadata. - Updated public help/registry usage strings for lifecycle commands. - Exercised `destroy -y` through an existing lifecycle CLI subprocess test. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
marked this pull request as ready for review
May 5, 2026 00:26
cv
enabled auto-merge (squash)
May 5, 2026 00:26
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
ericksoa
requested changes
May 5, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Reviewed current head 3cb1dd8. Requesting changes for the share help regression introduced by the oclif subcommand split.
alpha share --help is routed through sandbox:share without preserving the help flag (src/lib/legacy-oclif-dispatch.ts:152), so it exits 1 with bare usage instead of normal help. The subcommand help paths also expose internal command IDs; after npm run build:cli, alpha share mount --help exits 0 but prints USAGE $ nemoclaw sandbox:share:mount <name> share mount ..., unlike the adjacent sandbox-scoped commands whose tests assert no sandbox:* internals leak in help output.
Local validation run on this head:
npm run build:clipassed.npx vitest run src/lib/share-command.test.ts test/cli.test.ts -t sharepassed, but it does not covershare --helpor share subcommand help output.- Manual probes reproduced the regression:
alpha share --helpexit 1;alpha share mount --helpexit 0 withsandbox:share:mountin usage.
I would fix this by keeping share help on the public legacy-dispatch path, the same way other sandbox-scoped help is handled, and adding CLI coverage for alpha share --help plus alpha share mount|unmount|status --help asserting public usage and no sandbox:share* leakage.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
This comment was marked as outdated.
This comment was marked as outdated.
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/command-registry.ts (1)
176-192:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the registry in sync with the other new
-yaliases.This file is the help/docs source of truth, but
upgrade-sandboxesandgcstill advertise the old flag syntax farther down inCOMMANDS. After this change, runtime support and canonical usage drift apart again.Suggested follow-up in this file
{ usage: "nemoclaw upgrade-sandboxes", description: "Detect and rebuild stale sandboxes", - flags: "(--check, --auto)", + flags: "(--check, --auto, --yes|-y)", group: "Upgrade", scope: "global", }, @@ { usage: "nemoclaw gc", description: "Remove orphaned sandbox Docker images", - flags: "(--yes|--force, --dry-run)", + flags: "(--yes|-y|--force, --dry-run)", group: "Cleanup", scope: "global", },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/command-registry.ts` around lines 176 - 192, The COMMANDS registry entries for the "upgrade-sandboxes" and "gc" commands are still using the old flag syntax; locate the COMMANDS array entries whose usage strings are "nemoclaw upgrade-sandboxes" and "nemoclaw gc" and update their flags fields to include the new short alias -y (e.g. change flags to include "[--yes|-y|--force]" or "[--yes|-y]" as appropriate) so the help/docs match the runtime flag aliases used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/legacy-oclif-dispatch.ts`:
- Around line 149-154: The branch handling case "share" currently strips a
top-level "--help"/"-h" flag before dispatching to sandbox:share so
ShareCommand.run() never sees the flag; change the logic so when shareSub is
"--help" or "-h" you return a help dispatch instead of removing the flag—e.g.
return { kind: "oclif", commandId: "sandbox:share", args: [sandboxName,
"--help"] } (or "-h"), otherwise keep existing behavior for real subcommands;
update the case "share" block around the shareSub/shareArgs variables so
ShareCommand.run() receives the help flag.
---
Outside diff comments:
In `@src/lib/command-registry.ts`:
- Around line 176-192: The COMMANDS registry entries for the "upgrade-sandboxes"
and "gc" commands are still using the old flag syntax; locate the COMMANDS array
entries whose usage strings are "nemoclaw upgrade-sandboxes" and "nemoclaw gc"
and update their flags fields to include the new short alias -y (e.g. change
flags to include "[--yes|-y|--force]" or "[--yes|-y]" as appropriate) so the
help/docs match the runtime flag aliases used elsewhere.
🪄 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: 12254678-1400-43b8-9723-21e875b0149c
📒 Files selected for processing (9)
src/lib/command-registry.tssrc/lib/destroy-cli-command.tssrc/lib/legacy-oclif-dispatch.tssrc/lib/maintenance-cli-commands.tssrc/lib/oclif-commands.tssrc/lib/rebuild-cli-command.tssrc/lib/share-command.test.tssrc/lib/share-command.tstest/cli.test.ts
Collaborator
Author
|
Correcting my previous comment (shell ate the backticks). Addressed the requested share help regression in
Validation run locally:
|
ericksoa
approved these changes
May 5, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed current head b492934. The share-help blocker from my prior review is fixed: share --help and share mount|unmount|status --help now route through public sandbox-scoped help instead of leaking sandbox:share* internals, with CLI coverage added.
Local validation on this head:
npm run build:clipassed.npx vitest run src/lib/legacy-oclif-dispatch.test.ts src/lib/share-command.test.ts test/cli.test.ts -t "share|maintenance command help"passed.- Manual probes for
alpha share --help,alpha share mount --help,alpha share unmount --help, andalpha share status --helpall exit 0 and print public usage.
CodeRabbit's remaining registry-sync note about root help not advertising the new global -y aliases is valid but non-blocking for this share-command split: command-specific help and runtime parsing are correct.
cv
added a commit
that referenced
this pull request
May 5, 2026
## Summary Move `debug` flag parsing into its oclif adapter so unknown flags and missing flag values fail through the parser before diagnostic collection. The debug action helpers remain available for existing unit tests and now also accept already-parsed options. ## Stack Navigation - Position: 20 of 60 - Previous PR: [#2911 — refactor(cli): split share into oclif subcommands](#2911) - Next PR: [#2913 — refactor(cli): model onboard flags with oclif](#2913) ## Changes - Modeled `--quick`/`-q`, `--output`/`-o`, and `--sandbox` as oclif flags. - Added debug command examples. - Added `runDebugCommandWithOptions` for adapters that already have parsed options. - Updated CLI and unit tests for oclif-owned parse errors and parsed-option execution. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added sandbox diagnostics (`doctor` command) to troubleshoot sandbox and gateway health. * Enhanced logs command with `--tail`, `--since`, and `--follow` options. * Expanded skill installation workflow for sandbox customization. * **Improvements** * Added CLI usage examples and shorthand flags (e.g., `-y`, `-v`) across commands. * Improved command validation and error messaging for required arguments. * Enhanced sandbox lifecycle management and recovery workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
This was referenced May 5, 2026
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
4 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
Split the sandbox share command into explicit oclif subcommands for mount, unmount, and status. This keeps the public
nemoclaw <name> share ...syntax stable while moving subcommand routing and unknown-subcommand handling into oclif.Stack Navigation
Changes
sandbox:share,sandbox:share:mount,sandbox:share:unmount, andsandbox:share:statuscommand adapters.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
-yshort flag for--yesacross destroy, rebuild, upgrade, and garbage-collection; rebuild docs also show-v/--verbose.sandbox:doctorcommand and added CLI usage examples for several commands.Bug Fixes
Tests