From fce8b158d7eb1a150ca9c47ac1515ac66f100cfa Mon Sep 17 00:00:00 2001 From: Timur Mashara <62305878+tmashara@users.noreply.github.com> Date: Mon, 2 Nov 2026 14:07:27 +0100 Subject: [PATCH] CLI: reject blank and stray flag values; report/merge take --x=value Flag values go through one walker, so a blank value (--threshold= used to read as 0) and a missing one exit 2 naming the flag, and a flag that takes no value refuses --flag=value (--dry-run=false used to still dry-run). A numeric flag takes a negative number so the range check names it. --width must be a positive integer. A bare --sitemap with a .xml route or origin asks for --sitemap=. report and merge share the walker: --x=value works and a missing value no longer swallows the next flag. Co-Authored-By: Claude Opus 5.5 --- src/cli.ts | 113 +++++++++++++++++------------------- tests/unit/cli-args.test.ts | 58 +++++++++++++++--- tests/unit/report.test.ts | 18 ++++++ 3 files changed, 121 insertions(+), 68 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index ce473ef..b20cb93 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -62,6 +62,28 @@ const num = (flag: string, raw: string): number => { return n; }; +function* flags(argv: string[]) { + for (let i = 0; i < argv.length; i++) { + const raw = argv[i]!; + const eq = raw.startsWith("--") ? raw.indexOf("=") : -1; + const v = eq === -1 ? raw : raw.slice(0, eq); + const inline = eq === -1 ? undefined : raw.slice(eq + 1); + let took = false; + // A numeric flag takes a negative number, so the range check can name the problem. + const value = (numeric = false) => { + const s = inline ?? argv[i + 1]; + const isFlag = + inline === undefined && s?.startsWith("-") && !(numeric && Number.isFinite(Number(s))); + if (s === undefined || !s.trim() || isFlag) throw new Error(`${v} needs a value`); + if (inline === undefined) i++; + took = true; + return s.trim(); + }; + yield { v, inline, value }; + if (inline !== undefined && !took) throw new Error(`${v} takes no value`); + } +} + function parseArgs(argv: string[]): Args { const a: Args = { dryRun: false, @@ -76,23 +98,7 @@ function parseArgs(argv: string[]): Args { const origins: string[] = []; const sel: { both?: string; left?: string; right?: string } = {}; const state: typeof sel = {}; - for (let i = 0; i < argv.length; i++) { - const raw = argv[i]!; - // Accept both `--flag value` and `--flag=value`. - const eq = raw.startsWith("--") ? raw.indexOf("=") : -1; - const v = eq === -1 ? raw : raw.slice(0, eq); - const inline = eq === -1 ? undefined : raw.slice(eq + 1); - // Require a value: the inline `--flag=value`, or the next argv token — but not - // another flag, which means the value was simply omitted. Only advance `i` when - // a positional token is actually consumed. - const next = () => { - if (inline !== undefined) return inline; - const peek = argv[i + 1]; - if (peek === undefined || peek.startsWith("-")) { - throw new Error(`${v} needs a value`); - } - return argv[++i]!; - }; + for (const { v, inline, value } of flags(argv)) { if (v === "--help" || v === "-h") a.help = true; else if (v === "--version" || v === "-v") a.version = true; else if (v === "--dry-run") a.dryRun = true; @@ -108,56 +114,50 @@ function parseArgs(argv: string[]): Args { else if (v === "--no-freeze-video") a.freezeVideo = false; else if (v === "--sitemap") { if (inline === "") throw new Error("--sitemap= needs a URL or path"); - a.sitemap = inline ?? true; - } else if (v === "--accept") { - a.accept = next().trim(); - if (!a.accept) throw new Error("--accept needs a reason"); - } else if (v === "--left") a.left = next(); - else if (v === "--right") a.right = next(); - else if (v === "--out") a.outDir = next(); - else if (v === "--config") a.config = next(); - else if (v === "--mask") { - const s = next().trim(); - if (!s) throw new Error("--mask needs a value"); - (a.masks ??= []).push(s); - } else if (v === "--only") { - const s = next().trim(); - if (!s) throw new Error("--only needs a value"); - (a.only ??= []).push(s); - } else if (v === "--hide") { - const s = next().trim(); - if (!s) throw new Error("--hide needs a value"); - (a.hideSelectors ??= []).push(s); - } else if (v === "--selector" || v === "--left-selector" || v === "--right-selector") { - const s = next(); - if (!s) throw new Error(`${v} needs a value`); - sel[v === "--selector" ? "both" : v === "--left-selector" ? "left" : "right"] = s; + a.sitemap = inline === undefined ? true : value(); + } else if (v === "--accept") a.accept = value(); + else if (v === "--left") a.left = value(); + else if (v === "--right") a.right = value(); + else if (v === "--out") a.outDir = value(); + else if (v === "--config") a.config = value(); + else if (v === "--mask") (a.masks ??= []).push(value()); + else if (v === "--only") (a.only ??= []).push(value()); + else if (v === "--hide") (a.hideSelectors ??= []).push(value()); + else if (v === "--selector" || v === "--left-selector" || v === "--right-selector") { + sel[v === "--selector" ? "both" : v === "--left-selector" ? "left" : "right"] = value(); } else if ( v === "--storage-state" || v === "--left-storage-state" || v === "--right-storage-state" ) { - const s = next(); - if (!s) throw new Error(`${v} needs a value`); - state[v === "--storage-state" ? "both" : v === "--left-storage-state" ? "left" : "right"] = s; + state[v === "--storage-state" ? "both" : v === "--left-storage-state" ? "left" : "right"] = + value(); } else if (v === "--width") { - const w = num("--width", next()); - if (w <= 0) throw new Error(`--width expects a positive number, got ${w}`); + const w = num("--width", value(true)); + if (!Number.isInteger(w) || w < 1) { + throw new Error(`--width expects a positive integer, got ${w}`); + } (a.widths ??= []).push(w); } else if (v === "--concurrency") { - const n = num("--concurrency", next()); + const n = num("--concurrency", value(true)); if (!Number.isInteger(n) || n < 1) { throw new Error(`--concurrency expects a positive integer, got ${n}`); } a.concurrency = n; } else if (v === "--threshold") { - const t = num("--threshold", next()); + const t = num("--threshold", value(true)); if (t < 0 || t > 1) throw new Error(`--threshold expects 0..1, got ${t}`); a.threshold = t; } else if (v.startsWith("-")) throw new Error(`unknown flag: ${v}`); else if (/^https?:\/\//i.test(v)) origins.push(v); else a.routes.push(v); } + const xml = a.sitemap === true && [...origins, ...a.routes].find((t) => /\.xml$/i.test(t)); + if (xml) { + throw new Error( + `use --sitemap=${xml} to read that sitemap; bare --sitemap reads the right side's`, + ); + } if (origins.length > 2) { throw new Error( `at most two origins (left, right), got ${origins.length}: ${origins.join(" ")}`, @@ -388,12 +388,9 @@ async function report(argv: string[]): Promise { "--markdown"?: string; "--base-url"?: string; } = {}; - for (let i = 0; i < argv.length; i++) { - const v = argv[i]!; + for (const { v, value } of flags(argv)) { if (v === "--verdict" || v === "--title" || v === "--markdown" || v === "--base-url") { - const value = argv[++i]; - if (value === undefined) throw new Error(`${v} needs a value`); - opt[v] = value; + opt[v] = value(); } else if (v.startsWith("-") || dir !== undefined) throw new Error(`unexpected argument: ${v}`); else dir = v; @@ -427,13 +424,9 @@ async function merge(argv: string[]): Promise { try { const dirs: string[] = []; let out: string | undefined; - for (let i = 0; i < argv.length; i++) { - const v = argv[i]!; - if (v === "--out") { - out = argv[++i]; - if (out === undefined) throw new Error("--out needs a value"); - } else if (v.startsWith("-") || dirs.length === 2) - throw new Error(`unexpected argument: ${v}`); + for (const { v, value } of flags(argv)) { + if (v === "--out") out = value(); + else if (v.startsWith("-") || dirs.length === 2) throw new Error(`unexpected argument: ${v}`); else dirs.push(v); } if (dirs.length !== 2 || !out) diff --git a/tests/unit/cli-args.test.ts b/tests/unit/cli-args.test.ts index 0933a4d..ee6da45 100644 --- a/tests/unit/cli-args.test.ts +++ b/tests/unit/cli-args.test.ts @@ -151,8 +151,45 @@ describe("flag validation", () => { test("a trailing flag with no value is rejected", () => rejects(["/a", "--left", L, "--right", R, "--out"], "--out needs a value")); - test("a negative number is not mistaken for a value", () => - rejects(["/a", "--left", L, "--right", R, "--width", "-5"], "--width needs a value")); + test("a negative number reaches the range check", () => + rejects( + ["/a", "--left", L, "--right", R, "--width", "-5"], + "--width expects a positive integer, got -5", + )); + + test("a blank number is rejected, not read as 0", async () => { + await rejects(["/a", "--left", L, "--right", R, "--threshold="], "--threshold needs a value"); + await rejects( + ["/a", "--left", L, "--right", R, "--threshold", " "], + "--threshold needs a value", + ); + await rejects(["/a", "--left", L, "--right", R, "--width="], "--width needs a value"); + }); + + test("--width rejects fractions", () => + rejects( + ["/a", "--left", L, "--right", R, "--width", "1.5"], + "--width expects a positive integer, got 1.5", + )); + + test("a flag without a value rejects an inline one", () => + rejects(["/a", "--left", L, "--right", R, "--dry-run=false"], "--dry-run takes no value")); + + test("a bare --sitemap refuses a sitemap given as a route or origin", async () => { + await rejects( + ["--dry-run", L, R, "--sitemap", "custom.xml"], + "use --sitemap=custom.xml to read that sitemap", + ); + await rejects( + ["--dry-run", "--left", L, "--sitemap", "https://prod.test/sitemap.xml", "/a"], + "use --sitemap=https://prod.test/sitemap.xml to read that sitemap", + ); + }); + + test("= inside an origin or route is not an inline value", async () => { + const r = await runCli(["--dry-run", `${L}/?a=b`, R, "/a?x=1"], { noOut: true }); + assert.equal(r.code, 0, r.stderr); + }); test("--mask needs a value", async () => { await rejects(["/a", "--left", L, "--right", R, "--dry-run", "--mask"], "--mask needs a value"); @@ -199,7 +236,7 @@ describe("flag validation", () => { test("--width rejects zero and below", () => rejects( ["/a", "--left", L, "--right", R, "--width", "0"], - "--width expects a positive number", + "--width expects a positive integer", )); test("--concurrency rejects fractions", () => @@ -221,7 +258,10 @@ describe("flag validation", () => { )); test("--threshold rejects below 0", () => - rejects(["/a", "--left", L, "--right", R, "--threshold", "-0.5"], "--threshold needs a value")); + rejects( + ["/a", "--left", L, "--right", R, "--threshold", "-0.5"], + "--threshold expects 0..1, got -0.5", + )); test("--threshold accepts the boundaries", async () => { for (const t of ["0", "1", "0.975"]) { @@ -365,8 +405,10 @@ describe("selectors", () => { "--left-selector needs --right-selector or --selector", )); - test("an empty selector exits 2", () => - rejects(["--dry-run", L, R, "/a", "--selector="], "--selector needs a value")); + test("an empty selector exits 2", async () => { + await rejects(["--dry-run", L, R, "/a", "--selector="], "--selector needs a value"); + await rejects(["--dry-run", L, R, "/a", "--selector", " "], "--selector needs a value"); + }); test("a route's own @selector beats --selector", async () => { const r = await dry(["/a @.x", "/b", "--selector", ".card"]); @@ -450,9 +492,9 @@ describe("storage state", () => { }); describe("accept and rerun", () => { - test("--accept needs a reason", async () => { + test("--accept needs a non-blank value", async () => { await rejects(["--dry-run", L, R, "/a", "--accept"], "--accept needs a value"); - await rejects(["/a", "--left", L, "--right", R, "--accept", " "], "--accept needs a reason"); + await rejects(["/a", "--left", L, "--right", R, "--accept", " "], "--accept needs a value"); }); test("--accept and --rerun-failed need a run, not --dry-run", async () => { diff --git a/tests/unit/report.test.ts b/tests/unit/report.test.ts index b3704c7..d7e0752 100644 --- a/tests/unit/report.test.ts +++ b/tests/unit/report.test.ts @@ -202,6 +202,18 @@ describe("renderdiff report --markdown", () => { assert.equal(r.code, 2); assert.match(r.stderr, /--base-url needs --markdown/); }); + + test("takes --flag=value, and a missing value doesn't swallow the next flag", async () => { + const out = tempDir(); + writeResults([shot("/")], meta(), out, []); + const md = path.join(out, "pr.md"); + const ok = await runCli(["report", out, `--markdown=${md}`], { noOut: true }); + assert.equal(ok.code, 0, ok.stderr); + assert.ok(fs.existsSync(md)); + const r = await runCli(["report", out, "--title", "--markdown", md], { noOut: true }); + assert.equal(r.code, 2); + assert.match(r.stderr, /--title needs a value/); + }); }); describe("verdict", () => { @@ -387,6 +399,12 @@ describe("renderdiff merge", () => { const merge = (a: string, b: string, out: string) => runCli(["merge", a, b, "--out", out], { noOut: true }); + test("--out needs a value", async () => { + const r = await runCli(["merge", tempDir(), tempDir(), "--out"], { noOut: true }); + assert.equal(r.code, 2); + assert.match(r.stderr, /--out needs a value/); + }); + test("replaces matching shots, adds the rest, and copies only what results.json references", async () => { const [a, b] = [full(), partial()]; const out = path.join(tempDir(), "merged");