From 611dbd237829e061a0436eabb9f0614d7c832b75 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Aug 2026 17:58:36 +0000 Subject: [PATCH] fix(cli): stop skills update deleting skills the manifest never covered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hyperframes skills update` deleted skills that the same command had just installed, from every agent directory on the machine, and reported them as "no longer published". `skills add --skill '*'` installs every skill in the repo — including the repo-native ones under `.claude/skills/` and `.agents/skills/` — and the upstream lock attributes all of them to `heygen-com/hyperframes`. The published manifest is generated from `/skills` only (gen-skills-manifest.ts), so it never lists those. detectRemoved read that silence as "removed upstream" and pruned them, so `check || update` could not converge: `add` reinstalled them and the next `update` deleted them again. Scope removed-detection to skills the manifest is actually authoritative for, using the lock's `skillPath` — the only field that separates a skill installed from `skills/` from one installed out of the same repo's other skill roots (`source` is identical for both). An entry with no `skillPath` is treated as not covered: this is a delete path, so unknown provenance fails safe. Also resolve the prune's manifest canonically. Its notion of "still published" could otherwise come from any `skills-manifest.json` within 16 parent directories of cwd, which — since HyperFrames' own manifest declares `source: heygen-com/hyperframes` — matches lock attribution and drives deletion. The install-side check already did this (#2176); the deleting path did not, and the comment claiming that was deliberate and "tested separately" had no such test. An explicit `--source` still wins. Verified end to end against the real CLI in a sandboxed HOME. Before: `add` installed 25 skills, `update` printed "Removing 6 skill(s) no longer published: captions-overlay, changelog-video, cut-the-curve, motion-doctrine, oversized-cursor, seam-craft" and deleted all six (27 dirs -> 21). After: no removal line, 27 -> 27. Both new regression tests fail on the pre-fix source. Fixes #3111 --- packages/cli/src/commands/skills.test.ts | 44 ++++++++- packages/cli/src/commands/skills.ts | 12 ++- packages/cli/src/utils/skillsManifest.test.ts | 97 ++++++++++++++++++- packages/cli/src/utils/skillsManifest.ts | 45 ++++++++- 4 files changed, 187 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/commands/skills.test.ts b/packages/cli/src/commands/skills.test.ts index 500c082eb8..771ae2afa6 100644 --- a/packages/cli/src/commands/skills.test.ts +++ b/packages/cli/src/commands/skills.test.ts @@ -425,11 +425,40 @@ describe("hyperframes skills", () => { await runSkillsUpdate(); - // The update engine's own check (first call) must ask for canonical; - // the prune's check (last call, tested separately) intentionally doesn't. + // The update engine's own check (first call) must ask for canonical. So + // must the prune's (see the GH #3111 regression below) — every caller that + // decides what is "still published" resolves the same way. expect(checkSkills).toHaveBeenNthCalledWith(1, expect.objectContaining({ canonical: true })); }); + // GH #3111 — silent, permanent data loss. The prune deletes; its notion of + // "no longer published" must therefore come from the canonical repo, never + // from resolveLatestManifest's findRepoManifest shortcut, which accepts any + // `skills-manifest.json` within 16 parent directories of cwd. HyperFrames' + // own manifest declares `source: heygen-com/hyperframes`, so such a file + // matches lock attribution, and every published skill missing from it is + // removed from every agent dir on the machine. + // + // Reproduced on the pre-fix build: running `skills update` from a hyperframes + // checkout whose manifest listed 19 of the 25 published skills printed + // "Removing 6 skill(s) no longer published: captions-overlay, changelog-video, + // cut-the-curve, motion-doctrine, oversized-cursor, seam-craft" and deleted + // all six — every one of them currently published. + it("resolves the prune's manifest canonically, so a local manifest can never drive deletion", async () => { + setPlatform("linux"); + const { checkSkills } = await import("../utils/skillsManifest.js"); + + await runSkillsUpdate(); + + // The prune's check is the LAST call; assert on every call so a future + // caller can't reintroduce a non-canonical deletion path. + const calls = vi.mocked(checkSkills).mock.calls; + expect(calls.length).toBeGreaterThan(1); + for (const [arg] of calls) { + expect(arg).toEqual(expect.objectContaining({ canonical: true })); + } + }); + // Retired-skill regression (variant 2): `skills remove` is a silent no-op // for a lock entry with no on-disk bundle (upstream scans disk, not the // lock, to decide what's "installed" — see pruneOrphanedLockEntries's @@ -496,9 +525,14 @@ describe("hyperframes skills", () => { await runSkillsUpdate({ source: "owner/repo", dir: "/custom/skills" }); // The last checkSkills call is the prune's — the update engine's own check - // (first call) intentionally uses default detection, matching where the - // install actually lands. - expect(checkSkills).toHaveBeenLastCalledWith({ source: "owner/repo", dir: "/custom/skills" }); + // (first call) doesn't take --source/--dir, matching where the install + // actually lands. `canonical` rides along on every call (GH #3111); an + // explicit --source still wins over it inside resolveLatestManifest. + expect(checkSkills).toHaveBeenLastCalledWith({ + source: "owner/repo", + dir: "/custom/skills", + canonical: true, + }); }); // Skill names come from lock-file JSON keys; a flag-like / shell-special name diff --git a/packages/cli/src/commands/skills.ts b/packages/cli/src/commands/skills.ts index d7dcbd98d9..086fcd122a 100644 --- a/packages/cli/src/commands/skills.ts +++ b/packages/cli/src/commands/skills.ts @@ -727,7 +727,17 @@ const updateCommand = defineCommand({ // failure doesn't fail the update — the install the CI contract gates on // already succeeded. try { - const { skills, scope } = await checkSkills({ dir, source }); + // `canonical: true` for the same reason the install's target selection + // uses it (see updateSkills) — and more urgently, because this branch + // DELETES. Without it, resolveLatestManifest takes the findRepoManifest + // shortcut: any `skills-manifest.json` within 16 parent dirs of cwd + // becomes "latest". HyperFrames' own repo manifest declares + // `source: heygen-com/hyperframes`, so a checkout (or any project + // carrying a copy) matches attribution and every published skill absent + // from that local file is deleted globally as "no longer published". + // An explicit `--source` still wins — canonical only decides what + // "latest" means when no source was given. GH #3111. + const { skills, scope } = await checkSkills({ dir, source, canonical: true }); const removed = skills.filter((s) => s.status === "removed").map((s) => s.name); if (removed.length) { console.log(); diff --git a/packages/cli/src/utils/skillsManifest.test.ts b/packages/cli/src/utils/skillsManifest.test.ts index 586ffab6b4..7828a953b9 100644 --- a/packages/cli/src/utils/skillsManifest.test.ts +++ b/packages/cli/src/utils/skillsManifest.test.ts @@ -495,8 +495,27 @@ describe("checkSkills removed-upstream detection", () => { writeFileSync(manifestPath, JSON.stringify(manifest)); return { home, opts: { source: manifestPath, cwd: project, home } }; } - function writeGlobalLock(home: string, skills: Record): void { - writeFileSync(join(home, ".agents/.skill-lock.json"), JSON.stringify({ version: 3, skills })); + // Upstream writes a `skillPath` on every lock entry it creates (verified + // against a real ~/.agents/.skill-lock.json written by skills@1.5.22), and + // removed-detection now reads it to tell manifest-covered skills apart from + // ones installed out of the repo's other skill roots — see + // manifestCoversSkill / GH #3111. Default to the covered root so each existing + // fixture keeps meaning "a normally published skill"; pass skillPath + // explicitly to model anything else. + function writeGlobalLock( + home: string, + skills: Record, + ): void { + const withPaths = Object.fromEntries( + Object.entries(skills).map(([name, entry]) => [ + name, + { skillPath: `skills/${name}/SKILL.md`, ...entry }, + ]), + ); + writeFileSync( + join(home, ".agents/.skill-lock.json"), + JSON.stringify({ version: 3, skills: withPaths }), + ); } it("flags a lock-attributed skill the manifest dropped, ignoring other sources", async () => { @@ -514,6 +533,70 @@ describe("checkSkills removed-upstream detection", () => { expect(res.summary.removed).toBe(1); }); + // GH #3111 — the data-loss regression. `skills add --skill '*'` installs every + // skill in the repo, including the repo-native ones under `.claude/skills/` + // and `.agents/skills/`, and attributes them all to our source. The published + // manifest is generated from `/skills` ONLY (gen-skills-manifest.ts), + // so it never lists them — and reading that silence as "no longer published" + // deleted them from every agent directory on the machine, immediately after + // the same command installed them. + // + // Reproduced end-to-end pre-fix against the real CLI: `skills add` installed + // 25 skills, then `skills update` printed "Removing 6 skill(s) no longer + // published: captions-overlay, changelog-video, cut-the-curve, motion-doctrine, + // oversized-cursor, seam-craft" and deleted all six. Their lock entries carried + // `.agents/skills//SKILL.md`; the survivors carried `skills//…`. + it("never prunes a skill installed outside the manifest's coverage root", async () => { + const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); + writeGlobalLock(home, { + alpha: { source: "test" }, // published, in the manifest → untouched + gamma: { + source: "test", // ours, absent from the manifest… + skillPath: ".agents/skills/gamma/SKILL.md", // …but the manifest never covered it + }, + }); + + const res = await checkSkills(opts); + + const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status])); + expect(byName.gamma).not.toBe("removed"); + expect(res.summary.removed).toBe(0); + }); + + // The other half of the contract: the coverage filter must not blunt the + // retired-skill convergence #2176 added. A skill installed FROM `skills/` and + // since dropped from the manifest is still a real removal. + it("still prunes a manifest-covered skill that was genuinely dropped upstream", async () => { + const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); + writeGlobalLock(home, { + alpha: { source: "test" }, + gamma: { source: "test", skillPath: "skills/gamma/SKILL.md" }, + }); + + const res = await checkSkills(opts); + + expect(Object.fromEntries(res.skills.map((s) => [s.name, s.status])).gamma).toBe("removed"); + expect(res.summary.removed).toBe(1); + }); + + // Fail safe on unknown provenance: an entry written by an older upstream that + // recorded no skillPath cannot be shown to be manifest-covered, and this is a + // DELETE path — so it is left alone rather than guessed at. + it("leaves an entry with no skillPath alone rather than guessing", async () => { + const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); + writeFileSync( + join(home, ".agents/.skill-lock.json"), + JSON.stringify({ + version: 3, + skills: { alpha: { source: "test" }, gamma: { source: "test" } }, // no skillPath at all + }), + ); + + const res = await checkSkills(opts); + + expect(res.summary.removed).toBe(0); + }); + it("a removed skill alone makes an update available (no outdated/missing)", async () => { // Manifest lists only alpha, with its REAL hash → "current" (not outdated). const { home, opts } = setup({ source: "test", skills: {} }); @@ -591,7 +674,10 @@ describe("checkSkills removed-upstream detection", () => { join(project, "skills-lock.json"), JSON.stringify({ version: 1, - skills: { alpha: { source: "test" }, gamma: { source: "test" } }, + skills: { + alpha: { source: "test", skillPath: "skills/alpha/SKILL.md" }, + gamma: { source: "test", skillPath: "skills/gamma/SKILL.md" }, + }, }), ); @@ -618,7 +704,10 @@ describe("checkSkills removed-upstream detection", () => { join(project, "skills-lock.json"), JSON.stringify({ version: 1, - skills: { alpha: { source: "test" }, gamma: { source: "test" } }, + skills: { + alpha: { source: "test", skillPath: "skills/alpha/SKILL.md" }, + gamma: { source: "test", skillPath: "skills/gamma/SKILL.md" }, + }, }), ); const home = join(root, "home2"); diff --git a/packages/cli/src/utils/skillsManifest.ts b/packages/cli/src/utils/skillsManifest.ts index 5145cc0348..5213b00329 100644 --- a/packages/cli/src/utils/skillsManifest.ts +++ b/packages/cli/src/utils/skillsManifest.ts @@ -429,6 +429,13 @@ export function diffSkills( interface LockEntry { source?: string; sourceUrl?: string; + /** + * Path of the skill's SKILL.md within the source repo, as upstream records it + * (`skills/general-video/SKILL.md`, `.agents/skills/seam-craft/SKILL.md`). The + * only field distinguishing skills the published manifest covers from ones + * installed out of the same repo's other skill roots — see manifestCoversSkill. + */ + skillPath?: string; } /** The slice of the vercel-labs/skills lock file we read. */ @@ -508,7 +515,42 @@ interface RemovedResult { lockMissing: boolean; } -/** Skills the lock attributes to our source that the manifest no longer ships. */ +/** + * The repo directory the published manifest is generated from — see + * `packages/cli/scripts/gen-skills-manifest.ts`, which walks `/skills` + * and nothing else. Anything installed from a DIFFERENT root of the same repo + * (`.claude/skills/`, `.agents/skills/` — the repo-native contributor skills) is + * outside the manifest's coverage, so the manifest says nothing about it. + */ +const MANIFEST_COVERAGE_ROOT = "skills/"; + +/** + * Is the manifest authoritative about whether this skill still exists upstream? + * + * Only for skills installed from the directory the manifest is generated from. + * The lock records where in the repo each skill came from (`skillPath`, e.g. + * `skills/general-video/SKILL.md` vs `.agents/skills/seam-craft/SKILL.md`), and + * both carry the same `source`, so source attribution alone cannot tell them + * apart. An entry with no `skillPath` (older lock format) is treated as NOT + * covered — for a delete, unknown provenance must fail safe. GH #3111. + */ +function manifestCoversSkill(entry: LockEntry | undefined): boolean { + const path = entry?.skillPath; + return typeof path === "string" && path.startsWith(MANIFEST_COVERAGE_ROOT); +} + +/** + * Skills the lock attributes to our source that the manifest no longer ships. + * + * "Absent from the manifest" only means "removed upstream" for skills the + * manifest actually covers. `skills add --skill '*'` installs every skill in the + * repo — including the repo-native ones under `.claude/skills/` and + * `.agents/skills/`, which the published manifest deliberately omits — and + * attributes them all to our source. Without the coverage filter, every install + * is immediately followed by a prune that deletes those skills as "no longer + * published", so `check || update` never converges: `add` reinstalls them and + * the next `update` deletes them again, forever. GH #3111. + */ function detectRemoved( root: SkillRoot, latest: SkillsManifest, @@ -516,6 +558,7 @@ function detectRemoved( ): RemovedResult { const lock = readSkillLock(lockPathForScope(root.scope, opts)); const removed = skillsAttributedToSource(lock, latest.source) + .filter((name) => manifestCoversSkill(lock?.skills?.[name])) .filter((name) => !(name in latest.skills)) .sort() .map((name) => ({ name, status: "removed" as const }));