mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-01 19:42:03 +00:00
fix(cli): report unknown-flag errors + cover nested subcommands (HF#2033) [P2] (#2072)
* fix(cli): report unknown-flag errors + cover nested subcommands (HF#2033) Two flag-hygiene gaps behind the assertKnownFlags arc: 1. Telemetry loss: assertKnownFlags ran BEFORE the try/catch in the command wrapper, so an unknown-flag throw skipped reportCommandFailure entirely — zero signal on how often users hit bad flags. Moved the assertion inside the try so it reports like any other failure. 2. Nested-subcommand scope: cli.ts wraps only the top-level command loaders, so command groups' leaves (cloud/*, auth/*, figma/*, lambda/*, capture/*, skills) were never wrapped — citty dispatches to the leaf, whose run had no assertion and no failure reporting. So `hyperframes cloud render --badflag` silently ignored the flag. trackCommandFailures now recurses through cmd.subCommands (normalizing citty's Resolvable entries to loaders) and wraps every leaf. Identity is preserved for bare no-run/no-subcommand defs. Verified: `auth status --badflag` now errors "Unknown flag: --badflag" (previously silent); `auth --help` still dispatches; top-level `lint --badflag` still rejected. Tests: unknown-flag rejection is reported, and a nested subcommand's failure reaches onFailure. * test(cli): guard indexed subCommands access for noUncheckedIndexedAccess CI Typecheck (tsc, unlike the local tsup build) flagged the nested-subcommand test: indexing `subCommands["render"]` yields `T | undefined` under noUncheckedIndexedAccess, so invoking it tripped TS2722/TS18048. Guard the loader before calling it.
This commit is contained in:
@@ -59,6 +59,47 @@ describe("trackCommandFailures", () => {
|
||||
const cmd = await wrapped();
|
||||
await expect((cmd.run as () => Promise<unknown>)()).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();
|
||||
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 resolved = await wrapped();
|
||||
await expect(
|
||||
(resolved.run as (ctx: unknown) => Promise<unknown>)({ 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();
|
||||
const boom = new Error("nested boom");
|
||||
const group: CommandDef = {
|
||||
meta: { name: "cloud" },
|
||||
subCommands: {
|
||||
render: defineRun(() => Promise.reject(boom)),
|
||||
},
|
||||
};
|
||||
const wrapped = trackCommandFailures(() => Promise.resolve(group), onFailure);
|
||||
|
||||
const resolvedGroup = await wrapped();
|
||||
const subLoader = (resolvedGroup.subCommands as Record<string, () => Promise<CommandDef>>)
|
||||
.render;
|
||||
if (!subLoader) throw new Error("expected a wrapped 'render' subcommand loader");
|
||||
const leaf = await subLoader();
|
||||
await expect((leaf.run as () => Promise<unknown>)()).rejects.toBe(boom);
|
||||
expect(onFailure).toHaveBeenCalledWith(boom);
|
||||
});
|
||||
});
|
||||
|
||||
describe("reportCommandFailure", () => {
|
||||
|
||||
@@ -23,31 +23,62 @@ 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]) => {
|
||||
// Reject unknown flags before the command runs: citty silently ignores
|
||||
// them otherwise, dropping the value (e.g. `render --out x` fell back
|
||||
// to the default output path). A leaf command with a `run` is the right
|
||||
// place — nested command groups delegate to their own subcommands.
|
||||
assertKnownFlags(cmd, ctx?.rawArgs ?? []);
|
||||
try {
|
||||
return await run(ctx);
|
||||
} catch (err) {
|
||||
try {
|
||||
await onFailure(err);
|
||||
} catch {
|
||||
// Telemetry must never mask the real command failure.
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
},
|
||||
};
|
||||
});
|
||||
return () => load().then((cmd) => wrapCommand(cmd, onFailure));
|
||||
}
|
||||
|
||||
/**
|
||||
* Wrap a resolved command's `run` (assert-flags + report-failure) 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.
|
||||
* 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.
|
||||
*/
|
||||
function wrapCommand(
|
||||
cmd: AnyCommandDef,
|
||||
onFailure: (err: unknown) => void | Promise<void>,
|
||||
): AnyCommandDef {
|
||||
const run = cmd.run;
|
||||
// Nothing to wrap (no run, no nested subcommands) — preserve identity.
|
||||
if (typeof run !== "function" && !cmd.subCommands) return cmd;
|
||||
|
||||
const wrapped: AnyCommandDef = { ...cmd };
|
||||
if (typeof run === "function") {
|
||||
wrapped.run = async (ctx: Parameters<typeof run>[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.
|
||||
assertKnownFlags(cmd, ctx?.rawArgs ?? []);
|
||||
return await run(ctx);
|
||||
} catch (err) {
|
||||
try {
|
||||
await onFailure(err);
|
||||
} catch {
|
||||
// Telemetry must never mask the real command failure.
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
};
|
||||
}
|
||||
if (cmd.subCommands) {
|
||||
const wrappedSubs: Record<string, () => Promise<AnyCommandDef>> = {};
|
||||
for (const [name, sub] of Object.entries(cmd.subCommands)) {
|
||||
// citty subCommands are Resolvable<CommandDef>: a def, a promise, or a
|
||||
// (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),
|
||||
);
|
||||
}
|
||||
wrapped.subCommands = wrappedSubs;
|
||||
}
|
||||
return wrapped;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user