diff --git a/src/lib/actions/sandbox/skill-install.ts b/src/lib/actions/sandbox/skill-install.ts index af3bdfdfdef..8b57b9a7001 100644 --- a/src/lib/actions/sandbox/skill-install.ts +++ b/src/lib/actions/sandbox/skill-install.ts @@ -343,7 +343,10 @@ export async function installSandboxSkill( const verb = isUpdate ? "updated" : "installed"; console.log(` ${G}✓${R} Skill '${frontmatter.name}' ${verb}`); } else { - console.error(` Skill uploaded but verification failed at ${paths.uploadDir}/SKILL.md`); + console.error( + ` Skill uploaded but verification failed: SKILL.md missing at ${paths.uploadDir}` + + (paths.isOpenClaw && paths.mirrorDir ? ` or its agent mirror ${paths.mirrorDir}` : ""), + ); process.exit(1); } } finally { diff --git a/src/lib/skill-install.test.ts b/src/lib/skill-install.test.ts index 83702b6ea81..e710fb5d087 100644 --- a/src/lib/skill-install.test.ts +++ b/src/lib/skill-install.test.ts @@ -1,18 +1,19 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { describe, it, expect } from "vitest"; -import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from "node:fs"; -import { join } from "node:path"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; // Import from compiled dist/ so coverage is attributed correctly. import { - parseFrontmatter, - resolveSkillPaths, collectFiles, + parseFrontmatter, postInstall, - validateRelativePath, + resolveSkillPaths, shellQuote, + validateRelativePath, + verifyInstall, } from "../../dist/lib/skill-install"; describe("parseFrontmatter", () => { @@ -277,4 +278,92 @@ describe("postInstall", () => { rmSync(skillDir, { recursive: true, force: true }); } }); + + it("mirrors the uploaded skill into the OpenClaw home dir so the agent loads it", () => { + // Regression for #4819: on sandboxes whose agent $HOME differs from the + // OpenClaw state dir, `skills list` shows the upload dir while the agent + // loads skills from $HOME/.openclaw/skills. Install must populate that + // mirror — symmetric with `skill remove`, which deletes it. + const skillDir = mkdtempSync(join(tmpdir(), "skill-postinstall-mirror-")); + const commands: string[] = []; + try { + writeFileSync(skillDir + "/SKILL.md", "---\nname: report-writer\n---\n# Report\n"); + const paths = resolveSkillPaths(null, "report-writer"); + postInstall({ configFile: "/tmp/ssh-config", sandboxName: "alpha" }, paths, skillDir, { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "", stderr: "" }; + }, + }); + + // A command must copy the upload dir into the home mirror dir. + const mirrorCmd = commands.find( + (c) => c.includes(paths.uploadDir) && c.includes('"$HOME/.openclaw/skills/report-writer"'), + ); + expect(mirrorCmd, "postInstall should mirror the skill into $HOME/.openclaw/skills").toBeDefined(); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + } + }); + + it("warns when the OpenClaw home mirror cannot be created", () => { + const skillDir = mkdtempSync(join(tmpdir(), "skill-postinstall-mirror-fail-")); + try { + writeFileSync(skillDir + "/SKILL.md", "---\nname: report-writer\n---\n# Report\n"); + const paths = resolveSkillPaths(null, "report-writer"); + const result = postInstall( + { configFile: "/tmp/ssh-config", sandboxName: "alpha" }, + paths, + skillDir, + { + sshExecImpl: (_ctx, command) => ({ + // Fail only the mirror command; session refresh still succeeds. + status: command.includes("$HOME/.openclaw/skills") ? 1 : 0, + stdout: "", + stderr: "", + }), + }, + ); + + expect(result.success).toBe(true); + expect(result.messages.some((m) => m.startsWith("Warning:") && m.includes("mirror"))).toBe( + true, + ); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + } + }); +}); + +describe("verifyInstall", () => { + it("requires SKILL.md in the OpenClaw home mirror, not only the upload dir (#4819)", () => { + // The agent loads skills from the home mirror, so an install whose mirror + // copy failed must NOT verify as installed — otherwise the CLI reports + // success while the skill stays invisible to the agent. + const paths = resolveSkillPaths(null, "report-writer"); + const commands: string[] = []; + const ok = verifyInstall({ configFile: "/tmp/ssh-config", sandboxName: "alpha" }, paths, { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "EXISTS", stderr: "" }; + }, + }); + + expect(ok).toBe(true); + // The verification command must cover the home mirror SKILL.md. + expect(commands.some((c) => c.includes('"$HOME/.openclaw/skills/report-writer/SKILL.md"'))).toBe( + true, + ); + }); + + it("returns false when the upload dir has SKILL.md but the home mirror does not", () => { + const paths = resolveSkillPaths(null, "report-writer"); + const ok = verifyInstall({ configFile: "/tmp/ssh-config", sandboxName: "alpha" }, paths, { + // A combined `test -f A && test -f B` shell command fails (non-zero, + // no EXISTS) when the mirror file is absent. + sshExecImpl: () => ({ status: 1, stdout: "", stderr: "" }), + }); + + expect(ok).toBe(false); + }); }); diff --git a/src/lib/skill-install.ts b/src/lib/skill-install.ts index f6ca0d36194..bb28ac61a0e 100644 --- a/src/lib/skill-install.ts +++ b/src/lib/skill-install.ts @@ -16,18 +16,18 @@ 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"; +import { shellQuote, sshExec } from "./skill-remote"; export { validateSkillName } from "./skill-name"; export { checkExisting, type RemoveResult, removeSkill, - shellQuote, - sshExec, type SshContext, type SshResult, + shellQuote, + sshExec, verifyRemove, } from "./skill-remote"; @@ -242,6 +242,34 @@ export function postInstall( const runSsh = opts.sshExecImpl ?? sshExec; if (paths.isOpenClaw) { + // Mirror the uploaded skill into the agent's home dir + // ($HOME/.openclaw/skills/). The skill is uploaded to the OpenClaw + // state dir (uploadDir), which `openclaw skills list` reads, but the agent + // loads skills from $HOME/.openclaw/skills at session start. On sandboxes + // where the agent's $HOME differs from the state dir these paths diverge, + // so without this mirror the skill is listed but never invoked (#4819). + // `skill remove` already deletes this mirror, so install must create it to + // stay symmetric. The copy is skipped when both paths resolve to the same + // directory (the common case where $HOME is the state dir's parent), so it + // is a safe no-op there. + if (paths.mirrorDir) { + const src = shellQuote(paths.uploadDir); + // mirrorDir contains $HOME, which must expand on the remote shell, so we + // use double quotes (not shellQuote). Safe because skill names are + // restricted to [A-Za-z0-9._-] by parseFrontmatter / the name regex. + const dst = `"${paths.mirrorDir}"`; + const mirrorParent = `"${paths.mirrorDir.slice(0, paths.mirrorDir.lastIndexOf("/"))}"`; + const mirrorResult = runSsh( + ctx, + `[ ${src} -ef ${dst} ] || { mkdir -p ${mirrorParent} && rm -rf ${dst} && cp -a ${src} ${dst}; }`, + ); + if (!mirrorResult || mirrorResult.status !== 0) { + messages.push( + `Warning: failed to mirror skill into ${paths.mirrorDir} (agent may not load it)`, + ); + } + } + // Clear sessions.json so OpenClaw re-discovers skills on the next // session even after an in-place skill update. if (paths.sessionFile && !opts.skipRefresh) { @@ -258,10 +286,27 @@ export function postInstall( } /** - * Verify the SKILL.md file exists on the sandbox at the expected path. + * Verify the SKILL.md file exists on the sandbox. + * + * For OpenClaw the home mirror ($HOME/.openclaw/skills/) must also exist: + * that is the path the agent loads skills from at session start (#4819), so a + * successful upload whose mirror copy failed must NOT verify as installed — + * otherwise the CLI reports success while the skill stays invisible to the + * agent. This mirrors verifyRemove(), which already checks both paths. */ -export function verifyInstall(ctx: SshContext, paths: SkillPaths): boolean { - const target = shellQuote(`${paths.uploadDir}/SKILL.md`); - const result = sshExec(ctx, `test -f ${target} && echo EXISTS`); +export function verifyInstall( + ctx: SshContext, + paths: SkillPaths, + opts: { sshExecImpl?: typeof sshExec } = {}, +): boolean { + const checks = [`test -f ${shellQuote(`${paths.uploadDir}/SKILL.md`)}`]; + if (paths.isOpenClaw && paths.mirrorDir) { + // mirrorDir contains $HOME, which must expand on the remote shell, so we + // use double quotes (not shellQuote) — safe because skill names are + // restricted to [A-Za-z0-9._-]. + checks.push(`test -f "${paths.mirrorDir}/SKILL.md"`); + } + const runSsh = opts.sshExecImpl ?? sshExec; + const result = runSsh(ctx, `${checks.join(" && ")} && echo EXISTS`); return result !== null && result.stdout === "EXISTS"; }