mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-07 10:06:21 +00:00
fix(cli): stop skills update deleting skills the manifest never covered (#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 (#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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user