diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 0b5472af5c5..eaaac09da52 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -825,6 +825,19 @@ Files with unsafe path characters are rejected to prevent shell injection. If the skill already exists on the sandbox, the command updates it in place and preserves chat history. For new installs, the agent session index is refreshed so the agent discovers the skill on the next session. +### `nemoclaw skill remove ` + +Remove an installed skill from a running sandbox by skill name. +The command validates the skill name, removes the sandbox upload directory, removes the OpenClaw home-directory mirror when present, and refreshes the agent session index so the remaining skills are rediscovered on the next session. + +```bash +nemoclaw my-assistant skill remove my-skill +``` + +Use the skill name from the `SKILL.md` frontmatter, not the local directory name. +Skill names must contain only alphanumeric characters, dots, hyphens, and underscores, and cannot be `.` or `..`. +For non-OpenClaw agents, restart the agent gateway if prompted so the removal takes effect. + ### `nemoclaw rebuild` Upgrade a sandbox to the current agent version while preserving workspace state. diff --git a/src/commands/sandbox/skill.test.ts b/src/commands/sandbox/skill.test.ts index 3a2ccd40d07..8a26d4c107a 100644 --- a/src/commands/sandbox/skill.test.ts +++ b/src/commands/sandbox/skill.test.ts @@ -4,19 +4,27 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const installSandboxSkill = vi.hoisted(() => vi.fn().mockResolvedValue(undefined)); +const removeSandboxSkill = vi.hoisted(() => vi.fn().mockResolvedValue(undefined)); vi.mock("../../lib/actions/sandbox/skill-install", () => ({ installSandboxSkill, + removeSandboxSkill, })); import SkillCliCommand from "./skill"; import SkillInstallCliCommand from "./skill/install"; +import SkillRemoveCliCommand from "./skill/remove"; const rootDir = process.cwd(); +function clearSkillMocks(): void { + installSandboxSkill.mockClear(); + removeSandboxSkill.mockClear(); +} + describe("SkillCliCommand", () => { beforeEach(() => { - installSandboxSkill.mockClear(); + clearSkillMocks(); }); it("records a parser-style failure when the sandbox name is missing", async () => { @@ -29,6 +37,7 @@ describe("SkillCliCommand", () => { expect(process.exitCode).toBe(2); expect(error).toHaveBeenCalledWith("Missing required sandboxName for skill."); expect(installSandboxSkill).not.toHaveBeenCalled(); + expect(removeSandboxSkill).not.toHaveBeenCalled(); } finally { process.exitCode = previousExitCode; } @@ -37,7 +46,7 @@ describe("SkillCliCommand", () => { describe("SkillInstallCliCommand", () => { beforeEach(() => { - installSandboxSkill.mockClear(); + clearSkillMocks(); }); it("runs skill install with typed action options", async () => { @@ -55,3 +64,33 @@ describe("SkillInstallCliCommand", () => { expect(installSandboxSkill).not.toHaveBeenCalled(); }); }); + +describe("SkillRemoveCliCommand", () => { + beforeEach(() => { + clearSkillMocks(); + }); + + it("runs skill remove with typed action options", async () => { + await SkillRemoveCliCommand.run(["alpha", "my-skill"], rootDir); + + expect(removeSandboxSkill).toHaveBeenCalledWith("alpha", { + command: "remove", + name: "my-skill", + }); + }); + + it("allows help as a removable skill name", async () => { + await SkillRemoveCliCommand.run(["alpha", "help"], rootDir); + + expect(removeSandboxSkill).toHaveBeenCalledWith("alpha", { + command: "remove", + name: "help", + }); + }); + + it("requires a skill name before dispatch", async () => { + await expect(SkillRemoveCliCommand.run(["alpha"], rootDir)).rejects.toThrow(/skill/i); + + expect(removeSandboxSkill).not.toHaveBeenCalled(); + }); +}); diff --git a/src/commands/sandbox/skill.ts b/src/commands/sandbox/skill.ts index c3d7bb5535a..cd57f5b6670 100644 --- a/src/commands/sandbox/skill.ts +++ b/src/commands/sandbox/skill.ts @@ -8,9 +8,12 @@ export default class SkillCliCommand extends NemoClawCommand { static id = "sandbox:skill"; static strict = false; static summary = "Show skill command usage"; - static description = "Show skill install usage or report unknown skill subcommands."; - static usage = ["install "]; - static examples = ["<%= config.bin %> sandbox skill install alpha ./my-skill"]; + static description = "Show skill install/remove usage or report unknown skill subcommands."; + static usage = ["install ", "remove "]; + static examples = [ + "<%= config.bin %> sandbox skill install alpha ./my-skill", + "<%= config.bin %> sandbox skill remove alpha my-skill", + ]; public async run(): Promise { this.parsed = true; diff --git a/src/commands/sandbox/skill/remove.ts b/src/commands/sandbox/skill/remove.ts new file mode 100644 index 00000000000..4d1b565007e --- /dev/null +++ b/src/commands/sandbox/skill/remove.ts @@ -0,0 +1,36 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { Args } from "@oclif/core"; +import { removeSandboxSkill } from "../../../lib/actions/sandbox/skill-install"; +import { NemoClawCommand } from "../../../lib/cli/nemoclaw-oclif-command"; + +export default class SkillRemoveCliCommand extends NemoClawCommand { + static id = "sandbox:skill:remove"; + static strict = true; + static summary = "Remove an installed skill from the sandbox"; + static description = "Remove an installed SKILL.md agent skill from a running sandbox."; + static usage = [" "]; + static examples = ["<%= config.bin %> sandbox skill remove alpha my-skill"]; + static args = { + sandboxName: Args.string({ + name: "sandbox", + description: "Sandbox name", + required: true, + }), + skillName: Args.string({ + name: "skill", + description: "Skill name from SKILL.md frontmatter", + required: true, + }), + }; + static flags = {}; + + public async run(): Promise { + const { args } = await this.parse(SkillRemoveCliCommand); + await removeSandboxSkill(args.sandboxName, { + command: "remove", + name: args.skillName, + }); + } +} diff --git a/src/lib/actions/sandbox/skill-install.test.ts b/src/lib/actions/sandbox/skill-install.test.ts new file mode 100644 index 00000000000..7c9c3cb7e24 --- /dev/null +++ b/src/lib/actions/sandbox/skill-install.test.ts @@ -0,0 +1,221 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const captureSandboxSshConfig = vi.hoisted(() => vi.fn()); +const getSessionAgent = vi.hoisted(() => vi.fn()); +const ensureLiveSandboxOrExit = vi.hoisted(() => vi.fn()); +const skillInstall = vi.hoisted(() => ({ + validateSkillName: vi.fn(), + resolveSkillPaths: vi.fn(), + checkExisting: vi.fn(), + removeSkill: vi.fn(), + verifyRemove: vi.fn(), + parseFrontmatter: vi.fn(), + collectFiles: vi.fn(), + uploadDirectory: vi.fn(), + postInstall: vi.fn(), + verifyInstall: vi.fn(), +})); + +vi.mock("../../adapters/openshell/runtime", () => ({ + captureSandboxSshConfig, +})); + +vi.mock("../../agent/runtime", () => ({ + getSessionAgent, +})); + +vi.mock("../../skill-install", () => skillInstall); + +vi.mock("./gateway-state", () => ({ + ensureLiveSandboxOrExit, +})); + +import { installSandboxSkill, removeSandboxSkill } from "./skill-install"; + +const paths = { + uploadDir: "/sandbox/.openclaw/skills/demo-skill", + mirrorDir: "$HOME/.openclaw/skills/demo-skill", + sessionFile: "/sandbox/.openclaw/agents/main/sessions/sessions.json", + isOpenClaw: true, +}; + +const agent = { name: "openclaw", configPaths: { dir: "/sandbox/.openclaw" } }; + +function makeSkillDir(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-action-skill-")); + fs.writeFileSync(path.join(dir, "SKILL.md"), "---\nname: demo-skill\n---\n# Demo\n"); + return dir; +} + +function restoreExitCode(previousExitCode: typeof process.exitCode): void { + process.exitCode = previousExitCode; +} + +describe("sandbox skill action orchestration", () => { + let previousExitCode: typeof process.exitCode; + + beforeEach(() => { + previousExitCode = process.exitCode; + process.exitCode = undefined; + vi.clearAllMocks(); + + captureSandboxSshConfig.mockReturnValue({ status: 0, output: "Host openshell-alpha\n" }); + ensureLiveSandboxOrExit.mockResolvedValue(undefined); + getSessionAgent.mockReturnValue(agent); + skillInstall.validateSkillName.mockReturnValue(true); + skillInstall.resolveSkillPaths.mockReturnValue(paths); + skillInstall.checkExisting.mockReturnValue(true); + skillInstall.removeSkill.mockReturnValue({ + success: true, + removedUploadDir: true, + removedMirrorDir: true, + clearedSessions: true, + messages: [], + }); + skillInstall.verifyRemove.mockReturnValue(true); + skillInstall.parseFrontmatter.mockReturnValue({ name: "demo-skill" }); + skillInstall.collectFiles.mockReturnValue({ + files: ["SKILL.md"], + skippedDotfiles: [], + unsafePaths: [], + }); + skillInstall.uploadDirectory.mockReturnValue({ + uploaded: 1, + failed: [], + skippedDotfiles: [], + unsafePaths: [], + }); + skillInstall.postInstall.mockReturnValue({ success: true, messages: [] }); + skillInstall.verifyInstall.mockReturnValue(true); + }); + + afterEach(() => { + restoreExitCode(previousExitCode); + vi.restoreAllMocks(); + }); + + it("fails skill remove when SSH config capture fails", async () => { + captureSandboxSshConfig.mockReturnValue({ status: 1, output: "" }); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + const exit = vi.spyOn(process, "exit").mockImplementation(((code?: string | number | null) => { + throw new Error(`process.exit ${code}`); + }) as typeof process.exit); + + await expect(removeSandboxSkill("alpha", { name: "demo-skill" })).rejects.toThrow( + "process.exit 1", + ); + + expect(ensureLiveSandboxOrExit).toHaveBeenCalledWith("alpha"); + expect(captureSandboxSshConfig).toHaveBeenCalledWith("alpha", expect.any(Object)); + expect(error).toHaveBeenCalledWith(" Failed to obtain SSH configuration for the sandbox."); + expect(skillInstall.checkExisting).not.toHaveBeenCalled(); + expect(exit).toHaveBeenCalledWith(1); + }); + + it("treats unknown skill existence as fatal for remove and deletes the temp SSH config", async () => { + let tempConfig = ""; + skillInstall.checkExisting.mockImplementation((ctx) => { + tempConfig = ctx.configFile; + expect(fs.existsSync(tempConfig)).toBe(true); + return null; + }); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await removeSandboxSkill("alpha", { name: "demo-skill" }); + + expect(process.exitCode).toBe(1); + expect(error).toHaveBeenCalledWith( + " Could not check if skill 'demo-skill' exists — sandbox may be unreachable.", + ); + expect(skillInstall.removeSkill).not.toHaveBeenCalled(); + expect(skillInstall.verifyRemove).not.toHaveBeenCalled(); + expect(tempConfig).not.toBe(""); + expect(fs.existsSync(tempConfig)).toBe(false); + }); + + it("reports an absent skill for remove and deletes the temp SSH config", async () => { + let tempConfig = ""; + skillInstall.checkExisting.mockImplementation((ctx) => { + tempConfig = ctx.configFile; + return false; + }); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await removeSandboxSkill("alpha", { name: "demo-skill" }); + + expect(process.exitCode).toBe(1); + expect(error).toHaveBeenCalledWith(" Skill 'demo-skill' is not installed in sandbox 'alpha'."); + expect(skillInstall.removeSkill).not.toHaveBeenCalled(); + expect(skillInstall.verifyRemove).not.toHaveBeenCalled(); + expect(tempConfig).not.toBe(""); + expect(fs.existsSync(tempConfig)).toBe(false); + }); + + it("removes and verifies an existing skill, then deletes the temp SSH config", async () => { + let tempConfig = ""; + skillInstall.checkExisting.mockImplementation((ctx, resolvedPaths) => { + tempConfig = ctx.configFile; + expect(resolvedPaths).toBe(paths); + return true; + }); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + + await removeSandboxSkill("alpha", { name: "demo-skill" }); + + expect(ensureLiveSandboxOrExit).toHaveBeenCalledWith("alpha"); + expect(getSessionAgent).toHaveBeenCalledWith("alpha"); + expect(skillInstall.resolveSkillPaths).toHaveBeenCalledWith(agent, "demo-skill"); + expect(skillInstall.removeSkill).toHaveBeenCalledWith( + expect.objectContaining({ configFile: tempConfig, sandboxName: "alpha" }), + paths, + ); + expect(skillInstall.verifyRemove).toHaveBeenCalledWith( + expect.objectContaining({ configFile: tempConfig, sandboxName: "alpha" }), + paths, + ); + expect(log).toHaveBeenCalledWith(expect.stringContaining("Skill 'demo-skill' removed")); + expect(fs.existsSync(tempConfig)).toBe(false); + expect(process.exitCode).toBeUndefined(); + }); + + it("continues skill install when the existence probe is unknown because upload plus verify are authoritative", async () => { + const skillDir = makeSkillDir(); + let tempConfig = ""; + skillInstall.checkExisting.mockImplementation((ctx) => { + tempConfig = ctx.configFile; + return null; + }); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + + try { + await installSandboxSkill("alpha", { command: "install", path: skillDir }); + } finally { + fs.rmSync(skillDir, { recursive: true, force: true }); + } + + expect(error).toHaveBeenCalledWith( + expect.stringContaining( + "Warning: could not check sandbox for existing skill — treating as fresh install.", + ), + ); + expect(skillInstall.uploadDirectory).toHaveBeenCalledWith( + expect.objectContaining({ configFile: tempConfig, sandboxName: "alpha" }), + skillDir, + paths.uploadDir, + ); + expect(skillInstall.verifyInstall).toHaveBeenCalledWith( + expect.objectContaining({ configFile: tempConfig, sandboxName: "alpha" }), + paths, + ); + expect(log).toHaveBeenCalledWith(expect.stringContaining("Skill 'demo-skill' installed")); + expect(fs.existsSync(tempConfig)).toBe(false); + expect(process.exitCode).toBeUndefined(); + }); +}); diff --git a/src/lib/actions/sandbox/skill-install.ts b/src/lib/actions/sandbox/skill-install.ts index 956fc9441e4..af3bdfdfdef 100644 --- a/src/lib/actions/sandbox/skill-install.ts +++ b/src/lib/actions/sandbox/skill-install.ts @@ -16,15 +16,21 @@ import { ensureLiveSandboxOrExit } from "./gateway-state"; export function printSkillInstallUsage(): void { console.log(""); console.log(` Usage: ${CLI_NAME} skill install `); + console.log(` ${CLI_NAME} skill remove `); console.log(""); - console.log(" Deploy a skill directory to a running sandbox."); + console.log(" Deploy or remove a skill in a running sandbox."); + console.log(""); + console.log(" install Deploy a skill directory to the sandbox."); console.log( - " must be a skill directory containing a SKILL.md (with 'name:' frontmatter),", + " must be a skill directory containing a SKILL.md (with 'name:' frontmatter),", ); console.log( - " or a direct path to a SKILL.md file. All non-dot files in the directory are uploaded.", + " or a direct path to a SKILL.md file. All non-dot files in the directory are uploaded.", ); console.log(""); + console.log(" remove Remove an installed skill from the sandbox by name."); + console.log(" is the skill name from SKILL.md frontmatter (e.g. my-skill)."); + console.log(""); } export function looksLikeOpenClawPlugin(candidatePath: string): boolean { @@ -61,6 +67,12 @@ export type SkillInstallRequest = { extraArgs?: string[]; }; +export type SkillRemoveRequest = { + command?: string; + name?: string; + extraArgs?: string[]; +}; + export function printPluginInstallHint(): void { console.error(" This looks like an OpenClaw plugin, not a SKILL.md agent skill."); console.error(" `skill install` only accepts skill directories or direct SKILL.md paths."); @@ -69,6 +81,99 @@ export function printPluginInstallHint(): void { ); } +/** + * Remove an installed skill from a live sandbox by name. + */ +export async function removeSandboxSkill( + sandboxName: string, + request: SkillRemoveRequest = {}, +): Promise { + const skillName = request.name; + const extraArgs = request.extraArgs ?? []; + if (skillName === "--help" || skillName === "-h") { + printSkillInstallUsage(); + return; + } + if (extraArgs.length > 0) { + console.error(` Unknown argument(s) for skill remove: ${extraArgs.join(", ")}`); + console.error(` Usage: ${CLI_NAME} skill remove `); + process.exit(1); + } + if (!skillName) { + console.error(` Usage: ${CLI_NAME} skill remove `); + console.error(" is the skill name from the SKILL.md frontmatter."); + process.exit(1); + } + if (!skillInstall.validateSkillName(skillName)) { + console.error(` Invalid skill name: '${skillName}'`); + console.error(" Skill names must match [A-Za-z0-9._-] and must not be '.' or '..'."); + process.exit(1); + } + + await ensureLiveSandboxOrExit(sandboxName); + + const agent = agentRuntime.getSessionAgent(sandboxName); + const paths = skillInstall.resolveSkillPaths(agent, skillName); + + const sshConfigResult = captureSandboxSshConfig(sandboxName, { + ignoreError: true, + timeout: OPENSHELL_PROBE_TIMEOUT_MS, + }); + if (sshConfigResult.status !== 0) { + console.error(" Failed to obtain SSH configuration for the sandbox."); + process.exit(1); + } + + const tmpSshConfig = path.join( + os.tmpdir(), + `nemoclaw-ssh-skill-${process.pid}-${Date.now()}.conf`, + ); + fs.writeFileSync(tmpSshConfig, sshConfigResult.output, { mode: 0o600 }); + + try { + const ctx = { configFile: tmpSshConfig, sandboxName }; + + const existsCheck = skillInstall.checkExisting(ctx, paths); + if (existsCheck === null) { + console.error( + ` Could not check if skill '${skillName}' exists — sandbox may be unreachable.`, + ); + process.exitCode = 1; + return; + } + if (!existsCheck) { + console.error(` Skill '${skillName}' is not installed in sandbox '${sandboxName}'.`); + process.exitCode = 1; + return; + } + + const result = skillInstall.removeSkill(ctx, paths); + for (const msg of result.messages) { + if (msg.startsWith("Warning:")) { + console.error(` ${YW}${msg}${R}`); + } else { + console.log(` ${D}${msg}${R}`); + } + } + + const gone = skillInstall.verifyRemove(ctx, paths); + if (gone) { + console.log(` ${G}✓${R} Skill '${skillName}' removed`); + } else { + console.error(" Skill removal could not be verified."); + console.error(" The sandbox may be unreachable, or the skill directory may still exist."); + process.exitCode = 1; + return; + } + } finally { + try { + fs.unlinkSync(tmpSshConfig); + } catch { + /* ignore */ + } + } +} + /** * Install or update a local skill directory into a live sandbox and perform * any agent-specific post-install refresh needed for the new content to load. @@ -83,9 +188,18 @@ export async function installSandboxSkill( return; } + if (sub === "remove") { + await removeSandboxSkill(sandboxName, { + command: "remove", + name: request.path, + extraArgs: request.extraArgs, + }); + return; + } + if (sub !== "install") { console.error(` Unknown skill subcommand: ${sub}`); - console.error(" Valid subcommands: install"); + console.error(" Valid subcommands: install, remove"); process.exit(1); } @@ -187,8 +301,21 @@ export async function installSandboxSkill( try { const ctx = { configFile: tmpSshConfig, sandboxName }; - // 5. Check if skill already exists (update vs fresh install) - const isUpdate = skillInstall.checkExisting(ctx, paths); + // 5. Check if skill already exists (update vs fresh install). This probe is + // advisory for install only: stale SSH config files and transient remote + // shell startup failures can make the stat probe inconclusive even when a + // subsequent upload succeeds. Upload plus verifyInstall() remain the + // source of truth for install success; remove keeps null fatal because it + // is destructive. Once OpenShell exposes a typed stat API or SSH probe + // failures are reliably distinguishable from absent dirs across supported + // versions, remove this fallback and fail before upload. + const existingCheck = skillInstall.checkExisting(ctx, paths); + if (existingCheck === null) { + console.error( + ` ${YW}Warning: could not check sandbox for existing skill — treating as fresh install.${R}`, + ); + } + const isUpdate = existingCheck === true; // 6. Upload skill directory const { uploaded, failed } = skillInstall.uploadDirectory(ctx, skillDir, paths.uploadDir); diff --git a/src/lib/cli/command-registry.test.ts b/src/lib/cli/command-registry.test.ts index 4c6b5484460..85dcdc9070a 100644 --- a/src/lib/cli/command-registry.test.ts +++ b/src/lib/cli/command-registry.test.ts @@ -17,10 +17,10 @@ import { getRegisteredOclifCommandsMetadata } from "./oclif-metadata"; describe("command-registry", () => { describe("COMMANDS array", () => { - it("should contain exactly 63 commands", () => { + it("should contain exactly 64 commands", () => { // 28 global (22 visible + 6 hidden help/version aliases) - // 35 sandbox (29 visible + 6 hidden shields/config) - expect(COMMANDS).toHaveLength(63); + // 36 sandbox (30 visible + 6 hidden shields/config) + expect(COMMANDS).toHaveLength(64); }); it("should have no duplicate usage strings", () => { @@ -52,9 +52,9 @@ describe("command-registry", () => { }); describe("sandboxCommands()", () => { - it("should return exactly 35 entries", () => { - // 29 visible + 6 hidden (shields×3 + config get/set/rotate-token) - expect(sandboxCommands()).toHaveLength(35); + it("should return exactly 36 entries", () => { + // 30 visible + 6 hidden (shields×3 + config get/set/rotate-token) + expect(sandboxCommands()).toHaveLength(36); }); it("every entry has scope sandbox", () => { @@ -65,10 +65,10 @@ describe("command-registry", () => { }); describe("visibleCommands()", () => { - it("should exclude 12 hidden commands (51 visible)", () => { + it("should exclude 12 hidden commands (52 visible)", () => { // 6 hidden global (help, --help, -h, version, --version, -v) + // 6 hidden sandbox (shields×3, config get/set/rotate-token) - expect(visibleCommands()).toHaveLength(51); + expect(visibleCommands()).toHaveLength(52); }); it("no visible command has hidden=true", () => { @@ -168,6 +168,12 @@ describe("command-registry", () => { expect(list).not.toContain("nemoclaw config set"); expect(list).not.toContain("nemoclaw config rotate-token"); }); + + it("uses distinct placeholders for sandbox and skill names", () => { + const command = COMMANDS.find((entry) => entry.commandId === "sandbox:skill:remove"); + expect(command?.usage).toBe("nemoclaw skill remove"); + expect(command?.flags).toBe(""); + }); }); describe("globalCommandTokens()", () => { diff --git a/src/lib/cli/public-argv-translation.test.ts b/src/lib/cli/public-argv-translation.test.ts index 4a47d7d5d09..1514b989d1d 100644 --- a/src/lib/cli/public-argv-translation.test.ts +++ b/src/lib/cli/public-argv-translation.test.ts @@ -211,6 +211,11 @@ describe("translatePublicSandboxArgv", () => { "sandbox:snapshot:restore", ["alpha", "latest"], ); + expectNative( + translatePublicSandboxArgv("alpha", "skill", ["remove", "my-skill"]), + "sandbox:skill:remove", + ["alpha", "my-skill"], + ); }); it("translates unknown parent subcommands to native oclif argv for oclif-owned errors", () => { diff --git a/src/lib/cli/public-display-defaults.ts b/src/lib/cli/public-display-defaults.ts index c9ff085db07..9f3c1b81278 100644 --- a/src/lib/cli/public-display-defaults.ts +++ b/src/lib/cli/public-display-defaults.ts @@ -369,6 +369,13 @@ const PUBLIC_DISPLAY_LAYOUT: Record = { "flags": "" } ], + "sandbox:skill:remove": [ + { + "group": "Skills", + "order": 16.1, + "flags": "" + } + ], "sandbox:snapshot:create": [ { "group": "Sandbox Management", diff --git a/src/lib/skill-install.test.ts b/src/lib/skill-install.test.ts index 363188b4b3b..83702b6ea81 100644 --- a/src/lib/skill-install.test.ts +++ b/src/lib/skill-install.test.ts @@ -71,9 +71,14 @@ describe("parseFrontmatter", () => { }); it("rejects names with invalid characters", () => { - expect(() => parseFrontmatter("---\nname: my skill\n---\n")).toThrow("invalid characters"); - expect(() => parseFrontmatter("---\nname: ../escape\n---\n")).toThrow("invalid characters"); - expect(() => parseFrontmatter("---\nname: a/b\n---\n")).toThrow("invalid characters"); + expect(() => parseFrontmatter("---\nname: my skill\n---\n")).toThrow("is invalid"); + expect(() => parseFrontmatter("---\nname: ../escape\n---\n")).toThrow("is invalid"); + expect(() => parseFrontmatter("---\nname: a/b\n---\n")).toThrow("is invalid"); + }); + + it("rejects dot and double-dot as skill names in frontmatter", () => { + expect(() => parseFrontmatter("---\nname: .\n---\n")).toThrow("is invalid"); + expect(() => parseFrontmatter("---\nname: ..\n---\n")).toThrow("is invalid"); }); }); @@ -194,6 +199,7 @@ describe("resolveSkillPaths", () => { it("returns OpenClaw defaults when agent is null", () => { const paths = resolveSkillPaths(null, "weather"); expect(paths.uploadDir).toBe("/sandbox/.openclaw/skills/weather"); + expect(paths.mirrorDir).toBe("$HOME/.openclaw/skills/weather"); expect(paths.sessionFile).toBe( "/sandbox/.openclaw/agents/main/sessions/sessions.json", ); @@ -209,6 +215,7 @@ describe("resolveSkillPaths", () => { }; const paths = resolveSkillPaths(agent, "my-skill"); expect(paths.uploadDir).toBe("/sandbox/.openclaw/skills/my-skill"); + expect(paths.mirrorDir).toBe("$HOME/.openclaw/skills/my-skill"); expect(paths.sessionFile).toBe( "/sandbox/.openclaw/agents/main/sessions/sessions.json", ); @@ -224,6 +231,7 @@ describe("resolveSkillPaths", () => { }; const paths = resolveSkillPaths(agent, "demo-skill"); expect(paths.uploadDir).toBe("/sandbox/.hermes/skills/demo-skill"); + expect(paths.mirrorDir).toBeNull(); expect(paths.sessionFile).toBeNull(); expect(paths.isOpenClaw).toBe(false); }); @@ -237,6 +245,7 @@ describe("resolveSkillPaths", () => { }; const paths = resolveSkillPaths(agent, "test-skill"); expect(paths.uploadDir).toBe("/sandbox/.future/skills/test-skill"); + expect(paths.mirrorDir).toBeNull(); expect(paths.sessionFile).toBeNull(); expect(paths.isOpenClaw).toBe(false); }); diff --git a/src/lib/skill-install.ts b/src/lib/skill-install.ts index f83141edd68..f6ca0d36194 100644 --- a/src/lib/skill-install.ts +++ b/src/lib/skill-install.ts @@ -1,13 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 // -// Skill install logic for `nemoclaw skill install `. +// Skill install/remove logic for `nemoclaw skill install ` +// and `nemoclaw skill remove `. // Validates a local SKILL.md, uploads it to the sandbox via SSH, and // performs agent-specific post-install steps (session refresh for // OpenClaw). Non-OpenClaw agents get a "restart gateway" hint until a // generic refresh contract is defined in the manifest schema. -import { spawnSync } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; @@ -15,6 +15,21 @@ import path from "node:path"; import YAML from "yaml"; import { isRecord } from "./core/json-types"; +import { validateSkillName } from "./skill-name"; +import { shellQuote, sshExec } from "./skill-remote"; +import type { SshContext, SshResult } from "./skill-remote"; + +export { validateSkillName } from "./skill-name"; +export { + checkExisting, + type RemoveResult, + removeSkill, + shellQuote, + sshExec, + type SshContext, + type SshResult, + verifyRemove, +} from "./skill-remote"; // ── Frontmatter parsing ────────────────────────────────────────── @@ -67,9 +82,9 @@ export function parseFrontmatter(content: string): SkillFrontmatter { throw new Error("SKILL.md frontmatter is missing required 'name' field"); } - if (!/^[A-Za-z0-9._-]+$/.test(nameValue)) { + if (!validateSkillName(nameValue)) { throw new Error( - `SKILL.md name '${nameValue}' contains invalid characters. Only [A-Za-z0-9._-] allowed.`, + `SKILL.md name '${nameValue}' is invalid. Use [A-Za-z0-9._-] and do not use '.' or '..'.`, ); } @@ -81,6 +96,8 @@ export function parseFrontmatter(content: string): SkillFrontmatter { export interface SkillPaths { /** Upload target directory for the skill */ uploadDir: string; + /** OpenClaw-only mirror directory under the remote home dir, or null */ + mirrorDir: string | null; /** OpenClaw-only: session index to clear, or null */ sessionFile: string | null; /** Whether the agent is OpenClaw (drives refresh behavior) */ @@ -105,6 +122,7 @@ export function resolveSkillPaths( return { uploadDir, + mirrorDir: isOpenClaw ? `$HOME/.openclaw/skills/${skillName}` : null, sessionFile: isOpenClaw ? `${dir}/agents/main/sessions/sessions.json` : null, isOpenClaw, }; @@ -112,12 +130,6 @@ export function resolveSkillPaths( // ── Shell safety ───────────────────────────────────────────────── -// Re-export shellQuote from runner.ts — a repo-wide test enforces -// a single definition lives in runner.ts. -const { shellQuote } = require("./runner"); - -export { shellQuote }; - const SAFE_PATH_RE = /^[A-Za-z0-9._\-/]+$/; /** @@ -131,61 +143,7 @@ export function validateRelativePath(rel: string): boolean { return segments.every((s) => s !== "" && s !== ".." && s !== "."); } -// ── SSH helpers ────────────────────────────────────────────────── - -export interface SshContext { - configFile: string; - sandboxName: string; -} - -export interface SshResult { - status: number; - stdout: string; - stderr: string; -} - -/** - * Run a command on the sandbox via SSH with optional stdin content. - * Uses the same SSH flags as executeSandboxCommand in sandbox-process-recovery-action.ts. - */ -export function sshExec( - ctx: SshContext, - command: string, - opts: { input?: string | Buffer; timeout?: number } = {}, -): SshResult | null { - try { - const result = spawnSync( - "ssh", - [ - "-F", - ctx.configFile, - "-o", - "StrictHostKeyChecking=no", - "-o", - "UserKnownHostsFile=/dev/null", - "-o", - "ConnectTimeout=10", - "-o", - "LogLevel=ERROR", - `openshell-${ctx.sandboxName}`, - command, - ], - { - encoding: "utf-8", - stdio: [opts.input !== undefined ? "pipe" : "ignore", "pipe", "pipe"], - input: opts.input, - timeout: opts.timeout ?? 30_000, - }, - ); - return { - status: result.status ?? 1, - stdout: (result.stdout || "").trim(), - stderr: (result.stderr || "").trim(), - }; - } catch { - return null; - } -} +// ── Upload helpers ─────────────────────────────────────────────── /** * Upload a file to the sandbox by piping its content through SSH stdin. @@ -299,15 +257,6 @@ export function postInstall( return { success: true, messages }; } -/** - * Check whether a skill already exists on the sandbox at the upload path. - */ -export function checkExisting(ctx: SshContext, paths: SkillPaths): boolean { - const target = shellQuote(`${paths.uploadDir}/SKILL.md`); - const result = sshExec(ctx, `test -f ${target} && echo EXISTS`); - return result !== null && result.stdout === "EXISTS"; -} - /** * Verify the SKILL.md file exists on the sandbox at the expected path. */ diff --git a/src/lib/skill-name.ts b/src/lib/skill-name.ts new file mode 100644 index 00000000000..b3083a04688 --- /dev/null +++ b/src/lib/skill-name.ts @@ -0,0 +1,16 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Validate that a skill name supplied on the CLI or in SKILL.md frontmatter is + * safe to use as a remote path segment. + * Rejects anything that isn't a valid skill name ([A-Za-z0-9._-]). + */ +export function validateSkillName(name: string): boolean { + return ( + name.length > 0 && + name !== "." && + name !== ".." && + /^[A-Za-z0-9._-]+$/.test(name) + ); +} diff --git a/src/lib/skill-remote.test.ts b/src/lib/skill-remote.test.ts new file mode 100644 index 00000000000..5ac7cf194de --- /dev/null +++ b/src/lib/skill-remote.test.ts @@ -0,0 +1,156 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { validateSkillName } from "../../dist/lib/skill-name"; +import { checkExisting, removeSkill, verifyRemove } from "../../dist/lib/skill-remote"; +import { resolveSkillPaths } from "../../dist/lib/skill-install"; + +describe("validateSkillName", () => { + it("accepts valid skill names", () => { + expect(validateSkillName("my-skill")).toBe(true); + expect(validateSkillName("my_skill")).toBe(true); + expect(validateSkillName("my.skill")).toBe(true); + expect(validateSkillName("MySkill123")).toBe(true); + expect(validateSkillName("digicon-zeiss-ai-strategy")).toBe(true); + }); + + it("rejects empty string", () => { + expect(validateSkillName("")).toBe(false); + }); + + it("rejects names with spaces", () => { + expect(validateSkillName("my skill")).toBe(false); + }); + + it("rejects names with shell metacharacters", () => { + expect(validateSkillName("my;skill")).toBe(false); + expect(validateSkillName("my$skill")).toBe(false); + expect(validateSkillName("my/skill")).toBe(false); + expect(validateSkillName("../escape")).toBe(false); + expect(validateSkillName("my`skill`")).toBe(false); + }); + + it("rejects dot and double-dot to prevent directory traversal on rm -rf", () => { + expect(validateSkillName(".")).toBe(false); + expect(validateSkillName("..")).toBe(false); + }); +}); + +describe("removeSkill (unit — no SSH)", () => { + it("returns success=false and a warning when sshExec returns null (sandbox unreachable)", () => { + const paths = resolveSkillPaths(null, "test-skill"); + + const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; + const result = removeSkill(ctx, paths); + + expect(result.success).toBe(false); + expect(result.removedUploadDir).toBe(false); + expect(result.messages.some((m) => m.startsWith("Warning:"))).toBe(true); + }); + + it("success is false for OpenClaw when mirrorDir removal fails even if uploadDir was removed", () => { + const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; + const paths = resolveSkillPaths(null, "test-skill"); + const result = removeSkill(ctx, paths, { + sshExecImpl: (_ctx, command) => ({ + status: command.includes("$HOME/.openclaw/skills") ? 1 : 0, + stdout: "", + stderr: "", + }), + }); + + expect(result.removedUploadDir).toBe(true); + expect(result.removedMirrorDir).toBe(false); + expect(result.success).toBe(false); + }); + + it("removes OpenClaw upload and mirror dirs, then clears sessions", () => { + const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; + const paths = resolveSkillPaths(null, "test-skill"); + const commands: string[] = []; + const result = removeSkill(ctx, paths, { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "", stderr: "" }; + }, + }); + + expect(result.success).toBe(true); + expect(result.clearedSessions).toBe(true); + expect(commands).toEqual([ + "rm -rf '/sandbox/.openclaw/skills/test-skill'", + 'rm -rf "$HOME/.openclaw/skills/test-skill"', + "printf '{}' > '/sandbox/.openclaw/agents/main/sessions/sessions.json'", + ]); + }); +}); + +describe("verifyRemove (unit — no SSH)", () => { + it("returns false when SSH is unreachable (conservative — treat failure as not-gone)", () => { + const paths = resolveSkillPaths(null, "test-skill"); + const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; + expect(verifyRemove(ctx, paths)).toBe(false); + }); + + it("returns false for non-OpenClaw paths when SSH is unreachable", () => { + const paths = resolveSkillPaths( + { name: "hermes", configPaths: { dir: "/sandbox/.hermes" } }, + "test-skill", + ); + const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; + expect(verifyRemove(ctx, paths)).toBe(false); + }); + + it("verifies both OpenClaw skill directories are gone", () => { + const paths = resolveSkillPaths(null, "test-skill"); + const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; + const commands: string[] = []; + const gone = verifyRemove(ctx, paths, { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "GONE", stderr: "" }; + }, + }); + + expect(gone).toBe(true); + expect(commands).toEqual([ + "test ! -e '/sandbox/.openclaw/skills/test-skill' && test ! -e \"$HOME/.openclaw/skills/test-skill\" && echo GONE || echo EXISTS", + ]); + }); +}); + +describe("checkExisting (unit — no SSH)", () => { + it("returns null when SSH is unreachable for OpenClaw paths", () => { + const paths = resolveSkillPaths(null, "test-skill"); + const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; + expect(checkExisting(ctx, paths)).toBeNull(); + }); + + it("returns null when SSH is unreachable for non-OpenClaw paths", () => { + const paths = resolveSkillPaths( + { name: "hermes", configPaths: { dir: "/sandbox/.hermes" } }, + "test-skill", + ); + const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; + expect(checkExisting(ctx, paths)).toBeNull(); + }); + + it("probes skill directories so removal can clean partial uploads", () => { + const paths = resolveSkillPaths(null, "test-skill"); + const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; + const commands: string[] = []; + const exists = checkExisting(ctx, paths, { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "EXISTS", stderr: "" }; + }, + }); + + expect(exists).toBe(true); + expect(commands[0]).toContain("test -e '/sandbox/.openclaw/skills/test-skill'"); + expect(commands[0]).toContain('test -e "$HOME/.openclaw/skills/test-skill"'); + expect(commands[0]).not.toContain("SKILL.md"); + }); +}); diff --git a/src/lib/skill-remote.ts b/src/lib/skill-remote.ts new file mode 100644 index 00000000000..8d3c71a51e4 --- /dev/null +++ b/src/lib/skill-remote.ts @@ -0,0 +1,181 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { spawnSync } from "node:child_process"; + +import type { SkillPaths } from "./skill-install"; + +// Re-export shellQuote from runner.ts — a repo-wide test enforces +// a single definition lives in runner.ts. +const { shellQuote } = require("./runner"); + +export { shellQuote }; + +export interface SshContext { + configFile: string; + sandboxName: string; +} + +export interface SshResult { + status: number; + stdout: string; + stderr: string; +} + +/** + * Run a command on the sandbox via SSH with optional stdin content. + * Uses the same SSH flags as executeSandboxCommand in sandbox-process-recovery-action.ts. + */ +export function sshExec( + ctx: SshContext, + command: string, + opts: { input?: string | Buffer; timeout?: number } = {}, +): SshResult | null { + try { + const result = spawnSync( + "ssh", + [ + "-F", + ctx.configFile, + "-o", + "StrictHostKeyChecking=no", + "-o", + "UserKnownHostsFile=/dev/null", + "-o", + "ConnectTimeout=10", + "-o", + "LogLevel=ERROR", + `openshell-${ctx.sandboxName}`, + command, + ], + { + encoding: "utf-8", + stdio: [opts.input !== undefined ? "pipe" : "ignore", "pipe", "pipe"], + input: opts.input, + timeout: opts.timeout ?? 30_000, + }, + ); + return { + status: result.status ?? 1, + stdout: (result.stdout || "").trim(), + stderr: (result.stderr || "").trim(), + }; + } catch { + return null; + } +} + +/** + * Check whether a skill directory already exists on the sandbox at the upload + * path or (for OpenClaw) the mirror path. Probing directories instead of only + * SKILL.md lets `skill remove` clean up partial uploads whose manifest write + * failed after the directory was created. + * + * Returns: + * true — skill exists + * false — skill is absent + * null — SSH probe failed; existence could not be determined + */ +export function checkExisting( + ctx: SshContext, + paths: SkillPaths, + opts: { sshExecImpl?: typeof sshExec } = {}, +): boolean | null { + const checks = [`test -e ${shellQuote(paths.uploadDir)}`]; + if (paths.isOpenClaw && paths.mirrorDir) { + checks.push(`test -e "${paths.mirrorDir}"`); + } + const runSsh = opts.sshExecImpl ?? sshExec; + const result = runSsh(ctx, `{ ${checks.join(" || ")}; } && echo EXISTS || echo ABSENT`); + if (result === null || result.status !== 0) { + return null; + } + if (result.stdout === "EXISTS") return true; + if (result.stdout === "ABSENT") return false; + return null; +} + +export interface RemoveResult { + success: boolean; + removedUploadDir: boolean; + removedMirrorDir: boolean; + clearedSessions: boolean; + messages: string[]; +} + +/** + * Remove a skill from the sandbox by name. + * Deletes the immutable upload directory, the OpenClaw mirror directory + * (if applicable), and clears sessions.json so the agent re-discovers + * the remaining skills on the next session. + * + * Only the named skill directory is deleted — other skills are untouched. + */ +export function removeSkill( + ctx: SshContext, + paths: SkillPaths, + opts: { sshExecImpl?: typeof sshExec } = {}, +): RemoveResult { + const messages: string[] = []; + const runSsh = opts.sshExecImpl ?? sshExec; + + // 1. Remove the immutable upload directory (/sandbox/.openclaw/skills//) + const uploadDir = shellQuote(paths.uploadDir); + const removeUpload = runSsh(ctx, `rm -rf ${uploadDir}`); + const removedUploadDir = removeUpload !== null && removeUpload.status === 0; + if (!removedUploadDir) { + messages.push(`Warning: failed to remove upload directory ${paths.uploadDir}`); + } + + // 2. Remove the OpenClaw mirror ($HOME/.openclaw/skills//) + // mirrorDir contains $HOME which must expand on the remote shell, so we + // use double quotes (not shellQuote). This is safe because skill names + // are restricted to [A-Za-z0-9._-] by parseFrontmatter / the name + // validation regex, so $HOME expansion is the only variable substitution. + let removedMirrorDir = false; + if (paths.isOpenClaw && paths.mirrorDir) { + const removeMirror = runSsh(ctx, `rm -rf "${paths.mirrorDir}"`); + removedMirrorDir = removeMirror !== null && removeMirror.status === 0; + if (!removedMirrorDir) { + messages.push(`Warning: failed to remove mirror directory ${paths.mirrorDir}`); + } + } + + // 3. Clear sessions.json so the agent re-discovers the remaining skills. + let clearedSessions = false; + if (paths.isOpenClaw && paths.sessionFile) { + const clearResult = runSsh(ctx, `printf '{}' > ${shellQuote(paths.sessionFile)}`); + clearedSessions = clearResult !== null && clearResult.status === 0; + if (!clearedSessions) { + messages.push("Warning: failed to clear sessions (agent may need manual restart)"); + } + } else if (!paths.isOpenClaw) { + messages.push("Restart the agent gateway for the removal to take effect."); + } + + return { + success: removedUploadDir && (!paths.isOpenClaw || removedMirrorDir), + removedUploadDir, + removedMirrorDir, + clearedSessions, + messages, + }; +} + +/** + * Verify the skill directory no longer exists on the sandbox. + * For OpenClaw sandboxes, both the upload dir and the mirror dir must be gone. + */ +export function verifyRemove( + ctx: SshContext, + paths: SkillPaths, + opts: { sshExecImpl?: typeof sshExec } = {}, +): boolean { + const checks = [`test ! -e ${shellQuote(paths.uploadDir)}`]; + if (paths.isOpenClaw && paths.mirrorDir) { + checks.push(`test ! -e "${paths.mirrorDir}"`); + } + const runSsh = opts.sshExecImpl ?? sshExec; + const result = runSsh(ctx, `${checks.join(" && ")} && echo GONE || echo EXISTS`); + return result !== null && result.stdout === "GONE"; +}