src/nemoclaw.ts (1)

1340-1568: Split skill install and skill remove into 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.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/lib/skill-install.ts (1)

405-420: ⚠️ Potential issue | 🟠 Major

Require mirror deletion for OpenClaw before reporting removal success.

On Line [406], success ignores mirror deletion; and Lines [417-420] verify only uploadDir. 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).

src/lib/skill-install.ts (1)

66-70: Consolidate skill-name validation into one shared helper.

The same rule is duplicated in parseFrontmatter and validateSkillName. 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.

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.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/lib/skill-install.ts (1)

354-363: ⚠️ Potential issue | 🟠 Major

Preserve the tri-state on non-zero SSH exits.

sshExec() only returns null when spawnSync() throws. A normal SSH transport/auth failure still comes back with a non-zero status, so this path still collapses probe failures into false instead of null. 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() and checkExisting(): if SSH failures can produce a non-zero exit without throwing, this function should map that case to null.

#!/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 | 🟡 Minor

Mention 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.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/nemoclaw.ts (1)

2496-2499: ⚠️ Potential issue | 🟡 Minor

Invalid-name message is incomplete for reserved names.

Line 2498 says only regex constraints, but validation also rejects "." and "..”. Please align the CLI error to avoid misleading users.

✏️ 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 match [A-Za-z0-9._-] and must not 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 2496 - 2499, Update the CLI error output in the
block that checks skillInstall.validateSkillName(skillName) to mention reserved
names in addition to the regex constraint: change the two console.error lines so
they state that names must match [A-Za-z0-9._-] and that the single-segment
reserved names "." and ".." are disallowed (reference
skillInstall.validateSkillName and the variable skillName to locate the check).
Ensure the message remains concise and printed before process.exit(1).

src/nemoclaw.ts (1)

2710-2728: Deduplicate skill help output via printSkillInstallUsage().

The dispatcher reprints the same usage/help block already provided by printSkillInstallUsage(), which risks drift.

♻️ Suggested refactor

 async function sandboxSkillInstall(sandboxName, args = []) {
   const sub = args[0];
   if (!sub || sub === "help" || sub === "--help" || sub === "-h") {
-    console.log("");
-    console.log("  Usage: nemoclaw <sandbox> skill install <path>");
-    console.log("         nemoclaw <sandbox> skill remove <name>");
-    console.log("");
-    console.log("  Deploy or remove a skill in a running sandbox.");
-    console.log("");
-    console.log("  install <path>  Deploy a skill directory to the sandbox.");
-    console.log(
-      "    <path> 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.",
-    );
-    console.log("");
-    console.log("  remove <name>   Remove an installed skill from the sandbox by name.");
-    console.log("    <name> is the skill name from SKILL.md frontmatter (e.g. my-skill).");
-    console.log("");
+    printSkillInstallUsage();
     return;
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/nemoclaw.ts` around lines 2710 - 2728, The large duplicated help/usage
block for the "skill install/remove" command should be replaced by a single call
to the existing helper printSkillInstallUsage(); locate the conditional that
checks sub (the block starting with if (!sub || sub === "help" || sub ===
"--help" || sub === "-h")) and remove the console.log calls, then call
printSkillInstallUsage() instead so the help text is maintained in one place.

src/nemoclaw.ts (1)

2483-2737: Please run the sandbox lifecycle E2E set for this CLI-path change.

Given this PR updates src/nemoclaw.ts sandbox skill command flow, run the recommended targeted nightly jobs before merge:

• sandbox-survival-e2e
• sandbox-operations-e2e
• skip-permissions-e2e

As per coding guidelines: src/nemoclaw.ts changes should run the listed E2E jobs via gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sandbox-survival-e2e,sandbox-operations-e2e,skip-permissions-e2e.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/nemoclaw.ts` around lines 2483 - 2737, The change touches the sandbox
skill CLI flow in src/nemoclaw.ts (see sandboxSkillRemove, sandboxSkillDeploy
and sandboxSkillInstall), so before merging run the targeted nightly E2E jobs:
invoke the GitHub Actions workflow nightly-e2e.yaml for this branch and pass the
three jobs sandbox-survival-e2e,sandbox-operations-e2e,skip-permissions-e2e
(e.g. use gh workflow run nightly-e2e.yaml --ref <branch> -f
jobs=sandbox-survival-e2e,sandbox-operations-e2e,skip-permissions-e2e) and
attach the run results to the PR.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/nemoclaw.ts (1)

2695-2771: Please run the recommended sandbox lifecycle E2Es on this new command path.

skill remove now drives live sandbox SSH, remote deletion, session refresh, and reconnect-on-next-session behavior. I'd want the recommended sandbox-survival-e2e and sandbox-operations-e2e coverage on this PR before merge.

As per coding guidelines src/nemoclaw.ts: “Changes affect sandbox lifecycle commands. E2E test recommendation: sandbox-survival-e2e — gateway restart recovery; sandbox-operations-e2e — process recovery after gateway kill.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/nemoclaw.ts` around lines 2695 - 2771, The new sandboxSkillRemove
implementation in src/nemoclaw.ts modifies live sandbox lifecycle (SSH removal,
session refresh, reconnect behavior) but lacks E2E verification; run the
recommended E2Es (sandbox-survival-e2e and sandbox-operations-e2e) exercising
the new command path (sandboxSkillRemove / "sandbox <name> skill remove <name>")
and update or add tests to cover SSH-driven remote deletion, session
refresh/reconnect-on-next-session, and gateway restart/process-kill recovery; if
any test fails, adjust sandboxSkillRemove or its helpers
(agentRuntime.getSessionAgent, skillInstall.removeSkill, verifyRemove, and
ensureLiveSandboxOrExit usage) until the E2Es pass.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes