fix(cli): stop skills update deleting skills the manifest never covered

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

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();

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<string, { source: string }>): 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<string, { source: string; skillPath?: string }>,
+  ): 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 `<repoRoot>/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/<name>/SKILL.md`; the survivors carried `skills/<name>/…`.
+  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");

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,14 +515,50 @@ 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 `<repoRoot>/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,
   opts: { cwd?: string; home?: string },
 ): 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 }));