From 5d2eda47b026a475df340bc006fe78ce9adb20bd Mon Sep 17 00:00:00 2001 From: miga-heygen Date: Sat, 12 Sep 2026 00:25:51 +0000 Subject: [PATCH] fix(cli): reject extra positional arguments passed to add `add ` silently installed only `` and exited 0, discarding `` and `` with no warning. citty binds only the first positional token to `name`; every token after it still lands in `args._`, but nothing read it. `add` now rejects the extra arguments up front, before touching the registry, naming exactly what was dropped. Co-Authored-By: Miguel Angel --- packages/cli/src/commands/add.test.ts | 78 ++++++++++++++++++++++++++- packages/cli/src/commands/add.ts | 24 ++++++++- 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/add.test.ts b/packages/cli/src/commands/add.test.ts index 2f3d7ce5e1..acf30fd746 100644 --- a/packages/cli/src/commands/add.test.ts +++ b/packages/cli/src/commands/add.test.ts @@ -1,17 +1,20 @@ import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; +import { parseArgs as parseCittyArgs, type ArgsDef } from "citty"; import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { RegistryItem, RegistryManifest } from "@hyperframes/core"; -import { +import addCommand, { AddError, buildSnippet, describeInstallFailure, + formatExtraPositionalsError, parseVariableValues, remapTarget, runAdd, } from "./add.js"; import { trackRegistryItemAdded } from "../telemetry/events.js"; +import { CliUsageError } from "../utils/commandResult.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 @@ -260,6 +263,17 @@ describe("add command pure helpers", () => { expect(buildSnippet(EXAMPLE_ITEM, "index.html")).toBe(""); }); }); + + describe("formatExtraPositionalsError", () => { + it("names every extra argument, singular wording for one", () => { + expect(formatExtraPositionalsError(["b"])).toContain("extra argument: b"); + }); + + it("names every extra argument, plural wording for more than one", () => { + const msg = formatExtraPositionalsError(["b", "c"]); + expect(msg).toContain("extra arguments: b, c"); + }); + }); }); describe("runAdd (integration, mocked registry)", () => { @@ -470,6 +484,68 @@ describe("variable values in the snippet", () => { }); }); +describe("add command run() — extra positional arguments", () => { + let dir: string; + let errorSpy: ReturnType; + + beforeEach(() => { + dir = tmp(); + errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + errorSpy.mockRestore(); + vi.unstubAllGlobals(); + rmSync(dir, { recursive: true, force: true }); + }); + + // citty binds only the first positional to `name`; `_` carries every + // positional token exactly as citty itself would populate it. + async function runCommand(args: Record): Promise { + await (addCommand.run as (ctx: { args: Record }) => Promise)({ args }); + } + + it("rejects `add `, naming the dropped arguments and touching no registry", async () => { + const fetchSpy = vi.fn(); + vi.stubGlobal("fetch", fetchSpy); + + await expect( + runCommand({ name: "a", _: ["a", "b", "c"], dir, clipboard: true }), + ).rejects.toThrow(CliUsageError); + + // Failing fast means no partial install either: `a` never touches the + // registry, so an invalid multi-name invocation can't half-succeed and + // leave the project in a state that depends on install order. + expect(fetchSpy).not.toHaveBeenCalled(); + expect(errorSpy.mock.calls.flat().join(" ")).toContain("b, c"); + }); + + it("rejects extra positionals under real citty parsing, with a flag interleaved", async () => { + const fetchSpy = vi.fn(); + vi.stubGlobal("fetch", fetchSpy); + + // `add a --dir b`: exercises citty's own parser rather than a + // hand-built args object, proving `_` really does exclude the `--dir` + // flag and its value while still keeping both positional tokens. + const parsed = parseCittyArgs(["a", "--dir", dir, "b"], addCommand.args as ArgsDef); + expect(parsed._).toEqual(["a", "b"]); + + await expect(runCommand(parsed as unknown as Record)).rejects.toThrow( + CliUsageError, + ); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it("does not fire on a normal single-item invocation", async () => { + mockFetch(); + writeRegistryConfig(dir); + + await expect( + runCommand({ name: "my-block", _: ["my-block"], dir, clipboard: false, json: true }), + ).resolves.toBeUndefined(); + }); +}); + describe("describeInstallFailure", () => { it("explains a bare transport failure instead of echoing it", () => { // What the user actually sees after copying a command off the catalog page. diff --git a/packages/cli/src/commands/add.ts b/packages/cli/src/commands/add.ts index c37ea85152..9762a428b9 100644 --- a/packages/cli/src/commands/add.ts +++ b/packages/cli/src/commands/add.ts @@ -1,4 +1,4 @@ -import { failCommand } from "../utils/commandResult.js"; +import { failCommand, failUsage } from "../utils/commandResult.js"; import { defineCommand } from "citty"; import type { Example } from "./_examples.js"; @@ -380,6 +380,19 @@ export async function runAdd(opts: RunAddArgs): Promise { }; } +// ── Extra-positional-argument guard ───────────────────────────────────────── +// One item or tag per invocation is the contract — a tag is the bulk path. +// citty binds only the FIRST positional token to `name`; every token after it +// still lands in `args._` but was never read here, so `add a b c` behaved +// exactly like `add a` — same exit code, same output, `b` and `c` never +// installed and never mentioned. +export function formatExtraPositionalsError(extra: string[]): string { + return ( + `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." + ); +} + // ── Command ───────────────────────────────────────────────────────────────── export default defineCommand({ @@ -433,6 +446,15 @@ export default defineCommand({ const projectDir = resolve(args.dir ?? process.cwd()); const json = args.json === true; const skipClipboard = args.clipboard === false; + + const extraPositionals = args._.slice(1); + if (extraPositionals.length > 0) { + const msg = formatExtraPositionalsError(extraPositionals); + 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.