skill install: handle a repository that is a collection of skills

src/integrations.mjs

@@ -253,7 +253,7 @@ export async function mcpCommand(tokens, { run, installedSet } = {}) {
 }
 
 /** Run `/skill …`. `tokens` are the words after `skill`. `run`/`installedSet` are injectable for tests. */
-export async function skillCommand(tokens, { run, installedSet } = {}) {
+export async function skillCommand(tokens, { run, installedSet, settle } = {}) {
  const verb = tokens[0];
  if (!verb || verb === "list") { printSkillTargets(tokens.slice(1).includes("--json")); return 0; }
  if (verb !== "install") {
@@ -287,7 +287,10 @@ export async function skillCommand(tokens, { run, installedSet } = {}) {
 
  const spec = { source, name: skillName(source, name) };
  console.log(info(`installing skill ${bone(spec.name)} → ${ash(source)} across skills engines…`));
- const results = await runSkillInstall(planSkillInstall(spec, { installedSet }), run ? { run } : {});
+ const results = await runSkillInstall(planSkillInstall(spec, { installedSet }), {
+ ...(run ? { run } : {}),
+ ...(settle ? { settle } : {}),
+ });
  summarize(results);
  return anyFailed(results) ? 1 : 0;
 }

src/skills.mjs

@@ -1,6 +1,7 @@
 // Install Agent Skills across every engine that has a skills primitive, from one
 // source (a git URL or local path). Gemini installs natively; Claude clones the
 // source into its personal skills dir. See prd/0003.
+import fs from "node:fs";
 import os from "node:os";
 import path from "node:path";
 import { ENGINES, isInstalled, ranOk, runCmd } from "./engines.mjs";
@@ -38,6 +39,61 @@ export function skillName(source, override) {
  return named(sanitize(path.basename(path.resolve(raw)))) || "skill";
 }
 
+/**
+ * What a freshly cloned skill source actually contains.
+ *
+ * A repository is not always one skill. `SKILL.md` at the root is the common
+ * shape and the one this module assumed. But a repository can equally be a
+ * *collection* — subdirectories that each hold a `SKILL.md` — and every engine
+ * that discovers skills by scanning looks exactly one level deep. Cloning a
+ * collection whole therefore lands every skill one level too deep, where
+ * nothing will ever find them, while `git clone` still exits 0 and the install
+ * reports success. Detecting the shape is what makes that failure impossible.
+ */
+export function skillCollection(dir) {
+ if (!fs.existsSync(dir)) return { kind: "empty", names: [] };
+ if (fs.existsSync(path.join(dir, "SKILL.md"))) return { kind: "single", names: [] };
+ const names = fs
+ .readdirSync(dir, { withFileTypes: true })
+ .filter((d) => d.isDirectory() && !d.name.startsWith("."))
+ .filter((d) => fs.existsSync(path.join(dir, d.name, "SKILL.md")))
+ .map((d) => d.name)
+ .sort();
+ return names.length ? { kind: "collection", names } : { kind: "empty", names: [] };
+}
+
+/**
+ * Settle a fresh clone into the shape the engine scans, and report what it was.
+ *
+ * `single` is left exactly as cloned. `collection` has each skill moved up
+ * beside its siblings and the wrapper removed — the wrapper holds the
+ * repository's own README, tooling and CI, none of which is a skill. `empty`
+ * removes the clone rather than leaving a directory that can never resolve.
+ *
+ * A skill whose name is already taken is left alone and reported in `kept`:
+ * this runs inside the user's real skills directory, so a name collision must
+ * never silently replace a skill they already had.
+ */
+export function settleSkillClone(dir) {
+ const { kind, names } = skillCollection(dir);
+ if (kind === "single") return { kind, installed: [path.basename(dir)], kept: [] };
+ if (kind === "empty") {
+ fs.rmSync(dir, { recursive: true, force: true });
+ return { kind, installed: [], kept: [] };
+ }
+ const parent = path.dirname(dir);
+ const installed = [];
+ const kept = [];
+ for (const name of names) {
+ const dest = path.join(parent, name);
+ if (fs.existsSync(dest)) { kept.push(name); continue; }
+ fs.renameSync(path.join(dir, name), dest);
+ installed.push(name);
+ }
+ fs.rmSync(dir, { recursive: true, force: true });
+ return { kind, installed, kept };
+}
+
 /**
  * The install action for one engine: a spawnable { cmd, args } or a { skip }
  * reason. `spec: { source, name }`.
@@ -47,13 +103,19 @@ export function skillInstallAction(key, spec) {
  switch (key) {
  case "gemini":
  return { cmd: "gemini", args: ["skills", "install", source, "--scope", "user"] };
- case "claude":
+ case "claude": {
  // Claude has no `skill install`; clone the source into its skills dir.
- return { cmd: "git", args: ["clone", "--depth", "1", source, path.join(claudeSkillsDir(), name)] };
- case "kimi":
+ // `settle` is the cloned path: a scanning engine needs the clone resolved
+ // into one-level-deep skills afterwards (see settleSkillClone).
+ const dir = path.join(claudeSkillsDir(), name);
+ return { cmd: "git", args: ["clone", "--depth", "1", source, dir], settle: dir };
+ }
+ case "kimi": {
  // Kimi Code discovers skills by scanning directories, with no install
  // command of its own — so clone into the one it scans, as Claude does.
- return { cmd: "git", args: ["clone", "--depth", "1", source, path.join(kimiSkillsDir(), name)] };
+ const dir = path.join(kimiSkillsDir(), name);
+ return { cmd: "git", args: ["clone", "--depth", "1", source, dir], settle: dir };
+ }
  default:
  return { skip: "no skills primitive" };
  }
@@ -81,13 +143,23 @@ export function planSkillInstall(spec, { installedSet } = {}) {
  * [{ key, status: "installed"|"skipped"|"failed"|"not-installed", reason? }].
  * `run` is injectable for tests.
  */
-export async function runSkillInstall(plan, { run = runCmd } = {}) {
+export async function runSkillInstall(plan, { run = runCmd, settle = settleSkillClone } = {}) {
  const results = [];
  for (const item of plan) {
  if (item.skip) { results.push({ key: item.key, status: "skipped", reason: item.skip }); continue; }
  if (!item.installed) { results.push({ key: item.key, status: "not-installed" }); continue; }
  const r = await run(item.cmd, item.args);
- results.push({ key: item.key, status: ranOk(r) ? "installed" : "failed", code: r.code, signal: r.signal ?? null });
+ const base = { key: item.key, code: r.code, signal: r.signal ?? null };
+ if (!ranOk(r)) { results.push({ ...base, status: "failed" }); continue; }
+ if (!item.settle) { results.push({ ...base, status: "installed" }); continue; }
+
+ // The clone succeeded, which is not the same as a skill being installed.
+ const { kind, installed, kept } = settle(item.settle);
+ if (kind === "empty") {
+ results.push({ ...base, status: "failed", reason: "no SKILL.md at the root or in any subdirectory" });
+ continue;
+ }
+ results.push({ ...base, status: "installed", kind, skills: installed, ...(kept.length ? { kept } : {}) });
  }
  return results;
 }

test/integrations-exit-code.test.mjs

@@ -64,7 +64,11 @@ test("mcp add still exits 0 when every engine registered the server", async () =
 });
 
 test("skill install still exits 0 when every engine installed the skill", async () => {
- const code = await quietly(() => skillCommand(INSTALL, { run: OK, installedSet: ALL }));
+ // `run` is stubbed, so no clone lands and the real settle would correctly
+ // report an empty directory. This test is about the exit code, not about
+ // what the clone contained.
+ const settle = () => ({ kind: "single", installed: ["some-skill"], kept: [] });
+ const code = await quietly(() => skillCommand(INSTALL, { run: OK, installedSet: ALL, settle }));
  assert.equal(code, 0);
 });
 

test/skill-install-collections.test.mjs

@@ -0,0 +1,159 @@
+// A skills repository is not always one skill. Engines that discover skills by
+// scanning look exactly one level deep, so cloning a *collection* whole lands
+// every skill one level too deep — where nothing finds them — while `git clone`
+// exits 0 and the install reports success. These tests pin the shape detection
+// that makes that silent failure impossible.
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import test from "node:test";
+
+import {
+ planSkillInstall, runSkillInstall, settleSkillClone, skillCollection,
+} from "../src/skills.mjs";
+
+const tmp = () => fs.mkdtempSync(path.join(os.tmpdir(), "moshcode-skills-"));
+const skill = (dir, name) => {
+ fs.mkdirSync(path.join(dir, name), { recursive: true });
+ fs.writeFileSync(path.join(dir, name, "SKILL.md"), `---\nname: ${name}\n---\n`);
+};
+
+// --- shape detection ---------------------------------------------------------
+
+test("a SKILL.md at the root is one skill", () => {
+ const root = tmp();
+ const dir = path.join(root, "some-skill");
+ fs.mkdirSync(dir);
+ fs.writeFileSync(path.join(dir, "SKILL.md"), "---\nname: some-skill\n---\n");
+ assert.deepEqual(skillCollection(dir), { kind: "single", names: [] });
+});
+
+test("subdirectories holding SKILL.md are a collection", () => {
+ const root = tmp();
+ const dir = path.join(root, "a-collection");
+ fs.mkdirSync(dir);
+ skill(dir, "beta");
+ skill(dir, "alpha");
+ // A collection's own tooling must not be mistaken for a skill.
+ fs.mkdirSync(path.join(dir, "bin"));
+ fs.writeFileSync(path.join(dir, "README.md"), "# not a skill\n");
+ assert.deepEqual(skillCollection(dir), { kind: "collection", names: ["alpha", "beta"] });
+});
+
+test("a repository with no SKILL.md anywhere is empty, not a collection", () => {
+ const root = tmp();
+ const dir = path.join(root, "not-skills");
+ fs.mkdirSync(path.join(dir, "src"), { recursive: true });
+ fs.writeFileSync(path.join(dir, "README.md"), "# nope\n");
+ assert.deepEqual(skillCollection(dir), { kind: "empty", names: [] });
+});
+
+test("dot-directories are not skills", () => {
+ const root = tmp();
+ const dir = path.join(root, "c");
+ fs.mkdirSync(dir);
+ skill(dir, ".hidden");
+ assert.equal(skillCollection(dir).kind, "empty");
+});
+
+// --- settling ----------------------------------------------------------------
+
+test("settling a collection lifts each skill one level and drops the wrapper", () => {
+ const root = tmp();
+ const dir = path.join(root, "a-collection");
+ fs.mkdirSync(dir);
+ skill(dir, "alpha");
+ skill(dir, "beta");
+
+ const res = settleSkillClone(dir);
+
+ assert.deepEqual(res.installed, ["alpha", "beta"]);
+ assert.equal(fs.existsSync(dir), false, "the wrapper must not survive");
+ for (const name of ["alpha", "beta"]) {
+ assert.ok(fs.existsSync(path.join(root, name, "SKILL.md")), `${name} must sit one level deep`);
+ }
+});
+
+test("settling leaves a single skill exactly where it was cloned", () => {
+ const root = tmp();
+ const dir = path.join(root, "some-skill");
+ fs.mkdirSync(dir);
+ fs.writeFileSync(path.join(dir, "SKILL.md"), "---\nname: some-skill\n---\n");
+
+ const res = settleSkillClone(dir);
+
+ assert.equal(res.kind, "single");
+ assert.deepEqual(res.installed, ["some-skill"]);
+ assert.ok(fs.existsSync(path.join(dir, "SKILL.md")));
+});
+
+test("settling never replaces a skill the user already had", () => {
+ const root = tmp();
+ fs.mkdirSync(path.join(root, "alpha"));
+ fs.writeFileSync(path.join(root, "alpha", "SKILL.md"), "MINE");
+
+ const dir = path.join(root, "a-collection");
+ fs.mkdirSync(dir);
+ skill(dir, "alpha");
+ skill(dir, "beta");
+
+ const res = settleSkillClone(dir);
+
+ assert.deepEqual(res.kept, ["alpha"]);
+ assert.deepEqual(res.installed, ["beta"]);
+ assert.equal(fs.readFileSync(path.join(root, "alpha", "SKILL.md"), "utf8"), "MINE");
+});
+
+test("settling an empty clone removes it rather than leaving a dead directory", () => {
+ const root = tmp();
+ const dir = path.join(root, "not-skills");
+ fs.mkdirSync(dir);
+ fs.writeFileSync(path.join(dir, "README.md"), "# nope\n");
+
+ assert.equal(settleSkillClone(dir).kind, "empty");
+ assert.equal(fs.existsSync(dir), false);
+});
+
+// --- the fan-out reports what actually happened ------------------------------
+
+const SPEC = { source: "https://github.com/acme/a-collection", name: "a-collection" };
+const claudeOnly = () => planSkillInstall(SPEC, { installedSet: new Set(["claude"]) });
+const ok = async () => ({ ok: true, code: 0 });
+const byKey = (r) => Object.fromEntries(r.map((x) => [x.key, x]));
+
+test("a collection install reports the skills it actually installed", async () => {
+ const results = await runSkillInstall(claudeOnly(), {
+ run: ok,
+ settle: () => ({ kind: "collection", installed: ["alpha", "beta"], kept: [] }),
+ });
+ const claude = byKey(results).claude;
+ assert.equal(claude.status, "installed");
+ assert.equal(claude.kind, "collection");
+ assert.deepEqual(claude.skills, ["alpha", "beta"]);
+});
+
+test("a clone that contains no skill is a failure, not a silent success", async () => {
+ const results = await runSkillInstall(claudeOnly(), {
+ run: ok,
+ settle: () => ({ kind: "empty", installed: [], kept: [] }),
+ });
+ const claude = byKey(results).claude;
+ assert.equal(claude.status, "failed", "git exiting 0 must not read as installed");
+ assert.match(claude.reason, /no SKILL\.md/);
+});
+
+test("a failed clone is not settled at all", async () => {
+ let settled = false;
+ const results = await runSkillInstall(claudeOnly(), {
+ run: async () => ({ ok: false, code: 128 }),
+ settle: () => { settled = true; return { kind: "empty", installed: [], kept: [] }; },
+ });
+ assert.equal(byKey(results).claude.status, "failed");
+ assert.equal(settled, false, "nothing to settle when the clone never landed");
+});
+
+test("gemini installs natively and is never settled", () => {
+ const gemini = planSkillInstall(SPEC, { installedSet: new Set(["gemini"]) }).find((p) => p.key === "gemini");
+ assert.equal(gemini.settle, undefined);
+});

test/skill-stray-flag.test.mjs

@@ -75,24 +75,29 @@ test("the error names the flag skill install does take, and how to escape a real
 
 // --- controls: the opposite direction ---------------------------------------
 
+// `run` is stubbed here, so no clone actually lands and the real settle would
+// (correctly) report an empty directory. These tests are about flag parsing,
+// not about what the clone contained.
+const settled = () => ({ kind: "single", installed: ["y"], kept: [] });
+
 test("a normal git URL still installs across the skills engines", async () => {
  const { run, calls } = spy();
- const { code } = await capture(() => skillCommand(["install", URL], { run, installedSet: ALL }));
+ const { code } = await capture(() => skillCommand(["install", URL], { run, installedSet: ALL, settle: settled }));
  assert.equal(code, 0);
  assert.ok(calls.some((c) => c.startsWith("git clone") && c.includes(URL)), `expected a clone of the source, got ${calls.join(" | ")}`);
  assert.ok(calls.some((c) => c.startsWith(`${ENGINES.gemini.bin} skills install ${URL}`)), `expected gemini to be handed the source, got ${calls.join(" | ")}`);
 });
 
 test("--name still parses and still names the skill", async () => {
  const { run, calls } = spy();
- const { code } = await capture(() => skillCommand(["install", URL, "--name", "renamed"], { run, installedSet: ALL }));
+ const { code } = await capture(() => skillCommand(["install", URL, "--name", "renamed"], { run, installedSet: ALL, settle: settled }));
  assert.equal(code, 0);
  assert.ok(calls.some((c) => c.includes("/renamed")), `expected the clone to land in .../renamed, got ${calls.join(" | ")}`);
 });
 
 test("a local path source is untouched by the guard", async () => {
  const { run, calls } = spy();
- const { code } = await capture(() => skillCommand(["install", "./my-skill"], { run, installedSet: ALL }));
+ const { code } = await capture(() => skillCommand(["install", "./my-skill"], { run, installedSet: ALL, settle: settled }));
  assert.equal(code, 0);
  assert.ok(calls.some((c) => c.includes("./my-skill")), `expected the path to survive, got ${calls.join(" | ")}`);
 });

test/skills.test.mjs

@@ -44,11 +44,20 @@ test("skillInstallAction: gemini installs natively, claude clones into its skill
  assert.deepEqual(gemini, { cmd: "gemini", args: ["skills", "install", "https://x/y", "--scope", "user"] });
 
  const claude = skillInstallAction("claude", { source: "https://x/y", name: "y" });
- assert.deepEqual(claude, { cmd: "git", args: ["clone", "--depth", "1", "https://x/y", path.join(claudeSkillsDir(), "y")] });
+ assert.deepEqual(claude, {
+ cmd: "git",
+ args: ["clone", "--depth", "1", "https://x/y", path.join(claudeSkillsDir(), "y")],
+ // Carried so the runner can resolve the clone into the depth engines scan.
+ settle: path.join(claudeSkillsDir(), "y"),
+ });
 
  // Kimi Code discovers skills by scanning dirs too, so it clones into its own.
  const kimi = skillInstallAction("kimi", { source: "https://x/y", name: "y" });
- assert.deepEqual(kimi, { cmd: "git", args: ["clone", "--depth", "1", "https://x/y", path.join(kimiSkillsDir(), "y")] });
+ assert.deepEqual(kimi, {
+ cmd: "git",
+ args: ["clone", "--depth", "1", "https://x/y", path.join(kimiSkillsDir(), "y")],
+ settle: path.join(kimiSkillsDir(), "y"),
+ });
 });
 
 test("kimiSkillsDir follows KIMI_CODE_HOME, which is what moves kimi's skills", () => {