fix(skills,cli): close four reproduced contract gaps from the CLI feedback digest (#2476)

* fix(cli): invalidate the skills nudge cache after a successful install/update/check The passive "N skills out of date or missing" nudge reads a 24h config cache that only the background check (on non-skills commands) ever wrote. The skills commands themselves are excluded from the nudge pipeline, so a successful `skills update`/install/check never refreshed or dropped the cached verdict — the pre-install count kept printing on every other command for up to 24h. Reconcile commands now drop the cached verdict (counts + timestamp) so the next command's background check re-runs for real. The offline presence-only path deliberately keeps the cache: that run learned nothing about freshness. * fix(skills): win32-safe npx spawns in media-use + accurate whisper wording The Whisper transcribe fallback and the Kokoro local-TTS delegation both spawned a bare "npx" via execFileSync — on Windows npx is npx.cmd, which spawn cannot exec, so both paths died with `spawnSync npx ENOENT`. Route them through the skill's existing resolveSpawnCommand (node + npx-cli.js on win32, no shell:true), same as the audio engine's TTS spawns. Also corrects the "bundled with the hyperframes CLI" claim about whisper.cpp: it is resolved from PATH / installed via Homebrew / built from source with git+cmake on first use, and models download from HuggingFace — nothing whisper is shipped in the package. * feat(skills): canonical fully-silent marker + auth status exit-code docs product-launch's Step 3.1 gate said "or the project is marked silent" but nothing defined how to mark one, and audio.mjs unconditionally retrieved BGM. Define the canonical marker — `music: none` in the storyboard's top YAML block, plus no SCRIPT.md — and honor it: audio generate produces nothing (removing stale audio_meta.json, since absence is what assemble treats as silent), and `music: none` with narration keeps TTS while turning BGM off. Also documents the `auth status` exit-code contract (exit 1 while signed out is the normal offline state, not a failure) in the product-launch Step 0 note and the CLI skill's cloud reference. * fix(skills): transient-init retry for standalone animation-map and contrast-report The standalone helpers called initializeSession exactly once, so a valid modular project — whose sub-composition timelines register asynchronously — could hit the readiness deadline and die with the transient "zero duration / Runtime ready: false" diagnostic the render pipeline retries (probeStage). Add initializeSessionWithRetry to the shared package-loader (both byte-identical copies): close the crashed session and retry once with a fresh browser, gated by the engine's canonical isTransientBrowserError — now re-exported from @hyperframes/producer, with a frozen fallback pattern list for older published packages. The "Runtime ready: true" fast-fail (a genuine authoring bug) still fails without a retry. * feat(skills): extend the fully-silent marker to faceless-explainer and pr-to-video Both workflows reuse product-launch's audio model — their Step 3.1 gates carried the same undefined "marked silent" phrase, and their (intentionally identical) audio.mjs copies had the same unconditional BGM retrieve. Port the `music: none` marker handling into both copies, define the marker in their SKILL.md Step 3.1 and story-design references, and turn the copies' "intentionally identical" header claim into a byte-identity pin test so the next fix can't silently miss one of them. * test(cli): reset the prune mock explicitly instead of relying on restoreAllMocks The converge test's toHaveBeenCalledTimes(1) held only because vitest 3's vi.restoreAllMocks() clears vi.fn() call state; vitest 4 restores spies only, so the count would accumulate across tests and fail. Reset pruneOrphanedLockEntries in beforeEach like the other manifest mocks — passes under both vitest 3.2.4 (pinned) and vitest 4. * test(skills): close review findings — package-loader pin, whisper win32 parity, quoted-none Review follow-ups on #2476: - package-loader.mjs byte-identity pin (the elevated concern): the two copies now carry initializeSessionWithRetry + FALLBACK_TRANSIENT_PATTERNS, exactly the shared-logic shape a future fix could land in one copy and miss in the other — same enforcement as the audio.mjs pin. - whisper win32 call-site parity: runWhisper's npx resolution lifted into lib/npx-sync.mjs (resolveNpxInvocation, injectable params matching the localTtsGenerate idiom) with the same three-branch coverage as the Kokoro site — plus the hard-fail contract (throws actionably, since the whisper fallback has no next provider to fall through to). - quoted music: "none" pin: the vendored storyboard parser strips matching quotes at parse time (stripQuotes), so the silent marker already accepts the quoted spelling — pinned so that stays true.
2026-09-03 04:38:33 +00:00 · 2026-07-15 22:22:16 +08:00
parent 1895286189
commit 7d21cc9b8a
31 changed files with 1040 additions and 70 deletions
@@ -112,6 +112,15 @@ vi.mock("../utils/skillsMirror.js", () => ({
  mirrorGlobalSkills: vi.fn(() => ({ source: null, mirrored: [] })),
 }));

+// The reconcile commands drop the background nudge's cached verdict on
+// success (the stale-24h-cache fix). Stub it so these tests never touch the
+// dev machine's real ~/.hyperframes config; the invalidation behavior itself
+// is unit-tested in skillsUpdateCheck.test.ts.
+const invalidateSkillsCache = vi.fn();
+vi.mock("../utils/skillsUpdateCheck.js", () => ({
+  invalidateSkillsCache: (...args: unknown[]) => invalidateSkillsCache(...args),
+}));
+
 // The global install command this CLI runs (after `skills add <url>` and the
 // per-name `--skill` selection).
 const GLOBAL_ARGS_TAIL = [
@@ -133,7 +142,7 @@ function setPlatform(platform: NodeJS.Platform): void {

 /** Invoke a `skills <name>` subcommand from a freshly-imported module. */
 async function runSkillsSub(
-  name: "update",
+  name: "update" | "check",
  args: Record<string, unknown> = {},
  positionals: string[] = [],
 ): Promise<void> {
@@ -175,11 +184,19 @@ 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.
-    const { checkSkills, presentSkills } = await import("../utils/skillsManifest.js");
+    // 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");
    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(() => []);
    // Each test asserts on process.exitCode; isolate it from the runner's own.
    prevExitCode = process.exitCode;
    process.exitCode = 0;
@@ -570,6 +587,65 @@ describe("hyperframes skills", () => {
    expect(state.spawnCalls).toHaveLength(0);
    expect(process.exitCode).toBe(1);
  });
+
+  // The stale-24h-cache regression: the skills commands are excluded from the
+  // background nudge pipeline (cli.ts), so unless they drop the cached verdict
+  // themselves, a successful install keeps the pre-install "N out of date or
+  // missing" nag alive on every other command for up to 24h.
+  describe("nudge-cache invalidation", () => {
+    it("skills update drops the cached nudge verdict on success", async () => {
+      setPlatform("linux");
+      invalidateSkillsCache.mockClear();
+
+      await runSkillsUpdate();
+
+      expect(process.exitCode).toBe(0);
+      expect(invalidateSkillsCache).toHaveBeenCalled();
+    });
+
+    it("skills update keeps the cached verdict when the install fails", async () => {
+      setPlatform("linux");
+      invalidateSkillsCache.mockClear();
+      state.spawnExitCode = 1; // `skills add` exits non-zero → strict throw
+
+      await runSkillsUpdate();
+
+      expect(process.exitCode).toBe(1);
+      expect(invalidateSkillsCache).not.toHaveBeenCalled();
+    });
+
+    it("skills update keeps the cached verdict on the offline (presence-only) path", async () => {
+      setPlatform("linux");
+      invalidateSkillsCache.mockClear();
+      const { checkSkills } = await import("../utils/skillsManifest.js");
+      vi.mocked(checkSkills).mockRejectedValue(new Error("offline"));
+
+      await runSkillsUpdate();
+
+      // Presence-only run never learned anything about freshness — the cached
+      // verdict is still the best information available.
+      expect(invalidateSkillsCache).not.toHaveBeenCalled();
+    });
+
+    it("bare `skills` (full install) drops the cached nudge verdict", async () => {
+      setPlatform("linux");
+      invalidateSkillsCache.mockClear();
+
+      const { default: skillsCmd } = await import("./skills.js");
+      await skillsCmd.run?.({ args: {}, rawArgs: [], cmd: skillsCmd } as never);
+
+      expect(invalidateSkillsCache).toHaveBeenCalled();
+    });
+
+    it("skills check drops the cached verdict — the fresh result supersedes it", async () => {
+      setPlatform("linux");
+      invalidateSkillsCache.mockClear();
+
+      await runSkillsSub("check", { json: true });
+
+      expect(invalidateSkillsCache).toHaveBeenCalled();
+    });
+  });
 });

 // The router contract: `/hyperframes` picks a workflow, then runs
@@ -585,11 +661,16 @@ describe("hyperframes skills update <names>", () => {
    state.spawnExitCode = 0;
    state.gitMissing = false;
    vi.resetModules();
-    const { checkSkills, presentSkills } = await import("../utils/skillsManifest.js");
+    // Same explicit resets as the describe above (incl. the vitest-4-proofing
+    // note on pruneOrphanedLockEntries).
+    const { checkSkills, presentSkills, pruneOrphanedLockEntries } =
+      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(() => []);
    prevExitCode = process.exitCode;
    process.exitCode = 0;
  });
@@ -16,6 +16,7 @@ import {
  type SkillsCheckResult,
 } from "../utils/skillsManifest.js";
 import { mirrorGlobalSkills } from "../utils/skillsMirror.js";
+import { invalidateSkillsCache } from "../utils/skillsUpdateCheck.js";
 import { trackSkillsInstallSkipped } from "../telemetry/events.js";
 import type { Example } from "./_examples.js";

@@ -368,6 +369,13 @@ export async function updateSkills(
    await installSkills(result.installed, { cwd: opts.cwd, strict });
    verifyInstalled(result.installed, { strict, cwd: opts.cwd });
  }
+  // The install (or the fresh canonical check confirming everything current)
+  // supersedes whatever the background nudge cached before it — drop the
+  // cached verdict so the next command re-checks instead of nagging from the
+  // pre-install snapshot for up to 24h. Deliberately NOT done on the offline
+  // (presence-only) path above: that run never learned anything about
+  // freshness, so the cached verdict is the best information we still have.
+  invalidateSkillsCache();
  return result;
 }

@@ -555,6 +563,11 @@ const checkCommand = defineCommand({
    if (args.json) console.log(JSON.stringify(withMeta(result), null, 2));
    else renderCheck(result);

+    // This check just displayed a fresh verdict, superseding whatever the
+    // background nudge cached — invalidate so the next command's nudge agrees
+    // with what the user was just shown instead of a pre-check snapshot.
+    invalidateSkillsCache();
+
    // Exit non-zero when installed skills are stale, so agents and CI can gate:
    //   hyperframes skills check || npx hyperframes skills update
    if (result.updateAvailable) process.exitCode = 1;
@@ -748,6 +761,11 @@ export default defineCommand({
    // citty runs this parent handler even when a subcommand matches; guard on
    // the positional so bare `hyperframes skills` installs, while
    // `hyperframes skills check|update` does not also re-install.
-    if (!args._?.[0]) await installSkills("*");
+    if (!args._?.[0]) {
+      await installSkills("*");
+      // Same as updateSkills: a full install supersedes the background
+      // nudge's cached pre-install verdict.
+      invalidateSkillsCache();
+    }
  },
 });
@@ -11,6 +11,7 @@ let config: FakeConfig;

 vi.mock("../telemetry/config.js", () => ({
  readConfig: () => ({ ...config }),
+  readConfigFresh: () => ({ ...config }),
  writeConfig: (next: FakeConfig) => {
    config = { ...next };
  },
@@ -108,4 +109,71 @@ describe("skillsUpdateCheck", () => {
    });
    expect(text).toContain("1 HyperFrames skill out of date or missing");
  });
+
+  // Regression: the stale-24h-cache bug. A successful `skills update`/install
+  // never wrote the cache, and the skills commands are excluded from the nudge
+  // pipeline entirely — so the pre-install "20 out of date or missing" verdict
+  // kept printing on every other command until the TTL expired.
+  // invalidateSkillsCache() is the fix: reconcile commands drop the cached
+  // verdict so the next command re-checks for real.
+  describe("invalidateSkillsCache", () => {
+    const PRE_INSTALL_CACHE = {
+      lastSkillsCheck: new Date().toISOString(), // fresh — inside the 24h TTL
+      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 () => {
+      config = { ...PRE_INSTALL_CACHE };
+      const { checkSkillsForUpdate } = await import("./skillsUpdateCheck.js");
+
+      const meta = await checkSkillsForUpdate();
+
+      expect(mockCheckSkills).not.toHaveBeenCalled();
+      expect(meta).toEqual({ updateAvailable: true, outdated: 12, missing: 8, removed: 0 });
+    });
+
+    it("drops the cached verdict so the next background check re-runs for real", async () => {
+      config = { ...PRE_INSTALL_CACHE };
+      mockCheckSkills.mockResolvedValue({
+        location: "/home/user/.claude/skills",
+        updateAvailable: false,
+        summary: { current: 20, outdated: 0, missing: 0, coreMissing: 0, removed: 0 },
+      });
+
+      const { checkSkillsForUpdate, invalidateSkillsCache } =
+        await import("./skillsUpdateCheck.js");
+      invalidateSkillsCache();
+
+      // All five cached fields are gone — timestamp AND counts.
+      expect(config["lastSkillsCheck"]).toBeUndefined();
+      expect(config["skillsUpdateAvailable"]).toBeUndefined();
+      expect(config["skillsOutdatedCount"]).toBeUndefined();
+      expect(config["skillsMissingCount"]).toBeUndefined();
+      expect(config["skillsRemovedCount"]).toBeUndefined();
+
+      const meta = await checkSkillsForUpdate();
+      expect(mockCheckSkills).toHaveBeenCalledWith({ canonical: true });
+      expect(meta).toEqual({ updateAvailable: false, outdated: 0, missing: 0, removed: 0 });
+    });
+
+    it("counts are cleared, not just the timestamp — an offline machine goes quiet instead of resurrecting stale counts", async () => {
+      config = { ...PRE_INSTALL_CACHE };
+      mockCheckSkills.mockRejectedValue(new Error("offline"));
+
+      const { checkSkillsForUpdate, invalidateSkillsCache, printSkillsUpdateNotice } =
+        await import("./skillsUpdateCheck.js");
+      invalidateSkillsCache();
+
+      // Refresh fails (offline) → falls back to cached meta, which is now empty.
+      const meta = await checkSkillsForUpdate();
+      expect(meta).toEqual({ updateAvailable: false, outdated: 0, missing: 0, removed: 0 });
+
+      const writeSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true);
+      printSkillsUpdateNotice();
+      expect(writeSpy).not.toHaveBeenCalled();
+    });
+  });
 });
@@ -6,7 +6,7 @@
 // check on their own, but they DO run render/lint/validate — so we piggyback
 // the reminder on the commands they already run.

-import { readConfig, writeConfig } from "../telemetry/config.js";
+import { readConfig, readConfigFresh, writeConfig } from "../telemetry/config.js";
 import { checkSkills } from "./skillsManifest.js";
 import { updateNoticesSuppressed } from "./updateCheck.js";

@@ -66,6 +66,40 @@ async function refreshSkillsCache(): Promise<SkillsUpdateMeta> {
  };
 }

+/**
+ * Drop the cached verdict (counts + timestamp) so the next command's
+ * background check re-runs instead of nagging from a pre-reconcile snapshot.
+ *
+ * Called after a `skills` install/update/check has reconciled or re-measured
+ * the install. Those commands are excluded from the nudge pipeline entirely
+ * (see cli.ts), so nothing else refreshes the cache when they run — without
+ * this, the last background verdict (taken BEFORE the install) keeps printing
+ * "N skills out of date or missing" on every other command for up to 24h
+ * after a successful install/update.
+ *
+ * Counts are cleared along with the timestamp — not left behind — so an
+ * offline machine (where the next refresh fails and falls back to the cached
+ * meta) goes quiet rather than resurrecting the stale pre-install counts.
+ *
+ * Best-effort: a config write failure must never fail the skills command
+ * that just succeeded.
+ */
+export function invalidateSkillsCache(): void {
+  try {
+    // Fresh read narrows the lost-update window against a concurrently
+    // running CLI process that wrote other config fields in the meantime.
+    const config = readConfigFresh();
+    delete config.lastSkillsCheck;
+    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
+  }
+}
+
 /**
 * Refresh the skills freshness cache if it is older than 24h. Best-effort:
 * any failure (offline, no manifest published yet, no skills installed) leaves