mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 12:54:29 +00:00
fix(cli): skip AI skills install when git is unavailable (#1803)
* fix(cli): skip AI skills install when git is unavailable init and `skills update` route through installAllSkills, which shells out to `npx skills add`. That CLI clones the repo with git, so on a machine without git the clone aborts mid-run and dumps a noisy multi-line `spawn git ENOENT` / "Installation failed" / "Canceled" block. init still exited 0 and scaffolded the project, but the output read like a hard failure (and surfaced as exit 1 on some platforms). Detect git up front alongside the existing npx check via a small table-driven preflight: best-effort callers (init) print one calm line and continue; strict callers (`skills update`) throw so the check-or-update recovery contract still fails loudly. The skills freshness check already degrades gracefully without git, so the happy path is unchanged. * feat(cli): record a diagnostic event when a skills install is skipped for a missing prerequisite When init's best-effort skills install bails because git (or npx) is absent from PATH, the skip was silent, so the rare boxes that hit it (fresh Windows without git) were invisible. Emit one low-cardinality event (reason: git_missing / npx_missing) on the best-effort skip path only, never on the happy path or the strict throw. Reuses the existing typed-event pattern, and trackEvent's opt-out gate already applies.
This commit is contained in:
@@ -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<string>) => {
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<void> {
|
||||
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<void> {
|
||||
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();
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user