fix: address Copilot review round 1 on #2323

All nine findings taken.

Two budgets I had lowered are restored as named exceptions rather than folded
into the shared value. cli's e2e child timer goes back to 15s and the two tests
that spawn it get E2E_SPAWN_MS = 25s, so the bound carrying the diagnostic still
wins without taking load tolerance off the one suite that spawns a real process.
App.test.tsx's three-modal sequence keeps THREE_MODAL_SEQUENCE_MS = 20s — a
value chosen against an observed failure, which the shared ceiling would have
replaced with a general one.

Three real false-positive paths in the guard are closed. It now discovers every
Vitest config under clients/ and fails on one it is not configured to check —
without which "a project with no row is an error" was vacuous, since an
undiscovered config yields no project to reject. It strips comments before
looking for configure() and requires the exact expected value, so neither a
commented-out call nor asyncUtilTimeout: 1 passes. And it compares each
resolved project's setupFiles against the site it claims to configure, so
deleting a setupFiles entry can no longer leave both projects reported as
configured while Testing Library silently reverts to 1000ms. Each has a test,
and the setupFiles check was observed to fire against a real deletion.

Three stale comments the change had invalidated: main.yml still counted four
durable guards, PreviewAnnotations.stories.tsx still said the storybook project
had no setupFiles, and the new AGENTS.md rule read as though no Group F row had
been settled when this PR settles F6. browser-timeouts.test.mjs gains the
purpose header this repo's files carry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>

scripts/lib/browser-timeouts.test.mjs

@@ -1,3 +1,16 @@
+/**
+ * Pins the invariants of `browser-timeouts.mjs` — a constants module, so what
+ * is testable about it is the shape of the values rather than any logic.
+ *
+ * Worth pinning anyway, because the values are the module's whole contract and
+ * a smoke cannot report a bad one. A budget that arrived as `undefined` or a
+ * string reaches Playwright as "no timeout" or NaN and a smoke then hangs
+ * against the job's own ceiling rather than failing with a cause. And the
+ * ordering below IS the semantics documented on each key: the names say what
+ * each budget has to absorb, so a `nested` wait outliving its enclosing
+ * `roundTrip` one would mean the names had stopped describing the values.
+ */
+
 import test from "node:test";
 import assert from "node:assert/strict";
 

scripts/verify-test-timeouts.mjs

@@ -39,8 +39,8 @@
  * both `asyncUtilTimeout` assertions unmet.
  */
 
-import { existsSync, readFileSync } from "node:fs";
-import { dirname, join, resolve } from "node:path";
+import { existsSync, readdirSync, readFileSync } from "node:fs";
+import { dirname, join, relative, resolve } from "node:path";
 import { fileURLToPath } from "node:url";
 
 const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), "..");
@@ -114,6 +114,19 @@ export const ASYNC_UTIL_SITES = Object.freeze([
   },
 ]);
 
+/**
+ * The value those sites must configure — stated here for the same reason the
+ * Vitest budgets are: a guard that accepts any number at all would pass on
+ * `asyncUtilTimeout: 1` (Copilot).
+ */
+export const EXPECTED_ASYNC_UTIL_TIMEOUT = 5_000;
+
+/**
+ * Directory every Vitest config in this repo lives one level under. Discovery
+ * (below) walks it rather than trusting a hand-written list.
+ */
+export const CLIENTS_DIR = "clients";
+
 /**
  * Compare one resolved project against its row.
  *
@@ -152,25 +165,151 @@ export function checkProject(name, config, expected = EXPECTED_PROJECTS) {
 }
 
 /**
- * Does this source configure `asyncUtilTimeout`, through the right import?
+ * Strip `//` and block comments so a commented-out call cannot satisfy a check.
+ *
+ * Comment-aware rather than exact, deliberately: a naive scan of the raw source
+ * would accept a `// configure({ asyncUtilTimeout: 5000 })` left behind by
+ * someone disabling it, which is the most likely way this stops being
+ * configured (Copilot). String literals are not parsed out — a `configure(` in
+ * a string would still be accepted — but these two files are fifteen lines each
+ * and the false-positive that matters is the commented one.
+ *
+ * @param {string} source
+ * @returns {string}
+ */
+export function stripComments(source) {
+  return source
+    .replace(/\/\*[\s\S]*?\*\//g, "")
+    .replace(/(^|\n)\s*\/\/[^\n]*/g, "$1");
+}
+
+/**
+ * Does this source configure `asyncUtilTimeout` at the expected value, through
+ * the right import?
  *
  * Deliberately a source check and not a runtime one: the value only exists
  * inside a running test environment, and a `configure` call reached by no
- * project would satisfy a runtime probe of this module just as well.
+ * project would satisfy a runtime probe of this module just as well. What
+ * closes that second gap is `checkSetupFilesWiring` below, which asks the
+ * resolved project whether it actually loads the file.
  *
  * @param {string} source
  * @param {string} from module the `configure` must come from
+ * @param {number} [expected] the value it must configure
  * @returns {string[]}
  */
-export function checkAsyncUtilSource(source, from) {
+export function checkAsyncUtilSource(
+  source,
+  from,
+  expected = EXPECTED_ASYNC_UTIL_TIMEOUT,
+) {
+  const code = stripComments(source);
   const failures = [];
-  if (!new RegExp(`from\\s+["']${from.replace("/", "\\/")}["']`).test(source)) {
+  if (!new RegExp(`from\\s+["']${from.replace(/\//g, "\\/")}["']`).test(code)) {
     failures.push(`does not import from "${from}"`);
   }
-  if (!/\bconfigure\s*\(/.test(source)) {
+  if (!/\bconfigure\s*\(/.test(code)) {
     failures.push("does not call configure()");
-  } else if (!/asyncUtilTimeout\s*:\s*\d+/.test(source)) {
+    return failures;
+  }
+  const match = /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(code);
+  if (!match) {
     failures.push("calls configure() without an asyncUtilTimeout");
+  } else if (Number(match[1].replace(/_/g, "")) !== expected) {
+    failures.push(
+      `configures asyncUtilTimeout as ${match[1]}, expected ${expected}`,
+    );
+  }
+  return failures;
+}
+
+/**
+ * Is each declared site actually loaded by the project it claims to configure?
+ *
+ * Without this the guard reads two files and reports both projects configured
+ * while `setupFiles` had been deleted from `vite.config.ts` and Testing Library
+ * had silently gone back to its 1000ms default (Copilot). Vitest resolves
+ * `setupFiles` to absolute paths, so compare by suffix against the declared
+ * repo-relative path.
+ *
+ * @param {string} name project name as this guard knows it
+ * @param {unknown} setupFiles the project's resolved `setupFiles`
+ * @param {readonly {file: string, project: string}[]} [sites]
+ * @returns {string[]}
+ */
+export function checkSetupFilesWiring(
+  name,
+  setupFiles,
+  sites = ASYNC_UTIL_SITES,
+) {
+  const site = sites.find((s) => s.project === name);
+  if (!site) return [];
+  const loaded = Array.isArray(setupFiles) ? setupFiles : [];
+  const wanted = site.file.split("/").join("/");
+  const found = loaded.some(
+    (f) => typeof f === "string" && f.split("\\").join("/").endsWith(wanted),
+  );
+  return found
+    ? []
+    : [
+        `project "${name}" does not load ${site.file} as a setupFile, so its ` +
+          `asyncUtilTimeout never takes effect`,
+      ];
+}
+
+/**
+ * Every Vitest config under `clients/`, as repo-relative directories.
+ *
+ * Discovery rather than the hand-written `CONFIG_ROOTS` list, so that a new
+ * client with its own config cannot go unwatched — which would have made the
+ * "a project with no row is an error" promise vacuous, since an undiscovered
+ * config yields no project to reject (Copilot). `CONFIG_ROOTS` still exists to
+ * say which project names each config must produce; this is what proves the
+ * list is complete.
+ *
+ * @param {string} [root] absolute repo root
+ * @returns {string[]}
+ */
+export function discoverConfigRoots(root = repoRoot) {
+  const clients = resolve(root, CLIENTS_DIR);
+  if (!existsSync(clients)) return [];
+  return readdirSync(clients, { withFileTypes: true })
+    .filter((e) => e.isDirectory())
+    .map((e) => join(clients, e.name))
+    .filter((dir) =>
+      ["vitest.config.ts", "vite.config.ts"].some((f) =>
+        existsSync(join(dir, f)),
+      ),
+    )
+    .map((dir) => relative(root, dir).split("\\").join("/"))
+    .sort();
+}
+
+/**
+ * Compare what is on disk against what this guard is configured to check.
+ *
+ * @param {string[]} discovered
+ * @param {readonly {root: string}[]} [configured]
+ * @returns {string[]}
+ */
+export function checkConfigRootCoverage(discovered, configured = CONFIG_ROOTS) {
+  const known = new Set(configured.map((c) => c.root));
+  const failures = [];
+  for (const root of discovered) {
+    if (!known.has(root)) {
+      failures.push(
+        `${root} has a Vitest config that this guard does not check — add it to ` +
+          `CONFIG_ROOTS and EXPECTED_PROJECTS rather than leaving its budgets unstated`,
+      );
+    }
+  }
+  for (const root of known) {
+    if (!discovered.includes(root)) {
+      failures.push(
+        `CONFIG_ROOTS names ${root}, which has no Vitest config on disk — a stale ` +
+          `row silently stops checking anything`,
+      );
+    }
   }
   return failures;
 }
@@ -243,6 +382,9 @@ async function main() {
       checked += 1;
       failures.push(
         ...checkProject(name, project.config).map((f) => `${root}: ${f}`),
+        ...checkSetupFilesWiring(name, project.config.setupFiles).map(
+          (f) => `${root}: ${f}`,
+        ),
       );
     }
     for (const name of expectedNames) {

scripts/verify-test-timeouts.test.mjs

@@ -18,12 +18,17 @@ import assert from "node:assert/strict";
 import {
   ASYNC_UTIL_SITES,
   CONFIG_ROOTS,
+  EXPECTED_ASYNC_UTIL_TIMEOUT,
   EXPECTED_INTEGRATION_TIMEOUTS,
   EXPECTED_PROJECTS,
   EXPECTED_TIMEOUTS,
   checkAsyncUtilSource,
+  checkConfigRootCoverage,
   checkProject,
+  checkSetupFilesWiring,
+  discoverConfigRoots,
   identifyProject,
+  stripComments,
 } from "./verify-test-timeouts.mjs";
 
 const ok = { ...EXPECTED_TIMEOUTS };
@@ -148,3 +153,95 @@ test("both web projects have an asyncUtilTimeout site", () => {
     "unit",
   ]);
 });
+
+test("a commented-out configure() does not satisfy the check", () => {
+  // The most likely way this stops being configured is someone disabling it in
+  // place, and a raw-source scan would read that as configured (Copilot).
+  const lineComment = `import { configure } from "storybook/test";
+// configure({ asyncUtilTimeout: 5000 });`;
+  assert.deepEqual(checkAsyncUtilSource(lineComment, "storybook/test"), [
+    "does not call configure()",
+  ]);
+
+  const blockComment = `import { configure } from "storybook/test";
+/* configure({ asyncUtilTimeout: 5000 }); */`;
+  assert.deepEqual(checkAsyncUtilSource(blockComment, "storybook/test"), [
+    "does not call configure()",
+  ]);
+});
+
+test("the configured value must be the expected one, not merely present", () => {
+  const source = `import { configure } from "storybook/test";
+configure({ asyncUtilTimeout: 1 });`;
+  assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
+    `configures asyncUtilTimeout as 1, expected ${EXPECTED_ASYNC_UTIL_TIMEOUT}`,
+  ]);
+});
+
+test("a numeric separator in the configured value is read, not rejected", () => {
+  const source = `import { configure } from "storybook/test";
+configure({ asyncUtilTimeout: 5_000 });`;
+  assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []);
+});
+
+test("stripComments leaves executable code alone", () => {
+  const source = `/** header */
+import { configure } from "storybook/test"; // trailing
+configure({ asyncUtilTimeout: 5000 });`;
+  const code = stripComments(source);
+  assert.match(code, /configure\(\{ asyncUtilTimeout: 5000 \}\)/);
+  assert.doesNotMatch(code, /header/);
+});
+
+test("a project that does not load its declared setup file is caught", () => {
+  // Reading the file proves it configures the timeout; only this proves the
+  // project ever loads it. Deleting the setupFiles entry used to leave the
+  // guard reporting both projects configured (Copilot).
+  const failures = checkSetupFilesWiring("unit", []);
+  assert.equal(failures.length, 1);
+  assert.match(failures[0], /does not load .*setup\.ts as a setupFile/);
+});
+
+test("a setup file loaded under an absolute path is recognized", () => {
+  assert.deepEqual(
+    checkSetupFilesWiring("unit", [
+      "/repo/clients/web/src/test/setup.ts",
+      "/repo/other.ts",
+    ]),
+    [],
+  );
+});
+
+test("a project with no declared site is not required to load one", () => {
+  assert.deepEqual(checkSetupFilesWiring("cli", []), []);
+  assert.deepEqual(checkSetupFilesWiring("cli", undefined), []);
+});
+
+test("a Vitest config this guard does not check is an error", () => {
+  const failures = checkConfigRootCoverage([
+    "clients/web",
+    "clients/cli",
+    "clients/tui",
+    "clients/launcher",
+    "clients/desktop",
+  ]);
+  assert.equal(failures.length, 1);
+  assert.match(failures[0], /clients\/desktop has a Vitest config/);
+});
+
+test("a stale row naming a config that no longer exists is an error", () => {
+  const failures = checkConfigRootCoverage(["clients/web", "clients/cli"]);
+  assert.equal(failures.length, 2);
+  for (const f of failures) assert.match(f, /has no Vitest config on disk/);
+});
+
+test("discovery finds exactly the configs this guard is set up to check", () => {
+  // The two halves have to agree against the real repo, not only against
+  // fixtures — that agreement is what makes the unknown-project check below
+  // mean anything.
+  assert.deepEqual(
+    discoverConfigRoots().sort(),
+    CONFIG_ROOTS.map((c) => c.root).sort(),
+  );
+  assert.deepEqual(checkConfigRootCoverage(discoverConfigRoots()), []);
+});