diff --git a/packages/cli/src/commands/skills.test.ts b/packages/cli/src/commands/skills.test.ts index 0ca3042c3..a674bff8b 100644 --- a/packages/cli/src/commands/skills.test.ts +++ b/packages/cli/src/commands/skills.test.ts @@ -17,10 +17,16 @@ type ExecCall = { }; const originalPlatform = process.platform; -const state: { execCalls: ExecCall[]; spawnCalls: SpawnCall[]; spawnExitCode: number } = { +const state: { + execCalls: ExecCall[]; + spawnCalls: SpawnCall[]; + spawnExitCode: number; + gitMissing: boolean; +} = { execCalls: [], spawnCalls: [], spawnExitCode: 0, + gitMissing: false, }; vi.mock("node:child_process", () => ({ @@ -30,6 +36,10 @@ vi.mock("node:child_process", () => ({ execFile: vi.fn(), execFileSync: vi.fn((command: string, args: ReadonlyArray) => { state.execCalls.push({ command, args }); + // Simulate `git` absent from PATH: execFileSync throws ENOENT like the OS would. + if (state.gitMissing && command === "git") { + throw Object.assign(new Error("spawn git ENOENT"), { code: "ENOENT" }); + } return Buffer.from("11.0.0"); }), spawn: vi.fn( @@ -49,6 +59,14 @@ vi.mock("@clack/prompts", () => ({ }, })); +// Capture the prerequisite-skip telemetry event without touching the real +// PostHog client. trackSkillsInstallSkipped already gates on the telemetry +// opt-out inside trackEvent, so the command calls it unconditionally. +const trackSkillsInstallSkipped = vi.fn(); +vi.mock("../telemetry/events.js", () => ({ + trackSkillsInstallSkipped: (...args: unknown[]) => trackSkillsInstallSkipped(...args), +})); + // `skills update` calls checkSkills() to find skills removed upstream, then // prunes them. Mock it so these tests don't touch the real FS / network; the // default returns nothing removed, and the prune test overrides per-call. @@ -101,6 +119,8 @@ describe("hyperframes skills", () => { state.execCalls = []; state.spawnCalls = []; state.spawnExitCode = 0; + state.gitMissing = false; + trackSkillsInstallSkipped.mockClear(); vi.resetModules(); // Each test asserts on process.exitCode; isolate it from the runner's own. prevExitCode = process.exitCode; @@ -304,4 +324,47 @@ describe("hyperframes skills", () => { expect(state.spawnCalls[0]?.args).toContain("add"); expect(process.exitCode).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; + + const { default: skillsCmd } = await import("./skills.js"); + await skillsCmd.run?.({ args: {}, rawArgs: [], cmd: skillsCmd } as never); + + expect(state.spawnCalls).toHaveLength(0); + expect(process.exitCode).toBe(0); + // Diagnostic instrumentation: the skip records why, so rare boxes hitting + // this (fresh Windows without git) are visible instead of silently no-op. + expect(trackSkillsInstallSkipped).toHaveBeenCalledWith({ reason: "git_missing" }); + }); + + // The happy path must never emit the prerequisite-skip event — it's a + // skip-only diagnostic, not a per-install signal. + it("bare `skills` does not emit the skip event when prerequisites are present", async () => { + setPlatform("linux"); + + const { default: skillsCmd } = await import("./skills.js"); + await skillsCmd.run?.({ args: {}, rawArgs: [], cmd: skillsCmd } as never); + + expect(state.spawnCalls.length).toBeGreaterThan(0); + expect(trackSkillsInstallSkipped).not.toHaveBeenCalled(); + }); + + // The strict recovery path (`skills check || skills update`) must fail loudly + // when git is missing, not silently no-op, else the `||` chain passes while + // nothing got installed. + it("skills update exits non-zero when git is unavailable", async () => { + setPlatform("linux"); + state.gitMissing = true; + + await runSkillsUpdate(); + + expect(state.spawnCalls).toHaveLength(0); + expect(process.exitCode).toBe(1); + }); }); diff --git a/packages/cli/src/commands/skills.ts b/packages/cli/src/commands/skills.ts index c74e4aff8..198fd4705 100644 --- a/packages/cli/src/commands/skills.ts +++ b/packages/cli/src/commands/skills.ts @@ -12,6 +12,7 @@ import { type SkillsCheckResult, } from "../utils/skillsManifest.js"; import { mirrorGlobalSkills } from "../utils/skillsMirror.js"; +import { trackSkillsInstallSkipped } from "../telemetry/events.js"; import type { Example } from "./_examples.js"; export const examples: Example[] = [ @@ -31,6 +32,20 @@ function hasNpx(): boolean { } } +// The upstream `skills` CLI clones the repo with `git`. When git is missing +// (common on fresh Windows boxes) the clone aborts mid-run with a noisy, +// multi-line `spawn git ENOENT` / "Installation failed" block, so detect git +// up front and skip cleanly instead of letting that surface. `git` resolves as +// a real executable on every platform, so no cmd.exe wrapping is needed. +function hasGit(): boolean { + try { + execFileSync("git", ["--version"], { stdio: "ignore", timeout: 5000 }); + return true; + } catch { + return false; + } +} + function spawnNpx(args: string[], opts: { cwd?: string } = {}): Promise { const npx = buildNpxCommand(args); return new Promise((resolve, reject) => { @@ -147,18 +162,51 @@ function mirrorToInstalledAgents(): void { } } +// The install shells out to `npx skills add`, and that CLI clones the repo with +// git — so both must be on PATH. Each entry pairs a detector with the strict +// error (thrown so the `check || update` recovery contract fails loudly) and a +// best-effort report (one calm line; init carries on and still scaffolds). +const SKILLS_TOOLING: ReadonlyArray<{ + has: () => boolean; + error: string; + // Low-cardinality tag for the skip telemetry event (e.g. "git_missing"). + reason: string; + report: () => void; +}> = [ + { + has: hasNpx, + error: "npx not found. Install Node.js and retry.", + reason: "npx_missing", + report: () => clack.log.error(c.error("npx not found. Install Node.js and retry.")), + }, + { + has: hasGit, + error: "git not found. Install git and retry to add AI coding skills.", + reason: "git_missing", + // Skip cleanly rather than letting the upstream clone dump a noisy + // multi-line `spawn git ENOENT` / "Installation failed" abort. + report: () => console.log(c.dim("Skipping AI coding skills: git not available.")), + }, +]; + +/** True if the install can proceed; otherwise reports (or throws, when strict). */ +function skillsToolingReady(strict: boolean): boolean { + for (const tool of SKILLS_TOOLING) { + if (tool.has()) continue; + if (strict) throw new Error(tool.error); + tool.report(); + // Surface the rare best-effort skip (init on a box missing git/npx); the + // event respects the telemetry opt-out inside trackEvent. + trackSkillsInstallSkipped({ reason: tool.reason }); + return false; + } + return true; +} + export async function installAllSkills( opts: { cwd?: string; extraArgs?: string[]; strict?: boolean } = {}, ): Promise { - if (!hasNpx()) { - const msg = "npx not found. Install Node.js and retry."; - // strict callers (e.g. `skills update`) need a real failure so a recovery - // command can't exit 0 having done nothing; best-effort callers (init) just - // warn and carry on. - if (opts.strict) throw new Error(msg); - clack.log.error(c.error(msg)); - return; - } + if (!skillsToolingReady(opts.strict ?? false)) return; for (const source of SOURCES) { console.log(); diff --git a/packages/cli/src/telemetry/events.ts b/packages/cli/src/telemetry/events.ts index f444aa67a..74a7854ff 100644 --- a/packages/cli/src/telemetry/events.ts +++ b/packages/cli/src/telemetry/events.ts @@ -348,6 +348,15 @@ export function trackTranscribeUnavailable(props: { optional: boolean }): void { trackEvent("transcribe_unavailable", { optional: props.optional }); } +// A skills install was skipped because a required prerequisite binary is +// absent from PATH (e.g. git on a fresh Windows box). Best-effort callers +// (init) skip cleanly rather than crash, so the skip is otherwise invisible; +// this surfaces the rare environments that hit it. `reason` is a low-cardinality +// binary tag (e.g. "git_missing"), never a path or free text. +export function trackSkillsInstallSkipped(props: { reason: string }): void { + trackEvent("cli skill install skipped", { reason: props.reason }); +} + export function trackRenderFeedback(props: { rating: number; renderDurationMs?: number;