refactor(cli): improve snapshot command metadata - #2908
Conversation
|
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. |
|
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 (1)
📝 WalkthroughWalkthroughSnapshot CLI was converted to a strict oclif command with explicit args parsing and examples; help flags are intercepted and not forwarded as positional snapshot subcommand args. Tests were added for the new parsing/dispatch behavior. The installer TTY acceptance check was tightened. ChangesSnapshot Command Flow
Installer TTY Acceptance
sequenceDiagram
participant User as CLI user
participant Oclif as oclif command
participant Dispatcher as legacy oclif dispatch
participant Sandbox as sandbox:snapshot action
User->>Oclif: run "alpha snapshot [--help|subcmd]"
Oclif->>Dispatcher: resolve dispatch for snapshot
alt no subcmd or --help/-h
Dispatcher->>Sandbox: oclif dispatch with args [sandboxName]
else explicit subcommand (e.g., create/list/restore)
Dispatcher->>Sandbox: oclif dispatch with subcommand handling
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 docstrings
🧪 Generate unit tests (beta)
Comment |
## Summary Tighten oclif-owned validation and help metadata for policy and messaging channel mutation commands. This moves missing custom policy path and channel-name validation into strict command parsing before action dispatch. ## Stack Navigation - Position: 15 of 60 - Previous PR: [#2906 — refactor(cli): improve sandbox diagnostic command metadata](#2906) - Next PR: [#2908 — refactor(cli): improve snapshot command metadata](#2908) ## Changes - Added examples for `policy-add`, `policy-remove`, and channel add/remove/start/stop commands. - Removed the hidden raw `policy-add` adapter so missing `--from-file` and `--from-dir` values are handled by oclif. - Made channel mutation commands require a `<channel>` arg before dispatch. - Updated command registry metadata for channel mutation arguments. - Added unit and CLI coverage for missing parser-owned values. ## 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 usage examples to channel and policy mutation command help documentation. * **Bug Fixes** * Channel mutation commands now enforce the channel argument as required. * Policy add command validates that `--from-file` flag includes a path value. * **Chores** * Removed legacy command variant for policy operations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/install.sh`:
- Around line 1628-1630: The current guard incorrectly rejects piped interactive
installs by requiring stdin to be a TTY; update the conditional around
NON_INTERACTIVE to allow cases where /dev/tty exists (the documented fallback)
so interactive piped installs can prompt. Concretely, change the if that checks
[ "${NON_INTERACTIVE:-}" != "1" ] && [ ! -t 0 ] to also allow when /dev/tty is
present (e.g. [ ! -t 0 ] && [ ! -e /dev/tty ]), leaving the existing error
message and references to --yes-i-accept-third-party-software /
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE untouched; this ensures code paths that
open /dev/tty for prompts remain reachable during curl ... | bash flows.
🪄 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: 6941f1b6-3dca-4ac2-83eb-334d1937af11
📒 Files selected for processing (1)
scripts/install.sh
| if [ "${NON_INTERACTIVE:-}" != "1" ] && [ ! -t 0 ]; then | ||
| error "Interactive third-party software acceptance requires a TTY. Re-run in a terminal or pass --yes-i-accept-third-party-software (or set NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1)." | ||
| fi |
There was a problem hiding this comment.
Piped install now hard-fails against the documented default flow
At Line 1628, this condition rejects all curl ... | bash runs unless NON_INTERACTIVE=1 (or equivalent) is pre-set. That conflicts with the usage text at Line 483-484 and effectively makes the existing /dev/tty fallback paths unreachable for interactive piped installs.
Suggested minimal fix
- if [ "${NON_INTERACTIVE:-}" != "1" ] && [ ! -t 0 ]; then
- error "Interactive third-party software acceptance requires a TTY. Re-run in a terminal or pass --yes-i-accept-third-party-software (or set NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1)."
- fi
+ if [ "${NON_INTERACTIVE:-}" != "1" ] && [ ! -t 0 ]; then
+ if { exec 3</dev/tty; } 2>/dev/null; then
+ exec 3<&-
+ else
+ error "Interactive third-party software acceptance requires a TTY. Re-run in a terminal or pass --yes-i-accept-third-party-software (or set NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1)."
+ fi
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "${NON_INTERACTIVE:-}" != "1" ] && [ ! -t 0 ]; then | |
| error "Interactive third-party software acceptance requires a TTY. Re-run in a terminal or pass --yes-i-accept-third-party-software (or set NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1)." | |
| fi | |
| if [ "${NON_INTERACTIVE:-}" != "1" ] && [ ! -t 0 ]; then | |
| if { exec 3</dev/tty; } 2>/dev/null; then | |
| exec 3<&- | |
| else | |
| error "Interactive third-party software acceptance requires a TTY. Re-run in a terminal or pass --yes-i-accept-third-party-software (or set NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1)." | |
| fi | |
| fi |
🤖 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 `@scripts/install.sh` around lines 1628 - 1630, The current guard incorrectly
rejects piped interactive installs by requiring stdin to be a TTY; update the
conditional around NON_INTERACTIVE to allow cases where /dev/tty exists (the
documented fallback) so interactive piped installs can prompt. Concretely,
change the if that checks [ "${NON_INTERACTIVE:-}" != "1" ] && [ ! -t 0 ] to
also allow when /dev/tty is present (e.g. [ ! -t 0 ] && [ ! -e /dev/tty ]),
leaving the existing error message and references to
--yes-i-accept-third-party-software / NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE
untouched; this ensures code paths that open /dev/tty for prompts remain
reachable during curl ... | bash flows.
prekshivyas
left a comment
There was a problem hiding this comment.
LGTM after rebase. Snapshot work is clean (5 files / +67 / -13).
SnapshotCommandparent flipsstrict=false→strict=truewith explicitArgs.stringfor sandboxName, replacing the hand-rolledif (!sandboxName)error.<name> snapshot bogusnow rejected withUnexpected argument: bogusinstead of silently dispatched. Tested.legacy-oclif-dispatch.tsadds a help intercept routingsnapshot --help(and baresnapshot) through the parent adapter, preserving public help text without leakingsandbox:snapshotcommand-id. Tested.- Examples added to all four snapshot commands.
One scope-drift nit (non-blocking but worth flagging for description hygiene):
scripts/install.sh removes the /dev/tty fallback from the license-acceptance check. Was: skip license error if NON_INTERACTIVE=1 OR stdin TTY OR /dev/tty openable. Now: skip only if NON_INTERACTIVE=1 OR stdin TTY. The inline comment explains the rationale (curl|bash mode leaves partial install on decline) and installer-hash-check PASSed, so the hash file was updated. But the change isn't mentioned in the human-written PR body — only CodeRabbit's auto-summary surfaced it. Consider pulling that line into the description before merge so the squash-merge commit message reflects what landed. No new test for the fail-fast path; if any user is on a curl|bash flow that relied on /dev/tty fallback, this will now fail fast (which is the intended fix per the comment).
CI: pr.yaml mostly green; pr-self-hosted builds and wsl-e2e/macos-e2e/checks still in flight at review time. src/nemoclaw.ts untouched — cumulative ~16-orphan debt unchanged.
## Summary Move missing `skill install <path>` validation into the oclif adapter while keeping plugin-shape detection in the skill install action. This also adds help examples for the parent skill command and direct install command. ## Stack Navigation - Position: 17 of 60 - Previous PR: [#2908 — refactor(cli): improve snapshot command metadata](#2908) - Next PR: [#2910 — refactor(cli): add lifecycle confirmation flag aliases](#2910) ## Changes - Added examples for `skill` and `skill install` command metadata. - Made `sandbox:skill:install` require a skill directory or `SKILL.md` path before dispatch. - Kept filesystem and OpenClaw plugin detection in the action layer for friendly remediation text. - Updated CLI/unit coverage for missing install paths. ## 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 * **Documentation** * Added usage examples for the `skill install` command, showing how to install a skill package from a directory or SKILL.md file. * **Bug Fixes** * The `skill install` command now enforces the requirement for a path argument. The command previously allowed invocation without specifying a path. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
Improve the oclif shape for sandbox snapshot commands by adding examples and making the parent command a strict adapter. This keeps public snapshot help stable while allowing unknown snapshot subcommands to fail before reaching the snapshot action.
Stack Navigation
Changes
create,list, andrestoresubcommands.sandbox:snapshotcommand strict and sandbox-arg aware.nemoclaw <name> snapshot --helpthrough the parent adapter while preserving public usage text.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
Bug Fixes
Tests