diff --git a/design/src/cli.ts b/design/src/cli.ts index a55d92555..b266c6f38 100644 --- a/design/src/cli.ts +++ b/design/src/cli.ts @@ -25,6 +25,7 @@ import { evolve } from "./evolve"; import { generateDesignToCodePrompt } from "./design-to-code"; import { serve } from "./serve"; import { gallery } from "./gallery"; +import { normalizeIntFlag } from "./flag-utils"; import { daemonStatus as daemonStatusClient, ensureDaemon, @@ -137,7 +138,7 @@ async function main(): Promise { briefFile: flags["brief-file"] as string, output: (flags.output as string) || "/tmp/gstack-mockup.png", check: !!flags.check, - retry: flags.retry ? parseInt(flags.retry as string) : 0, + retry: normalizeIntFlag(flags.retry, { name: "retry", def: 0, min: 0 }), size: flags.size as string, quality: flags.quality as string, }); @@ -163,7 +164,7 @@ async function main(): Promise { if (flags["no-daemon"]) { await serve({ html: outputPath, - timeout: flags.timeout ? parseInt(flags.timeout as string) : 600, + timeout: normalizeIntFlag(flags.timeout, { name: "timeout", def: 600, min: 1 }), }); } else { await publishToDaemon({ @@ -197,7 +198,9 @@ async function main(): Promise { await variants({ brief: flags.brief as string, briefFile: flags["brief-file"] as string, - count: flags.count ? parseInt(flags.count as string) : 3, + // #2032: pass the RAW flag through — variants() normalizes at its + // consumption site (a pre-parseInt here would silently truncate "3.7"). + count: flags.count, outputDir: (flags["output-dir"] as string) || "/tmp/gstack-variants/", size: flags.size as string, quality: flags.quality as string, diff --git a/design/src/flag-utils.ts b/design/src/flag-utils.ts new file mode 100644 index 000000000..9959a653b --- /dev/null +++ b/design/src/flag-utils.ts @@ -0,0 +1,74 @@ +/** + * Integer flag normalization for the design CLI (#2032). + * + * The CLI's flag parser yields a string ("3"), boolean true (bare flag with + * no value), or undefined (flag absent). parseInt on those produced NaN that + * flowed silently into loop bounds and setTimeout: + * --count abc → for (i < NaN) never runs → ZERO variants, exit 0 + * --retry abc → attempt <= NaN is false → generate() silent no-op + * --timeout abc → setTimeout(NaN) fires ~immediately → serve dies at boot + * + * Contract (matches the --viewports precedent, variants.ts: error LOUDLY on + * nonsense; these commands spend real image-API money, so a silent fixup + * hides typos from calling agents): + * - undefined → default (flag absent) + * - true / "" (bare flag) → error: requires a value + * - non-numeric / non-integer → error ("3.7" is rejected, not truncated) + * - below min → error + * - above max (when given) → clamp to max, stderr warning + * - repeated flag → parser is last-wins before we ever see it + */ + +export interface IntFlagSpec { + name: string; + def: number; + min: number; + max?: number; +} + +export type IntFlagResult = + | { ok: true; value: number; warning?: string } + | { ok: false; error: string }; + +/** Pure decision function — unit-testable without process.exit. */ +export function parseIntFlag(raw: unknown, spec: IntFlagSpec): IntFlagResult { + const { name, def, min, max } = spec; + const bounds = `an integer >= ${min}${max !== undefined ? ` (max ${max})` : ""}`; + + if (raw === undefined || raw === false) return { ok: true, value: def }; + if (raw === true) { + return { ok: false, error: `--${name} requires a value. Expected ${bounds}.` }; + } + + const s = String(raw).trim(); + if (s === "") { + return { ok: false, error: `--${name} requires a value. Expected ${bounds}.` }; + } + if (!/^-?\d+$/.test(s)) { + return { ok: false, error: `Invalid --${name}: "${s}" is not an integer. Expected ${bounds}.` }; + } + + const n = parseInt(s, 10); + if (n < min) { + return { ok: false, error: `Invalid --${name}: ${n} is below the minimum of ${min}.` }; + } + if (max !== undefined && n > max) { + return { + ok: true, + value: max, + warning: `--${name} ${n} exceeds the maximum of ${max}; using ${max}.`, + }; + } + return { ok: true, value: n }; +} + +/** CLI wrapper: loud exit(1) on invalid input, stderr warning on clamp. */ +export function normalizeIntFlag(raw: unknown, spec: IntFlagSpec): number { + const r = parseIntFlag(raw, spec); + if (!r.ok) { + console.error(r.error); + process.exit(1); + } + if (r.warning) console.error(r.warning); + return r.value; +} diff --git a/design/src/variants.ts b/design/src/variants.ts index 257079dea..15be75e5c 100644 --- a/design/src/variants.ts +++ b/design/src/variants.ts @@ -8,11 +8,17 @@ import fs from "fs"; import path from "path"; import { requireApiKey } from "./auth"; import { parseBrief } from "./brief"; +import { normalizeIntFlag } from "./flag-utils"; export interface VariantsOptions { brief?: string; briefFile?: string; - count: number; + /** + * Raw CLI flag value or a number. Normalized inside variants() (#2032): + * nonsense errors loudly; above STYLE_VARIATIONS.length clamps with a + * warning — past that index variants degrade to duplicate base-brief runs. + */ + count?: number | string | boolean; outputDir: string; size?: string; quality?: string; @@ -153,7 +159,15 @@ export async function variants(options: VariantsOptions): Promise { return; } - const count = Math.min(options.count, 7); // Cap at 7 style variations + // #2032: normalize at the consumption site so every caller (CLI or + // programmatic) gets the loud-on-nonsense contract; the ceiling derives + // from STYLE_VARIATIONS so it self-adjusts when styles are added. + const count = normalizeIntFlag(options.count, { + name: "count", + def: 3, + min: 1, + max: STYLE_VARIATIONS.length, + }); const size = options.size || "1536x1024"; console.error(`Generating ${count} variants...`); diff --git a/test/design-flag-utils.test.ts b/test/design-flag-utils.test.ts new file mode 100644 index 000000000..9e3734081 --- /dev/null +++ b/test/design-flag-utils.test.ts @@ -0,0 +1,113 @@ +/** + * design/src/flag-utils.ts — integer-flag contract (#2032, eng-review 7A/8A). + * + * Lives under test/ (NOT design/test/) deliberately: design/test/ is invisible + * to the bun test glob, scripts/test-free-shards TEST_ROOTS, and every CI + * workflow (eng-review 11A), so a tripwire there guards nothing. flag-utils is + * a pure module, so importing it from here is clean. + * + * The bug class: the design CLI parser yields string | true | undefined; + * parseInt produced NaN that flowed silently into loop bounds and setTimeout — + * `variants --count abc` generated ZERO variants and exited 0, `generate + * --retry abc` was a silent no-op, `serve --timeout abc` died at boot. + */ + +import { describe, test, expect } from "bun:test"; +import { spawnSync } from "child_process"; +import * as path from "path"; +import { parseIntFlag } from "../design/src/flag-utils"; + +const ROOT = path.resolve(import.meta.dir, ".."); + +const COUNT_SPEC = { name: "count", def: 3, min: 1, max: 7 } as const; +const RETRY_SPEC = { name: "retry", def: 0, min: 0 } as const; +const TIMEOUT_SPEC = { name: "timeout", def: 600, min: 1 } as const; + +describe("parseIntFlag contract (#2032, codex 17a-c)", () => { + test("undefined → default (flag absent)", () => { + expect(parseIntFlag(undefined, COUNT_SPEC)).toEqual({ ok: true, value: 3 }); + expect(parseIntFlag(undefined, RETRY_SPEC)).toEqual({ ok: true, value: 0 }); + }); + + test("non-numeric string → error, never a silent default ('--count abc')", () => { + const r = parseIntFlag("abc", COUNT_SPEC); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error).toContain('"abc" is not an integer'); + }); + + test("bare flag (parser yields true) → error 'requires a value'", () => { + const r = parseIntFlag(true, COUNT_SPEC); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error).toContain("requires a value"); + }); + + test("empty string → error 'requires a value'", () => { + const r = parseIntFlag("", COUNT_SPEC); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error).toContain("requires a value"); + }); + + test("non-integer '3.7' → error (rejected, not silently truncated to 3)", () => { + const r = parseIntFlag("3.7", COUNT_SPEC); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error).toContain("not an integer"); + }); + + test("below min → error ('--count 0' billed the user for 1 they asked 0 of; now loud)", () => { + expect(parseIntFlag("0", COUNT_SPEC).ok).toBe(false); + expect(parseIntFlag("-2", COUNT_SPEC).ok).toBe(false); + // retry allows 0 (min: 0) + expect(parseIntFlag("0", RETRY_SPEC)).toEqual({ ok: true, value: 0 }); + }); + + test("above max → clamp WITH warning (capability limit, not a user mistake)", () => { + const r = parseIntFlag("99", COUNT_SPEC); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.value).toBe(7); + expect(r.warning).toContain("exceeds the maximum"); + } + }); + + test("in-range integers pass through untouched", () => { + expect(parseIntFlag("3", COUNT_SPEC)).toEqual({ ok: true, value: 3 }); + expect(parseIntFlag(5, COUNT_SPEC)).toEqual({ ok: true, value: 5 }); + expect(parseIntFlag("120", TIMEOUT_SPEC)).toEqual({ ok: true, value: 120 }); + }); + + test("retry-NaN and timeout-NaN are errors, not silent no-ops (#2032 siblings)", () => { + // Pre-fix: --retry abc → generate() loop never ran (attempt <= NaN), + // printed null, exited 0. --timeout abc → setTimeout(NaN) ≈ immediate + // SERVE_TIMEOUT. Both members of the same NaN class, same file. + expect(parseIntFlag("abc", RETRY_SPEC).ok).toBe(false); + expect(parseIntFlag("abc", TIMEOUT_SPEC).ok).toBe(false); + }); + + test("NaN number input (legacy pre-parsed callers) → error", () => { + expect(parseIntFlag(Number.NaN, COUNT_SPEC).ok).toBe(false); + }); +}); + +describe("normalizeIntFlag CLI wrapper (exit-1 semantics)", () => { + function runWrapper(rawExpr: string, specExpr: string): { status: number; stderr: string } { + const script = ` + import { normalizeIntFlag } from "${ROOT}/design/src/flag-utils"; + const v = normalizeIntFlag(${rawExpr}, ${specExpr}); + console.log("VALUE:" + v); + `; + const res = spawnSync("bun", ["-e", script], { encoding: "utf-8", cwd: ROOT }); + return { status: res.status ?? -1, stderr: res.stderr ?? "" }; + } + + test("invalid input exits 1 with the error on stderr", () => { + const r = runWrapper('"abc"', '{ name: "count", def: 3, min: 1, max: 7 }'); + expect(r.status).toBe(1); + expect(r.stderr).toContain("not an integer"); + }); + + test("clamp warns on stderr but exits 0", () => { + const r = runWrapper('"99"', '{ name: "count", def: 3, min: 1, max: 7 }'); + expect(r.status).toBe(0); + expect(r.stderr).toContain("exceeds the maximum"); + }); +});