fix(cli): reject directory --composition and add --browser-timeout (#1199) (#1200)

* fix(cli): reject directory --composition and add --browser-timeout (#1199) Two unrelated symptoms from issue #1199, fixed together: 1. `--composition .` (or any directory path) used to slip past the existsSync check in render.ts and explode downstream as `EISDIR: illegal operation on a directory, read` when the producer readFileSync'd the entry. The CLI now treats `.` / `""` as "omit the flag" (falls back to index.html) and rejects other directory paths with an actionable error pointing at the .html shape. 2. The 60s Puppeteer page.goto timeout in frameCapture.ts was hard- coded, so heavy compositions (many videos / fonts / asset requests) could not complete `domcontentloaded` in time. Add a configurable `pageNavigationTimeout` to EngineConfig (default 60_000, env fallback PRODUCER_PAGE_NAVIGATION_TIMEOUT_MS) and expose it as `--browser-timeout <seconds>` on `hyperframes render`. The flag threads through both renderLocal (via resolveConfig) and the docker bridge (via buildDockerRunArgs). Tests: - render.test.ts: forwards/omits pageNavigationTimeout into resolveConfig - dockerRunArgs.test.ts: forwards/omits --browser-timeout (seconds) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): address PR #1200 review — extract validators, tighten bounds Addresses Vai's blockers and Miguel's nits on PR #1200: - Vai blocker 1 (fallow CRAP) + blocker 3 (no argv tests): Extract --browser-timeout and --composition validators into pure helpers in utils/renderArgs.ts with a structured-result discriminant. Drops ~45 lines of inline validation from run(), reducing its CRAP score 1290→978 and cyclomatic 75→65. 19 new unit tests cover the parse branches (sub-ms, overflow, NaN, Infinity, empty, negative, ".", "./", whitespace, directory, missing, ../escape, sibling-prefix). - Vai blocker 2 (sub-ms → timeout:0 = "no timeout"): reject inputs that round to <1 ms. Puppeteer treats page.goto({timeout:0}) as wait-forever, so --browser-timeout 0.0004 silently flipped the semantics. Now rejected with an explicit "rounds to 0 ms" error. - Vai important 5 (1e10 accepted → setTimeout overflow): cap at 86_400s (24h). Above Node's TIMEOUT_MAX ≈ 2^31-1 ms setTimeout fires immediately, the opposite of "long timeout." - Vai important 4 (related timeouts unmentioned): CLI help and docs now flag PRODUCER_PUPPETEER_PROTOCOL_TIMEOUT_MS and the 45s playerReadyTimeout as the other knobs heavy compositions may need. - Vai nit 7 (s/ms unit mismatch): help text and docs row both call out the SECONDS-vs-MILLISECONDS difference between flag and env. - Vai nit 8 / Miguel nit (composition flag discoverability): the --composition description now says "Pass `.` (or omit the flag) to render the project's index.html." - Miguel nit (dead branch): the entryFile === "" unreachable branch is gone. New helper uses `if (!trimmed || trimmed === ".")`. Also adds a trailing-separator guard on the project-containment check (sibling-prefix bypass: /proj-evil/x.html no longer slips past startsWith('/proj')) — flagged by the code review. The three remaining fallow complexity findings on render.ts (run, renderDocker, trackRenderMetrics) are inherited from main; this PR reduces run() but does not refactor it. Suppressed with fallow-ignore-next-line markers and inline rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): diverge --browser-timeout error messages per Vai nit 5 The `not-a-number` and `not-positive` branches in browserTimeoutErrorMessage shared the generic "Must be a positive number of seconds" message even though the discriminant carried distinct kinds. Diverge them so users see the specific failure mode: --browser-timeout abc → "Got \"abc\", which is not a number." --browser-timeout -5 → "Got \"-5\" seconds, which is not positive." The shared hint ("pass a positive number of seconds, e.g. 180") is preserved on both branches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-09-07 10:06:21 +00:00 · 2026-06-04 16:47:10 -04:00
parent 8228932e17
commit 6affe2d212
9 changed files with 528 additions and 26 deletions
@@ -0,0 +1,173 @@
+import { describe, expect, it } from "vitest";
+import type { Stats } from "node:fs";
+import { resolve } from "node:path";
+import {
+  MAX_PAGE_NAVIGATION_TIMEOUT_SECONDS,
+  parseBrowserTimeoutMsArg,
+  parseCompositionEntryArg,
+  type BrowserTimeoutParseResult,
+  type CompositionEntryParseResult,
+} from "./renderArgs.js";
+
+function expectBrowserTimeoutErr(result: BrowserTimeoutParseResult): { kind: string; raw: string } {
+  if (result.ok) throw new Error(`expected error, got value=${result.value}`);
+  return result.error;
+}
+
+function expectCompositionErr(result: CompositionEntryParseResult): {
+  kind: string;
+  entryFile: string;
+} {
+  if (result.ok) throw new Error(`expected error, got value=${result.value}`);
+  return result.error;
+}
+
+/** Build a fake `Stats` for the in-memory stat adapter. */
+function fakeStats(kind: "file" | "directory"): Stats {
+  return {
+    isFile: () => kind === "file",
+    isDirectory: () => kind === "directory",
+  } as Stats;
+}
+
+/**
+ * Build a stat adapter from a path → kind map. Throws ENOENT for any
+ * path not in the map, matching `statSync` behaviour.
+ */
+function makeStat(entries: Record<string, "file" | "directory">): (path: string) => Stats {
+  return (path) => {
+    const kind = entries[path];
+    if (!kind) {
+      const err = new Error(`ENOENT: no such file or directory, stat '${path}'`);
+      (err as NodeJS.ErrnoException).code = "ENOENT";
+      throw err;
+    }
+    return fakeStats(kind);
+  };
+}
+
+describe("parseBrowserTimeoutMsArg", () => {
+  it("returns undefined when the flag is absent", () => {
+    expect(parseBrowserTimeoutMsArg(undefined)).toEqual({ ok: true, value: undefined });
+  });
+
+  it("converts whole seconds to milliseconds", () => {
+    expect(parseBrowserTimeoutMsArg("180")).toEqual({ ok: true, value: 180_000 });
+  });
+
+  it("accepts fractional seconds and rounds to integer ms", () => {
+    expect(parseBrowserTimeoutMsArg("90.5")).toEqual({ ok: true, value: 90_500 });
+    expect(parseBrowserTimeoutMsArg("0.001")).toEqual({ ok: true, value: 1 });
+  });
+
+  it("rejects non-numeric input", () => {
+    const err = expectBrowserTimeoutErr(parseBrowserTimeoutMsArg("abc"));
+    expect(err.kind).toBe("not-a-number");
+  });
+
+  it("rejects Infinity and NaN", () => {
+    expect(expectBrowserTimeoutErr(parseBrowserTimeoutMsArg("Infinity")).kind).toBe("not-a-number");
+    expect(expectBrowserTimeoutErr(parseBrowserTimeoutMsArg("NaN")).kind).toBe("not-a-number");
+  });
+
+  it("rejects zero and negative values", () => {
+    expect(expectBrowserTimeoutErr(parseBrowserTimeoutMsArg("0")).kind).toBe("not-positive");
+    expect(expectBrowserTimeoutErr(parseBrowserTimeoutMsArg("-5")).kind).toBe("not-positive");
+    expect(expectBrowserTimeoutErr(parseBrowserTimeoutMsArg("")).kind).toBe("not-positive");
+  });
+
+  it("rejects sub-millisecond inputs that would round to timeout: 0 (Puppeteer 'no timeout' sentinel)", () => {
+    // Regression for issue #1199 follow-up: --browser-timeout 0.0004 passes
+    // > 0 in seconds but Math.round(0.4) = 0 ms, which Puppeteer interprets
+    // as "wait forever". The validator must reject before the multiply.
+    expect(expectBrowserTimeoutErr(parseBrowserTimeoutMsArg("0.0004")).kind).toBe("too-small");
+  });
+
+  it("rejects values above the 24h cap to prevent setTimeout overflow", () => {
+    expect(expectBrowserTimeoutErr(parseBrowserTimeoutMsArg("1e10")).kind).toBe("too-large");
+    expect(
+      expectBrowserTimeoutErr(
+        parseBrowserTimeoutMsArg(String(MAX_PAGE_NAVIGATION_TIMEOUT_SECONDS + 1)),
+      ).kind,
+    ).toBe("too-large");
+  });
+
+  it("accepts values right at the 24h cap", () => {
+    expect(parseBrowserTimeoutMsArg(String(MAX_PAGE_NAVIGATION_TIMEOUT_SECONDS))).toEqual({
+      ok: true,
+      value: MAX_PAGE_NAVIGATION_TIMEOUT_SECONDS * 1000,
+    });
+  });
+});
+
+describe("parseCompositionEntryArg", () => {
+  const PROJECT = "/proj";
+  const stat = makeStat({
+    [resolve(PROJECT, "index.html")]: "file",
+    [resolve(PROJECT, "compositions/intro.html")]: "file",
+    [resolve(PROJECT, "compositions")]: "directory",
+    [PROJECT]: "directory",
+  });
+
+  it("returns undefined when the flag is absent", () => {
+    expect(parseCompositionEntryArg(undefined, PROJECT, stat)).toEqual({
+      ok: true,
+      value: undefined,
+    });
+  });
+
+  it("normalizes '.' to undefined so the producer falls back to index.html (issue #1199)", () => {
+    expect(parseCompositionEntryArg(".", PROJECT, stat)).toEqual({ ok: true, value: undefined });
+  });
+
+  it("normalizes './' to undefined", () => {
+    expect(parseCompositionEntryArg("./", PROJECT, stat)).toEqual({ ok: true, value: undefined });
+  });
+
+  it("normalizes empty / whitespace-only to undefined", () => {
+    expect(parseCompositionEntryArg("", PROJECT, stat)).toEqual({ ok: true, value: undefined });
+    expect(parseCompositionEntryArg("   ", PROJECT, stat)).toEqual({ ok: true, value: undefined });
+  });
+
+  it("passes through a valid .html file path", () => {
+    expect(parseCompositionEntryArg("compositions/intro.html", PROJECT, stat)).toEqual({
+      ok: true,
+      value: "compositions/intro.html",
+    });
+  });
+
+  it("strips a leading ./ before resolution", () => {
+    expect(parseCompositionEntryArg("./compositions/intro.html", PROJECT, stat)).toEqual({
+      ok: true,
+      value: "compositions/intro.html",
+    });
+  });
+
+  it("rejects a directory path with 'not-a-file' (the EISDIR cause)", () => {
+    const err = expectCompositionErr(parseCompositionEntryArg("compositions", PROJECT, stat));
+    expect(err).toEqual({ kind: "not-a-file", entryFile: "compositions" });
+  });
+
+  it("rejects a non-existent file with 'not-found'", () => {
+    const err = expectCompositionErr(parseCompositionEntryArg("missing.html", PROJECT, stat));
+    expect(err).toEqual({ kind: "not-found", entryFile: "missing.html" });
+  });
+
+  it("rejects a path that escapes the project directory", () => {
+    const err = expectCompositionErr(parseCompositionEntryArg("../escape.html", PROJECT, stat));
+    expect(err.kind).toBe("outside-project");
+  });
+
+  it("rejects a sibling-prefix path (trailing-separator guard)", () => {
+    // Without the trailing-separator guard, `/proj-evil/x.html`.startsWith('/proj')
+    // returns true and the sibling-directory escape passes validation.
+    const siblingStat = makeStat({
+      "/proj-evil/x.html": "file",
+      [PROJECT]: "directory",
+    });
+    const err = expectCompositionErr(
+      parseCompositionEntryArg("../proj-evil/x.html", PROJECT, siblingStat),
+    );
+    expect(err.kind).toBe("outside-project");
+  });
+});