mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-01 19:42:03 +00:00
feat(cli): report command failure reasons to telemetry (de-blind browser/info) (#1484)
Observability showed `browser` (~75% fail, ~1.3k users/day) and `info` (~60% fail) failing at high rates with no captured reason — only `cli_command_result success=false`. citty's `runMain` catches a command's thrown error and `process.exit(1)`s without re-throwing, so a thrown failure never reached the existing `cli_error` telemetry (which only fired from the uncaughtException / unhandledRejection handlers). Wrap every command's `run()` at the dispatch boundary (cli.ts) so a thrown failure reports its reason via `cli_error` (kind=command_error) before being re-thrown unchanged — citty's print + exit-1 behavior is preserved. This de-blinds every throw-style command at once: `browser ensure` (Chrome download), `tts`, `inspect`, `render`, etc. Paths that bypass the wrapper are handled inline: - `browser` self-exits (`path` download failure, unknown subcommand) — report inline; the ARM64 `ensure` branch previously swallowed a failed install and returned success, now reports and exits 1. - `resolveProject()` self-exits on InvalidProjectError (the dominant `info` failure — run outside a project) — report inline before exit. Hardening: - PII: `trackCliError` now redacts error_message + stack_trace via redactTelemetryString (matching render_* events) — CLI errors and stacks carry absolute install paths / cache dirs / user args. - Race: the wrapper awaits an on-demand telemetry import before re-throwing, so a command that fails before the lazy telemetry import settles still reports (a telemetry failure is swallowed and never masks the real error). Pure helpers in utils/command-failure-tracking.ts with unit tests for the throw / success / no-run / onFailure-rejection cases, the reporter wiring, and trackCliError redaction. CommandDef<any> mirrors citty's SubCommandsDef. Known scope: commands that print + `process.exit(1)` on their own validation paths (tts/validate/lint argument errors) remain wrapper-blind — follow-up.
This commit is contained in:
+17
-2
@@ -101,6 +101,7 @@ try {
|
||||
|
||||
import { defineCommand, runMain } from "citty";
|
||||
import type { ArgsDef, CommandDef } from "citty";
|
||||
import { reportCommandFailure, trackCommandFailures } from "./utils/command-failure-tracking.js";
|
||||
|
||||
const isHelp = process.argv.includes("--help") || process.argv.includes("-h");
|
||||
|
||||
@@ -108,7 +109,7 @@ const isHelp = process.argv.includes("--help") || process.argv.includes("-h");
|
||||
// CLI definition — all commands are lazy-loaded via dynamic import()
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const subCommands = {
|
||||
const commandLoaders = {
|
||||
init: () => import("./commands/init.js").then((m) => m.default),
|
||||
add: () => import("./commands/add.js").then((m) => m.default),
|
||||
catalog: () => import("./commands/catalog.js").then((m) => m.default),
|
||||
@@ -142,6 +143,17 @@ const subCommands = {
|
||||
auth: () => import("./commands/auth.js").then((m) => m.default),
|
||||
};
|
||||
|
||||
// Wrap each command's run() so a thrown failure reports its reason to telemetry
|
||||
// before citty catches the error and exits 1. The error is re-thrown unchanged,
|
||||
// preserving citty's print + exit-1 behavior. Commands that call process.exit()
|
||||
// themselves (e.g. `browser path`) bypass this and report inline.
|
||||
const subCommands = Object.fromEntries(
|
||||
Object.entries(commandLoaders).map(([name, load]) => [
|
||||
name,
|
||||
trackCommandFailures(load, (err) => reportCommandFailure(command, err)),
|
||||
]),
|
||||
);
|
||||
|
||||
const main = defineCommand({
|
||||
meta: {
|
||||
name: "hyperframes",
|
||||
@@ -156,7 +168,10 @@ const main = defineCommand({
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const cliCommandArg = process.argv[2];
|
||||
const command = cliCommandArg && cliCommandArg in subCommands ? cliCommandArg : "unknown";
|
||||
// Explicit annotation breaks a type cycle: `subCommands` references `command`
|
||||
// (in the failure reporter) and `command` references `subCommands` (the `in`
|
||||
// check), so its type can't be inferred from its own initializer.
|
||||
const command: string = cliCommandArg && cliCommandArg in subCommands ? cliCommandArg : "unknown";
|
||||
const hasJsonFlag = process.argv.includes("--json");
|
||||
|
||||
// Captured references — populated when the lazy imports resolve.
|
||||
|
||||
@@ -17,7 +17,7 @@ import {
|
||||
CACHE_DIR,
|
||||
isLinuxArm,
|
||||
} from "../browser/manager.js";
|
||||
import { trackBrowserInstall } from "../telemetry/events.js";
|
||||
import { trackBrowserInstall, trackCommandFailure } from "../telemetry/events.js";
|
||||
|
||||
async function runEnsure(): Promise<void> {
|
||||
clack.intro(c.bold("hyperframes browser ensure"));
|
||||
@@ -50,8 +50,12 @@ async function runEnsure(): Promise<void> {
|
||||
console.log();
|
||||
clack.outro(c.success("Chromium ready. You can now render on ARM64."));
|
||||
} catch (err) {
|
||||
// The ARM64 auto-install failed: the browser is NOT ready, so this is a
|
||||
// real failure (exit 1), not a success. Report it and stop swallowing.
|
||||
trackCommandFailure("browser", err);
|
||||
clack.log.error(err instanceof Error ? err.message : String(err));
|
||||
clack.outro(c.warn("Manual setup required (see instructions above)."));
|
||||
process.exit(1);
|
||||
}
|
||||
return;
|
||||
}
|
||||
@@ -108,6 +112,7 @@ async function runPath(): Promise<void> {
|
||||
const ensured = await ensureBrowser();
|
||||
process.stdout.write(ensured.executablePath + "\n");
|
||||
} catch (err: unknown) {
|
||||
trackCommandFailure("browser", err);
|
||||
console.error(err instanceof Error ? err.message : "Failed to find browser");
|
||||
process.exit(1);
|
||||
}
|
||||
@@ -167,6 +172,7 @@ ${c.bold("EXAMPLES:")}
|
||||
case "clear":
|
||||
return runClear();
|
||||
default:
|
||||
trackCommandFailure("browser", `Unknown subcommand: ${subcommand}`);
|
||||
console.error(
|
||||
`${c.error("Unknown subcommand:")} ${subcommand}\n\nRun ${c.accent("hyperframes browser --help")} for usage.`,
|
||||
);
|
||||
|
||||
@@ -5,7 +5,7 @@ vi.mock("./client.js", () => ({
|
||||
trackEvent: (...args: unknown[]) => trackEvent(...args),
|
||||
}));
|
||||
|
||||
const { trackRenderError, trackRenderObservation, trackCommandFailure } =
|
||||
const { trackRenderError, trackRenderObservation, trackCommandFailure, trackCliError } =
|
||||
await import("./events.js");
|
||||
|
||||
describe("render telemetry events", () => {
|
||||
@@ -52,6 +52,27 @@ describe("render telemetry events", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("trackCliError", () => {
|
||||
beforeEach(() => {
|
||||
trackEvent.mockClear();
|
||||
});
|
||||
|
||||
it("redacts install paths from error_message and stack_trace", () => {
|
||||
trackCliError({
|
||||
error_name: "Error",
|
||||
error_message: "ENOENT: open '/Users/alice/project/index.html'",
|
||||
stack_trace: "Error: boom\n at /Users/alice/.cache/hyperframes/chrome/headless",
|
||||
command: "info",
|
||||
kind: "command_error",
|
||||
});
|
||||
|
||||
const [, props] = trackEvent.mock.calls[0] as [string, Record<string, string>];
|
||||
expect(props.error_message).not.toContain("/Users/alice");
|
||||
expect(props.error_message).toContain("[path]");
|
||||
expect(props.stack_trace).not.toContain("/Users/alice");
|
||||
});
|
||||
});
|
||||
|
||||
describe("trackCommandFailure", () => {
|
||||
beforeEach(() => {
|
||||
trackEvent.mockClear();
|
||||
@@ -68,7 +89,8 @@ describe("trackCommandFailure", () => {
|
||||
command: "transcribe",
|
||||
error_name: "Error",
|
||||
error_message: "ffmpeg is required to extract audio",
|
||||
stack_trace: err.stack,
|
||||
// stack_trace is asserted (redacted) in the trackCliError suite; the
|
||||
// raw err.stack no longer matches once paths are stripped.
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
@@ -277,8 +277,13 @@ export function trackCliError(props: {
|
||||
}): void {
|
||||
trackEvent("cli_error", {
|
||||
error_name: props.error_name,
|
||||
error_message: props.error_message.slice(0, 1000),
|
||||
stack_trace: props.stack_trace?.slice(0, 2000),
|
||||
// Redact before truncating — CLI messages and stack traces carry absolute
|
||||
// install paths (/Users/...), cache dirs, and user-supplied args. Same
|
||||
// redaction the render_* events already apply.
|
||||
error_message: redactTelemetryMessage(props.error_message).slice(0, 1000),
|
||||
stack_trace: props.stack_trace
|
||||
? redactTelemetryMessage(props.stack_trace).slice(0, 2000)
|
||||
: undefined,
|
||||
command: props.command,
|
||||
kind: props.kind,
|
||||
});
|
||||
|
||||
@@ -0,0 +1,81 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import type { CommandDef } from "citty";
|
||||
|
||||
const trackCommandFailure = vi.fn();
|
||||
vi.mock("../telemetry/events.js", () => ({
|
||||
trackCommandFailure: (...args: unknown[]) => trackCommandFailure(...args),
|
||||
}));
|
||||
|
||||
const { trackCommandFailures, reportCommandFailure } =
|
||||
await import("./command-failure-tracking.js");
|
||||
|
||||
function defineRun(run: CommandDef["run"]): CommandDef {
|
||||
return { meta: { name: "test" }, run };
|
||||
}
|
||||
|
||||
describe("trackCommandFailures", () => {
|
||||
it("reports the error and re-throws when run() rejects", async () => {
|
||||
const onFailure = vi.fn();
|
||||
const boom = new Error("ffmpeg not found");
|
||||
const wrapped = trackCommandFailures(
|
||||
() => Promise.resolve(defineRun(() => Promise.reject(boom))),
|
||||
onFailure,
|
||||
);
|
||||
|
||||
const cmd = await wrapped();
|
||||
await expect((cmd.run as () => Promise<unknown>)()).rejects.toBe(boom);
|
||||
expect(onFailure).toHaveBeenCalledWith(boom);
|
||||
});
|
||||
|
||||
it("does not report when run() succeeds, and returns its value", async () => {
|
||||
const onFailure = vi.fn();
|
||||
const wrapped = trackCommandFailures(
|
||||
() => Promise.resolve(defineRun(() => Promise.resolve("ok" as unknown as void))),
|
||||
onFailure,
|
||||
);
|
||||
|
||||
const cmd = await wrapped();
|
||||
await expect((cmd.run as () => Promise<unknown>)()).resolves.toBe("ok");
|
||||
expect(onFailure).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("passes through a command with no run() untouched", async () => {
|
||||
const onFailure = vi.fn();
|
||||
const parent: CommandDef = { meta: { name: "parent" } };
|
||||
const wrapped = trackCommandFailures(() => Promise.resolve(parent), onFailure);
|
||||
|
||||
const cmd = await wrapped();
|
||||
expect(cmd).toBe(parent);
|
||||
expect(onFailure).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("awaits onFailure and re-throws the ORIGINAL error even if onFailure rejects", async () => {
|
||||
const boom = new Error("original failure");
|
||||
const wrapped = trackCommandFailures(
|
||||
() => Promise.resolve(defineRun(() => Promise.reject(boom))),
|
||||
() => Promise.reject(new Error("telemetry is down")),
|
||||
);
|
||||
|
||||
const cmd = await wrapped();
|
||||
await expect((cmd.run as () => Promise<unknown>)()).rejects.toBe(boom);
|
||||
});
|
||||
});
|
||||
|
||||
describe("reportCommandFailure", () => {
|
||||
beforeEach(() => {
|
||||
trackCommandFailure.mockReset();
|
||||
});
|
||||
|
||||
it("forwards the command and error to trackCommandFailure", async () => {
|
||||
const err = new Error("ENOENT /Users/me/project/index.html");
|
||||
await reportCommandFailure("info", err);
|
||||
expect(trackCommandFailure).toHaveBeenCalledWith("info", err);
|
||||
});
|
||||
|
||||
it("never throws when the telemetry call throws", async () => {
|
||||
trackCommandFailure.mockImplementationOnce(() => {
|
||||
throw new Error("telemetry blew up");
|
||||
});
|
||||
await expect(reportCommandFailure("browser", new Error("x"))).resolves.toBeUndefined();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,59 @@
|
||||
import type { CommandDef } from "citty";
|
||||
|
||||
// citty types subcommands as `CommandDef<any>` (SubCommandsDef); mirror that so
|
||||
// each command's specific args type is accepted without per-command generics.
|
||||
type AnyCommandDef = CommandDef<any>;
|
||||
|
||||
/**
|
||||
* Wrap a lazy command loader so a thrown failure is reported via `onFailure`
|
||||
* before it propagates. citty's `runMain` catches command errors and exits 1
|
||||
* without re-throwing, so this is the only place to capture the reason. The
|
||||
* error is re-thrown unchanged, preserving citty's print + exit-1 behavior.
|
||||
*
|
||||
* `onFailure` is awaited so it can resolve the (lazily-loaded) telemetry module
|
||||
* before the error propagates — otherwise a command that throws before the
|
||||
* telemetry import settles would lose its event. A throw from `onFailure` is
|
||||
* swallowed so telemetry can never mask the real command failure.
|
||||
*
|
||||
* Commands that call `process.exit()` themselves bypass this (the process is
|
||||
* already gone) and must report their failure inline.
|
||||
*/
|
||||
export function trackCommandFailures(
|
||||
load: () => Promise<AnyCommandDef>,
|
||||
onFailure: (err: unknown) => void | Promise<void>,
|
||||
): () => Promise<AnyCommandDef> {
|
||||
return () =>
|
||||
load().then((cmd) => {
|
||||
const run = cmd.run;
|
||||
if (typeof run !== "function") return cmd;
|
||||
return {
|
||||
...cmd,
|
||||
run: async (ctx: Parameters<typeof run>[0]) => {
|
||||
try {
|
||||
return await run(ctx);
|
||||
} catch (err) {
|
||||
try {
|
||||
await onFailure(err);
|
||||
} catch {
|
||||
// Telemetry must never mask the real command failure.
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
},
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Report a command failure to telemetry, loading the telemetry module on demand
|
||||
* (keeps it off the CLI cold-start path) and awaiting it so the event is
|
||||
* enqueued before the caller re-throws / exits. Best-effort — never throws.
|
||||
*/
|
||||
export async function reportCommandFailure(command: string, err: unknown): Promise<void> {
|
||||
try {
|
||||
const { trackCommandFailure } = await import("../telemetry/events.js");
|
||||
trackCommandFailure(command, err);
|
||||
} catch {
|
||||
// ignore: a telemetry failure must not affect the command's exit path
|
||||
}
|
||||
}
|
||||
@@ -1,6 +1,7 @@
|
||||
import { existsSync, statSync } from "node:fs";
|
||||
import { resolve, basename } from "node:path";
|
||||
import { errorBox } from "../ui/format.js";
|
||||
import { trackCommandFailure } from "../telemetry/events.js";
|
||||
|
||||
export interface ProjectDir {
|
||||
dir: string;
|
||||
@@ -55,6 +56,11 @@ export function resolveProject(dirArg: string | undefined): ProjectDir {
|
||||
return resolveProjectOrThrow(dirArg);
|
||||
} catch (err) {
|
||||
if (err instanceof InvalidProjectError) {
|
||||
// Self-exit (not a throw) so the cli.ts wrapper never sees it — report
|
||||
// inline. argv[2] is the running command (info / inspect / render / ...).
|
||||
// This is the dominant failure for read-only commands like `info` run
|
||||
// outside a project; the redaction in trackCliError strips the dir path.
|
||||
trackCommandFailure(process.argv[2] ?? "unknown", err);
|
||||
errorBox(err.title, err.hint, err.suggestion);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user