Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/lib/actions/sandbox/skill-install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
101 changes: 95 additions & 6 deletions src/lib/skill-install.test.ts
Original file line number Diff line number Diff line change
@@ -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", () => {
Expand Down Expand Up @@ -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);
});
});
59 changes: 52 additions & 7 deletions src/lib/skill-install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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/<name>). 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) {
Expand All @@ -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/<name>) 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";
}
Loading