From 4178a8f95d3d3439ddde6b5d20a317ef01c97d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Fri, 2 Oct 2026 18:34:53 +0000 Subject: [PATCH] fix(cli): reject extra positional arguments on every command (#4905) * fix(cli): reject extra positional arguments on every command * fix(cli): read the command name without assuming a resolved meta * fix(cli): join free-text words for catalog and tts, reject a bad repo root * test(cli): load the listed commands through static imports * fix(cli): keep a tts file path out of the word join and read the parsed json flag * fix(cli): apply the file exception only to a tts .txt path * fix(cli): keep usageLine private and test the json fallback for undeclared json * fix(cli): keep extra arguments and the repo root value out of telemetry * test(cli): type the skills update lookup * fix(cli): read json mode from the parsed flag, reject tts words with --text-file, restore add's hint * fix(cli): check tts words with --text-file before --list and test --json=false --- packages/cli/src/cli.commands.test.ts | 30 +++ packages/cli/src/commands/add.test.ts | 24 +- packages/cli/src/commands/add.ts | 13 +- .../cli/src/commands/lambda/repoRoot.test.ts | 34 +++ packages/cli/src/commands/lambda/repoRoot.ts | 8 +- packages/cli/src/commands/tts.test.ts | 23 ++ packages/cli/src/commands/tts.ts | 18 +- packages/cli/src/telemetry/events.test.ts | 19 ++ .../utils/command-failure-tracking.test.ts | 207 +++++++++++++++++- .../cli/src/utils/command-failure-tracking.ts | 16 +- .../cli/src/utils/reject-extra-positionals.ts | 83 +++++++ packages/cli/src/utils/textFile.ts | 8 + 12 files changed, 444 insertions(+), 39 deletions(-) create mode 100644 packages/cli/src/commands/lambda/repoRoot.test.ts create mode 100644 packages/cli/src/utils/reject-extra-positionals.ts create mode 100644 packages/cli/src/utils/textFile.ts diff --git a/packages/cli/src/cli.commands.test.ts b/packages/cli/src/cli.commands.test.ts index dc45671e04..db63f8f09b 100644 --- a/packages/cli/src/cli.commands.test.ts +++ b/packages/cli/src/cli.commands.test.ts @@ -2,6 +2,11 @@ import { readFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; +import type { CommandDef } from "citty"; +import { + ACCEPTS_EXTRA_POSITIONALS, + JOINS_EXTRA_POSITIONALS, +} from "./utils/reject-extra-positionals.js"; const cliSource = readFileSync(join(dirname(fileURLToPath(import.meta.url)), "cli.ts"), "utf8"); const helpSource = readFileSync(join(dirname(fileURLToPath(import.meta.url)), "help.ts"), "utf8"); @@ -12,7 +17,32 @@ function commandLoaderBlock(): string { return match![1]!; } +// Vite cannot resolve a template-string import, so each listed top-level command is named here. +const LIST_COMMANDS: Record Promise<{ default: unknown }>> = { + catalog: () => import("./commands/catalog.js"), + compare: () => import("./commands/compare.js"), + figma: () => import("./commands/figma.js"), + skills: () => import("./commands/skills.js"), + timeline: () => import("./commands/timeline.js"), + tts: () => import("./commands/tts.js"), +}; + describe("CLI command registration", () => { + it.each([...ACCEPTS_EXTRA_POSITIONALS, ...JOINS_EXTRA_POSITIONALS])( + "names a real command in the extra-positional lists: %s", + async (path) => { + const [top, ...subs] = path.split(" "); + expect(commandLoaderBlock()).toMatch(new RegExp(`\\b${top}:\\s*\\(\\)\\s*=>`)); + let cmd = (await LIST_COMMANDS[top!]!()).default as CommandDef; + expect(cmd.meta).toMatchObject({ name: top }); + for (const sub of subs) { + const entry = (cmd.subCommands as Record)[sub!]; + cmd = (typeof entry === "function" ? await entry() : entry) as CommandDef; + expect(cmd, `${path} resolves to a subcommand`).toBeTruthy(); + } + }, + ); + it("registers keyframes as the only keyframe inspection command", () => { const loaders = commandLoaderBlock(); diff --git a/packages/cli/src/commands/add.test.ts b/packages/cli/src/commands/add.test.ts index 2865d80088..4bbd5a49cf 100644 --- a/packages/cli/src/commands/add.test.ts +++ b/packages/cli/src/commands/add.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; -import { parseArgs as parseCittyArgs, type ArgsDef } from "citty"; +import { runCommand as runCittyCommand, type CommandDef } from "citty"; import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -18,6 +18,7 @@ import addCommand, { } from "./add.js"; import { trackRegistryItemAdded } from "../telemetry/events.js"; import { CliUsageError } from "../utils/commandResult.js"; +import { trackCommandFailures } from "../utils/command-failure-tracking.js"; // Assert the emitted payload rather than the transport: `shouldTrack()` is // already false under test (dev mode / no PostHog key), so a real call would @@ -539,22 +540,21 @@ describe("add command run() — extra positional arguments", () => { rmSync(dir, { recursive: true, force: true }); }); - async function runCommand(args: Record): Promise { - await (addCommand.run as (ctx: { args: Record }) => Promise)({ args }); + async function runWrapped(rawArgs: string[]): Promise { + const cmd = await trackCommandFailures(() => Promise.resolve(addCommand as CommandDef))(); + await runCittyCommand(cmd, { rawArgs }); } - it("rejects `add a --dir b c` under real citty parsing, before any registry call", async () => { + it("rejects `add a --dir b c` through the CLI wrapper, before any registry call", async () => { const fetchSpy = vi.fn(); vi.stubGlobal("fetch", fetchSpy); - const parsed = parseCittyArgs(["a", "--dir", dir, "b", "c"], addCommand.args as ArgsDef); - expect(parsed._).toEqual(["a", "b", "c"]); - - await expect(runCommand(parsed as unknown as Record)).rejects.toThrow( - CliUsageError, - ); + await expect(runWrapped(["a", "--dir", dir, "b", "c"])).rejects.toThrow(CliUsageError); expect(fetchSpy).not.toHaveBeenCalled(); - expect(errorSpy.mock.calls.flat().join(" ")).toContain("extra arguments: b, c"); + expect(errorSpy.mock.calls.flat().join(" ")).toContain( + "Unexpected extra arguments for hyperframes add: b, c", + ); + expect(errorSpy.mock.calls.flat().join(" ")).toContain("Run add once per item"); }); it("does not fire on a normal single-item invocation", async () => { @@ -562,7 +562,7 @@ describe("add command run() — extra positional arguments", () => { writeRegistryConfig(dir); await expect( - runCommand({ name: "my-block", _: ["my-block"], dir, clipboard: false, json: true }), + runWrapped(["my-block", "--dir", dir, "--no-clipboard", "--json"]), ).resolves.toBeUndefined(); }); }); diff --git a/packages/cli/src/commands/add.ts b/packages/cli/src/commands/add.ts index e905a1f4b9..43b077d97f 100644 --- a/packages/cli/src/commands/add.ts +++ b/packages/cli/src/commands/add.ts @@ -1,4 +1,4 @@ -import { failCommand, failUsage } from "../utils/commandResult.js"; +import { failCommand } from "../utils/commandResult.js"; import { defineCommand } from "citty"; import type { Example } from "./_examples.js"; @@ -491,17 +491,6 @@ export default defineCommand({ const json = args.json === true; const skipClipboard = args.clipboard === false; - // citty binds only the first positional to `name`; the rest land in `args._` and were silently dropped. - const extra = args._.slice(1); - if (extra.length > 0) { - const msg = - `add installs one item or tag per invocation. Got extra argument${extra.length === 1 ? "" : "s"}: ` + - `${extra.join(", ")}. Run add once per item, or pass a single tag to install every item tagged with it.`; - if (json) console.log(JSON.stringify({ ok: false, error: msg })); - else console.error(c.error(msg)); - failUsage(); - } - const hasConfigBefore = existsSync(projectConfigPath(projectDir)); // Try single item first. If it fails, check if the name matches a tag. diff --git a/packages/cli/src/commands/lambda/repoRoot.test.ts b/packages/cli/src/commands/lambda/repoRoot.test.ts new file mode 100644 index 0000000000..9642c69b89 --- /dev/null +++ b/packages/cli/src/commands/lambda/repoRoot.test.ts @@ -0,0 +1,34 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { repoRoot } from "./repoRoot.js"; + +describe("repoRoot", () => { + const dirs: string[] = []; + const tempDir = () => { + const dir = mkdtempSync(join(tmpdir(), "hf-repo-root-")); + dirs.push(dir); + return dir; + }; + + afterEach(() => { + vi.unstubAllEnvs(); + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + it("returns HYPERFRAMES_REPO_ROOT when it is a hyperframes checkout", () => { + const root = tempDir(); + mkdirSync(join(root, "packages", "aws-lambda"), { recursive: true }); + writeFileSync(join(root, "packages", "aws-lambda", "package.json"), "{}"); + vi.stubEnv("HYPERFRAMES_REPO_ROOT", root); + expect(repoRoot()).toBe(root); + }); + + it("throws on a HYPERFRAMES_REPO_ROOT that is not a checkout instead of ignoring it", () => { + const root = tempDir(); + vi.stubEnv("HYPERFRAMES_REPO_ROOT", root); + expect(() => repoRoot()).toThrow("HYPERFRAMES_REPO_ROOT is not a hyperframes checkout"); + expect(() => repoRoot()).not.toThrow(root); + }); +}); diff --git a/packages/cli/src/commands/lambda/repoRoot.ts b/packages/cli/src/commands/lambda/repoRoot.ts index b7f6628db0..93268b31d2 100644 --- a/packages/cli/src/commands/lambda/repoRoot.ts +++ b/packages/cli/src/commands/lambda/repoRoot.ts @@ -14,8 +14,12 @@ import { fileURLToPath } from "node:url"; export function repoRoot(): string { const override = process.env.HYPERFRAMES_REPO_ROOT; - if (override && existsSync(resolve(override, "packages", "aws-lambda", "package.json"))) { - return override; + if (override) { + if (existsSync(resolve(override, "packages", "aws-lambda", "package.json"))) return override; + throw new Error( + "[hyperframes lambda] HYPERFRAMES_REPO_ROOT is not a hyperframes checkout " + + "(no packages/aws-lambda/package.json there). Point it at the repo root, or unset it.", + ); } let dir = dirname(fileURLToPath(import.meta.url)); for (let depth = 0; depth < 12; depth++) { diff --git a/packages/cli/src/commands/tts.test.ts b/packages/cli/src/commands/tts.test.ts index 341c9964be..914b399edb 100644 --- a/packages/cli/src/commands/tts.test.ts +++ b/packages/cli/src/commands/tts.test.ts @@ -3,6 +3,7 @@ import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { assertKnownFlags } from "../utils/reject-unknown-flags.js"; +import { CliUsageError } from "../utils/commandResult.js"; const synthesizeMock = vi.fn().mockResolvedValue({ durationSeconds: 1, @@ -42,4 +43,26 @@ describe("tts command", () => { expect.objectContaining({ lang: "en-us" }), ); }); + + it("rejects text words with --text-file even under --list", async () => { + const input = join(dir, "script.txt"); + writeFileSync(input, "From the file\n"); + vi.spyOn(console, "error").mockImplementation(() => {}); + + await expect( + ttsCommand.run!({ args: { "text-file": input, input: "hello", list: true } } as never), + ).rejects.toThrow(CliUsageError); + }); + + it("rejects text words together with --text-file instead of dropping the words", async () => { + const input = join(dir, "script.txt"); + writeFileSync(input, "From the file\n"); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + await expect( + ttsCommand.run!({ args: { "text-file": input, input: "hello world" } } as never), + ).rejects.toThrow(CliUsageError); + expect(synthesizeMock).not.toHaveBeenCalled(); + expect(errorSpy.mock.calls.flat().join(" ")).toContain("not both"); + }); }); diff --git a/packages/cli/src/commands/tts.ts b/packages/cli/src/commands/tts.ts index 7cc49a2069..2e15a0b1f2 100644 --- a/packages/cli/src/commands/tts.ts +++ b/packages/cli/src/commands/tts.ts @@ -1,8 +1,9 @@ -import { failCommand } from "../utils/commandResult.js"; +import { failCommand, failUsage } from "../utils/commandResult.js"; +import { isTextFile } from "../utils/textFile.js"; // fallow-ignore-file code-duplication import { defineCommand } from "citty"; import type { Example } from "./_examples.js"; -import { existsSync, readFileSync } from "node:fs"; +import { readFileSync } from "node:fs"; export const examples: Example[] = [ ["Generate speech from text", 'hyperframes tts "Welcome to HyperFrames"'], @@ -20,7 +21,7 @@ export const examples: Example[] = [ ["Read text from a file", "hyperframes tts script.txt"], ["List available voices", "hyperframes tts --list"], ]; -import { resolve, extname } from "node:path"; +import { resolve } from "node:path"; import * as clack from "@clack/prompts"; import { c } from "../ui/colors.js"; import { errorBox } from "../ui/format.js"; @@ -84,6 +85,11 @@ export default defineCommand({ }, // fallow-ignore-next-line complexity async run({ args }) { + if (args["text-file"] && args.input) { + console.error(c.error("Pass text to speak or --text-file, not both.")); + failUsage(); + } + // ── List voices mode ────────────────────────────────────────────── if (args.list) { return listVoices(args.json); @@ -97,10 +103,8 @@ export default defineCommand({ } let text: string; - const maybeFile = resolve(input); - - if (existsSync(maybeFile) && extname(maybeFile).toLowerCase() === ".txt") { - text = readFileSync(maybeFile, "utf-8").trim(); + if (isTextFile(input)) { + text = readFileSync(resolve(input), "utf-8").trim(); if (!text) { console.error(c.error("File is empty.")); failCommand(); diff --git a/packages/cli/src/telemetry/events.test.ts b/packages/cli/src/telemetry/events.test.ts index ff33f82f69..7930c84772 100644 --- a/packages/cli/src/telemetry/events.test.ts +++ b/packages/cli/src/telemetry/events.test.ts @@ -1130,6 +1130,25 @@ describe("trackCliError", () => { }); describe("trackCommandFailure", () => { + it("reports an extra-positional usage error by count, never the arguments", async () => { + const { resolveExtraPositionals } = await import("../utils/reject-extra-positionals.js"); + const quiet = vi.spyOn(console, "error").mockImplementation(() => {}); + const cmd = { args: { dir: { type: "positional" } } } as Parameters< + typeof resolveExtraPositionals + >[0]; + let thrown: unknown; + try { + resolveExtraPositionals(cmd, "render", { _: ["./p", "Jane", "555-0100"] }); + } catch (error) { + thrown = error; + } + quiet.mockRestore(); + trackCommandFailure("render", thrown); + const payload = JSON.stringify(trackEvent.mock.calls.at(-1)); + expect(payload).toContain("2 unexpected extra arguments for hyperframes render"); + expect(payload).not.toMatch(/Jane|555-0100/); + }); + beforeEach(() => { trackEvent.mockClear(); }); diff --git a/packages/cli/src/utils/command-failure-tracking.test.ts b/packages/cli/src/utils/command-failure-tracking.test.ts index 7afdfc8926..6a81aaa58d 100644 --- a/packages/cli/src/utils/command-failure-tracking.test.ts +++ b/packages/cli/src/utils/command-failure-tracking.test.ts @@ -1,5 +1,10 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; -import type { CommandDef } from "citty"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { runCommand, type CommandDef } from "citty"; +import { CliUsageError } from "./commandResult.js"; +import { resolveExtraPositionals } from "./reject-extra-positionals.js"; const trackCommandFailure = vi.fn(); vi.mock("../telemetry/events.js", () => ({ @@ -123,6 +128,204 @@ describe("trackCommandFailures", () => { }); }); +describe("trackCommandFailures: extra positionals", () => { + const leaf = (name: string, run = vi.fn()): CommandDef => + ({ + meta: { name }, + args: { + dir: { type: "positional", required: false }, + json: { type: "boolean", default: false }, + }, + run, + }) as CommandDef; + const wrap = (cmd: CommandDef) => trackCommandFailures(() => Promise.resolve(cmd))(); + let errorSpy: ReturnType; + let logSpy: ReturnType; + const tempDirs: string[] = []; + const tempDir = () => { + const dir = mkdtempSync(join(tmpdir(), "hf-join-")); + tempDirs.push(dir); + return dir; + }; + + beforeEach(() => { + errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + }); + + afterEach(() => { + errorSpy.mockRestore(); + logSpy.mockRestore(); + for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + it("throws a presented error whose message carries a count, never the arguments", async () => { + const error = await runCommand(await wrap(leaf("render")), { + rawArgs: ["./proj", "secret-launch", "Jane"], + }).catch((e: unknown) => e); + expect(error).toBeInstanceOf(CliUsageError); + expect((error as CliUsageError).result.presented).toBe(true); + expect((error as Error).message).toBe("2 unexpected extra arguments for hyperframes render"); + }); + + it("labels a required positional and an optional one [NAME]", async () => { + const cmd = { + meta: { name: "keyframes" }, + args: { file: { type: "positional" }, at: { type: "positional", required: false } }, + run: vi.fn(), + }; + await expect( + runCommand(await wrap(cmd as CommandDef), { rawArgs: ["a", "b", "c"] }), + ).rejects.toThrow(CliUsageError); + expect(errorSpy.mock.calls.flat().join("\n")).toContain( + "Usage: hyperframes keyframes [AT]", + ); + }); + + it.each([ + ["figma asset", () => import("../commands/figma/asset.js")], + [ + "skills update", + async () => ({ + default: ( + (await import("../commands/skills.js")).default.subCommands as Record + ).update, + }), + ], + ])("lets the real %s command read several positionals", async (path, load) => { + const def = (await (load as () => Promise<{ default: unknown }>)()).default; + const cmd = (typeof def === "function" ? await def() : def) as CommandDef; + const parsed: Record = { _: ["one", "two", "three"] }; + expect(() => resolveExtraPositionals(cmd, path as string, parsed)).not.toThrow(); + }); + + it("rejects a leaf's extra positional, naming it and the usage line, before run()", async () => { + const run = vi.fn(); + await expect( + runCommand(await wrap(leaf("render", run)), { rawArgs: ["./proj", "out.mp4"] }), + ).rejects.toThrow(CliUsageError); + expect(run).not.toHaveBeenCalled(); + expect(errorSpy.mock.calls.flat().join("\n")).toBe( + "Unexpected extra argument for hyperframes render: out.mp4\nUsage: hyperframes render [DIR] [OPTIONS]", + ); + }); + + it.each(["--json", "--json=true", "--json=1"])( + "reports the rejection as JSON on stdout under %s", + async (flag) => { + await expect( + runCommand(await wrap(leaf("lint")), { rawArgs: ["a", "b", flag] }), + ).rejects.toThrow(CliUsageError); + expect(JSON.parse(logSpy.mock.calls[0]![0] as string)).toEqual({ + ok: false, + error: + "Unexpected extra argument for hyperframes lint: b\nUsage: hyperframes lint [DIR] [OPTIONS]", + }); + }, + ); + + it("rejects instead of joining when the first word names a file", async () => { + const run = vi.fn(); + const file = join(tempDir(), "script.txt"); + writeFileSync(file, "hello"); + await expect( + runCommand(await wrap(leaf("tts", run)), { rawArgs: [file, "extra"] }), + ).rejects.toThrow(CliUsageError); + expect(errorSpy.mock.calls.flat().join("\n")).toContain("hyperframes tts: extra"); + expect(run).not.toHaveBeenCalled(); + }); + + it("still joins catalog words when the first one names a folder", async () => { + const run = vi.fn(); + const folder = tempDir(); + await runCommand(await wrap(leaf("catalog", run)), { rawArgs: [folder, "player"] }); + expect(run.mock.calls[0]![0].args.dir).toBe(`${folder} player`); + }); + + it.each(["catalog", "tts"])( + "joins %s's trailing words into its last positional", + async (name) => { + const run = vi.fn(); + await runCommand(await wrap(leaf(name, run)), { rawArgs: ["lower", "third", "--json"] }); + expect(run.mock.calls[0]![0].args.dir).toBe("lower third"); + }, + ); + + it("reads JSON mode from the parsed flag: --json=1 without a declared flag, not after --", async () => { + const bare = { meta: { name: "docs" }, args: { topic: { type: "positional" } }, run: vi.fn() }; + await expect( + runCommand(await wrap(bare as CommandDef), { rawArgs: ["a", "b", "--json=1"] }), + ).rejects.toThrow(CliUsageError); + expect(JSON.parse(logSpy.mock.calls[0]![0] as string)).toMatchObject({ ok: false }); + logSpy.mockClear(); + await expect( + runCommand(await wrap(bare as CommandDef), { rawArgs: ["a", "b", "--", "--json"] }), + ).rejects.toThrow(CliUsageError); + expect(logSpy).not.toHaveBeenCalled(); + expect(errorSpy.mock.calls.flat().join("\n")).toContain("hyperframes docs: b, --json"); + }); + + it("reports JSON under --json=true for a command that declares no json flag", async () => { + const bare = { + meta: { name: "snapshot" }, + args: { dir: { type: "positional" } }, + run: vi.fn(), + }; + await expect( + runCommand(await wrap(bare as CommandDef), { rawArgs: ["a", "b", "--json=true"] }), + ).rejects.toThrow(CliUsageError); + expect(JSON.parse(logSpy.mock.calls[0]![0] as string)).toMatchObject({ ok: false }); + }); + + it("reports text on stderr under --json=false for a command that declares no json flag", async () => { + const bare = { + meta: { name: "snapshot" }, + args: { dir: { type: "positional" } }, + run: vi.fn(), + }; + await expect( + runCommand(await wrap(bare as CommandDef), { rawArgs: ["a", "b", "--json=false"] }), + ).rejects.toThrow(CliUsageError); + expect(logSpy).not.toHaveBeenCalled(); + expect(errorSpy.mock.calls.flat().join("\n")).toContain("hyperframes snapshot: b"); + }); + + it("runs a leaf given no more positionals than it declares", async () => { + const run = vi.fn(); + await runCommand(await wrap(leaf("lint", run)), { rawArgs: ["a", "--json"] }); + expect(run).toHaveBeenCalledOnce(); + }); + + it("names a nested subcommand by its full path", async () => { + const group = { + meta: { name: "cloud" }, + subCommands: { get: leaf("get") }, + run: vi.fn(), + } as CommandDef; + await expect(runCommand(await wrap(group), { rawArgs: ["get", "id1", "id2"] })).rejects.toThrow( + CliUsageError, + ); + expect(errorSpy.mock.calls.flat().join("\n")).toContain( + "hyperframes cloud get: id2\nUsage: hyperframes cloud get [DIR]", + ); + }); + + it("lets an opted-out command read extra positionals", async () => { + const run = vi.fn(); + await runCommand(await wrap(leaf("compare", run)), { rawArgs: ["a", "b", "c"] }); + expect(run).toHaveBeenCalledOnce(); + }); + + it("does not count the subcommand name against the group citty also runs", async () => { + const run = vi.fn(); + const sub = vi.fn(); + const group = { meta: { name: "auth" }, subCommands: { status: leaf("status", sub) }, run }; + await runCommand(await wrap(group as CommandDef), { rawArgs: ["status", "--json"] }); + expect(sub).toHaveBeenCalledOnce(); + expect(run).toHaveBeenCalledOnce(); + }); +}); + describe("reportCommandFailure", () => { beforeEach(() => { trackCommandFailure.mockReset(); diff --git a/packages/cli/src/utils/command-failure-tracking.ts b/packages/cli/src/utils/command-failure-tracking.ts index 1f545a40dd..6773ab0caf 100644 --- a/packages/cli/src/utils/command-failure-tracking.ts +++ b/packages/cli/src/utils/command-failure-tracking.ts @@ -1,4 +1,5 @@ import type { CommandDef } from "citty"; +import { resolveExtraPositionals } from "./reject-extra-positionals.js"; import { assertKnownFlags } from "./reject-unknown-flags.js"; // citty types subcommands as `CommandDef` (SubCommandsDef); mirror that so @@ -13,11 +14,16 @@ type AnyCommandDef = CommandDef; export function trackCommandFailures( load: () => Promise, ): () => Promise { - return () => load().then((cmd) => wrapCommand(cmd)); + return () => load().then((cmd) => wrapCommand(cmd, commandName(cmd))); +} + +function commandName(cmd: AnyCommandDef): string { + const name = (cmd.meta as { name?: unknown } | undefined)?.name; + return typeof name === "string" ? name : ""; } /** - * Wrap a resolved command's `run` (assert-flags) AND + * Wrap a resolved command's `run` (assert flags and positional count) AND * recursively wrap every entry in its `subCommands`. Two HF#2033 fixes live * here: * 1. `assertKnownFlags` runs in the wrapped command, so an unknown-flag @@ -26,7 +32,7 @@ export function trackCommandFailures( * `lambda/*`, `capture/*`, `skills`). Without it, a nested command's * unknown flags would bypass the leaf's guard. */ -function wrapCommand(cmd: AnyCommandDef): AnyCommandDef { +function wrapCommand(cmd: AnyCommandDef, path: string): AnyCommandDef { const run = cmd.run; // Nothing to wrap (no run, no nested subcommands) — preserve identity. if (typeof run !== "function" && !cmd.subCommands) return cmd; @@ -54,6 +60,8 @@ function wrapCommand(cmd: AnyCommandDef): AnyCommandDef { firstPositional != null && Object.prototype.hasOwnProperty.call(cmd.subCommands, firstPositional); if (!delegatesToSub) assertKnownFlags(cmd, rawArgs); + // Groups read `args._[0]` to pick fallback help, so only leaves get the count check. + if (!cmd.subCommands) resolveExtraPositionals(cmd, path, ctx?.args); return await run(ctx); }; } @@ -64,7 +72,7 @@ function wrapCommand(cmd: AnyCommandDef): AnyCommandDef { // (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), + wrapCommand(c as AnyCommandDef, `${path} ${name}`), ); } wrapped.subCommands = wrappedSubs; diff --git a/packages/cli/src/utils/reject-extra-positionals.ts b/packages/cli/src/utils/reject-extra-positionals.ts new file mode 100644 index 0000000000..e4ea67410d --- /dev/null +++ b/packages/cli/src/utils/reject-extra-positionals.ts @@ -0,0 +1,83 @@ +import type { ArgsDef, CommandDef } from "citty"; +import { CliUsageError } from "./commandResult.js"; +import { isTextFile } from "./textFile.js"; + +// citty binds each declared positional in order and leaves the rest in `args._`, where they were +// silently dropped (`render ./proj out.mp4` rendered to the default path). Reject them up front. + +// Leaf commands that read extra positionals from `args._` on purpose, by command path. +export const ACCEPTS_EXTRA_POSITIONALS = new Set([ + "compare", + "figma asset", + "skills update", + "timeline set", +]); + +// Free-text commands: their last positional takes every remaining word (`tts hello world`). +export const JOINS_EXTRA_POSITIONALS = new Set(["catalog", "tts"]); + +const EXTRA_POSITIONAL_HINTS: Record = { + add: "Run add once per item, or pass a single tag to install every item tagged with it.", +}; + +// citty parses `--json` on every command (`--json=1` as "1"); after `--` it is a positional. +function wantsJson(value: unknown): boolean { + return value === true || (typeof value === "string" && value !== "false" && value !== "0"); +} + +function declaredArgs(cmd: CommandDef): ArgsDef { + const raw = cmd.args; + return raw && typeof raw === "object" ? (raw as ArgsDef) : {}; +} + +function usageLine(path: string, args: ArgsDef): string { + const parts = ["hyperframes", path]; + let hasOptions = false; + for (const [name, def] of Object.entries(args)) { + if (def.type !== "positional") { + hasOptions = true; + continue; + } + const optional = def.required === false || def.default !== undefined; + parts.push(optional ? `[${name.toUpperCase()}]` : `<${name.toUpperCase()}>`); + } + if (hasOptions) parts.push("[OPTIONS]"); + return parts.join(" "); +} + +/** Join a free-text command's trailing words, else throw a usage error naming the extras. */ +export function resolveExtraPositionals( + cmd: CommandDef, + path: string, + parsed: Record | undefined, +): void { + if (ACCEPTS_EXTRA_POSITIONALS.has(path)) return; + const args = declaredArgs(cmd); + const positionals = Object.entries(args).filter(([, def]) => def.type === "positional"); + const given = (Array.isArray(parsed?._) ? parsed._ : []).map(String); + const extra = given.slice(positionals.length); + if (extra.length === 0) return; + const last = positionals.at(-1)?.[0]; + const words = given.slice(positionals.length - 1); + // `tts script.txt extra` reads the file, so its first word is a path, not free text. + const ttsFile = path === "tts" && isTextFile(words[0]!); + if (parsed && last && JOINS_EXTRA_POSITIONALS.has(path) && !ttsFile) { + parsed[last] = words.join(" "); + return; + } + + const plural = extra.length === 1 ? "" : "s"; + const message = + `Unexpected extra argument${plural} for hyperframes ${path}: ${extra.join(", ")}\n` + + `Usage: ${usageLine(path, args)}` + + (EXTRA_POSITIONAL_HINTS[path] ? `\n${EXTRA_POSITIONAL_HINTS[path]}` : ""); + if (wantsJson(parsed?.json)) console.log(JSON.stringify({ ok: false, error: message })); + else console.error(message); + // Telemetry reads the thrown message: the command path and a count, never the arguments. + throw new CliUsageError( + `${extra.length} unexpected extra argument${plural} for hyperframes ${path}`, + { + presented: true, + }, + ); +} diff --git a/packages/cli/src/utils/textFile.ts b/packages/cli/src/utils/textFile.ts new file mode 100644 index 0000000000..9d50c823e3 --- /dev/null +++ b/packages/cli/src/utils/textFile.ts @@ -0,0 +1,8 @@ +import { existsSync } from "node:fs"; +import { extname, resolve } from "node:path"; + +/** True when `input` names an existing `.txt` file, which `tts` reads instead of speaking the path. */ +export function isTextFile(input: string): boolean { + const path = resolve(input); + return existsSync(path) && extname(path).toLowerCase() === ".txt"; +}