diff --git a/README.md b/README.md index 1842f2ff482..00c403787c0 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ HyperFrames ships 7 skills. Make any video by asking for it; the `/hyperframes` For standalone installation, default to the **core set** (`/hyperframes`, `/hyperframes-cli`, `/hyperframes-registry`, `/media-use`). `npx hyperframes skills update` installs exactly that from anywhere; the interactive picker (`npx skills add heygen-com/hyperframes`) lists it as the "Core Skills" group, nothing pre-selected. The picker is interactive-only — a non-interactive or agent run without `--skill` installs all 7. Use `npx skills add heygen-com/hyperframes --all` to install the complete published set deliberately (skips the picker), or `npx skills add heygen-com/hyperframes --skill ` for just one (bare name, no leading `/`). -Standalone installs stay lean after that: `npx hyperframes init` keeps the **core set** fresh (plus whatever is already installed) and never expands a partial install; `/figma`, `/slideshow` and `/remotion-to-hyperframes` install **on demand** with `npx hyperframes skills update `. Nothing re-pulls the full set behind your back. +Standalone installs stay lean after that: `npx hyperframes init` installs the **core set** only when it is missing and never touches a skill you already have (`npx hyperframes skills update` refreshes them, and never removes one); `/figma`, `/slideshow` and `/remotion-to-hyperframes` install **on demand** with `npx hyperframes skills update `. Nothing re-pulls the full set behind your back. ### Plugin packages diff --git a/docs/packages/cli.mdx b/docs/packages/cli.mdx index d929a1ac81e..65260443c74 100644 --- a/docs/packages/cli.mdx +++ b/docs/packages/cli.mdx @@ -113,7 +113,7 @@ npx hyperframes init my-video --tailwind | `--keep-original` | Copy the `--video` file as is instead of converting a non-web codec to H.264 MP4 | | `--model` | Whisper model for that transcription (for example `small.en`, `medium.en`, `large`) | | `--language` | Language code for transcription (`en`, `es`, `ja`, …). Filters out speech in other languages. | -| `--skip-skills` | Currently ignored. Set `HYPERFRAMES_SKIP_SKILLS=1` to opt out in CI or tests. | +| `--skip-skills` | Leave AI coding skills alone: `init` does not install the core set. `HYPERFRAMES_SKIP_SKILLS=1` does the same. Pass `--video` or `--audio` and the CLI transcribes the audio with Whisper and patches the captions into the composition for you. `--skip-transcribe` turns @@ -1862,9 +1862,10 @@ npx hyperframes skills update slideshow Bare `skills` installs everything. `skills update` keeps a deliberate partial installation partial: it refreshes the core set plus whatever is already installed, and never expands beyond that unless you name a skill. Naming one -adds it. `check` and `update` both accept `--json`, plus `--dir` and `--source` -to point removed-detection at a different skills directory or source — those two -scope the prune, not the install. +adds it. `check` and `update` both accept `--json`, and `check` also accepts +`--dir` and `--source`. `update` never removes a skill, and it leaves a skill the +install record attributes to another publisher alone, even when the name matches +one of ours. Set `HYPERFRAMES_SKIP_SKILLS=1` when something else manages the skills (CI, tests, or an install pinned to one commit). `skills update` and bare `skills` then exit 0 @@ -1872,7 +1873,7 @@ with a one-line notice and leave the installed skills unchanged. The CLI installs from the current HyperFrames GitHub source and links the global bundles into whichever compatible agents it finds. After scaffolding, `init` -also refreshes the core set plus any HyperFrames skills you already have. +installs the core set if it is missing. It never refreshes or removes a skill you already have; run `skills update` for that. #### `fatal: active post-checkout hook found during git clone` diff --git a/packages/cli/src/cli.lifecycle.test.ts b/packages/cli/src/cli.lifecycle.test.ts index e52e537300d..80bad22df78 100644 --- a/packages/cli/src/cli.lifecycle.test.ts +++ b/packages/cli/src/cli.lifecycle.test.ts @@ -321,6 +321,32 @@ describe("CLI lifecycle", () => { }); }); +describe("stale-skills notice under --json", () => { + it("prints on a --json run while the self-update notices stay off", async () => { + const printSkillsUpdateNotice = vi.fn(); + const printUpdateNotice = vi.fn(); + mockInitCommand(vi.fn()); + mockTelemetry(); + vi.doMock("./utils/autoUpdate.js", () => ({ + reportCompletedUpdate: vi.fn(), + scheduleBackgroundInstall: vi.fn(), + })); + vi.doMock("./utils/updateCheck.js", () => ({ + cachedUpdateCheck: () => ({ current: "1.0.0", latest: "1.0.0", updateAvailable: false }), + printUpdateNotice, + printStalePinNotice: vi.fn(), + })); + vi.doMock("./utils/skillsUpdateCheck.js", () => ({ printSkillsUpdateNotice })); + vi.doMock("./utils/backgroundChecks.js", () => ({ launchBackgroundChecks: vi.fn() })); + + process.argv = ["node", "cli.ts", "init", "--json"]; + await import("./cli.js"); + + await vi.waitFor(() => expect(printSkillsUpdateNotice).toHaveBeenCalledTimes(1)); + expect(printUpdateNotice).not.toHaveBeenCalled(); + }); +}); + const MOCKED_MODULES = [ "./commands/init.js", "./telemetry/events.js", diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index afae651083a..df351fa4b56 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -266,31 +266,32 @@ if ( // cached check while reporting fresh results of its own). if ( !isHelp && - !hasJsonFlag && command !== "upgrade" && command !== "events" && command !== "telemetry" && command !== "skills" && command !== "usage" ) { - // Report any completed auto-install from the previous run first, before - // kicking off the next check — so the user sees "updated to vX" once and - // we don't over-print. - import("./utils/autoUpdate.js").then((mod) => mod.reportCompletedUpdate()).catch(() => {}); - - import("./utils/updateCheck.js") - .then(async (mod) => { - _printUpdateNotice = mod.printUpdateNotice; - _printStalePinNotice = mod.printStalePinNotice; - const result = mod.cachedUpdateCheck(); - if (result.updateAvailable) { - const auto = await import("./utils/autoUpdate.js").catch(() => null); - auto?.scheduleBackgroundInstall(result.latest, result.current); - } - }) - .catch(() => {}); + if (!hasJsonFlag) { + // Report any completed auto-install from the previous run first, before + // kicking off the next check — so the user sees "updated to vX" once and + // we don't over-print. + import("./utils/autoUpdate.js").then((mod) => mod.reportCompletedUpdate()).catch(() => {}); + + import("./utils/updateCheck.js") + .then(async (mod) => { + _printUpdateNotice = mod.printUpdateNotice; + _printStalePinNotice = mod.printStalePinNotice; + const result = mod.cachedUpdateCheck(); + if (result.updateAvailable) { + const auto = await import("./utils/autoUpdate.js").catch(() => null); + auto?.scheduleBackgroundInstall(result.latest, result.current); + } + }) + .catch(() => {}); + } - // Skills freshness nudge — same gating as the CLI self-update notice. + // Skills freshness nudge: stderr only, so it also runs under --json, which agents pass. import("./utils/skillsUpdateCheck.js") .then((mod) => { _printSkillsUpdateNotice = mod.printSkillsUpdateNotice; @@ -342,8 +343,8 @@ async function finalizeCli(result: CommandResult): Promise { if (!hasJsonFlag) { _printUpdateNotice?.(); _printStalePinNotice?.(); - _printSkillsUpdateNotice?.(); } + _printSkillsUpdateNotice?.(); if (_appHistoryNotice) process.stderr.write(`◇ ${_appHistoryNotice}\n`); process.exitCode = exitCode; } diff --git a/packages/cli/src/commands/init.test.ts b/packages/cli/src/commands/init.test.ts index 72ed7235647..1632dc60971 100644 --- a/packages/cli/src/commands/init.test.ts +++ b/packages/cli/src/commands/init.test.ts @@ -30,13 +30,20 @@ const tailwindScript = // TypeScript-aware runtime. vitest runs under node, so `process.execPath` // would be node and couldn't load the entry. This repo hard-depends on bun // (package.json scripts), so assuming it's on PATH is safe. -function runInit(args: string[]): { status: number; stdout: string; stderr: string } { +function runInit( + args: string[], + opts: { skillsEnvOptOut?: boolean } = {}, +): { status: number; stdout: string; stderr: string } { + const env = { ...process.env, HYPERFRAMES_SKIP_SKILLS: "1" } as Record< + string, + string | undefined + >; + // Default keeps tests offline and fast; a test passes false to rely on the flag alone. + if (opts.skillsEnvOptOut === false) delete env.HYPERFRAMES_SKIP_SKILLS; const res = spawnSync("bun", ["run", cliEntry, "init", ...args], { encoding: "utf-8", timeout: 30_000, - // The `--skip-skills` flag is neutered (see init.ts); the GitHub skills check - // is opted out only via this env var, so tests stay offline and fast. - env: { ...process.env, HYPERFRAMES_SKIP_SKILLS: "1" }, + env, }); return { status: res.status ?? -1, @@ -139,6 +146,21 @@ describe("hyperframes init flag rename", () => { } }); + it("--skip-skills alone leaves skills alone, without the env opt-out", () => { + const dir = mkdtempSync(join(tmpdir(), "hf-init-test-")); + const target = join(dir, "proj"); + try { + const res = runInit([target, "--example", "blank", "--non-interactive", "--skip-skills"], { + skillsEnvOptOut: false, + }); + expect(res.status).toBe(0); + expect(res.stdout).not.toContain("Checking AI coding skills"); + expect(res.stdout).not.toContain("temporarily ignored"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it("--tailwind enables Tailwind utilities in scaffolded HTML", () => { const dir = mkdtempSync(join(tmpdir(), "hf-init-test-")); const target = join(dir, "proj"); diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index 2f53c45ff45..2412b58967c 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -669,19 +669,8 @@ async function scaffoldProject( } } -/** - * Keep the AI coding skills present and current — TARGETED, not the full - * set. Guarantees the core set (the `/hyperframes` entry router + shared - * domain skills) and refreshes any skill already installed; the end-user - * workflow skills are NOT pulled here — they install on demand when their - * workflow is triggered (`hyperframes skills update `, which the router - * runs before entering a workflow). Re-running `init` on an up-to-date machine - * is a no-op, and `init` never expands a deliberate partial install. - * Best-effort: offline, it degrades to a presence check and never breaks init. - * The install itself lands once GLOBALLY (~/.claude/skills + ~/.agents/skills) - * and mirrors into every other installed agent, so it is project-independent — - * the check is global-first to match. - */ +// Installs the core set when missing, globally; an installed skill is never refreshed or removed +// here (that is `hyperframes skills update`). Offline it only checks presence. async function keepSkillsCurrent(destDir: string): Promise { const { updateSkills } = await import("./skills.js"); @@ -689,9 +678,9 @@ async function keepSkillsCurrent(destDir: string): Promise { console.log(c.bold("Checking AI coding skills against GitHub...")); // Wrap defensively (non-strict already swallows most failures): a // skills-install failure can never break `init` itself — it warns and - // proceeds, since --skip-skills no longer escapes this path. + // proceeds. try { - const result = await updateSkills({ refreshInstalled: true, cwd: destDir }); + const result = await updateSkills({ onlyMissing: true, cwd: destDir }); if (result.presenceOnly) { // Freshness never got checked (GitHub unreachable) — don't claim // "up to date"; the engine already reported what it could verify or @@ -829,17 +818,7 @@ export default defineCommand({ const audioFlag = args.audio; const skipTranscribe = args["skip-transcribe"] === true; const keepOriginal = args["keep-original"] === true; - // Temporary measure while the skills.sh registry sync lags GitHub main: the - // `--skip-skills` FLAG is neutered so an agent (or user) that passes it can - // NOT dodge the GitHub skills freshness check. The "don't pass --skip-skills" - // guidance lives in SKILL.md, which ships through the same laggy skills.sh - // channel and can't be relied on to reach the agent — so the guarantee has to - // live in the CLI, the one channel that updates promptly (`npx - // hyperframes@latest`). CI and unit tests still opt out via the - // HYPERFRAMES_SKIP_SKILLS=1 env var, which the agent/user CLI path never sets. - // Revert to `args["skip-skills"] === true` once skills.sh catches up. - const skipSkills = process.env.HYPERFRAMES_SKIP_SKILLS === "1"; - const skipSkillsFlagIgnored = args["skip-skills"] === true && !skipSkills; + const skipSkills = args["skip-skills"] === true || process.env.HYPERFRAMES_SKIP_SKILLS === "1"; const tailwind = args.tailwind === true; const nonInteractive = args["non-interactive"] === true || args.agent === true; const modelFlag = args.model; @@ -850,15 +829,6 @@ export default defineCommand({ ); const interactive = !nonInteractive && process.stdout.isTTY === true; - if (skipSkillsFlagIgnored) { - console.log( - c.dim( - "Note: --skip-skills is temporarily ignored — init always checks AI skills " + - "against GitHub while the skills.sh registry catches up.", - ), - ); - } - let resolutionPreset: CanvasResolution | undefined; if (args.resolution !== undefined) { resolutionPreset = normalizeResolutionFlag(args.resolution); @@ -1227,10 +1197,8 @@ export default defineCommand({ const files = readdirSync(destDir); clack.note(files.map((f) => c.accent(f)).join("\n"), c.success(`Created ${name}/`)); - // Check skills against GitHub and refresh only what's stale — the core set - // plus anything already installed; workflow skills install on demand. The - // --skip-skills flag is temporarily neutered (see above); CI/tests opt out - // via HYPERFRAMES_SKIP_SKILLS=1. + // Install the core set if it is missing; installed skills are left alone. + // --skip-skills (or HYPERFRAMES_SKIP_SKILLS=1) leaves skills alone. if (!skipSkills) { await keepSkillsCurrent(destDir); } diff --git a/packages/cli/src/commands/skills.test.ts b/packages/cli/src/commands/skills.test.ts index c166859537e..65d76020631 100644 --- a/packages/cli/src/commands/skills.test.ts +++ b/packages/cli/src/commands/skills.test.ts @@ -107,11 +107,6 @@ vi.mock("../utils/skillsManifest.js", async (importOriginal) => { checkSkills: vi.fn(async () => DEFAULT_CHECK), hyperframesSkillNames: vi.fn(() => ["hyperframes"]), presentSkills: vi.fn((names: readonly string[]) => [...names]), - // Default: nothing left to prune after `runSkillsRemove`. The real - // (unmocked) fs-level behavior is covered in skillsManifest.test.ts; - // here we only assert the wiring — what update passes in, and that it - // isn't reached when there's nothing removed. - pruneOrphanedLockEntries: vi.fn(() => []), }; }); @@ -192,19 +187,11 @@ describe("hyperframes skills", () => { vi.resetModules(); // vi.resetModules re-imports skills.js but the manifest mock's vi.fn // instances persist — restore their default behavior for each test. - // pruneOrphanedLockEntries is reset explicitly too: relying on afterEach's - // vi.restoreAllMocks() to clear vi.fn() call state is vitest-3-specific - // (vitest 4 restores spies only), and call-count assertions like the - // twice-in-a-row convergence test would then see counts accumulated from - // earlier tests. - const { checkSkills, presentSkills, pruneOrphanedLockEntries } = - await import("../utils/skillsManifest.js"); + const { checkSkills, presentSkills } = await import("../utils/skillsManifest.js"); vi.mocked(checkSkills).mockReset(); vi.mocked(checkSkills).mockImplementation(async () => DEFAULT_CHECK as never); vi.mocked(presentSkills).mockReset(); vi.mocked(presentSkills).mockImplementation((names: readonly string[]) => [...names]); - vi.mocked(pruneOrphanedLockEntries).mockReset(); - vi.mocked(pruneOrphanedLockEntries).mockImplementation(() => []); await resetCommandResult(); }); @@ -373,83 +360,33 @@ describe("hyperframes skills", () => { // `skills add` never deletes, so update must separately prune skills the // manifest dropped (renames/removals) for `check || update` to fully reconcile. - it("skills update prunes skills removed upstream, in the attributed scope", async () => { + it("skills update never removes a skill, even one check once called retired", async () => { setPlatform("linux"); const { checkSkills } = await import("../utils/skillsManifest.js"); - // First call feeds the targeted install; the second is the prune detection. - vi.mocked(checkSkills) - .mockResolvedValueOnce(DEFAULT_CHECK as never) - .mockResolvedValueOnce({ - scope: "global", - skills: [{ name: "graphic-overlays", status: "removed" }], - } as never); - - await runSkillsUpdate(); - - // install first, then a `skills remove` for the dropped skill - expect(state.spawnCalls[0]?.args).toContain("add"); - const removeCall = state.spawnCalls.find((s) => s.args.includes("remove")); - expect(removeCall, "expected a `skills remove` spawn").toBeDefined(); - expect(removeCall!.args).toContain("graphic-overlays"); - expect(removeCall!.args).toContain("--yes"); - expect(removeCall!.args).toContain("-g"); // attributed from the global lock → remove globally - expect(await commandExitCode()).toBe(0); - }); - - it("skills update removes every retired skill check reports, in one call", async () => { - setPlatform("linux"); - const { checkSkills, RETIRED_SKILLS } = await import("../utils/skillsManifest.js"); - vi.mocked(checkSkills) - .mockResolvedValueOnce(DEFAULT_CHECK as never) - .mockResolvedValueOnce({ - scope: "global", - skills: RETIRED_SKILLS.map((name) => ({ name, status: "removed" })), - } as never); + vi.mocked(checkSkills).mockImplementation( + async () => + ({ + ...DEFAULT_CHECK, + skills: [...DEFAULT_CHECK.skills, { name: "graphic-overlays", status: "removed" }], + }) as never, + ); await runSkillsUpdate(); - const removeCalls = state.spawnCalls.filter((s) => s.args.includes("remove")); - expect(removeCalls).toHaveLength(1); - for (const name of RETIRED_SKILLS) expect(removeCalls[0]!.args).toContain(name); - expect(removeCalls[0]!.args).toContain("-g"); + expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false); expect(await commandExitCode()).toBe(0); }); - // The scope the skill was attributed from drives the remove scope: a - // project-scoped removal must NOT pass -g (which would target a different, - // possibly user-owned, global skill of the same name). - it("skills update prunes in project scope without -g", async () => { + it("updateSkills with onlyMissing installs what is missing and leaves outdated skills alone", async () => { setPlatform("linux"); - const { checkSkills } = await import("../utils/skillsManifest.js"); - vi.mocked(checkSkills) - .mockResolvedValueOnce(DEFAULT_CHECK as never) - .mockResolvedValueOnce({ - scope: "project", - skills: [{ name: "graphic-overlays", status: "removed" }], - } as never); + const { updateSkills } = await import("./skills.js"); - await runSkillsUpdate(); + const result = await updateSkills({ onlyMissing: true }); - const removeCall = state.spawnCalls.find((s) => s.args.includes("remove")); - expect(removeCall, "expected a `skills remove` spawn").toBeDefined(); - expect(removeCall!.args).toContain("graphic-overlays"); - expect(removeCall!.args).not.toContain("-g"); - }); - - it("skills update does not prune when nothing was removed upstream", async () => { - setPlatform("linux"); - await runSkillsUpdate(); - expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false); + expect(result.installed).toEqual(["hyperframes-registry"]); + expect(skillFlagValues(state.spawnCalls[0]?.args ?? [])).toEqual(["hyperframes-registry"]); }); - // Retired-skill regression (variant 1): the update engine's OWN targeted- - // install check must resolve the canonical (published) manifest, never a - // stale local `skills-manifest.json` a checkout might still have lying - // around — see resolveLatestManifest's in-repo shortcut. Without this, a - // skill retired upstream but still listed locally gets forced into - // `targets` (isCoreSkill pattern-matches `hyperframes-*`), `skills add` - // silently declines to install something that doesn't exist canonically, - // and the old code strict-threw on a "failure" that was never real. it("checks freshness against the canonical manifest, never a possibly-stale local one", async () => { setPlatform("linux"); const { checkSkills } = await import("../utils/skillsManifest.js"); @@ -475,151 +412,6 @@ describe("hyperframes skills", () => { // "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 - // doc comment). `skills update` must self-heal that lock entry itself so - // `check || update` actually converges instead of re-flagging it forever. - it("self-heals an orphaned lock entry after `skills remove` no-ops on it", async () => { - setPlatform("linux"); - const { checkSkills, pruneOrphanedLockEntries } = await import("../utils/skillsManifest.js"); - vi.mocked(checkSkills) - .mockResolvedValueOnce(DEFAULT_CHECK as never) - .mockResolvedValueOnce({ - scope: "global", - skills: [{ name: "hyperframes-captions", status: "removed" }], - } as never); - vi.mocked(pruneOrphanedLockEntries).mockReturnValueOnce(["hyperframes-captions"]); - - await runSkillsUpdate(); - - expect(pruneOrphanedLockEntries).toHaveBeenCalledWith(["hyperframes-captions"], "global"); - expect(await commandExitCode()).toBe(0); - }); - - // The idempotent-second-run contract at the command level: once nothing is - // left attributed as removed (the fs-level idempotency of the prune itself - // is covered directly in skillsManifest.test.ts), a second `skills update` - // must be a clean no-op — no `skills remove` spawn, no prune call finding - // anything, still exit 0. - it("running update twice in a row converges — the second run prunes nothing", async () => { - setPlatform("linux"); - const { checkSkills, pruneOrphanedLockEntries } = await import("../utils/skillsManifest.js"); - vi.mocked(checkSkills) - .mockResolvedValueOnce(DEFAULT_CHECK as never) - .mockResolvedValueOnce({ - scope: "global", - skills: [{ name: "hyperframes-captions", status: "removed" }], - } as never); - vi.mocked(pruneOrphanedLockEntries).mockReturnValueOnce(["hyperframes-captions"]); - - await runSkillsUpdate(); - expect(await commandExitCode()).toBe(0); - expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(true); - - // Second run: nothing attributed as removed anymore (the lock entry was - // pruned above), so there's nothing left to reconcile. - state.spawnCalls = []; - vi.mocked(checkSkills) - .mockResolvedValueOnce(DEFAULT_CHECK as never) - .mockResolvedValueOnce({ scope: "global", skills: [] } as never); - - await runSkillsUpdate(); - expect(await commandExitCode()).toBe(0); - expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false); - // Nothing to prune this time — pruneOrphanedLockEntries isn't even reached. - expect(pruneOrphanedLockEntries).toHaveBeenCalledTimes(1); - }); - - // `update`'s prune runs the same removed-detection as `check`, so its - // --source/--dir must reach the internal checkSkills() — otherwise the prune - // reconciles against defaults even when the user pointed elsewhere. - it("skills update plumbs --source/--dir to its prune detection (parity with check)", async () => { - setPlatform("linux"); - const { checkSkills } = await import("../utils/skillsManifest.js"); - - await runSkillsUpdate({ source: "owner/repo", dir: "/custom/skills" }); - - // The last checkSkills call is the prune's — the update engine's own check - // (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 - // must never reach the spawn (esp. the Windows cmd.exe path). - it("skills update never passes a non-slug skill name to remove", async () => { - setPlatform("linux"); - const { checkSkills } = await import("../utils/skillsManifest.js"); - vi.mocked(checkSkills) - .mockResolvedValueOnce(DEFAULT_CHECK as never) - .mockResolvedValueOnce({ - scope: "global", - skills: [ - { name: "graphic-overlays", status: "removed" }, - { name: "--config=evil.js", status: "removed" }, - ], - } as never); - - await runSkillsUpdate(); - - const removeCall = state.spawnCalls.find((s) => s.args.includes("remove")); - expect(removeCall, "expected a `skills remove` spawn for the valid name").toBeDefined(); - expect(removeCall!.args).toContain("graphic-overlays"); - expect(removeCall!.args).not.toContain("--config=evil.js"); - }); - - // The early-return guard in runSkillsRemove: when EVERY candidate name is - // rejected as non-slug, no `skills remove` is spawned at all (the prior test - // only covers a mix of valid + invalid). A spawn here would run `skills remove - // --yes` with no names — which the upstream CLI treats as "remove nothing" at - // best, or prompts interactively at worst — so we must not reach it. - it("skills update spawns no remove when every removed name is rejected", async () => { - setPlatform("linux"); - const { checkSkills } = await import("../utils/skillsManifest.js"); - vi.mocked(checkSkills) - .mockResolvedValueOnce(DEFAULT_CHECK as never) - .mockResolvedValueOnce({ - scope: "global", - skills: [ - { name: "--config=evil.js", status: "removed" }, - { name: "../escape", status: "removed" }, - ], - } as never); - - await runSkillsUpdate(); - - expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false); - // The install still ran and the update still succeeded — a cleanup no-op - // doesn't fail the update. - expect(state.spawnCalls[0]?.args).toContain("add"); - expect(await commandExitCode()).toBe(0); - }); - - // When git is missing the upstream `skills add` would clone-abort with a noisy - // `spawn git ENOENT` block. Detect it first and never spawn the install, so a - // best-effort caller (init) skips cleanly and a fresh boot without git still - // scaffolds the project. it("bare `skills` skips the install (no spawn) when git is unavailable", async () => { setPlatform("linux"); state.gitMissing = true; @@ -686,7 +478,7 @@ describe("hyperframes skills", () => { expect(String(logSpy.mock.calls.at(-1)?.[0] ?? "")).toMatch(/managed externally/); }); - it("bare `skills update` neither installs nor prunes", async () => { + it("bare `skills update` does not install", async () => { setPlatform("linux"); const { checkSkills } = await import("../utils/skillsManifest.js"); vi.mocked(checkSkills).mockImplementation( @@ -811,16 +603,11 @@ describe("hyperframes skills update ", () => { state.spawnExitCode = 0; state.gitMissing = false; vi.resetModules(); - // Same explicit resets as the describe above (incl. the vitest-4-proofing - // note on pruneOrphanedLockEntries). - const { checkSkills, presentSkills, pruneOrphanedLockEntries } = - await import("../utils/skillsManifest.js"); + const { checkSkills, presentSkills } = await import("../utils/skillsManifest.js"); vi.mocked(checkSkills).mockReset(); vi.mocked(checkSkills).mockImplementation(async () => DEFAULT_CHECK as never); vi.mocked(presentSkills).mockReset(); vi.mocked(presentSkills).mockImplementation((names: readonly string[]) => [...names]); - vi.mocked(pruneOrphanedLockEntries).mockReset(); - vi.mocked(pruneOrphanedLockEntries).mockImplementation(() => []); await resetCommandResult(); }); diff --git a/packages/cli/src/commands/skills.ts b/packages/cli/src/commands/skills.ts index e1db67cc455..f8d744840db 100644 --- a/packages/cli/src/commands/skills.ts +++ b/packages/cli/src/commands/skills.ts @@ -12,10 +12,8 @@ import { hyperframesSkillNames, isCoreSkill, presentSkills, - pruneOrphanedLockEntries, RETIRED_SKILLS, retiredSkillNotice, - SKILLS_CLI_LOCK_PATHS_VERIFIED_AT, type SkillDiff, type SkillsCheckResult, } from "../utils/skillsManifest.js"; @@ -148,20 +146,6 @@ function runSkillsAdd( // on the Windows `cmd.exe` spawn path, where arg escaping is fragile. const PLAIN_SKILL_NAME = /^[a-z0-9][a-z0-9._-]*$/i; -function runSkillsRemove(names: string[], opts: { global: boolean }): Promise { - const safe = names.filter((n) => PLAIN_SKILL_NAME.test(n)); - const rejected = names.filter((n) => !PLAIN_SKILL_NAME.test(n)); - if (rejected.length) { - clack.log.warn(c.warn(`Skipping unexpected skill name(s): ${rejected.join(", ")}`)); - } - if (!safe.length) return Promise.resolve(); - // `skills remove --yes` deletes the bundle dir, every agent symlink, and the - // lock entry non-interactively. `-g` targets the global install; without it, - // the project (cwd) install — we pass whichever scope detection attributed - // these names from, so we never reach into a scope we didn't inspect. - return spawnNpx(["skills", "remove", ...safe, ...(opts.global ? ["-g"] : []), "--yes"]); -} - // Use the full GitHub URL (not the `owner/repo` slug) as the clone source. The // freshness comes from --full-depth (see GLOBAL_INSTALL_ARGS_TAIL), which clones the // repo at latest `main`; the URL just names what to clone. Our freshness check @@ -169,7 +153,7 @@ function runSkillsRemove(names: string[], opts: { global: boolean }): Promise s.status !== "removed"); + const manifestSkills = check.skills; const manifestNames = new Set(manifestSkills.map((s) => s.name)); const unknown = requested.filter((name) => !manifestNames.has(name)); @@ -404,7 +388,9 @@ export async function updateSkills( isCoreSkill(s.name) || (opts.refreshInstalled === true && s.status !== "missing"), ); - const toInstall = targets.filter((s) => s.status === "missing" || s.status === "outdated"); + const toInstall = targets.filter( + (s) => s.status === "missing" || (s.status === "outdated" && opts.onlyMissing !== true), + ); const result: UpdateSkillsResult = { targets: targets.map((s) => s.name), installed: toInstall.map((s) => s.name), @@ -543,7 +529,6 @@ function renderCheck(result: SkillsCheckResult): void { if (summary.outdated) parts.push(c.warn(`↑ ${summary.outdated} outdated`)); if (summary.coreMissing) parts.push(c.warn(`◦ ${summary.coreMissing} core not installed`)); if (onDemandMissing) parts.push(c.dim(`◦ ${onDemandMissing} available on demand`)); - if (summary.removed) parts.push(c.warn(`✗ ${summary.removed} removed upstream`)); console.log(` ${parts.join(" ")}`); printSkillSection(result, "outdated", "Outdated:", "↑", c.warn); @@ -563,27 +548,6 @@ function renderCheck(result: SkillsCheckResult): void { c.dim, (s) => !isCoreSkill(s.name), ); - printSkillSection( - result, - "removed", - "Removed upstream (renamed or dropped — no longer published):", - "✗", - c.warn, - ); - - // Removed-detection cross-references the upstream skills lock. If that lock is - // absent where we expect it (e.g. upstream moved its path), removed-detection - // silently reports zero — so warn rather than imply a clean "up to date". - if (result.lockMissing) { - console.log(); - console.log( - ` ${c.warn(`! Skills lock not found — can't check for skills removed upstream.`)}`, - ); - console.log( - ` ${c.dim(` (lock paths verified against ${SKILLS_CLI_LOCK_PATHS_VERIFIED_AT})`)}`, - ); - } - console.log(); if (result.updateAvailable) { console.log(` ${c.accent("Update: npx hyperframes skills update")}`); @@ -719,29 +683,12 @@ const updateCommand = defineCommand({ meta: { name: "update", description: - "Update the core set plus every installed HyperFrames skill to the latest, and remove any no longer published. Pass skill names to also install those (how on-demand skills install) — without names it never expands a partial install", + "Update the core set plus every installed HyperFrames skill to the latest. Pass skill names to also install those (how on-demand skills install) — without names it never expands a partial install, and it never removes a skill", }, - // Mirror `check`'s flags: the prune step runs the same removed-detection, so it - // must respect the same overrides. Without these, `update`'s internal - // checkSkills() fell back to defaults — pruning the auto-detected install - // against the default manifest even when the user pointed `check` elsewhere. args: { json: { type: "boolean", description: "Output as JSON", default: false }, - dir: { - type: "string", - description: - "Skills dir for removed-detection only — scopes the prune, not the install (default: auto-detect)", - }, - source: { - type: "string", - description: - "Where 'latest' comes from for removed-detection (local path, owner/repo, or URL) — does not change the install source", - }, }, async run({ args }) { - const dir = args.dir; - const source = args.source; - // Positional skill names (e.g. `hyperframes skills update slideshow`) are // the ONLY way update expands an install: each named skill is guaranteed // present and current. A retired name installs nothing; the run says where @@ -753,7 +700,7 @@ const updateCommand = defineCommand({ return; } - // Exit 0 without installing, pruning or touching the nudge cache: the pinned + // Exit 0 without installing or touching the nudge cache: the pinned // install is the one the caller wants, so the skill's step keeps working. if (skillsManagedExternally()) { reportSkillsManagedExternally(requested, args.json === true); @@ -769,12 +716,6 @@ const updateCommand = defineCommand({ // about. Explicit full set: `hyperframes skills` or `npx skills add // heygen-com/hyperframes --all`. // - // Note: the upstream `skills add` CLI has no `--dir` flag (it installs into - // the resolved agent dirs), so `--dir` here scopes only the *prune* detection - // below, not the install. `--source` likewise drives where the prune's - // "latest" manifest comes from; the install always targets the canonical - // HyperFrames repo so `update` reliably refreshes the published skills. - // // strict: this is the documented recovery path for the agent/CI contract // `hyperframes skills check || hyperframes skills update`. If the install fails (no npx, `skills add` exits // non-zero, a named skill still absent afterwards) it must exit non-zero @@ -787,59 +728,6 @@ const updateCommand = defineCommand({ setCommandExitCode(1); return; } - - // `skills add` never deletes, so a skill renamed or dropped upstream - // (e.g. a retired workflow skill) would linger forever. Prune - // skills the lock attributes to our source that the manifest no longer - // ships, so `check || update` fully reconciles the install to the manifest. - // - // Safety: `removed` only ever contains skills the lock records as installed - // from our source (see detectRemoved) — never a user's own or another - // source's skills. We remove in the exact scope detection attributed from, - // so we never reach into a scope we didn't inspect. Best-effort: cleanup - // failure doesn't fail the update — the install the CI contract gates on - // already succeeded. - try { - // `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(); - console.log( - c.dim(`Removing ${removed.length} skill(s) no longer published: ${removed.join(", ")}`), - ); - await runSkillsRemove(removed, { global: scope === "global" }); - // Self-heal: `skills remove` only clears a lock entry for a name it - // found an on-disk bundle for (see pruneOrphanedLockEntries). A skill - // retired before it ever shipped a bundle to this machine has none, so - // the call above is a silent no-op for it — the lock entry lingers and - // would be re-flagged "removed" on every future run. Prune whatever is - // still attributed after the call so `check || update` converges - // instead of looping forever. Best-effort and scoped to exactly the - // lock the remove above targeted (same `scope`); a write failure here - // must not fail the update — the install already succeeded. - const scopeForPrune = scope ?? "global"; - const stillOrphaned = pruneOrphanedLockEntries(removed, scopeForPrune); - if (stillOrphaned.length) { - console.log( - c.dim( - `Reconciled ${stillOrphaned.length} orphaned lock entr${stillOrphaned.length === 1 ? "y" : "ies"} with no on-disk bundle: ${stillOrphaned.join(", ")}`, - ), - ); - } - } - } catch (err) { - clack.log.warn(c.warn(`Skipped removed-skill cleanup: ${(err as Error).message}`)); - } }, }); diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index 5c6683a93df..defa81dc376 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -448,7 +448,6 @@ export interface HyperframesConfig { /** How many skills were missing (not installed) at the last check. */ skillsMissingCount?: number; /** How many installed skills were flagged removed-upstream at the last check. */ - skillsRemovedCount?: number; /** * True once the DE parallel-router experiment ("HF_DE_PARALLEL_ROUTER") * has actually FAILED (its self-verify/generic-failure safety net fired — @@ -660,7 +659,6 @@ function passthroughFields(parsed: Partial): Partial { [ "no terminal", () => Object.defineProperty(process.stderr, "isTTY", { value: false, configurable: true }), - ["update"], + ["update", "skills"], ], ])("with %s, hands the child only what the parent used to check", (_, arrange, checks) => { arrange(); diff --git a/packages/cli/src/utils/skillsManifest.test.ts b/packages/cli/src/utils/skillsManifest.test.ts index e5cac235194..0bf5bb1073b 100644 --- a/packages/cli/src/utils/skillsManifest.test.ts +++ b/packages/cli/src/utils/skillsManifest.test.ts @@ -1,13 +1,11 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { - chmodSync, existsSync, mkdtempSync, mkdirSync, readdirSync, readFileSync, rmSync, - statSync, writeFileSync, } from "node:fs"; import { execFile } from "node:child_process"; @@ -23,8 +21,6 @@ import { isCoreSkill, MANIFEST_FILE, presentSkills, - pruneOrphanedLockEntries, - RETIRED_SKILLS, retiredSkillNotice, skillsAttributedToSource, type SkillsManifest, @@ -490,7 +486,14 @@ describe("skillsAttributedToSource", () => { }); }); -describe("checkSkills removed-upstream detection", () => { +describe("retiredSkillNotice", () => { + it("says where a retired skill's knowledge lives", () => { + expect(retiredSkillNotice("hyperframes-audio")).toContain("media-use"); + expect(retiredSkillNotice("motion-graphics")).toContain("/hyperframes skill"); + }); +}); + +describe("checkSkills foreign skills", () => { // The global lock path honours XDG_STATE_HOME; clear it so the fixture under // /.agents/.skill-lock.json is the one that's read. let xdg: string | undefined; @@ -548,309 +551,47 @@ describe("checkSkills removed-upstream detection", () => { ); } - it("flags a lock-attributed skill the manifest dropped, ignoring other sources", async () => { - const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); - writeGlobalLock(home, { - alpha: { source: "test" }, // still ours and in the manifest - gamma: { source: "test" }, // ours, dropped from manifest → removed - delta: { source: "someone/else" }, // a different source → never ours - }); - - const res = await checkSkills(opts); - const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status])); - expect(byName.gamma).toBe("removed"); - expect(byName.delta).toBeUndefined(); - 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 () => { + it("never compares or refreshes a skill another publisher installed under one of our names", 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 - }, - }); + writeGlobalLock(home, { alpha: { source: "someone/else" } }); 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); + expect(res.skills.find((s) => s.name === "alpha")).toBeUndefined(); + expect(res.foreign).toEqual(["alpha"]); + expect(res.updateAvailable).toBe(false); }); - // 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 () => { + it("never compares or refreshes a skill another publisher installed under one of our names", 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" }, - }); + writeGlobalLock(home, { alpha: { source: "someone/else" } }); 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); + expect(res.skills.find((s) => s.name === "alpha")).toBeUndefined(); + expect(res.foreign).toEqual(["alpha"]); + expect(res.updateAvailable).toBe(false); }); - // 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 - }), - ); + it("treats a skill with no lock entry as ours: provenance unknown, not foreign", async () => { + const { opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); const res = await checkSkills(opts); - expect(res.summary.removed).toBe(0); - }); - - // Retired skills (folded into guide folders of a remaining skill) are ours by - // name, so they are removed even where the generic rule fails safe: a lock - // entry with no skillPath, or a bundle on disk with no lock entry at all. - describe("retired skills", () => { - const retired = "hyperframes-core"; - - it("is one of RETIRED_SKILLS and absent from the skills/ tree", () => { - expect(RETIRED_SKILLS).toContain(retired); - const skillsRoot = join(dirname(fileURLToPath(import.meta.url)), "../../../../skills"); - for (const name of RETIRED_SKILLS) expect(existsSync(join(skillsRoot, name))).toBe(false); - }); - - it("flags a retired skill whose lock entry has no skillPath", 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" }, [retired]: { source: "test" } }, - }), - ); - - const res = await checkSkills(opts); - - expect(res.skills.filter((s) => s.status === "removed").map((s) => s.name)).toEqual([ - retired, - ]); - }); - - it("flags a retired bundle on disk that no lock attributes", async () => { - const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); - installSkills(join(home, ".agents/skills"), [retired]); - - const res = await checkSkills(opts); - - expect(res.skills.filter((s) => s.status === "removed").map((s) => s.name)).toEqual([ - retired, - ]); - expect(res.updateAvailable).toBe(true); - }); - - it("lists the eight retired workflow skills", () => { - for (const name of [ - "embedded-captions", - "faceless-explainer", - "general-video", - "motion-graphics", - "music-to-video", - "pr-to-video", - "product-launch-video", - "talking-head-recut", - ]) - expect(RETIRED_SKILLS).toContain(name); - }); - - it("flags a retired workflow the lock attributes to us, even without a skillPath", async () => { - const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); - installSkills(join(home, ".agents/skills"), ["general-video"]); - writeGlobalLock(home, { alpha: { source: "test" }, "general-video": { source: "test" } }); - - const res = await checkSkills(opts); - - expect(res.skills.filter((s) => s.status === "removed").map((s) => s.name)).toEqual([ - "general-video", - ]); - }); - - it("leaves a same-named workflow bundle alone when no lock attributes it to us", async () => { - // Workflow names are generic: another source's `motion-graphics` must survive. - const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); - installSkills(join(home, ".agents/skills"), ["motion-graphics"]); - - const res = await checkSkills(opts); - - expect(res.summary.removed).toBe(0); - }); - - it("explains where a retired skill's knowledge lives", () => { - expect(retiredSkillNotice("pr-to-video")).toMatch(/\/hyperframes skill and its references/); - expect(retiredSkillNotice("hyperframes-audio")).toMatch(/\/media-use skill/); - }); - - it("leaves a retired name alone while the manifest still publishes it", async () => { - const { home, opts } = setup({ - source: "test", - skills: { alpha: { hash: "x", files: 1 }, [retired]: { hash: "y", files: 1 } }, - }); - installSkills(join(home, ".agents/skills"), [retired]); - writeGlobalLock(home, { alpha: { source: "test" }, [retired]: { source: "test" } }); - - const res = await checkSkills(opts); - - expect(res.summary.removed).toBe(0); - }); + expect(res.foreign).toEqual([]); + expect(res.skills.find((s) => s.name === "alpha")?.status).toBe("outdated"); }); - 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: {} }); - const manifest: SkillsManifest = { - source: "test", - skills: { alpha: hashSkillBundle(join(home, ".agents/skills/alpha")) }, - }; - writeFileSync(opts.source, JSON.stringify(manifest)); + it("never reports a skill the manifest dropped as an update", async () => { + const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); writeGlobalLock(home, { alpha: { source: "test" }, gamma: { source: "test" } }); const res = await checkSkills(opts); - expect(res.summary).toEqual({ - current: 1, - outdated: 0, - missing: 0, - coreMissing: 0, - removed: 1, - }); - expect(res.updateAvailable).toBe(true); - }); - - it("reports no removed skills when there is no lock to attribute from", async () => { - const { opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); - // gamma is an orphan on disk, but with no lock we can't attribute it to us. - const res = await checkSkills(opts); - expect(res.summary.removed).toBe(0); - expect(res.skills.some((s) => s.status === "removed")).toBe(false); - // No install was located via auto-detect (skills live under /.agents/skills - // which IS discovered) — so lockMissing reflects the genuinely-absent lock. - expect(res.lockMissing).toBe(true); - }); - - // A `--dir` pointing at a global-scoped install (under $HOME) must resolve the - // GLOBAL lock (/.agents/.skill-lock.json), not the project lock - // (/skills-lock.json). Before this fix, locateInstall hardcoded scope - // "project" for every --dir, so the global lock was never read and - // removed-detection silently found nothing for --dir installs. - it("--dir under $HOME resolves the global lock so removed-detection works", async () => { - const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); - writeGlobalLock(home, { - alpha: { source: "test" }, // ours and still in the manifest - gamma: { source: "test" }, // ours, dropped from manifest → removed - }); - - const dir = join(home, ".agents/skills"); - const res = await checkSkills({ source: opts.source, dir, cwd: opts.cwd, home }); - expect(res.scope).toBe("global"); - const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status])); - expect(byName.gamma).toBe("removed"); - expect(res.summary.removed).toBe(1); - expect(res.lockMissing).toBe(false); - }); - - // Regression (Magi REQUEST_CHANGES at 88daa820): the common project-local case - // is *also* under $HOME — a project checkout at `/work/proj` with skills - // at `/.claude/skills`. A HOME-first heuristic misclassified this as - // global, so checkSkills read the global lock and `skills update` would prune - // with `-g` against a project install. CWD-containment must win: `--dir` under - // `cwd` resolves to PROJECT even when `cwd` itself is nested under $HOME. - it("--dir under a cwd that is itself nested under $HOME stays project-scoped", async () => { - const home = join(root, "home"); - const project = join(home, "work", "proj"); // cwd nested INSIDE home - const skillsDir = join(project, ".claude/skills"); - installSkills(skillsDir, ["alpha", "gamma"]); - const manifestPath = join(root, "m3.json"); - writeFileSync( - manifestPath, - JSON.stringify({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }), - ); - // Project lock at /skills-lock.json — only read if scope is "project". - // No global lock exists under $HOME, so a wrong-scope ("global") read would - // find no lock → zero removed (gamma would NOT be flagged). The project lock - // being read (gamma → removed) is the proof CWD-containment won. - writeFileSync( - join(project, "skills-lock.json"), - JSON.stringify({ - version: 1, - skills: { - alpha: { source: "test", skillPath: "skills/alpha/SKILL.md" }, - gamma: { source: "test", skillPath: "skills/gamma/SKILL.md" }, - }, - }), - ); - - const res = await checkSkills({ source: manifestPath, dir: skillsDir, cwd: project, home }); - expect(res.scope).toBe("project"); - const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status])); - expect(byName.gamma).toBe("removed"); - expect(res.summary.removed).toBe(1); - }); - - // The complementary case: a `--dir` under the project tree (not $HOME) stays - // project-scoped and reads /skills-lock.json. - it("--dir outside $HOME stays project-scoped and reads the project lock", async () => { - const project = join(root, "proj2"); - const skillsDir = join(project, ".claude/skills"); - installSkills(skillsDir, ["alpha", "gamma"]); - const manifestPath = join(root, "m2.json"); - writeFileSync( - manifestPath, - JSON.stringify({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }), - ); - // Project lock lives at /skills-lock.json — point cwd at the project. - writeFileSync( - join(project, "skills-lock.json"), - JSON.stringify({ - version: 1, - skills: { - alpha: { source: "test", skillPath: "skills/alpha/SKILL.md" }, - gamma: { source: "test", skillPath: "skills/gamma/SKILL.md" }, - }, - }), - ); - const home = join(root, "home2"); - mkdirSync(home, { recursive: true }); - const res = await checkSkills({ source: manifestPath, dir: skillsDir, cwd: project, home }); - expect(res.scope).toBe("project"); - const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status])); - expect(byName.gamma).toBe("removed"); - expect(res.summary.removed).toBe(1); + expect(res.skills.map((s) => s.name)).toEqual(["alpha"]); }); }); -// Regression coverage for "variant 1" of the retired-skill bug: `updateSkills` -// (see commands/skills.ts) resolves its own targeted-install check with -// `canonical: true` specifically so it never trusts a stale local -// `skills-manifest.json` — this is the mechanism that makes that safe. describe("checkSkills canonical bypass of the in-repo manifest shortcut", () => { afterEach(() => { vi.unstubAllGlobals(); @@ -936,90 +677,3 @@ describe("checkSkills canonical bypass of the in-repo manifest shortcut", () => expect(res.skills.map((s) => s.name)).toEqual(["from-explicit-source"]); }); }); - -describe("pruneOrphanedLockEntries", () => { - function writeLock(path: string, skills: Record): void { - writeFileSync(path, JSON.stringify({ version: 1, skills, dismissed: [] })); - } - - it("removes only the given names, leaving other entries and lock fields intact", () => { - const home = join(root, "home"); - mkdirSync(join(home, ".agents"), { recursive: true }); - const lockPath = join(home, ".agents", ".skill-lock.json"); - writeLock(lockPath, { - a: { source: "heygen-com/hyperframes" }, - b: { source: "heygen-com/hyperframes" }, - c: { source: "heygen-com/hyperframes" }, - }); - - const pruned = pruneOrphanedLockEntries(["a", "b"], "global", { home }); - - expect(pruned.sort()).toEqual(["a", "b"]); - const rewritten = JSON.parse(readFileSync(lockPath, "utf8")); - expect(Object.keys(rewritten.skills)).toEqual(["c"]); - expect(rewritten.version).toBe(1); // other lock fields survive the rewrite - }); - - it("is idempotent — a second call with the same names finds nothing left and no-ops", () => { - const home = join(root, "home"); - mkdirSync(join(home, ".agents"), { recursive: true }); - const lockPath = join(home, ".agents", ".skill-lock.json"); - writeLock(lockPath, { a: { source: "heygen-com/hyperframes" } }); - - const first = pruneOrphanedLockEntries(["a"], "global", { home }); - expect(first).toEqual(["a"]); - - const before = readFileSync(lockPath, "utf8"); - const second = pruneOrphanedLockEntries(["a"], "global", { home }); - expect(second).toEqual([]); - // No entries left to touch → the file is never rewritten a second time. - expect(readFileSync(lockPath, "utf8")).toBe(before); - }); - - it("writes atomically with no trailing newline, no leftover temp file, and preserves the file mode", () => { - const home = join(root, "home-atomic"); - mkdirSync(join(home, ".agents"), { recursive: true }); - const lockPath = join(home, ".agents", ".skill-lock.json"); - writeLock(lockPath, { - a: { source: "heygen-com/hyperframes" }, - b: { source: "heygen-com/hyperframes" }, - }); - chmodSync(lockPath, 0o640); - - const pruned = pruneOrphanedLockEntries(["a"], "global", { home }); - - expect(pruned).toEqual(["a"]); - const raw = readFileSync(lockPath, "utf8"); - expect(raw.endsWith("\n")).toBe(false); - expect(JSON.parse(raw).skills).toEqual({ b: { source: "heygen-com/hyperframes" } }); - // No `.tmp` sibling left behind by the temp-file + rename. - expect(readdirSync(join(home, ".agents"))).toEqual([".skill-lock.json"]); - // Original permissions survive the rewrite (POSIX only — Windows's fs - // layer reports 0o666 regardless of the mode we set, so the bits aren't - // meaningful there). - if (process.platform !== "win32") { - expect(statSync(lockPath).mode & 0o777).toBe(0o640); - } - }); - - it("no-ops without throwing when the lock file doesn't exist", () => { - const home = join(root, "home-without-lock"); - mkdirSync(home, { recursive: true }); - expect(pruneOrphanedLockEntries(["a"], "global", { home })).toEqual([]); - }); - - it("resolves the project lock at /skills-lock.json for scope: project", () => { - const project = join(root, "project"); - mkdirSync(project, { recursive: true }); - writeLock(join(project, "skills-lock.json"), { - a: { source: "heygen-com/hyperframes" }, - b: { source: "heygen-com/hyperframes" }, - }); - - const pruned = pruneOrphanedLockEntries(["a"], "project", { cwd: project }); - - expect(pruned).toEqual(["a"]); - const rewritten = JSON.parse(readFileSync(join(project, "skills-lock.json"), "utf8")); - expect(Object.keys(rewritten.skills)).toEqual(["b"]); - }); -}); diff --git a/packages/cli/src/utils/skillsManifest.ts b/packages/cli/src/utils/skillsManifest.ts index 71bcee87f07..ad9f9aa0de6 100644 --- a/packages/cli/src/utils/skillsManifest.ts +++ b/packages/cli/src/utils/skillsManifest.ts @@ -17,14 +17,7 @@ import { execFile } from "node:child_process"; import { createHash } from "node:crypto"; -import { - existsSync, - readdirSync, - readFileSync, - renameSync, - statSync, - writeFileSync, -} from "node:fs"; +import { existsSync, readdirSync, readFileSync, statSync } from "node:fs"; import { homedir } from "node:os"; import { isAbsolute, join, relative, resolve, sep } from "node:path"; import { promisify } from "node:util"; @@ -65,9 +58,7 @@ export interface SkillsManifest { skills: Record; } -// "removed" = installed from our source but no longer in the manifest (renamed -// or dropped upstream). Attributed via the skills lock — see detectRemoved. -export type SkillStatus = "current" | "outdated" | "missing" | "removed"; +export type SkillStatus = "current" | "outdated" | "missing"; export interface SkillDiff { name: string; @@ -97,16 +88,10 @@ export interface SkillsCheckResult { outdated: number; missing: number; coreMissing: number; - removed: number; }; skills: SkillDiff[]; - /** - * True when an install was located but the upstream skills lock was absent at - * the expected path, so removed-detection couldn't run (it silently reports - * zero removed). Lets the CLI warn instead of misreporting "up to date" — a - * guard against the lock path silently no-op'ing if upstream moves it. - */ - lockMissing: boolean; + /** Manifest-named skills the lock attributes to another publisher; left untouched. */ + foreign: string[]; } const DEFAULT_REPO_SLUG = "heygen-com/hyperframes"; @@ -342,7 +327,7 @@ function discoverSkillRoots(base: string, scope: "project" | "global"): SkillRoo /** * Decide whether an explicit `--dir` is a project- or global-scoped install, so - * removed-detection reads the *right* lock. The upstream `skills` CLI keeps two + * lock attribution reads the *right* lock. The upstream `skills` CLI keeps two * locks: a project lock at `/skills-lock.json` and a global lock under * `$HOME` (see lockPathForScope). * @@ -372,7 +357,7 @@ function scopeForDir(dir: string, home: string, cwd: string): "project" | "globa /** * Find the first skill root that actually contains HyperFrames skills. A * `--dir` override (if given) is treated as a `.../skills` directory directly; - * its scope is inferred (see scopeForDir) so removed-detection reads the right + * its scope is inferred (see scopeForDir) so lock attribution reads the right * lock. Otherwise scan global ($HOME) then project (cwd), auto-discovering hosts. * * Global is checked FIRST to match how agents actually load skills: Claude Code @@ -478,27 +463,15 @@ export function diffSkills( }; } -// ── Removed-upstream (orphaned) skills ──────────────────────────────────────── +// ── Install record (lock) ──────────────────────────────────────────────────── // -// `skills add` / `init` / `hyperframes skills update` only ever add or refresh — -// none of them delete a skill the manifest no longer publishes (e.g. the -// eight retired workflow skills), so a stale bundle lingers forever and -// the manifest-only diff above can't see it. We surface these by cross-checking -// the vercel-labs/skills lock: a skill the lock attributes to OUR manifest -// `source` that the manifest no longer lists is "removed". Attribution is the -// whole point — `.../skills` is shared across sources, so we never infer "ours" -// from a directory name alone (that would flag every other source's skills too). +// The vercel-labs/skills lock records which source each installed skill came +// from. `.../skills` is shared across sources, so a skill is never called ours +// from its directory name alone. interface LockEntry { source?: string; sourceUrl?: string; - /** - * Path of the skill's SKILL.md within the source repo, as upstream records it - * (`skills/slideshow/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. */ @@ -524,19 +497,20 @@ export function skillsAttributedToSource(lock: SkillLock | null, source: string) .map(([name]) => name); } -// Removed-detection reads the vercel-labs/skills lock, whose on-disk paths live -// in *their* repo, not ours — so if upstream moves the lock, our cross-reference -// silently finds nothing and `detectRemoved` no-ops without a peep. Pin the -// upstream version these paths were verified against so a future bump is a -// deliberate, reviewable edit (re-check src/skill-lock.ts `getSkillLockPath` -// and src/local-lock.ts `getLocalLockPath` when bumping): +/** + * The lock records another publisher as the skill's source. A skill with no + * lock entry is not "from another source": provenance is unknown, not foreign. + */ +function isFromOtherSource(entry: LockEntry | undefined, source: string): boolean { + const recorded = [repoSlug(entry?.source), repoSlug(entry?.sourceUrl)].filter(Boolean); + return recorded.length > 0 && !recorded.includes(repoSlug(source)); +} + +// Lock locations, verified against vercel-labs/skills@v1.5.13 (src/skill-lock.ts, +// src/local-lock.ts); re-check them when bumping: // - global: $XDG_STATE_HOME/skills/.skill-lock.json else ~/.agents/.skill-lock.json // - project: /skills-lock.json -// https://github.com/vercel-labs/skills/blob/v1.5.13/src/skill-lock.ts (global) -// https://github.com/vercel-labs/skills/blob/v1.5.13/src/local-lock.ts (project) -export const SKILLS_CLI_LOCK_PATHS_VERIFIED_AT = "vercel-labs/skills@v1.5.13"; - -/** Locate the vercel-labs/skills lock for a scope (paths pinned to the version above). */ +/** Locate the vercel-labs/skills lock for a scope. */ function lockPathForScope( scope: "project" | "global", opts: { cwd?: string; home?: string }, @@ -572,116 +546,6 @@ export function hyperframesSkillNames(opts: { return skillsAttributedToSource(readSkillLock(lockPath), DEFAULT_REPO_SLUG); } -interface RemovedResult { - removed: SkillDiff[]; - /** The lock was absent at the expected path — removed-detection silently no-ops. */ - lockMissing: boolean; -} - -/** - * 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/slideshow/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 attributed = skillsAttributedToSource(lock, latest.source); - const dropped = attributed.filter((name) => manifestCoversSkill(lock?.skills?.[name])); - // A retired name the lock attributes to us needs no skillPath (older locks - // lack one); a retired domain skill needs no lock entry at all when its - // bundle is on disk, because its prefix makes it ours by name. - const retired = RETIRED_SKILLS.filter( - (name) => - attributed.includes(name) || - (RETIRED_DOMAIN_SKILLS.includes(name) && existsSync(join(root.dir, name, "SKILL.md"))), - ); - const removed = [...new Set([...dropped, ...retired])] - .filter((name) => !(name in latest.skills)) - .sort() - .map((name) => ({ name, status: "removed" as const })); - return { removed, lockMissing: lock === null }; -} - -/** - * Remove `names` from the vercel-labs/skills lock at `scope`, writing the file - * back if anything changed. Self-heals the half of removed-upstream detection - * that `skills remove` can't: upstream's `remove` command scans ON-DISK skill - * directories to decide what's installed (see vercel-labs/skills' - * `removeCommand`), so a lock entry for a skill retired before it ever shipped - * a bundle to this machine has no on-disk dir to match. That makes `skills - * remove -g --yes` a silent no-op — it prints "No matching skills found - * for: …" and exits 0 WITHOUT touching the lock. Left alone, `detectRemoved` - * re-flags the same lock entry as "removed" on every future run, forever. - * - * Reuses the pinned lock path (see SKILLS_CLI_LOCK_PATHS_VERIFIED_AT above — - * re-check that comment before bumping the upstream version this is pinned - * against) so this writes to exactly where the upstream CLI itself reads and - * writes the lock. - * - * Idempotent by construction: only entries still present in the lock are ever - * touched, so calling this again with the same names — after the upstream - * `skills remove` no-op reported above has already run once — finds nothing - * left and returns `[]`. - */ -export function pruneOrphanedLockEntries( - names: readonly string[], - scope: "project" | "global", - opts: { cwd?: string; home?: string } = {}, -): string[] { - const path = lockPathForScope(scope, opts); - const lock = readSkillLock(path); - if (!lock?.skills) return []; - const pruned = names.filter((name) => name in lock.skills!); - if (pruned.length === 0) return []; - for (const name of pruned) delete lock.skills[name]; - // Atomic write (temp file + rename, same pattern as telemetry/autoUpdate.ts - // and utils/download.ts) so a crash mid-write can never leave a truncated - // lock behind. `path` is guaranteed to exist here (readSkillLock already - // returned a non-null lock), so preserving its mode on the temp file before - // the rename is safe. No trailing newline: matches the upstream - // vercel-labs/skills lock's on-disk shape, so a prune stays a minimal diff. - const mode = statSync(path).mode & 0o777; - const tmp = `${path}.tmp`; - writeFileSync(tmp, JSON.stringify(lock, null, 2), { mode }); - renameSync(tmp, path); - return pruned; -} - // ── Resolving the "latest" manifest ────────────────────────────────────────── /** Walk up from `cwd` to find a repo checkout that ships the manifest. */ @@ -828,21 +692,29 @@ export async function checkSkills( const latest = await resolveLatestManifest(opts.source, opts.cwd, { canonical: opts.canonical }); const skillNames = Object.keys(latest.skills); const root = locateInstall(skillNames, { dir: opts.dir, cwd: opts.cwd, home: opts.home }); - const installed = root ? hashInstalled(root, skillNames) : {}; - const diff = diffSkills(installed, latest); - const removedResult = root - ? detectRemoved(root, latest, { cwd: opts.cwd, home: opts.home }) - : { removed: [], lockMissing: false }; - const { removed, lockMissing } = removedResult; + // A directory sharing a name with one of ours can belong to another publisher. + // The lock says so; such a skill is never compared, refreshed or overwritten. + const lock = root ? readSkillLock(lockPathForScope(root.scope, opts)) : null; + const foreign = root + ? skillNames.filter( + (name) => + existsSync(join(root.dir, name, "SKILL.md")) && + isFromOtherSource(lock?.skills?.[name], latest.source), + ) + : []; + const ours = skillNames.filter((name) => !foreign.includes(name)); + const installed = root ? hashInstalled(root, ours) : {}; + const diff = diffSkills(installed, { + ...latest, + skills: Object.fromEntries(ours.map((name) => [name, latest.skills[name]!])), + }); return { location: root?.dir ?? null, agent: root?.agent ?? null, scope: root?.scope ?? null, - // Removed skills also mean the install isn't reconciled with the manifest — - // `skills update` now prunes them, so they count toward "update available". - updateAvailable: diff.updateAvailable || removed.length > 0, - summary: { ...diff.summary, removed: removed.length }, - skills: [...diff.skills, ...removed], - lockMissing, + updateAvailable: diff.updateAvailable, + summary: diff.summary, + skills: diff.skills, + foreign, }; } diff --git a/packages/cli/src/utils/skillsUpdateCheck.test.ts b/packages/cli/src/utils/skillsUpdateCheck.test.ts index d496476d28d..0b7b3c5d3b6 100644 --- a/packages/cli/src/utils/skillsUpdateCheck.test.ts +++ b/packages/cli/src/utils/skillsUpdateCheck.test.ts @@ -1,7 +1,4 @@ -// Nudge-count regression coverage: `refreshSkillsCache` must persist -// `summary.removed` (renamed/dropped skills), and the printed nudge total must -// include it — otherwise the background nudge undercounts what a plain -// `skills update` would actually reconcile (the "misleading 2 vs 3" bug). +// Nudge-count coverage: the printed total is outdated plus missing, as cached by refreshSkillsCache. import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -19,9 +16,10 @@ vi.mock("../telemetry/config.js", () => ({ }, })); -const gate = vi.hoisted(() => ({ suppressed: false, answers: true })); +const gate = vi.hoisted(() => ({ disabled: false, noTty: false, answers: true })); vi.mock("./updateCheck.js", () => ({ - updateNoticesSuppressed: () => gate.suppressed, + updateCheckDisabled: () => gate.disabled, + updateNoticesSuppressed: () => gate.disabled || gate.noTty, })); vi.mock("./hostAnswers.js", () => ({ hostAnswers: async () => gate.answers })); @@ -36,7 +34,8 @@ describe("skillsUpdateCheck", () => { config = {}; cachedRead = null; mockCheckSkills.mockReset(); - gate.suppressed = false; + gate.disabled = false; + gate.noTty = false; gate.answers = true; vi.stubEnv("HYPERFRAMES_SKIP_SKILLS", ""); }); @@ -47,7 +46,7 @@ describe("skillsUpdateCheck", () => { it.each([ ["HYPERFRAMES_SKIP_SKILLS is set", () => vi.stubEnv("HYPERFRAMES_SKIP_SKILLS", "1")], - ["the run shows no notices", () => (gate.suppressed = true)], + ["the update check is disabled (CI, dev, opt-out)", () => (gate.disabled = true)], ])("no background check is due when %s", async (_, arrange) => { const { skillsCheckDue } = await import("./skillsUpdateCheck.js"); expect(skillsCheckDue()).toBe(true); @@ -55,6 +54,12 @@ describe("skillsUpdateCheck", () => { expect(skillsCheckDue()).toBe(false); }); + it("a run without a TTY (an agent) still has its background check due", async () => { + gate.noTty = true; + const { skillsCheckDue } = await import("./skillsUpdateCheck.js"); + expect(skillsCheckDue()).toBe(true); + }); + it("does not check skills when DNS does not answer", async () => { gate.answers = false; const { refreshSkillsCache } = await import("./skillsUpdateCheck.js"); @@ -69,18 +74,18 @@ describe("skillsUpdateCheck", () => { mockCheckSkills.mockResolvedValue({ location: "/home/user/.claude/skills", updateAvailable: false, - summary: { current: 1, outdated: 0, missing: 0, coreMissing: 0, removed: 0 }, + summary: { current: 1, outdated: 0, missing: 0, coreMissing: 0 }, }); const { refreshSkillsCache } = await import("./skillsUpdateCheck.js"); await refreshSkillsCache(); expect(config).toMatchObject({ commandCount: 2, skillsOutdatedCount: 0 }); }); - it("refreshSkillsCache persists the removed count alongside outdated/missing", async () => { + it("refreshSkillsCache persists the outdated and core-missing counts", async () => { mockCheckSkills.mockResolvedValue({ location: "/home/user/.claude/skills", updateAvailable: true, - summary: { current: 1, outdated: 2, missing: 3, coreMissing: 1, removed: 3 }, + summary: { current: 1, outdated: 2, missing: 3, coreMissing: 1 }, }); const { refreshSkillsCache } = await import("./skillsUpdateCheck.js"); @@ -90,7 +95,6 @@ describe("skillsUpdateCheck", () => { skillsUpdateAvailable: true, skillsOutdatedCount: 2, skillsMissingCount: 1, - skillsRemovedCount: 3, }); // Must resolve against the canonical upstream manifest, not a possibly // stale in-repo skills-manifest.json, so this nudge agrees with what @@ -102,13 +106,11 @@ describe("skillsUpdateCheck", () => { mockCheckSkills.mockResolvedValue({ location: null, updateAvailable: false, - summary: { current: 0, outdated: 0, missing: 0, coreMissing: 0, removed: 0 }, + summary: { current: 0, outdated: 0, missing: 0, coreMissing: 0 }, }); const { refreshSkillsCache } = await import("./skillsUpdateCheck.js"); await refreshSkillsCache(); - - expect(config["skillsRemovedCount"]).toBeUndefined(); }); /** Drive printSkillsUpdateNotice from the given cache shape; returns what it wrote (if anything). */ @@ -130,34 +132,23 @@ describe("skillsUpdateCheck", () => { expect(await noticeTextFor({ skillsOutdatedCount: 2, skillsMissingCount: 1 })).toBeNull(); }); - it("the cached nudge total counts removed skills, not just outdated/missing", async () => { - // Cache pre-populated as if a prior refreshSkillsCache had run — only - // outdated + missing, no removed (the pre-fix shape). - const text = await noticeTextFor({ - skillsOutdatedCount: 1, - skillsMissingCount: 1, - skillsRemovedCount: 2, - }); - // 1 outdated + 1 missing + 2 removed = 4, not the pre-fix "2". - expect(text).toContain("4 HyperFrames skills out of date or missing"); + it("prints the stale-skills notice without a TTY, so an agent hears it", async () => { + gate.noTty = true; + const text = await noticeTextFor({ skillsOutdatedCount: 2, skillsMissingCount: 1 }); + expect(text).toContain("3 HyperFrames skills out of date or missing"); }); - it("prints nothing when outdated, missing, and removed are all zero", async () => { - const text = await noticeTextFor({ - skillsOutdatedCount: 0, - skillsMissingCount: 0, - skillsRemovedCount: 0, - }); - expect(text).toBeNull(); + it("prints nothing when the update check is disabled (CI, dev, opt-out)", async () => { + gate.disabled = true; + expect(await noticeTextFor({ skillsOutdatedCount: 2, skillsMissingCount: 1 })).toBeNull(); }); - it("a removed-only count (no outdated/missing) still triggers the nudge", async () => { + it("prints nothing when outdated and missing are both zero", async () => { const text = await noticeTextFor({ skillsOutdatedCount: 0, skillsMissingCount: 0, - skillsRemovedCount: 1, }); - expect(text).toContain("1 HyperFrames skill out of date or missing"); + expect(text).toBeNull(); }); // Regression: the stale-24h-cache bug. A successful `skills update`/install @@ -172,7 +163,6 @@ describe("skillsUpdateCheck", () => { skillsUpdateAvailable: true, skillsOutdatedCount: 12, skillsMissingCount: 8, - skillsRemovedCount: 0, }; it("a fresh cache short-circuits the background check with the stale verdict (the bug's precondition)", async () => { @@ -187,14 +177,14 @@ describe("skillsUpdateCheck", () => { mockCheckSkills.mockResolvedValue({ location: "/home/user/.claude/skills", updateAvailable: false, - summary: { current: 20, outdated: 0, missing: 0, coreMissing: 0, removed: 0 }, + summary: { current: 20, outdated: 0, missing: 0, coreMissing: 0 }, }); const { skillsCheckDue, refreshSkillsCache, invalidateSkillsCache } = await import("./skillsUpdateCheck.js"); invalidateSkillsCache(); - // All five cached fields are gone — timestamp AND counts. + // All four cached fields are gone — timestamp AND counts. expect(config["lastSkillsCheck"]).toBeUndefined(); expect(config["skillsUpdateAvailable"]).toBeUndefined(); expect(config["skillsOutdatedCount"]).toBeUndefined(); diff --git a/packages/cli/src/utils/skillsUpdateCheck.ts b/packages/cli/src/utils/skillsUpdateCheck.ts index d4cdf0d82d6..87b44f62a2c 100644 --- a/packages/cli/src/utils/skillsUpdateCheck.ts +++ b/packages/cli/src/utils/skillsUpdateCheck.ts @@ -9,7 +9,7 @@ import { readConfig, readConfigFresh, writeConfig } from "../telemetry/config.js"; import { hostAnswers } from "./hostAnswers.js"; import { checkSkills } from "./skillsManifest.js"; -import { updateNoticesSuppressed } from "./updateCheck.js"; +import { updateCheckDisabled } from "./updateCheck.js"; const CHECK_INTERVAL_MS = 24 * 60 * 60 * 1000; // 24 hours @@ -17,8 +17,6 @@ interface SkillsUpdateMeta { updateAvailable: boolean; outdated: number; missing: number; - /** Installed skills flagged removed-upstream (renamed/dropped) at the last check. */ - removed: number; } /** Synchronous read from cache — never fetches. */ @@ -28,7 +26,6 @@ function getSkillsUpdateMeta(): SkillsUpdateMeta { updateAvailable: config.skillsUpdateAvailable ?? false, outdated: config.skillsOutdatedCount ?? 0, missing: config.skillsMissingCount ?? 0, - removed: config.skillsRemovedCount ?? 0, }; } @@ -54,11 +51,6 @@ export async function refreshSkillsCache(): Promise { // Core-missing only: skills that install on demand (workflows not yet // triggered on this machine) are not "missing" worth nagging about. config.skillsMissingCount = result.summary.coreMissing; - // Removed-upstream skills are just as reconcilable as outdated/missing - // ones (a plain `skills update` prunes them) — omitting them here is what - // made the nudge undercount (e.g. reporting "2 skills out of date or - // missing" while a 3rd, renamed/dropped skill sat unmentioned). - config.skillsRemovedCount = result.summary.removed; writeConfig(config); } } @@ -90,33 +82,37 @@ export function invalidateSkillsCache(): void { delete config.skillsUpdateAvailable; delete config.skillsOutdatedCount; delete config.skillsMissingCount; - delete config.skillsRemovedCount; writeConfig(config); } catch { // best-effort — never break the command that just reconciled skills } } +/** Not tied to a TTY: agents run the CLI without one and are the ones that need to hear it. */ +function skillsNoticeOff(): boolean { + return process.env["HYPERFRAMES_SKIP_SKILLS"] === "1" || updateCheckDisabled(); +} + /** Whether a background skills check is due: this run shows notices and the 24 h cache is stale. */ export function skillsCheckDue(): boolean { - if (process.env["HYPERFRAMES_SKIP_SKILLS"] === "1" || updateNoticesSuppressed()) return false; + if (skillsNoticeOff()) return false; return !cacheFresh(readConfig().lastSkillsCheck, Date.now()); } -/** The stale-skills nudge text, or null when nothing is outdated, missing, or removed. */ +/** The stale-skills nudge text, or null when nothing is outdated or missing. */ function skillsNoticeText(meta: SkillsUpdateMeta): string | null { - const total = meta.outdated + meta.missing + meta.removed; + const total = meta.outdated + meta.missing; if (total < 1) return null; const noun = total === 1 ? "skill" : "skills"; return `\n ${total} HyperFrames ${noun} out of date or missing.\n Run: npx hyperframes skills update\n\n`; } /** - * Print a one-line nudge to stderr if installed skills are stale. Same gating - * as the CLI self-update notice (CI, non-TTY, dev, HYPERFRAMES_NO_UPDATE_CHECK). + * Print a one-line nudge to stderr if installed skills are stale. Off in CI, dev, + * HYPERFRAMES_NO_UPDATE_CHECK and HYPERFRAMES_SKIP_SKILLS; on without a TTY. */ export function printSkillsUpdateNotice(): void { - if (process.env["HYPERFRAMES_SKIP_SKILLS"] === "1" || updateNoticesSuppressed()) return; + if (skillsNoticeOff()) return; const text = skillsNoticeText(getSkillsUpdateMeta()); if (text) process.stderr.write(text); } diff --git a/skills-manifest.json b/skills-manifest.json index 1bab6bc8b50..e6442718b8a 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -6,11 +6,11 @@ "files": 2 }, "hyperframes": { - "hash": "b222ac5809fd2633", + "hash": "8fc4b05db245cbbb", "files": 77 }, "hyperframes-cli": { - "hash": "c7d7fb8c6106d8dd", + "hash": "d82c156c1b2069d1", "files": 13 }, "hyperframes-registry": { diff --git a/skills/hyperframes-cli/references/init-and-scaffold.md b/skills/hyperframes-cli/references/init-and-scaffold.md index 22cf29ea4a9..4d8c6620cd9 100644 --- a/skills/hyperframes-cli/references/init-and-scaffold.md +++ b/skills/hyperframes-cli/references/init-and-scaffold.md @@ -24,7 +24,7 @@ Other useful flags: - `--resolution` — preset: `landscape` (1920×1080), `portrait` (1080×1920), `landscape-4k`, `portrait-4k`, `square` (1080×1080), `square-4k`. Aliases: `1080p`, `4k`, `uhd`, `1080p-square`, `4k-square`. - `--skill=` — record the owning skill (e.g. `hyperframes`) in `hyperframes.json`, so every later render of this project — re-renders, `npm run render`, `--batch` — is attributed to it on anonymous telemetry without re-passing the flag. You rarely pass it by hand. -- `--skip-skills` — **temporarily ignored**: `init` always checks AI coding skills against GitHub while the skills.sh registry catches up. To opt out (CI/tests), set the `HYPERFRAMES_SKIP_SKILLS=1` env var instead. +- `--skip-skills` — leave AI coding skills alone: `init` does not install the core set. `HYPERFRAMES_SKIP_SKILLS=1` does the same. - `--skip-transcribe` — don't auto-transcribe `--audio` / `--video` with Whisper. - `--model`, `--language` — Whisper model / language for the auto-transcription. diff --git a/skills/hyperframes/references/skill-lifecycle.md b/skills/hyperframes/references/skill-lifecycle.md index eeaf8653b9b..2f397f01147 100644 --- a/skills/hyperframes/references/skill-lifecycle.md +++ b/skills/hyperframes/references/skill-lifecycle.md @@ -9,11 +9,15 @@ For a plugin installation, follow [plugin execution rules](plugin-installation.m ## What `init` does -`npx hyperframes init` checks GitHub and refreshes the core set plus other skills already installed. A current install is a no-op. Offline or rate-limited checks degrade gracefully and do not fail project scaffolding. +`npx hyperframes init` installs the core set when it is missing and leaves every skill that is already installed as it is: it never refreshes, replaces or removes one. Updating is `npx hyperframes skills update`, run by you. Offline or rate-limited checks degrade gracefully and do not fail project scaffolding. -The `--skip-skills` CLI flag is temporarily ignored. CI and tests may opt out with `HYPERFRAMES_SKIP_SKILLS=1`. +`--skip-skills` makes `init` leave skills alone. `HYPERFRAMES_SKIP_SKILLS=1` does the same for CI and tests. -`HYPERFRAMES_SKIP_SKILLS=1` also makes `skills update` (bare or named) and bare `skills` exit 0 without installing or pruning, so an install pinned to one commit stays pinned. +`HYPERFRAMES_SKIP_SKILLS=1` also makes `skills update` (bare or named) and bare `skills` exit 0 without installing, so an install pinned to one commit stays pinned. + +## Stale-skills notice + +After `render`, `lint` and `validate`, the CLI prints `N HyperFrames skills out of date or missing` to stderr when the cached check finds stale skills, so an agent run without a terminal sees it too. It is off in CI, in dev, with `HYPERFRAMES_NO_UPDATE_CHECK=1` and with `HYPERFRAMES_SKIP_SKILLS=1`, and runs with `--json` too, since it only writes stderr and leaves `--json` stdout untouched. ## Diagnose and update @@ -26,7 +30,7 @@ npx hyperframes skills ``` - `skills check` exits non-zero when an installed skill is stale or the core set is incomplete. Skills available on demand but not installed are not failures. -- Bare `skills update` refreshes the core set and everything already installed, and removes skills that are no longer published. +- Bare `skills update` refreshes the core set and the HyperFrames skills already installed. It never removes a skill, and it leaves a skill the install record attributes to another publisher alone, even when the name matches one of ours. - Named `skills update ` also installs those named skills. - Bare `skills` installs the full published set explicitly.