feat(cli): add nemoclaw <sandbox> skill remove command#1962
Open
sinha-abhash wants to merge 6 commits intoNVIDIA:mainfrom
Open
feat(cli): add nemoclaw <sandbox> skill remove command#1962sinha-abhash wants to merge 6 commits intoNVIDIA:mainfrom
sinha-abhash wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
Adds `nemoclaw <sandbox> skill remove <name>` to complement the existing `skill install` command, providing a complete skill lifecycle from the CLI. ## What this does - Removes the skill from the immutable upload dir: /sandbox/.openclaw/skills/<name>/ - Removes the OpenClaw mirror dir: $HOME/.openclaw/skills/<name>/ - Clears sessions.json so the agent re-discovers remaining skills on the next session (same mechanism as a fresh install) - Verifies the directory is gone after removal - Rejects invalid/unsafe skill names before any SSH operation - Prints a clear error when the named skill is not installed ## Changes src/lib/skill-install.ts - Updated module-level comment to mention skill remove - Added removeSkill() - removes upload dir, mirror dir, clears sessions - Added verifyRemove() - confirms directory is gone post-removal - Added validateSkillName() - rejects shell metacharacters in CLI input src/nemoclaw.ts - Extended sandboxSkillInstall() to dispatch install vs remove - Updated --help output to document both subcommands - Updated global help text to list skill remove - Validates skill name with validateSkillName() before any SSH ops - Checks skill exists before attempting removal (clear error if missing) - SSH config temp file always cleaned up in finally block src/lib/skill-install.test.ts - Added describe(validateSkillName) - valid names, empty, spaces, metacharacters - Added describe(removeSkill) - SSH-failure path returns success=false + warning - Added describe(verifyRemove) - SSH-failure returns false (conservative) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughAdds skill removal APIs and CLI handling; tightens skill-name validation to reject Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Nemoclaw as nemoclaw.ts
participant Agent as agentRuntime
participant SkillInstall as skill-install
participant SSH as SSH/RemoteFS
User->>Nemoclaw: sandbox skill remove <name>
Nemoclaw->>Nemoclaw: validateSkillName(name)
Nemoclaw->>Nemoclaw: ensureLiveSandboxOrExit()
Nemoclaw->>Agent: getSessionAgent(sandbox)
Agent-->>Nemoclaw: session + ctx
Nemoclaw->>SkillInstall: resolveSkillPaths(name)
SkillInstall-->>Nemoclaw: paths
Nemoclaw->>Nemoclaw: write temp SSH config (mode 0o600)
Nemoclaw->>SkillInstall: checkExisting(ctx, paths)
SkillInstall->>SSH: stat uploadDir/SKILL.md and mirrorDir/SKILL.md (if OpenClaw)
SSH-->>SkillInstall: exists / failure (-> null)
SkillInstall-->>Nemoclaw: boolean|null
Nemoclaw->>SkillInstall: removeSkill(ctx, paths)
SkillInstall->>SSH: rm -rf uploadDir (and mirrorDir if applicable), clear sessions or emit hint
SSH-->>SkillInstall: results + warnings
SkillInstall-->>Nemoclaw: RemoveResult (messages, success)
Nemoclaw->>SkillInstall: verifyRemove(ctx, paths)
SkillInstall->>SSH: stat for absence of uploadDir and mirrorDir
SSH-->>SkillInstall: absent|present
Nemoclaw->>Nemoclaw: delete temp SSH config
Nemoclaw-->>User: print messages and success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![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: 2
🧹 Nitpick comments (1)
src/nemoclaw.ts (1)
1340-1568: Splitskill installandskill removeinto dedicated handlers.
sandboxSkillInstall()now owns subcommand help, argument validation, two SSH-config lifecycles, and both install/remove workflows. That pushes this dispatcher well past the repo's complexity target and makes the next skill subcommand harder to change safely. Extract the remove flow and install flow into separate helpers, and leave this function to parse the subcommand and delegate.As per coding guidelines,
Limit cyclomatic complexity to 20 in JavaScript/TypeScript files, with target of 15.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1340 - 1568, Current sandboxSkillInstall is doing parsing plus both install and remove workflows; extract the remove and install workflows into two small helpers (e.g. handleSkillRemove(ctxNameOrSandbox, args) and handleSkillInstall(ctxNameOrSandbox, args) or similar) and leave sandboxSkillInstall to only parse subcommand, validate arguments, and delegate. Move the SSH-config lifecycle (captureOpenshell(...), tmpSshConfig creation, fs.writeFileSync, and fs.unlinkSync cleanup) into the helpers so each has its own try/finally and uses the same ctx shape ({ configFile, sandboxName }); move code that validates skillName (skillInstall.validateSkillName), checkExisting/remove/verifyRemove into the remove helper (use agentRuntime.getSessionAgent and skillInstall.resolveSkillPaths there) and move frontmatter parsing, collectFiles, uploadDirectory, postInstall, and verifyInstall into the install helper (again resolving agent and paths inside it). Ensure sandboxSkillInstall still performs top-level argument/usage printing and only calls the appropriate helper with sandboxName and the remaining args.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/skill-install.ts`:
- Around line 423-428: validateSkillName currently allows "." and ".." because
the regex matches them; update validateSkillName(name: string) to explicitly
reject a single "." and a double ".." (e.g., check name !== "." && name !==
"..") while keeping the existing /^[A-Za-z0-9._-]+$/ and length > 0 checks so
behavior remains the same for all other names; also ensure this change is
mirrored where the earlier frontmatter/skill-name validation is implemented so
both checks stay aligned.
- Around line 379-420: The removal currently reports success and verifyRemove()
checks only paths.uploadDir so if paths.mirrorDir (OpenClaw mirror) wasn't
removed the CLI can lie; update the removal return value and verification to
require the mirror deletion when paths.isOpenClaw is true: in the remover
function make success combine removedUploadDir && (if paths.isOpenClaw ?
removedMirrorDir : true) and in verifyRemove(ctx, paths) have sshExec test both
shellQuote(paths.uploadDir) and, when paths.isOpenClaw && paths.mirrorDir,
shellQuote(paths.mirrorDir) (both must be GONE); also update the existence check
logic used by checkExisting() to probe the mirror SKILL.md
(paths.mirrorDir/SKILL.md) in addition to uploadDir/SKILL.md so retries won't be
blocked.
---
Nitpick comments:
In `@src/nemoclaw.ts`:
- Around line 1340-1568: Current sandboxSkillInstall is doing parsing plus both
install and remove workflows; extract the remove and install workflows into two
small helpers (e.g. handleSkillRemove(ctxNameOrSandbox, args) and
handleSkillInstall(ctxNameOrSandbox, args) or similar) and leave
sandboxSkillInstall to only parse subcommand, validate arguments, and delegate.
Move the SSH-config lifecycle (captureOpenshell(...), tmpSshConfig creation,
fs.writeFileSync, and fs.unlinkSync cleanup) into the helpers so each has its
own try/finally and uses the same ctx shape ({ configFile, sandboxName }); move
code that validates skillName (skillInstall.validateSkillName),
checkExisting/remove/verifyRemove into the remove helper (use
agentRuntime.getSessionAgent and skillInstall.resolveSkillPaths there) and move
frontmatter parsing, collectFiles, uploadDirectory, postInstall, and
verifyInstall into the install helper (again resolving agent and paths inside
it). Ensure sandboxSkillInstall still performs top-level argument/usage printing
and only calls the appropriate helper with sandboxName and the remaining args.
🪄 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: Pro Plus
Run ID: 3f679918-19f5-4eff-b916-7665b1b7d101
📒 Files selected for processing (3)
src/lib/skill-install.test.tssrc/lib/skill-install.tssrc/nemoclaw.ts
wscurran
added
NemoClaw CLI
Contributor
|
✨ Thanks for submitting this PR that proposes an enhancement to add a skill remove command, which could help improve the user experience and provide a complete skill lifecycle from the CLI. Possibly related open PRs: Possibly related open issues: |
… parseFrontmatter Both functions accepted single-dot and double-dot names because the regex /^[A-Za-z0-9._-]+$/ allows '.'. In the remove path this would expand to rm -rf '/sandbox/.openclaw/skills/..' which deletes the parent .openclaw or skills/ directory instead of a single skill. Fix: add explicit name !== '.' && name !== '..' guards in both validateSkillName (CLI input) and parseFrontmatter (SKILL.md name field), keeping the two checks aligned as CodeRabbit flagged. Add test cases for '.' and '..' in both validateSkillName and parseFrontmatter describe blocks. All 33 skill-install tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/skill-install.ts (1)
405-420:⚠️ Potential issue | 🟠 MajorRequire mirror deletion for OpenClaw before reporting removal success.
On Line [406],
successignores mirror deletion; and Lines [417-420] verify onlyuploadDir. In OpenClaw mode, this can report “removed” while$HOME/.openclaw/skills/<name>still exists.Proposed fix
export function removeSkill( ctx: SshContext, paths: SkillPaths, ): RemoveResult { @@ return { - success: removedUploadDir, + success: removedUploadDir && (!paths.isOpenClaw || removedMirrorDir), removedUploadDir, removedMirrorDir, clearedSessions, messages, }; } @@ export function verifyRemove(ctx: SshContext, paths: SkillPaths): boolean { - const target = shellQuote(paths.uploadDir); - const result = sshExec(ctx, `test -e ${target} && echo EXISTS || echo GONE`); + const checks = [`test ! -e ${shellQuote(paths.uploadDir)}`]; + if (paths.isOpenClaw && paths.mirrorDir) { + checks.push(`test ! -e "${paths.mirrorDir}"`); + } + const result = sshExec(ctx, `${checks.join(" && ")} && echo GONE || echo EXISTS`); return result !== null && result.stdout === "GONE"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/skill-install.ts` around lines 405 - 420, The removal success currently ignores mirror deletion; update the code so success requires the mirror to be removed when applicable and make verifyRemove check both upload and mirror dirs: modify the caller that builds the result object to set success = removedUploadDir && (paths.mirrorDir ? removedMirrorDir : true) (use the existing removedUploadDir and removedMirrorDir flags), and change the verifyRemove(ctx: SshContext, paths: SkillPaths) function to shell-quote and test both paths.uploadDir and paths.mirrorDir (if paths.mirrorDir is provided) — return true only when both are gone (i.e., test each with sshExec and ensure both echo GONE).
🧹 Nitpick comments (1)
src/lib/skill-install.ts (1)
66-70: Consolidate skill-name validation into one shared helper.The same rule is duplicated in
parseFrontmatterandvalidateSkillName. Centralizing it reduces drift risk and keeps install/remove behavior consistent.Refactor sketch
+const SKILL_NAME_RE = /^[A-Za-z0-9._-]+$/; +function isValidSkillName(name: string): boolean { + return name.length > 0 && name !== "." && name !== ".." && SKILL_NAME_RE.test(name); +} @@ - if (nameValue === "." || nameValue === ".." || !/^[A-Za-z0-9._-]+$/.test(nameValue)) { + if (!isValidSkillName(nameValue)) { @@ export function validateSkillName(name: string): boolean { - return ( - name.length > 0 && - name !== "." && - name !== ".." && - /^[A-Za-z0-9._-]+$/.test(name) - ); + return isValidSkillName(name); }Also applies to: 427-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/skill-install.ts` around lines 66 - 70, Extract the skill-name validation logic into a single helper (e.g., isValidSkillName or validateSkillNameFormat) and use it from both parseFrontmatter and validateSkillName instead of duplicating the regex and "."/".." checks; implement the helper to return boolean or throw with the same error text (`SKILL.md name '${name}' contains invalid characters. Only [A-Za-z0-9._-] allowed.`) and replace the inline checks in parseFrontmatter and the existing validateSkillName to call the new helper so install/remove behavior is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/skill-install.ts`:
- Around line 405-420: The removal success currently ignores mirror deletion;
update the code so success requires the mirror to be removed when applicable and
make verifyRemove check both upload and mirror dirs: modify the caller that
builds the result object to set success = removedUploadDir && (paths.mirrorDir ?
removedMirrorDir : true) (use the existing removedUploadDir and removedMirrorDir
flags), and change the verifyRemove(ctx: SshContext, paths: SkillPaths) function
to shell-quote and test both paths.uploadDir and paths.mirrorDir (if
paths.mirrorDir is provided) — return true only when both are gone (i.e., test
each with sshExec and ensure both echo GONE).
---
Nitpick comments:
In `@src/lib/skill-install.ts`:
- Around line 66-70: Extract the skill-name validation logic into a single
helper (e.g., isValidSkillName or validateSkillNameFormat) and use it from both
parseFrontmatter and validateSkillName instead of duplicating the regex and
"."/".." checks; implement the helper to return boolean or throw with the same
error text (`SKILL.md name '${name}' contains invalid characters. Only
[A-Za-z0-9._-] allowed.`) and replace the inline checks in parseFrontmatter and
the existing validateSkillName to call the new helper so install/remove behavior
is consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a79c72d-a216-4456-a527-90d7ce0e709b
📒 Files selected for processing (2)
src/lib/skill-install.test.tssrc/lib/skill-install.ts
… OpenClaw mirror - removeSkill(): success now requires both uploadDir and mirrorDir to be removed for OpenClaw sandboxes (previously only checked uploadDir, so a surviving mirror would still show the skill as removed) - verifyRemove(): checks both uploadDir and mirrorDir via a compound SSH test before returning GONE (previously only checked uploadDir) - checkExisting(): probes both uploadDir/SKILL.md and mirrorDir/SKILL.md so a partial removal (uploadDir gone, mirrorDir surviving) no longer blocks a reinstall All 37 skill-install tests pass. Fixes address the Major issue flagged by CodeRabbit on PR NVIDIA#1962. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/skill-install.ts (1)
66-70: Consolidate skill-name validation into a single helper.Line [66] and Line [436] duplicate the same rules. Keeping one source of truth avoids future drift between install-time and CLI-time validation.
♻️ Proposed refactor
+function isValidSkillName(name: string): boolean { + return ( + name.length > 0 && + name !== "." && + name !== ".." && + /^[A-Za-z0-9._-]+$/.test(name) + ); +} + export function parseFrontmatter(content: string): SkillFrontmatter { ... - if (nameValue === "." || nameValue === ".." || !/^[A-Za-z0-9._-]+$/.test(nameValue)) { + if (!isValidSkillName(nameValue)) { throw new Error( `SKILL.md name '${nameValue}' contains invalid characters. Only [A-Za-z0-9._-] allowed.`, ); } ... } ... export function validateSkillName(name: string): boolean { - return ( - name.length > 0 && - name !== "." && - name !== ".." && - /^[A-Za-z0-9._-]+$/.test(name) - ); + return isValidSkillName(name); }Also applies to: 436-442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/skill-install.ts` around lines 66 - 70, Extract the duplicated validation into a single helper called e.g. validateSkillName(name: string) (or isValidSkillName returning boolean) that encapsulates the checks against "." and ".." and the /^[A-Za-z0-9._-]+$/ regex and the error message (`SKILL.md name '${name}' contains invalid characters. Only [A-Za-z0-9._-] allowed.`); replace the inline check in the install flow that uses nameValue and the duplicate check at the CLI side (the block around the other duplicate) to call this helper instead (either throw from validateSkillName or check boolean and throw at the call site) so both install-time and CLI-time use the same validation logic and message.
🤖 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/lib/skill-install.ts`:
- Around line 66-70: Extract the duplicated validation into a single helper
called e.g. validateSkillName(name: string) (or isValidSkillName returning
boolean) that encapsulates the checks against "." and ".." and the
/^[A-Za-z0-9._-]+$/ regex and the error message (`SKILL.md name '${name}'
contains invalid characters. Only [A-Za-z0-9._-] allowed.`); replace the inline
check in the install flow that uses nameValue and the duplicate check at the CLI
side (the block around the other duplicate) to call this helper instead (either
throw from validateSkillName or check boolean and throw at the call site) so
both install-time and CLI-time use the same validation logic and message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 68e25d3e-e4b7-4326-9fe4-4d96807d3b6e
📒 Files selected for processing (2)
src/lib/skill-install.test.tssrc/lib/skill-install.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/skill-install.test.ts
Author
Hi @wscurran, thanks for the review! #1845 and #1844 are related but not duplicates — #1845 (merged) implements skill install, and #1844 is the issue it closed. Skill removal was explicitly listed as out of scope in #1844 (see the "Out of scope" section: "Skill removal or update commands (follow-up)"). This PR is that follow-up, completing the skill lifecycle: Happy to add a reference to #1845 in the PR description to make the relationship clearer if that helps. |
Move validateSkillName before parseFrontmatter so parseFrontmatter can call it instead of duplicating the same dot/double-dot and regex checks inline. Removes the second definition at the bottom of the file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/skill-install.ts`:
- Around line 81-84: The error thrown when validateSkillName(nameValue) fails
should explicitly mention reserved-name restrictions (e.g., "." and "..") as
distinct from invalid character rules; update the message in the thrown Error
inside the SKILL.md name check to state both: allowed characters [A-Za-z0-9._-]
and that names cannot be "." or ".." (or other reserved names if applicable),
referencing validateSkillName and nameValue so maintainers can locate and adjust
the error string accordingly.
🪄 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: Pro Plus
Run ID: 7afe3706-6ea6-456a-a029-99ba7cd28a93
📒 Files selected for processing (1)
src/lib/skill-install.ts
Extract skill remove logic into sandboxSkillRemove() and skill install logic into sandboxSkillDeploy(). sandboxSkillInstall() is now a thin dispatcher that handles help and routes to the two handlers. Each function has a single responsibility. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
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 `@src/nemoclaw.ts`:
- Around line 1380-1382: skillInstall.checkExisting currently conflates “absent”
and “probe failed” (see src/lib/skill-install.ts:349-356); change checkExisting
to return a tri-state/result object (for example {exists: boolean|null, error?:
Error}) or throw on probe failure, and then update the caller here to
distinguish outcomes: if exists === false report the skill is not installed, if
exists === null or an error was returned/logged then surface the probe error
(console.error with the error) and exit (or propagate) instead of printing “not
installed”. Reference: skillInstall.checkExisting, variables ctx, paths,
skillName, sandboxName.
- Around line 1394-1399: The current message assumes verifyRemove succeeded in
proving deletion, but skillInstall.verifyRemove(ctx, paths) can return false on
SSH/verification failure; update the error path where gone is false (the block
using gone, skillName and paths.uploadDir) to log that removal is unverified
rather than asserting the dir still exists—e.g. change the console.error message
to indicate "could not verify removal" and include skillName and paths.uploadDir
(or include the underlying error if available from verifyRemove) so callers know
verification failed rather than deletion was proven to have failed.
🪄 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: Pro Plus
Run ID: 798ae3cd-318f-4d60-800e-b93fbfb611b6
📒 Files selected for processing (1)
src/nemoclaw.ts
- Clarify error message for invalid skill names to mention '.' and '..' - Change checkExisting() to return boolean|null (null = SSH probe failed) to distinguish absent from unreachable sandbox - Update both callers: remove exits with clear error on null; install treats null as fresh install with a warning - Fix verifyRemove failure message to say 'could not be verified' instead of asserting the upload dir still exists Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/lib/skill-install.ts (1)
354-363:⚠️ Potential issue | 🟠 MajorPreserve the tri-state on non-zero SSH exits.
sshExec()only returnsnullwhenspawnSync()throws. A normal SSH transport/auth failure still comes back with a non-zerostatus, so this path still collapses probe failures intofalseinstead ofnull. That means install can still treat an unreachable sandbox as a fresh install, and remove can still say the skill is not installed when the probe never completed.🩹 Proposed fix
export function checkExisting(ctx: SshContext, paths: SkillPaths): boolean | null { const checks = [`test -f ${shellQuote(`${paths.uploadDir}/SKILL.md`)}`]; if (paths.isOpenClaw && paths.mirrorDir) { checks.push(`test -f "${paths.mirrorDir}/SKILL.md"`); } const result = sshExec(ctx, `{ ${checks.join(" || ")}; } && echo EXISTS || echo ABSENT`); - if (result === null) { + if (result === null || result.status !== 0) { return null; } return result.stdout === "EXISTS"; }Verify by comparing
sshExec()andcheckExisting(): if SSH failures can produce a non-zero exit without throwing, this function should map that case tonull.#!/bin/bash sed -n '168,200p' src/lib/skill-install.ts printf '\n---\n' sed -n '344,364p' src/lib/skill-install.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/skill-install.ts` around lines 354 - 363, The checkExisting function currently treats any non-null sshExec() result as a definitive false/true, but sshExec can return a non-zero exit/status for SSH transport or auth failures (not just throw), so update checkExisting (referencing checkExisting, sshExec, SshContext, SkillPaths) to detect sshExec results where result.status !== 0 (or an error indicated in result.stderr) and return null in that case; otherwise keep the existing stdout === "EXISTS" logic. Ensure you surface the tri-state by returning null for SSH failures, true when stdout is "EXISTS", and false otherwise.src/nemoclaw.ts (1)
1353-1356:⚠️ Potential issue | 🟡 MinorMention reserved names in the CLI validation error.
validateSkillName()also rejects.and.., so this message is still misleading for those inputs. Surface the reserved-name rule here too so the CLI explains the actual failure.✏️ Suggested tweak
if (!skillInstall.validateSkillName(skillName)) { console.error(` Invalid skill name: '${skillName}'`); - console.error(" Skill names must match [A-Za-z0-9._-]."); + console.error(" Skill names must use [A-Za-z0-9._-] and cannot be '.' or '..'."); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1353 - 1356, The CLI error for invalid skill names is misleading because validateSkillName(skillName) also rejects the reserved names "." and ".."; update the messages in the block that checks validateSkillName(skillName) so the output mentions both the allowed pattern and the reserved-name rule (e.g., state that "." and ".." are reserved and not allowed), keeping the existing descriptive first line (`Invalid skill name: '${skillName}'`) and replacing the second line to include the regex constraint and the reserved names; locate this logic around the skillInstall.validateSkillName call and adjust the console.error strings accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/skill-install.ts`:
- Around line 354-363: The checkExisting function currently treats any non-null
sshExec() result as a definitive false/true, but sshExec can return a non-zero
exit/status for SSH transport or auth failures (not just throw), so update
checkExisting (referencing checkExisting, sshExec, SshContext, SkillPaths) to
detect sshExec results where result.status !== 0 (or an error indicated in
result.stderr) and return null in that case; otherwise keep the existing stdout
=== "EXISTS" logic. Ensure you surface the tri-state by returning null for SSH
failures, true when stdout is "EXISTS", and false otherwise.
In `@src/nemoclaw.ts`:
- Around line 1353-1356: The CLI error for invalid skill names is misleading
because validateSkillName(skillName) also rejects the reserved names "." and
".."; update the messages in the block that checks validateSkillName(skillName)
so the output mentions both the allowed pattern and the reserved-name rule
(e.g., state that "." and ".." are reserved and not allowed), keeping the
existing descriptive first line (`Invalid skill name: '${skillName}'`) and
replacing the second line to include the regex constraint and the reserved
names; locate this logic around the skillInstall.validateSkillName call and
adjust the console.error strings accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1df3f5b4-0338-409f-b1c7-2d29d57409ef
📒 Files selected for processing (2)
src/lib/skill-install.tssrc/nemoclaw.ts
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
2 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
Adds
nemoclaw <sandbox> skill remove <name>to complement the existingskill installcommand (#1844 / #1845), providing a complete skill lifecycle from the CLI.Motivation
After
skill installwas added, there was no way to cleanly remove a skill from a running sandbox. Users had to manually SSH in, delete two directories, and clearsessions.json— mirroring the same problemskill installwas created to solve.What this does
/sandbox/.openclaw/skills/<name>/$HOME/.openclaw/skills/<name>/sessions.jsonso the agent re-discovers remaining skills on the next session[A-Za-z0-9._-]before any SSH operation — shell injection rejected client-sideUsage
Run
nemoclaw <sandbox> skill --helpfor full usage.Edge cases tested
Changes
src/lib/skill-install.tsremoveSkill()— removes upload dir, mirror dir, clears sessionsverifyRemove()— confirms directory is gone post-removalvalidateSkillName()— rejects shell metacharacters in CLI name inputsrc/nemoclaw.tssandboxSkillInstall()to dispatchinstallvsremove--helpoutput and global help textfinallyblocksrc/lib/skill-install.test.tsvalidateSkillNametests — valid names, empty, spaces, metacharactersremoveSkillunit test — SSH-failure path returnssuccess=false+ warningverifyRemoveunit test — SSH-failure returnsfalse(conservative/safe)All 1611 existing tests continue to pass.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests