fix(skill): clarify plugin install errors (Fixes #2536) - #2585
Conversation
|
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)
📝 WalkthroughWalkthroughRefactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/cli.test.ts (1)
173-189: Add one test for theopenclaw.plugin.jsonmarker path.You already validate package.json metadata detection; adding the file-marker case would lock down both supported plugin-shape signals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 173 - 189, Add a second assertion path that exercises the openclaw.plugin.json marker: create a file named "openclaw.plugin.json" inside the test's pluginDir (e.g., using fs.writeFileSync) with minimal valid JSON to mark the directory as an OpenClaw plugin, then call runWithEnv the same way and assert the same failure code and output messages; you can either extend the existing "points plugin-shaped directories away from skill install" test to write that marker file in addition to package.json or create a new test using the same helpers (runWithEnv, writeSandboxRegistry) and the same expected messages.src/nemoclaw.ts (1)
2204-2217: Tighten plugin-shape detection to reduce false positives.
Boolean(packageJson?.openclaw)may classify non-plugin packages as plugins. Consider checking explicit plugin markers/shape inpackage.jsoninstead of anyopenclawobject.Suggested refinement
function looksLikeOpenClawPlugin(candidatePath: string): boolean { @@ const packageJsonPath = path.join(dir, "package.json"); if (!fs.existsSync(packageJsonPath)) return false; try { const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, "utf-8")); - return Boolean(packageJson?.openclaw); + const openclawBlock = packageJson?.openclaw; + return Boolean( + openclawBlock?.plugin || + packageJson?.["openclaw.plugin"] || + (Array.isArray(openclawBlock?.extensions) && openclawBlock.extensions.length > 0), + ); } catch { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 2204 - 2217, The current looksLikeOpenClawPlugin function uses Boolean(packageJson?.openclaw) which is too permissive; update the package.json check inside looksLikeOpenClawPlugin to detect an explicit plugin shape (e.g., require packageJson.openclaw to be an object with specific keys or a boolean flag) rather than any truthy value—for example ensure typeof packageJson.openclaw === "object" and it contains required markers like "plugin": true or "entry"/"main" (or require openclaw === true) and return true only when those explicit markers exist; modify the code that reads packageJson in looksLikeOpenClawPlugin to perform this stricter validation and fall back to false otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/nemoclaw.ts`:
- Around line 2204-2217: The current looksLikeOpenClawPlugin function uses
Boolean(packageJson?.openclaw) which is too permissive; update the package.json
check inside looksLikeOpenClawPlugin to detect an explicit plugin shape (e.g.,
require packageJson.openclaw to be an object with specific keys or a boolean
flag) rather than any truthy value—for example ensure typeof
packageJson.openclaw === "object" and it contains required markers like
"plugin": true or "entry"/"main" (or require openclaw === true) and return true
only when those explicit markers exist; modify the code that reads packageJson
in looksLikeOpenClawPlugin to perform this stricter validation and fall back to
false otherwise.
In `@test/cli.test.ts`:
- Around line 173-189: Add a second assertion path that exercises the
openclaw.plugin.json marker: create a file named "openclaw.plugin.json" inside
the test's pluginDir (e.g., using fs.writeFileSync) with minimal valid JSON to
mark the directory as an OpenClaw plugin, then call runWithEnv the same way and
assert the same failure code and output messages; you can either extend the
existing "points plugin-shaped directories away from skill install" test to
write that marker file in addition to package.json or create a new test using
the same helpers (runWithEnv, writeSandboxRegistry) and the same expected
messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f59ebb9-54aa-4aa6-bbad-fa339107afd7
📒 Files selected for processing (2)
src/nemoclaw.tstest/cli.test.ts
Fixes NVIDIA#2536 Signed-off-by: Deepak Jain <deepujain@gmail.com>
025b3c6 to
51e4b72
Compare
|
Rebased on latest main. Tightened plugin detection to explicit OpenClaw markers and added coverage for openclaw.plugin.json. npm run build:cli and npm test -- test/cli.test.ts pass. |
|
|
## Summary `nemoclaw <sandbox> skill install --help` was treated like a path, and plugin-shaped directories only got a generic missing SKILL.md error. This PR makes the help path work and gives plugin users a clearer next step. ## Changes - Print skill install usage when `--help`, `-h`, or `help` follows `install`. - Detect OpenClaw plugin-shaped directories through `openclaw.plugin.json` or `package.json` metadata. - Add a targeted hint that plugins should be baked into a custom sandbox image with `nemoclaw onboard --from`. - Add CLI tests for both flows. ## Testing - `npm run build:cli` passed. - `npm run typecheck:cli` passed. - `npm test -- test/cli.test.ts` passed: 61 tests. - Full `npm test -- --reporter=dot` was attempted. In this local checkout it still fails outside this change in installer/uninstall/onboard/build-context tests, including temp-source generated-dist lookup and a few timeout/status checks. ## Evidence it works The CLI test now verifies that `skill install --help` returns usage without a missing-file error and that plugin-shaped directories get the OpenClaw plugin hint. Fixes #2536 Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error detection and messaging for `skill install` when given plugin-shaped directories, and added a clear suggestion to use the sandbox onboarding workflow instead. * Enhanced `--help` handling to display skill-install usage immediately, including when `--help` is passed as the positional path. * **Tests** * Expanded CLI tests for `skill install` help and plugin-detection scenarios, and restored related start-test coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Deepak Jain <deepujain@gmail.com>
## Summary Refreshes the 0.0.29 documentation for user-facing changes merged in the past 24 hours. Version metadata stays on `0.0.29`. ## Changes - `docs/get-started/quickstart.md`, `docs/reference/commands.md`, and `docs/reference/troubleshooting.md`: Document dashboard port auto-allocation, `--control-ui-port`, and `nemoclaw list` dashboard URL output from [#2411](#2411). - `docs/inference/inference-options.md` and `docs/inference/switch-inference-providers.md`: Document local Ollama and local vLLM credential isolation from `OPENAI_API_KEY` from [#2580](#2580). - `docs/inference/inference-options.md`: Document Local NVIDIA NIM validation behavior from [#2505](#2505). - `docs/reference/commands.md`: Document the cloud-only NIM status display behavior from [#2622](#2622). - `docs/deployment/deploy-to-remote-gpu.md`: Clarify runtime propagation for `NEMOCLAW_PROXY_HOST` and `NEMOCLAW_PROXY_PORT` from [#2581](#2581). - `docs/workspace/backup-restore.md`: Document snapshot restore symlink handling for sandbox data paths from [#2488](#2488). - `docs/reference/commands.md`: Document `skill install --help` and OpenClaw plugin-shaped directory guidance from [#2585](#2585). ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [x] `make docs` builds without warnings (doc changes only) - [x] 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) ## AI Disclosure - [x] AI-assisted — tool: Codex --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added `--control-ui-port` flag for explicit dashboard port control * Implemented automatic port selection (18789–18799) when the default port is occupied * Clarified that local inference routes (Ollama, local vLLM) don't require `OPENAI_API_KEY` * Improved dashboard URL display in list and status commands * Enhanced symlink handling in workspace backup restoration * Updated multi-sandbox quickstart and troubleshooting guidance <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…#2585) ## Summary `nemoclaw <sandbox> skill install --help` was treated like a path, and plugin-shaped directories only got a generic missing SKILL.md error. This PR makes the help path work and gives plugin users a clearer next step. ## Changes - Print skill install usage when `--help`, `-h`, or `help` follows `install`. - Detect OpenClaw plugin-shaped directories through `openclaw.plugin.json` or `package.json` metadata. - Add a targeted hint that plugins should be baked into a custom sandbox image with `nemoclaw onboard --from`. - Add CLI tests for both flows. ## Testing - `npm run build:cli` passed. - `npm run typecheck:cli` passed. - `npm test -- test/cli.test.ts` passed: 61 tests. - Full `npm test -- --reporter=dot` was attempted. In this local checkout it still fails outside this change in installer/uninstall/onboard/build-context tests, including temp-source generated-dist lookup and a few timeout/status checks. ## Evidence it works The CLI test now verifies that `skill install --help` returns usage without a missing-file error and that plugin-shaped directories get the OpenClaw plugin hint. Fixes NVIDIA#2536 Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error detection and messaging for `skill install` when given plugin-shaped directories, and added a clear suggestion to use the sandbox onboarding workflow instead. * Enhanced `--help` handling to display skill-install usage immediately, including when `--help` is passed as the positional path. * **Tests** * Expanded CLI tests for `skill install` help and plugin-detection scenarios, and restored related start-test coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Deepak Jain <deepujain@gmail.com>
## Summary Refreshes the 0.0.29 documentation for user-facing changes merged in the past 24 hours. Version metadata stays on `0.0.29`. ## Changes - `docs/get-started/quickstart.md`, `docs/reference/commands.md`, and `docs/reference/troubleshooting.md`: Document dashboard port auto-allocation, `--control-ui-port`, and `nemoclaw list` dashboard URL output from [NVIDIA#2411](NVIDIA#2411). - `docs/inference/inference-options.md` and `docs/inference/switch-inference-providers.md`: Document local Ollama and local vLLM credential isolation from `OPENAI_API_KEY` from [NVIDIA#2580](NVIDIA#2580). - `docs/inference/inference-options.md`: Document Local NVIDIA NIM validation behavior from [NVIDIA#2505](NVIDIA#2505). - `docs/reference/commands.md`: Document the cloud-only NIM status display behavior from [NVIDIA#2622](NVIDIA#2622). - `docs/deployment/deploy-to-remote-gpu.md`: Clarify runtime propagation for `NEMOCLAW_PROXY_HOST` and `NEMOCLAW_PROXY_PORT` from [NVIDIA#2581](NVIDIA#2581). - `docs/workspace/backup-restore.md`: Document snapshot restore symlink handling for sandbox data paths from [NVIDIA#2488](NVIDIA#2488). - `docs/reference/commands.md`: Document `skill install --help` and OpenClaw plugin-shaped directory guidance from [NVIDIA#2585](NVIDIA#2585). ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [x] `make docs` builds without warnings (doc changes only) - [x] 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) ## AI Disclosure - [x] AI-assisted — tool: Codex --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added `--control-ui-port` flag for explicit dashboard port control * Implemented automatic port selection (18789–18799) when the default port is occupied * Clarified that local inference routes (Ollama, local vLLM) don't require `OPENAI_API_KEY` * Improved dashboard URL display in list and status commands * Enhanced symlink handling in workspace backup restoration * Updated multi-sandbox quickstart and troubleshooting guidance <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
nemoclaw <sandbox> skill install --helpwas treated like a path, and plugin-shaped directories only got a generic missing SKILL.md error. This PR makes the help path work and gives plugin users a clearer next step.Changes
--help,-h, orhelpfollowsinstall.openclaw.plugin.jsonorpackage.jsonmetadata.nemoclaw onboard --from.Testing
npm run build:clipassed.npm run typecheck:clipassed.npm test -- test/cli.test.tspassed: 61 tests.npm test -- --reporter=dotwas attempted. In this local checkout it still fails outside this change in installer/uninstall/onboard/build-context tests, including temp-source generated-dist lookup and a few timeout/status checks.Evidence it works
The CLI test now verifies that
skill install --helpreturns usage without a missing-file error and that plugin-shaped directories get the OpenClaw plugin hint.Fixes #2536
Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
Bug Fixes
skill installwhen given plugin-shaped directories, and added a clear suggestion to use the sandbox onboarding workflow instead.--helphandling to display skill-install usage immediately, including when--helpis passed as the positional path.Tests
skill installhelp and plugin-detection scenarios, and restored related start-test coverage.