Skip to content

CLI: reject blank and stray flag values; report/merge take --x=value #122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tmashara merged 1 commit into from
Nov 2, 2026
Merged

CLI: reject blank and stray flag values; report/merge take --x=value #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 53 additions & 60 deletions src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -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(" ")}`,
Expand Down Expand Up @@ -388,12 +388,9 @@ async function report(argv: string[]): Promise<never> {
"--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;
Expand Down Expand Up @@ -427,13 +424,9 @@ async function merge(argv: string[]): Promise<never> {
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)
Expand Down
58 changes: 50 additions & 8 deletions tests/unit/cli-args.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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", () =>
Expand All @@ -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"]) {
Expand Down Expand Up @@ -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"]);
Expand Down Expand Up @@ -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 () => {
Expand Down
18 changes: 18 additions & 0 deletions tests/unit/report.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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");
Expand Down
Loading