From 624c3da1835d8f9ec8c51823683a3a1e1dcfff1b Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Fri, 28 Aug 2026 21:38:29 +0000 Subject: [PATCH] skill install: handle a repository that is a collection of skills `skill install` assumed a repository is one skill: it clones the source into `/` and stops. Claude and Kimi discover skills by scanning exactly one level deep, so a repository whose skills live in subdirectories lands every one of them a level too deep, where nothing will ever find them. `git clone` still exits 0, so moshcode reports the install succeeded. The user gets a green summary and zero usable skills, with nothing to suggest otherwise. - `skillCollection(dir)` reports what a clone actually contains: `single` (a SKILL.md at the root), `collection` (subdirectories holding one), or `empty` - `settleSkillClone(dir)` resolves a clone into the shape engines scan. A single skill is left alone; a collection has each skill moved up beside its siblings and the wrapper removed, since the wrapper holds the repository's README, tooling and CI, none of which is a skill; an empty clone is removed rather than left as a directory that can never resolve - claude and kimi actions carry the clone target as `settle`, so the runner resolves them after a successful clone. Gemini installs natively and is never settled - a clone containing no SKILL.md anywhere is now reported as failed with that reason, instead of counting as installed - results carry `kind`, `skills` and `kept`, so the summary can say what landed 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. `settle` is injectable alongside `run`, matching how the suite already stubs subprocesses. Three existing tests stubbed `run` without it, so the real settle correctly found an empty directory where a stubbed clone never landed; they now stub both. Verified end to end against a real 13-skill collection: 0 discoverable before, 13 after, wrapper removed. Full suite green (2171 pass, 0 fail). Extends prd/0003. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01B7KVQWbU17PR2mCUn8jjpH --- src/integrations.mjs | 7 +- src/skills.mjs | 84 ++++++++++++- test/integrations-exit-code.test.mjs | 6 +- test/skill-install-collections.test.mjs | 159 ++++++++++++++++++++++++ test/skill-stray-flag.test.mjs | 11 +- test/skills.test.mjs | 13 +- 6 files changed, 266 insertions(+), 14 deletions(-) create mode 100644 test/skill-install-collections.test.mjs diff --git a/src/integrations.mjs b/src/integrations.mjs index 825758e4..4df48e74 100644 --- a/src/integrations.mjs +++ b/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; } diff --git a/src/skills.mjs b/src/skills.mjs index b4619241..ef289adf 100644 --- a/src/skills.mjs +++ b/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; } diff --git a/test/integrations-exit-code.test.mjs b/test/integrations-exit-code.test.mjs index cacbaa18..08dd1f92 100644 --- a/test/integrations-exit-code.test.mjs +++ b/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); }); diff --git a/test/skill-install-collections.test.mjs b/test/skill-install-collections.test.mjs new file mode 100644 index 00000000..58adb7d6 --- /dev/null +++ b/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); +}); diff --git a/test/skill-stray-flag.test.mjs b/test/skill-stray-flag.test.mjs index 685a8589..bcba7494 100644 --- a/test/skill-stray-flag.test.mjs +++ b/test/skill-stray-flag.test.mjs @@ -75,9 +75,14 @@ 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(" | ")}`); @@ -85,14 +90,14 @@ test("a normal git URL still installs across the skills engines", async () => { 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(" | ")}`); }); diff --git a/test/skills.test.mjs b/test/skills.test.mjs index 53d8b6ab..8cafa732 100644 --- a/test/skills.test.mjs +++ b/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", () => {