Skip to content

Commit ed3ff98

Browse files
authored
fix(cli): stop skills update deleting skills the manifest never covered (heygen-com#3118)
`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 `<repoRoot>/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 (heygen-com#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 heygen-com#3111
1 parent b08cefe commit ed3ff98

4 files changed

Lines changed: 187 additions & 11 deletions

File tree

‎packages/cli/src/commands/skills.test.ts‎

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -425,11 +425,40 @@ describe("hyperframes skills", () => {
425425

426426
await runSkillsUpdate();
427427

428-
// The update engine's own check (first call) must ask for canonical;
429-
// the prune's check (last call, tested separately) intentionally doesn't.
428+
// The update engine's own check (first call) must ask for canonical. So
429+
// must the prune's (see the GH #3111 regression below) — every caller that
430+
// decides what is "still published" resolves the same way.
430431
expect(checkSkills).toHaveBeenNthCalledWith(1, expect.objectContaining({ canonical: true }));
431432
});
432433

434+
// GH #3111 — silent, permanent data loss. The prune deletes; its notion of
435+
// "no longer published" must therefore come from the canonical repo, never
436+
// from resolveLatestManifest's findRepoManifest shortcut, which accepts any
437+
// `skills-manifest.json` within 16 parent directories of cwd. HyperFrames'
438+
// own manifest declares `source: heygen-com/hyperframes`, so such a file
439+
// matches lock attribution, and every published skill missing from it is
440+
// removed from every agent dir on the machine.
441+
//
442+
// Reproduced on the pre-fix build: running `skills update` from a hyperframes
443+
// checkout whose manifest listed 19 of the 25 published skills printed
444+
// "Removing 6 skill(s) no longer published: captions-overlay, changelog-video,
445+
// cut-the-curve, motion-doctrine, oversized-cursor, seam-craft" and deleted
446+
// all six — every one of them currently published.
447+
it("resolves the prune's manifest canonically, so a local manifest can never drive deletion", async () => {
448+
setPlatform("linux");
449+
const { checkSkills } = await import("../utils/skillsManifest.js");
450+
451+
await runSkillsUpdate();
452+
453+
// The prune's check is the LAST call; assert on every call so a future
454+
// caller can't reintroduce a non-canonical deletion path.
455+
const calls = vi.mocked(checkSkills).mock.calls;
456+
expect(calls.length).toBeGreaterThan(1);
457+
for (const [arg] of calls) {
458+
expect(arg).toEqual(expect.objectContaining({ canonical: true }));
459+
}
460+
});
461+
433462
// Retired-skill regression (variant 2): `skills remove` is a silent no-op
434463
// for a lock entry with no on-disk bundle (upstream scans disk, not the
435464
// lock, to decide what's "installed" — see pruneOrphanedLockEntries's
@@ -496,9 +525,14 @@ describe("hyperframes skills", () => {
496525
await runSkillsUpdate({ source: "owner/repo", dir: "/custom/skills" });
497526

498527
// The last checkSkills call is the prune's — the update engine's own check
499-
// (first call) intentionally uses default detection, matching where the
500-
// install actually lands.
501-
expect(checkSkills).toHaveBeenLastCalledWith({ source: "owner/repo", dir: "/custom/skills" });
528+
// (first call) doesn't take --source/--dir, matching where the install
529+
// actually lands. `canonical` rides along on every call (GH #3111); an
530+
// explicit --source still wins over it inside resolveLatestManifest.
531+
expect(checkSkills).toHaveBeenLastCalledWith({
532+
source: "owner/repo",
533+
dir: "/custom/skills",
534+
canonical: true,
535+
});
502536
});
503537

504538
// Skill names come from lock-file JSON keys; a flag-like / shell-special name

‎packages/cli/src/commands/skills.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -727,7 +727,17 @@ const updateCommand = defineCommand({
727727
// failure doesn't fail the update — the install the CI contract gates on
728728
// already succeeded.
729729
try {
730-
const { skills, scope } = await checkSkills({ dir, source });
730+
// `canonical: true` for the same reason the install's target selection
731+
// uses it (see updateSkills) — and more urgently, because this branch
732+
// DELETES. Without it, resolveLatestManifest takes the findRepoManifest
733+
// shortcut: any `skills-manifest.json` within 16 parent dirs of cwd
734+
// becomes "latest". HyperFrames' own repo manifest declares
735+
// `source: heygen-com/hyperframes`, so a checkout (or any project
736+
// carrying a copy) matches attribution and every published skill absent
737+
// from that local file is deleted globally as "no longer published".
738+
// An explicit `--source` still wins — canonical only decides what
739+
// "latest" means when no source was given. GH #3111.
740+
const { skills, scope } = await checkSkills({ dir, source, canonical: true });
731741
const removed = skills.filter((s) => s.status === "removed").map((s) => s.name);
732742
if (removed.length) {
733743
console.log();

‎packages/cli/src/utils/skillsManifest.test.ts‎

Lines changed: 93 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -495,8 +495,27 @@ describe("checkSkills removed-upstream detection", () => {
495495
writeFileSync(manifestPath, JSON.stringify(manifest));
496496
return { home, opts: { source: manifestPath, cwd: project, home } };
497497
}
498-
function writeGlobalLock(home: string, skills: Record<string, { source: string }>): void {
499-
writeFileSync(join(home, ".agents/.skill-lock.json"), JSON.stringify({ version: 3, skills }));
498+
// Upstream writes a `skillPath` on every lock entry it creates (verified
499+
// against a real ~/.agents/.skill-lock.json written by [email protected]), and
500+
// removed-detection now reads it to tell manifest-covered skills apart from
501+
// ones installed out of the repo's other skill roots — see
502+
// manifestCoversSkill / GH #3111. Default to the covered root so each existing
503+
// fixture keeps meaning "a normally published skill"; pass skillPath
504+
// explicitly to model anything else.
505+
function writeGlobalLock(
506+
home: string,
507+
skills: Record<string, { source: string; skillPath?: string }>,
508+
): void {
509+
const withPaths = Object.fromEntries(
510+
Object.entries(skills).map(([name, entry]) => [
511+
name,
512+
{ skillPath: `skills/${name}/SKILL.md`, ...entry },
513+
]),
514+
);
515+
writeFileSync(
516+
join(home, ".agents/.skill-lock.json"),
517+
JSON.stringify({ version: 3, skills: withPaths }),
518+
);
500519
}
501520

502521
it("flags a lock-attributed skill the manifest dropped, ignoring other sources", async () => {
@@ -514,6 +533,70 @@ describe("checkSkills removed-upstream detection", () => {
514533
expect(res.summary.removed).toBe(1);
515534
});
516535

536+
// GH #3111 — the data-loss regression. `skills add --skill '*'` installs every
537+
// skill in the repo, including the repo-native ones under `.claude/skills/`
538+
// and `.agents/skills/`, and attributes them all to our source. The published
539+
// manifest is generated from `<repoRoot>/skills` ONLY (gen-skills-manifest.ts),
540+
// so it never lists them — and reading that silence as "no longer published"
541+
// deleted them from every agent directory on the machine, immediately after
542+
// the same command installed them.
543+
//
544+
// Reproduced end-to-end pre-fix against the real CLI: `skills add` installed
545+
// 25 skills, then `skills update` printed "Removing 6 skill(s) no longer
546+
// published: captions-overlay, changelog-video, cut-the-curve, motion-doctrine,
547+
// oversized-cursor, seam-craft" and deleted all six. Their lock entries carried
548+
// `.agents/skills/<name>/SKILL.md`; the survivors carried `skills/<name>/…`.
549+
it("never prunes a skill installed outside the manifest's coverage root", async () => {
550+
const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } });
551+
writeGlobalLock(home, {
552+
alpha: { source: "test" }, // published, in the manifest → untouched
553+
gamma: {
554+
source: "test", // ours, absent from the manifest…
555+
skillPath: ".agents/skills/gamma/SKILL.md", // …but the manifest never covered it
556+
},
557+
});
558+
559+
const res = await checkSkills(opts);
560+
561+
const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status]));
562+
expect(byName.gamma).not.toBe("removed");
563+
expect(res.summary.removed).toBe(0);
564+
});
565+
566+
// The other half of the contract: the coverage filter must not blunt the
567+
// retired-skill convergence #2176 added. A skill installed FROM `skills/` and
568+
// since dropped from the manifest is still a real removal.
569+
it("still prunes a manifest-covered skill that was genuinely dropped upstream", async () => {
570+
const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } });
571+
writeGlobalLock(home, {
572+
alpha: { source: "test" },
573+
gamma: { source: "test", skillPath: "skills/gamma/SKILL.md" },
574+
});
575+
576+
const res = await checkSkills(opts);
577+
578+
expect(Object.fromEntries(res.skills.map((s) => [s.name, s.status])).gamma).toBe("removed");
579+
expect(res.summary.removed).toBe(1);
580+
});
581+
582+
// Fail safe on unknown provenance: an entry written by an older upstream that
583+
// recorded no skillPath cannot be shown to be manifest-covered, and this is a
584+
// DELETE path — so it is left alone rather than guessed at.
585+
it("leaves an entry with no skillPath alone rather than guessing", async () => {
586+
const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } });
587+
writeFileSync(
588+
join(home, ".agents/.skill-lock.json"),
589+
JSON.stringify({
590+
version: 3,
591+
skills: { alpha: { source: "test" }, gamma: { source: "test" } }, // no skillPath at all
592+
}),
593+
);
594+
595+
const res = await checkSkills(opts);
596+
597+
expect(res.summary.removed).toBe(0);
598+
});
599+
517600
it("a removed skill alone makes an update available (no outdated/missing)", async () => {
518601
// Manifest lists only alpha, with its REAL hash → "current" (not outdated).
519602
const { home, opts } = setup({ source: "test", skills: {} });
@@ -591,7 +674,10 @@ describe("checkSkills removed-upstream detection", () => {
591674
join(project, "skills-lock.json"),
592675
JSON.stringify({
593676
version: 1,
594-
skills: { alpha: { source: "test" }, gamma: { source: "test" } },
677+
skills: {
678+
alpha: { source: "test", skillPath: "skills/alpha/SKILL.md" },
679+
gamma: { source: "test", skillPath: "skills/gamma/SKILL.md" },
680+
},
595681
}),
596682
);
597683

@@ -618,7 +704,10 @@ describe("checkSkills removed-upstream detection", () => {
618704
join(project, "skills-lock.json"),
619705
JSON.stringify({
620706
version: 1,
621-
skills: { alpha: { source: "test" }, gamma: { source: "test" } },
707+
skills: {
708+
alpha: { source: "test", skillPath: "skills/alpha/SKILL.md" },
709+
gamma: { source: "test", skillPath: "skills/gamma/SKILL.md" },
710+
},
622711
}),
623712
);
624713
const home = join(root, "home2");

‎packages/cli/src/utils/skillsManifest.ts‎

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -429,6 +429,13 @@ export function diffSkills(
429429
interface LockEntry {
430430
source?: string;
431431
sourceUrl?: string;
432+
/**
433+
* Path of the skill's SKILL.md within the source repo, as upstream records it
434+
* (`skills/general-video/SKILL.md`, `.agents/skills/seam-craft/SKILL.md`). The
435+
* only field distinguishing skills the published manifest covers from ones
436+
* installed out of the same repo's other skill roots — see manifestCoversSkill.
437+
*/
438+
skillPath?: string;
432439
}
433440

434441
/** The slice of the vercel-labs/skills lock file we read. */
@@ -508,14 +515,50 @@ interface RemovedResult {
508515
lockMissing: boolean;
509516
}
510517

511-
/** Skills the lock attributes to our source that the manifest no longer ships. */
518+
/**
519+
* The repo directory the published manifest is generated from — see
520+
* `packages/cli/scripts/gen-skills-manifest.ts`, which walks `<repoRoot>/skills`
521+
* and nothing else. Anything installed from a DIFFERENT root of the same repo
522+
* (`.claude/skills/`, `.agents/skills/` — the repo-native contributor skills) is
523+
* outside the manifest's coverage, so the manifest says nothing about it.
524+
*/
525+
const MANIFEST_COVERAGE_ROOT = "skills/";
526+
527+
/**
528+
* Is the manifest authoritative about whether this skill still exists upstream?
529+
*
530+
* Only for skills installed from the directory the manifest is generated from.
531+
* The lock records where in the repo each skill came from (`skillPath`, e.g.
532+
* `skills/general-video/SKILL.md` vs `.agents/skills/seam-craft/SKILL.md`), and
533+
* both carry the same `source`, so source attribution alone cannot tell them
534+
* apart. An entry with no `skillPath` (older lock format) is treated as NOT
535+
* covered — for a delete, unknown provenance must fail safe. GH #3111.
536+
*/
537+
function manifestCoversSkill(entry: LockEntry | undefined): boolean {
538+
const path = entry?.skillPath;
539+
return typeof path === "string" && path.startsWith(MANIFEST_COVERAGE_ROOT);
540+
}
541+
542+
/**
543+
* Skills the lock attributes to our source that the manifest no longer ships.
544+
*
545+
* "Absent from the manifest" only means "removed upstream" for skills the
546+
* manifest actually covers. `skills add --skill '*'` installs every skill in the
547+
* repo — including the repo-native ones under `.claude/skills/` and
548+
* `.agents/skills/`, which the published manifest deliberately omits — and
549+
* attributes them all to our source. Without the coverage filter, every install
550+
* is immediately followed by a prune that deletes those skills as "no longer
551+
* published", so `check || update` never converges: `add` reinstalls them and
552+
* the next `update` deletes them again, forever. GH #3111.
553+
*/
512554
function detectRemoved(
513555
root: SkillRoot,
514556
latest: SkillsManifest,
515557
opts: { cwd?: string; home?: string },
516558
): RemovedResult {
517559
const lock = readSkillLock(lockPathForScope(root.scope, opts));
518560
const removed = skillsAttributedToSource(lock, latest.source)
561+
.filter((name) => manifestCoversSkill(lock?.skills?.[name]))
519562
.filter((name) => !(name in latest.skills))
520563
.sort()
521564
.map((name) => ({ name, status: "removed" as const }));

0 commit comments

Comments
 (0)