Skip to content

refactor(cli): require skill install path in oclif - #2909

Merged
cv merged 18 commits into
mainfrom
refactor/oclif-ux-skill-install
May 5, 2026
Merged

refactor(cli): require skill install path in oclif#2909
cv merged 18 commits into
mainfrom
refactor/oclif-ux-skill-install

Conversation

@cv

@cv cv commented May 3, 2026

Copy link
Copy Markdown
Collaborator

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

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

  • 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

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • 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 (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Carlos Villela cvillela@nvidia.com

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.

@cv cv self-assigned this May 3, 2026
@copy-pr-bot

copy-pr-bot Bot commented May 3, 2026

Copy link
Copy Markdown

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.

@coderabbitai

coderabbitai Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c4681afe-6e18-46ca-a895-ce926327c35f

📥 Commits

Reviewing files that changed from the base of the PR and between 0d14dca and 1daea70.

📒 Files selected for processing (3)
  • src/lib/skill-install-cli-command.test.ts
  • src/lib/skill-install-cli-command.ts
  • test/cli.test.ts

📝 Walkthrough

Walkthrough

The SkillInstallCliCommand now requires the path argument (previously optional), rejects execution when omitted, and always passes the path to the underlying install function. Tests and CLI integration verify this requirement is enforced.

Changes

Skill Install Path Requirement

Layer / File(s) Summary
Argument Requirement
src/lib/skill-install-cli-command.ts
The path argument in SkillInstallCliCommand is changed from required: false to required: true, ensuring validation rejects missing paths.
Core Implementation
src/lib/skill-install-cli-command.ts
The run() method now unconditionally passes ["install", args.skillPath] to sandboxSkillInstall() instead of conditionally passing ["install"] when the path was absent.
Command Examples
src/lib/skill-install-cli-command.ts
SkillCliCommand and SkillInstallCliCommand add static examples entries demonstrating sandbox:skill install usage with required paths (./my-skill and ./my-skill/SKILL.md).
Unit Tests
src/lib/skill-install-cli-command.test.ts
Test updated to assert that run(["alpha"], rootDir) rejects with a path-related error and does not invoke sandboxSkillInstall.
CLI Integration Tests
test/cli.test.ts
New test case verifies alpha skill install (without a path) exits with a non-zero code and error output mentioning "path".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A path now stands required, no longer a choice,
Where skills once installed without voice,
Now firmly demand their destination be clear—
No more silent skipping, the validation is here! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: making the skill install path argument required within the oclif framework.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/oclif-ux-skill-install

Comment @coderabbitai help to get the list of available commands and usage tips.

@cv cv added the v0.0.34 label May 4, 2026
@cv
cv requested a review from prekshivyas May 4, 2026 22:42
cv added a commit that referenced this pull request May 5, 2026
## 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
- Position: 16 of 60
- Previous PR: [#2907 — refactor(cli): tighten policy and channel parser
validation](#2907)
- Next PR: [#2909 — refactor(cli): require skill install path in
oclif](#2909)

## Changes
- Added examples for the snapshot parent plus `create`, `list`, and
`restore` subcommands.
- Made the parent `sandbox:snapshot` command strict and sandbox-arg
aware.
- Routed `nemoclaw <name> snapshot --help` through the parent adapter
while preserving public usage text.
- Added adapter and CLI coverage for parent usage and unknown snapshot
subcommands.

## 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 and clearer help documentation for snapshot
commands.

* **Bug Fixes**
* Help flag now shows snapshot usage instead of being treated as an
argument.
* Invalid snapshot subcommands are rejected earlier (extra/unknown args
now error).
* Installer: tightened non-interactive check to fail fast when stdin is
not a TTY.

* **Tests**
* Added tests verifying snapshot help display and invalid-subcommand
error handling.
<!-- 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>
Base automatically changed from refactor/oclif-ux-snapshot-commands to main May 5, 2026 00:11
@cv
cv marked this pull request as ready for review May 5, 2026 00:12
@cv
cv enabled auto-merge (squash) May 5, 2026 00:12
Signed-off-by: Carlos Villela <cvillela@nvidia.com>

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tiny, well-tested tightening — 3 files / +20 / -8.

SkillInstallCliCommand.skillPath flips to required: true; oclif now rejects <name> skill install (no path) at parse time before the action runs. run() simplifies accordingly. The PR description explicitly preserves filesystem + OpenClaw-plugin-shape detection in the action layer for the path-provided-but-invalid case — the friendly remediation text stays.

Tests: unit test renamed and rewritten to assert the rejection (/path/i thrown, action not called); new CLI integration test in test/cli.test.ts:797-805 asserts non-zero exit + "path" in output. Examples added to parent SkillCliCommand and SkillInstallCliCommand.

CI: pr.yaml mostly green (lint/dco/check-hash/legacy-path-guard/changes PASS); pr-self-hosted builds and macos/wsl-e2e + checks still in flight at review time. No failures.

Scope hygiene clean — no drive-by drift this round. src/nemoclaw.ts untouched.

@cv
cv merged commit fe699e7 into main May 5, 2026
15 checks passed
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
cv requested a review from cjagwani May 5, 2026 00:27
@wscurran wscurran added the refactor PR restructures code without intended behavior change label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor PR restructures code without intended behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants