From 73d3b4e1f491e5211a960bac86fbb674909be7ad Mon Sep 17 00:00:00 2001 From: James Date: Sun, 19 Jul 2026 06:57:35 -0400 Subject: [PATCH] fix(cli): report command failures once --- packages/cli/src/cli.commands.test.ts | 6 ++ packages/cli/src/cli.ts | 5 +- .../utils/command-failure-tracking.test.ts | 91 ++++++------------- .../cli/src/utils/command-failure-tracking.ts | 89 +++++++----------- 4 files changed, 68 insertions(+), 123 deletions(-) diff --git a/packages/cli/src/cli.commands.test.ts b/packages/cli/src/cli.commands.test.ts index d161ef9ae..a88ef016e 100644 --- a/packages/cli/src/cli.commands.test.ts +++ b/packages/cli/src/cli.commands.test.ts @@ -47,4 +47,10 @@ describe("CLI command registration", () => { expect(condition).toContain('command !== "events"'); expect(condition).toContain('command !== "skills"'); }); + + it("reports each command failure only at the executable boundary", () => { + expect(cliSource).toContain("trackCommandFailures(load)"); + expect(cliSource).not.toContain("trackCommandFailures(load,"); + expect(cliSource.match(/reportCommandFailure\(command, error\)/g)).toHaveLength(1); + }); }); diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index 609b76037..62256e1e2 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -163,10 +163,7 @@ const commandLoaders = { }; const subCommands = Object.fromEntries( - Object.entries(commandLoaders).map(([name, load]) => [ - name, - trackCommandFailures(load, (error) => reportCommandFailure(command, error)), - ]), + Object.entries(commandLoaders).map(([name, load]) => [name, trackCommandFailures(load)]), ); const main = defineCommand({ diff --git a/packages/cli/src/utils/command-failure-tracking.test.ts b/packages/cli/src/utils/command-failure-tracking.test.ts index 0f7475af1..7afdfc892 100644 --- a/packages/cli/src/utils/command-failure-tracking.test.ts +++ b/packages/cli/src/utils/command-failure-tracking.test.ts @@ -14,40 +14,32 @@ function defineRun(run: CommandDef["run"]): CommandDef { } describe("trackCommandFailures", () => { - it("reports the error and re-throws when run() rejects", async () => { - const onFailure = vi.fn(); + it("re-throws when run() rejects so the executable boundary can report it", async () => { const boom = new Error("ffmpeg not found"); - const wrapped = trackCommandFailures( - () => Promise.resolve(defineRun(() => Promise.reject(boom))), - onFailure, + const wrapped = trackCommandFailures(() => + Promise.resolve(defineRun(() => Promise.reject(boom))), ); const cmd = await wrapped(); await expect((cmd.run as () => Promise)()).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, + it("returns the command value when run() succeeds", async () => { + const wrapped = trackCommandFailures(() => + Promise.resolve(defineRun(() => Promise.resolve("ok" as unknown as void))), ); const cmd = await wrapped(); await expect((cmd.run as () => Promise)()).resolves.toBe("ok"); - expect(onFailure).not.toHaveBeenCalled(); }); it("rejects an unknown flag on a leaf command", async () => { - const wrapped = trackCommandFailures( - () => - Promise.resolve({ - meta: { name: "leaf" }, - args: { out: { type: "string" } }, - run: () => Promise.resolve(), - } as CommandDef), - vi.fn(), + const wrapped = trackCommandFailures(() => + Promise.resolve({ + meta: { name: "leaf" }, + args: { out: { type: "string" } }, + run: () => Promise.resolve(), + } as CommandDef), ); const cmd = await wrapped(); await expect( @@ -59,14 +51,12 @@ describe("trackCommandFailures", () => { // `figma component --name x`: --name belongs to the subcommand's // table; the group (subCommands + fallback-help run) must not reject it. const run = vi.fn(() => Promise.resolve()); - const wrapped = trackCommandFailures( - () => - Promise.resolve({ - meta: { name: "figma" }, - subCommands: { component: () => Promise.resolve({ meta: { name: "component" } }) }, - run, - } as unknown as CommandDef), - vi.fn(), + const wrapped = trackCommandFailures(() => + Promise.resolve({ + meta: { name: "figma" }, + subCommands: { component: () => Promise.resolve({ meta: { name: "component" } }) }, + run, + } as unknown as CommandDef), ); const cmd = await wrapped(); await expect( @@ -78,14 +68,12 @@ describe("trackCommandFailures", () => { }); it("still rejects an unknown flag when the group is NOT delegating", async () => { - const wrapped = trackCommandFailures( - () => - Promise.resolve({ - meta: { name: "figma" }, - subCommands: { component: () => Promise.resolve({ meta: { name: "component" } }) }, - run: () => Promise.resolve(), - } as unknown as CommandDef), - vi.fn(), + const wrapped = trackCommandFailures(() => + Promise.resolve({ + meta: { name: "figma" }, + subCommands: { component: () => Promise.resolve({ meta: { name: "component" } }) }, + run: () => Promise.resolve(), + } as unknown as CommandDef), ); const cmd = await wrapped(); await expect( @@ -94,49 +82,29 @@ describe("trackCommandFailures", () => { }); 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 wrapped = trackCommandFailures(() => Promise.resolve(parent)); 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)()).rejects.toBe(boom); - }); - - it("REPORTS an unknown-flag rejection to onFailure (HF#2033: assertion inside the try)", async () => { - // The flag assertion used to run before the try/catch, so an unknown-flag - // throw skipped telemetry. It must now be reported like any other failure. - const onFailure = vi.fn(); + it("re-throws an unknown-flag rejection to the executable boundary", async () => { const cmd = { meta: { name: "render" }, args: { output: { type: "string", alias: "o" } }, run: vi.fn(() => Promise.resolve()), } as unknown as CommandDef; - const wrapped = trackCommandFailures(() => Promise.resolve(cmd), onFailure); + const wrapped = trackCommandFailures(() => Promise.resolve(cmd)); const resolved = await wrapped(); await expect( (resolved.run as (ctx: unknown) => Promise)({ rawArgs: ["--nope", "x"] }), ).rejects.toThrow(/unknown flag/i); - expect(onFailure).toHaveBeenCalledTimes(1); expect(cmd.run).not.toHaveBeenCalled(); // body never ran — flag rejected first }); - it("recursively wraps nested subcommands so their failures report too (HF#2033)", async () => { - // cli.ts wraps only top-level loaders; a group's leaves (cloud/*, auth/*, …) - // were never wrapped, silently dropping unknown-flag + failure telemetry. - const onFailure = vi.fn(); + it("recursively wraps nested subcommands so their failures reach the boundary", async () => { const boom = new Error("nested boom"); const group: CommandDef = { meta: { name: "cloud" }, @@ -144,7 +112,7 @@ describe("trackCommandFailures", () => { render: defineRun(() => Promise.reject(boom)), }, }; - const wrapped = trackCommandFailures(() => Promise.resolve(group), onFailure); + const wrapped = trackCommandFailures(() => Promise.resolve(group)); const resolvedGroup = await wrapped(); const subLoader = (resolvedGroup.subCommands as Record Promise>) @@ -152,7 +120,6 @@ describe("trackCommandFailures", () => { if (!subLoader) throw new Error("expected a wrapped 'render' subcommand loader"); const leaf = await subLoader(); await expect((leaf.run as () => Promise)()).rejects.toBe(boom); - expect(onFailure).toHaveBeenCalledWith(boom); }); }); diff --git a/packages/cli/src/utils/command-failure-tracking.ts b/packages/cli/src/utils/command-failure-tracking.ts index efe65ef5e..f613399d2 100644 --- a/packages/cli/src/utils/command-failure-tracking.ts +++ b/packages/cli/src/utils/command-failure-tracking.ts @@ -6,43 +6,27 @@ import { assertKnownFlags } from "./reject-unknown-flags.js"; type AnyCommandDef = CommandDef; /** - * 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. + * Wrap a lazy command loader so leaf commands and nested subcommands share the + * unknown-flag guard. Errors propagate unchanged to the executable boundary, + * which is the sole command-failure telemetry reporter. */ export function trackCommandFailures( load: () => Promise, - onFailure: (err: unknown) => void | Promise, ): () => Promise { - return () => load().then((cmd) => wrapCommand(cmd, onFailure)); + return () => load().then((cmd) => wrapCommand(cmd)); } /** - * Wrap a resolved command's `run` (assert-flags + report-failure) AND + * Wrap a resolved command's `run` (assert-flags) AND * recursively wrap every entry in its `subCommands`. Two HF#2033 fixes live * here: - * 1. `assertKnownFlags` runs INSIDE the try, so an unknown-flag throw is - * routed through `onFailure` (telemetry) like any other failure — it - * used to throw before the try and lose the event entirely. + * 1. `assertKnownFlags` runs in the wrapped command, so an unknown-flag + * throw reaches the executable boundary like every other failure. * 2. Recursion covers nested command groups (`cloud/*`, `auth/*`, `figma/*`, - * `lambda/*`, `capture/*`, `skills`). cli.ts only wraps the top-level - * loaders, so before this a `hyperframes cloud render --badflag` silently - * ignored the flag and reported nothing — citty dispatches to the leaf, - * whose `run` was never wrapped. + * `lambda/*`, `capture/*`, `skills`). Without it, a nested command's + * unknown flags would bypass the leaf's guard. */ -function wrapCommand( - cmd: AnyCommandDef, - onFailure: (err: unknown) => void | Promise, -): AnyCommandDef { +function wrapCommand(cmd: AnyCommandDef): AnyCommandDef { const run = cmd.run; // Nothing to wrap (no run, no nested subcommands) — preserve identity. if (typeof run !== "function" && !cmd.subCommands) return cmd; @@ -50,36 +34,27 @@ function wrapCommand( const wrapped: AnyCommandDef = { ...cmd }; if (typeof run === "function") { wrapped.run = async (ctx: Parameters[0]) => { - try { - // Reject unknown flags before the command body: citty silently ignores - // them otherwise, dropping the value (e.g. `render --out x` fell back to - // the default output path). Inside the try so the rejection is reported. - // - // A command group (subCommands + fallback-help run) delegating to a - // subcommand must NOT assert here: the flags belong to the subcommand's - // table, not the group's, and would be falsely rejected (e.g. - // `figma component --name x`). The wrapped subcommand loaders - // below assert the leaf's own table, so typo protection is preserved. - // Heuristic caveat: "first non-dash token names a subcommand" is sound - // only while command groups declare no flags of their own — if a group - // grows a flag whose value could match a subcommand name, replace this - // with a real argv parse. - const rawArgs = ctx?.rawArgs ?? []; - const firstPositional = rawArgs.find((tok) => tok && !tok.startsWith("-")); - const delegatesToSub = - cmd.subCommands != null && - firstPositional != null && - Object.prototype.hasOwnProperty.call(cmd.subCommands, firstPositional); - if (!delegatesToSub) assertKnownFlags(cmd, rawArgs); - return await run(ctx); - } catch (err) { - try { - await onFailure(err); - } catch { - // Telemetry must never mask the real command failure. - } - throw err; - } + // Reject unknown flags before the command body: citty silently ignores + // them otherwise, dropping the value (e.g. `render --out x` fell back to + // the default output path). + // + // A command group (subCommands + fallback-help run) delegating to a + // subcommand must NOT assert here: the flags belong to the subcommand's + // table, not the group's, and would be falsely rejected (e.g. + // `figma component --name x`). The wrapped subcommand loaders + // below assert the leaf's own table, so typo protection is preserved. + // Heuristic caveat: "first non-dash token names a subcommand" is sound + // only while command groups declare no flags of their own — if a group + // grows a flag whose value could match a subcommand name, replace this + // with a real argv parse. + const rawArgs = ctx?.rawArgs ?? []; + const firstPositional = rawArgs.find((tok) => tok && !tok.startsWith("-")); + const delegatesToSub = + cmd.subCommands != null && + firstPositional != null && + Object.prototype.hasOwnProperty.call(cmd.subCommands, firstPositional); + if (!delegatesToSub) assertKnownFlags(cmd, rawArgs); + return await run(ctx); }; } if (cmd.subCommands) { @@ -89,7 +64,7 @@ function wrapCommand( // (possibly async) loader. Normalize to a loader that resolves then wraps. wrappedSubs[name] = () => Promise.resolve(typeof sub === "function" ? (sub as () => unknown)() : sub).then((c) => - wrapCommand(c as AnyCommandDef, onFailure), + wrapCommand(c as AnyCommandDef), ); } wrapped.subCommands = wrappedSubs;