build: add a browser-test exclusion check and a pre-push static-check hook

npm run check:browser-excludes finds tests that import a browser driver and asks `vitest list` whether the CI config still collects them; wired into CI. npm install now also installs a marker-owned pre-push hook that runs the static CI checks (~15s). Skip with CODEMAN_SKIP_PREPUSH=1; hand-written hooks are left alone.
2026-10-04 06:29:42 +02:00 · 2026-09-26 18:45:17 -04:00
parent 45ea2e1d32
commit 1d85909a06
10 changed files with 738 additions and 7 deletions
@@ -0,0 +1,149 @@
+#!/usr/bin/env node
+/**
+ * Browser-test exclusion check.
+ *
+ * `npm run test:ci` must never try to drive a real browser: CI runners (and any
+ * clean checkout) have no chromium, so such a file dies with
+ * `browserType.launch: Executable doesn't exist` and takes the whole suite with
+ * it. `config/vitest.ci.config.ts` therefore excludes every browser-driven test
+ * via `BROWSER_TEST_GLOBS` in `config/test-suites.ts`. That list is maintained
+ * BY HAND, and a new browser test simply does not appear in it unless someone
+ * remembers. The omission is invisible on a developer machine that has run
+ * `npx playwright install`, where the test passes, and only shows up on a clean
+ * runner.
+ *
+ * Two deliberate design choices:
+ *
+ * 1. **Detection is by CONTENT, not filename.** Matching `*.browser.test.ts`
+ *    would miss the browser tests that predate that convention
+ *    (`inline-rename`, `opencode-resize`, `webgl-fallback`,
+ *    `terminal-copy-shortcut`, `codex-predictive-echo`). What actually makes a
+ *    file dangerous is importing a browser driver, so that is what is tested.
+ *
+ * 2. **The exclusion side is answered by vitest itself**, via
+ *    `vitest list --filesOnly`, rather than by re-implementing glob matching
+ *    against the config's `exclude` array. Patterns there include `test/mobile/**`
+ *    and `perf-*`; a hand-rolled matcher that disagreed with vitest by even one
+ *    edge case would report a gap that does not exist, or miss one that does.
+ *    Asking the real resolver cannot drift from the real behaviour.
+ *
+ * The pure pieces are exported for test/check-browser-test-excludes.test.ts; the
+ * check itself only runs when this file is executed directly.
+ */
+import { readdirSync, readFileSync } from 'node:fs';
+import { join, dirname, relative, sep, resolve } from 'node:path';
+import { fileURLToPath } from 'node:url';
+import { execFileSync } from 'node:child_process';
+
+const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..');
+const CI_CONFIG = join('config', 'vitest.ci.config.ts');
+const SUITES_FILE = join('config', 'test-suites.ts');
+
+/** Importing any one of these means the test needs a real browser binary. */
+const BROWSER_DRIVER =
+  /\bfrom\s+['"](?:playwright|playwright-core|@playwright\/test|puppeteer|puppeteer-core)['"]|\b(?:require|import)\(\s*['"](?:playwright|playwright-core|@playwright\/test|puppeteer|puppeteer-core)['"]\s*\)/;
+
+/** @param {string} source */
+export function importsBrowserDriver(source) {
+  return BROWSER_DRIVER.test(source);
+}
+
+/** @param {string} dir @returns {string[]} */
+function walk(dir) {
+  const out = [];
+  for (const entry of readdirSync(dir, { withFileTypes: true })) {
+    const path = join(dir, entry.name);
+    if (entry.isDirectory()) out.push(...walk(path));
+    else if (entry.isFile() && entry.name.endsWith('.test.ts')) out.push(path);
+  }
+  return out;
+}
+
+/**
+ * Every `*.test.ts` under `<root>/test` that imports a browser driver, as sorted
+ * repo-relative POSIX paths (the form `vitest list` prints).
+ *
+ * @param {string} root
+ * @returns {string[]}
+ */
+export function findBrowserTests(root) {
+  return walk(join(root, 'test'))
+    .filter((file) => importsBrowserDriver(readFileSync(file, 'utf8')))
+    .map((file) => relative(root, file).split(sep).join('/'))
+    .sort();
+}
+
+/**
+ * Parse `vitest list --filesOnly` output into a set of repo-relative paths. Stray
+ * blank or decorative lines are ignored rather than assuming the format is pristine.
+ *
+ * @param {string} output
+ * @returns {Set<string>}
+ */
+export function parseVitestFileList(output) {
+  return new Set(
+    output
+      .split('\n')
+      .map((line) => line.trim())
+      .filter((line) => line.endsWith('.test.ts'))
+      .map((line) => line.replace(/^\.\//, ''))
+  );
+}
+
+/**
+ * @param {string[]} browserTests
+ * @param {Set<string>} ciFiles
+ * @returns {string[]} browser-driven files that the CI config would still collect
+ */
+export function findLeaks(browserTests, ciFiles) {
+  return browserTests.filter((file) => ciFiles.has(file));
+}
+
+function main() {
+  const browserTests = findBrowserTests(ROOT);
+
+  let collected;
+  try {
+    collected = execFileSync('npx', ['vitest', 'list', '--config', CI_CONFIG, '--filesOnly'], {
+      cwd: ROOT,
+      encoding: 'utf8',
+      stdio: ['ignore', 'pipe', 'pipe'],
+    });
+  } catch (err) {
+    console.error('✗ could not enumerate the CI test set via `vitest list`.');
+    console.error(err.stderr ? err.stderr.toString() : String(err));
+    process.exit(1);
+  }
+
+  const ciFiles = parseVitestFileList(collected);
+  if (ciFiles.size === 0) {
+    // An empty list would make every browser test look excluded: fail rather than pass vacuously.
+    console.error('✗ `vitest list` reported no test files; refusing to pass on an empty CI set.');
+    process.exit(1);
+  }
+
+  const leaked = findLeaks(browserTests, ciFiles);
+
+  if (leaked.length > 0) {
+    console.error(`✗ ${leaked.length} browser-driven test file(s) are NOT excluded from ${CI_CONFIG}:\n`);
+    for (const file of leaked) console.error(`    ${file}`);
+    console.error(`
+These import a browser driver, so on a runner with no chromium they fail with
+"browserType.launch: Executable doesn't exist" and take the suite down. Add each
+to BROWSER_TEST_GLOBS in ${SUITES_FILE} (${CI_CONFIG} derives its excludes from
+it, and \`npm run test:browser\` its includes).
+
+They may well pass on this machine; that is the trap. To reproduce a clean
+runner locally:
+  PLAYWRIGHT_BROWSERS_PATH=\$(mktemp -d) PUPPETEER_CACHE_DIR=\$(mktemp -d) npm run test:ci`);
+    process.exit(1);
+  }
+
+  console.log(
+    `✓ all ${browserTests.length} browser-driven test files are excluded from the CI suite (${ciFiles.size} files collected)`
+  );
+}
+
+if (process.argv[1] && resolve(process.argv[1]) === fileURLToPath(import.meta.url)) {
+  main();
+}
@@ -0,0 +1,172 @@
+/**
+ * @fileoverview Git hook bodies + install policy, shared by scripts/postinstall.js and
+ * pinned by test/git-hooks.test.ts.
+ *
+ * Why a pre-push hook: the static CI job (lockfile, typecheck, lint, format, frontend
+ * syntax, ...) fails often on things a contributor could have caught locally in seconds,
+ * and finding out after a push costs a full CI round-trip plus a fix-up commit. Running
+ * the same checks before the push surfaces those failures in ~15s instead.
+ *
+ * Why pre-PUSH and not pre-commit: a commit is cheap and local, a push is what CI and
+ * reviewers pick up. And why the STATIC tier only: the unit/integration suite takes
+ * minutes, which nobody tolerates per push, so a hook that ran it would be bypassed
+ * within a day. The checks below mirror the static CI job and measured ~15s total.
+ *
+ * ⚠️ This installer is deliberately MARKER-OWNED, unlike the older pre-commit installer in
+ * postinstall.js which overwrites whatever it finds. A developer's own pre-push hook must
+ * survive `npm install`.
+ */
+
+import { execFileSync } from 'node:child_process';
+import { chmodSync, existsSync, mkdirSync, readFileSync, realpathSync, writeFileSync } from 'node:fs';
+import { isAbsolute, join } from 'node:path';
+
+/** Ownership marker. Bump the version suffix when the body changes meaningfully. */
+export const PRE_PUSH_MARKER = '# codeman-managed-hook: pre-push v1';
+
+/**
+ * Checks that make up the fast tier, cheapest first so failures surface sooner. Each entry
+ * is the argument list for `npm run`, and each is a step of the static job in
+ * .github/workflows/ci.yml (test/git-hooks.test.ts pins that every script exists).
+ */
+export const PRE_PUSH_CHECKS = [
+  ['check:lockfile'],
+  ['generate:cli-catalog', '--', '--check'],
+  ['check:browser-excludes'],
+  ['check:frontend-syntax'],
+  ['format:check'],
+  ['lint'],
+  ['typecheck'],
+];
+
+/**
+ * Render the pre-push hook script.
+ *
+ * POSIX sh, not bash: this ships to whatever shell the contributor's git uses.
+ */
+export function renderPrePushHook() {
+  const runs = PRE_PUSH_CHECKS.map((args) => `run_check ${args.join(' ')}`).join('\n');
+
+  return `#!/bin/sh
+${PRE_PUSH_MARKER}
+# Installed by scripts/postinstall.js. Edit scripts/git-hooks.mjs, not this file:
+# it is regenerated on npm install. Delete the marker line above to take ownership
+# and the installer will leave your version alone.
+#
+# Skip once:   CODEMAN_SKIP_PREPUSH=1 git push
+# Skip always: remove this file.
+
+[ "$CODEMAN_SKIP_PREPUSH" = "1" ] && exit 0
+
+repo_root=$(git rev-parse --show-toplevel 2>/dev/null) || exit 0
+cd "$repo_root" || exit 0
+
+# Nothing to check without dependencies (fresh clone, or a worktree that never ran
+# npm install). Warn rather than blocking the push on a setup detail.
+if [ ! -d node_modules ]; then
+  echo "pre-push: node_modules missing, skipping checks (run 'npm install' to enable them)."
+  exit 0
+fi
+
+# git feeds us "<localref> <localsha> <remoteref> <remotesha>" per ref. A deletion has an
+# all-zero local sha and no tree worth checking; if every ref is a deletion, skip.
+has_content=0
+while read -r _localref localsha _remoteref _remotesha; do
+  [ -z "$localsha" ] && continue
+  case "$localsha" in
+    0000000000000000000000000000000000000000) ;;
+    *) has_content=1 ;;
+  esac
+done
+[ "$has_content" = "0" ] && exit 0
+
+log=$(mktemp "\${TMPDIR:-/tmp}/codeman-prepush.XXXXXX") || exit 0
+trap 'rm -f "$log"' EXIT
+
+failed=''
+run_check() {
+  if ! npm run --silent "$@" >"$log" 2>&1; then
+    echo ""
+    echo "pre-push: FAILED  npm run $*"
+    tail -n 25 "$log"
+    failed="$failed $1"
+  fi
+}
+
+echo "pre-push: running static checks (~15s)..."
+${runs}
+
+if [ -n "$failed" ]; then
+  echo ""
+  echo "pre-push: blocked by:$failed"
+  echo "Fix, or push anyway with:  CODEMAN_SKIP_PREPUSH=1 git push"
+  exit 1
+fi
+
+echo "pre-push: static checks passed."
+exit 0
+`;
+}
+
+/**
+ * Decide what to do with an existing hook file.
+ *
+ * @param {{ existing: string | null | undefined, next: string }} args
+ * @returns {'write' | 'up-to-date' | 'skip-foreign'}
+ */
+export function planHookInstall({ existing, next }) {
+  if (existing === null || existing === undefined || existing.trim() === '') return 'write';
+  if (!existing.includes(PRE_PUSH_MARKER)) return 'skip-foreign';
+  return existing === next ? 'up-to-date' : 'write';
+}
+
+/** @param {string} cwd @param {string[]} args */
+function git(cwd, args) {
+  return execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim();
+}
+
+/**
+ * Resolve the hooks directory for the checkout rooted at `repoRoot`, or null when there
+ * is nothing to install into.
+ *
+ * Asks git (`--git-path hooks`) rather than assuming `<root>/.git/hooks`: in a worktree
+ * `.git` is a FILE pointing at the parent repo, and `core.hooksPath` can move it anywhere.
+ *
+ * Returns null unless `repoRoot` is itself the top of a work tree. Without that guard, a
+ * copy of this package sitting inside SOMEONE ELSE's repository (e.g. under their
+ * node_modules) would resolve to their hooks directory and install Codeman's hook there.
+ *
+ * @param {string} repoRoot
+ * @returns {string | null}
+ */
+export function resolveGitHooksDir(repoRoot) {
+  try {
+    const top = git(repoRoot, ['rev-parse', '--show-toplevel']);
+    if (!top || realpathSync(top) !== realpathSync(repoRoot)) return null;
+    const hooks = git(repoRoot, ['rev-parse', '--git-path', 'hooks']);
+    if (!hooks) return null;
+    return isAbsolute(hooks) ? hooks : join(repoRoot, hooks);
+  } catch {
+    return null;
+  }
+}
+
+/**
+ * Install (or refresh) the managed pre-push hook in `hooksDir`, honouring
+ * {@link planHookInstall}: a hook without the marker is never touched.
+ *
+ * @param {string} hooksDir
+ * @returns {'write' | 'up-to-date' | 'skip-foreign'}
+ */
+export function installPrePushHook(hooksDir) {
+  const path = join(hooksDir, 'pre-push');
+  const next = renderPrePushHook();
+  const existing = existsSync(path) ? readFileSync(path, 'utf8') : null;
+  const action = planHookInstall({ existing, next });
+  if (action === 'write') {
+    mkdirSync(hooksDir, { recursive: true });
+    writeFileSync(path, next, { mode: 0o755 });
+    chmodSync(path, 0o755); // `mode` only applies when the file is created
+  }
+  return action;
+}
@@ -356,14 +356,17 @@ if (!isGlobalInstall) {
 }

 // ----------------------------------------------------------------------------
-// 5. Install git pre-commit hook (format check)
+// 5. Install git hooks (pre-commit format check, pre-push static checks)
 // ----------------------------------------------------------------------------

 if (!isGlobalInstall) {
    try {
        const { writeFileSync, mkdirSync } = await import('fs');
-        const gitHooksDir = join(import.meta.dirname, '..', '.git', 'hooks');
-        if (existsSync(join(import.meta.dirname, '..', '.git'))) {
+        const { resolveGitHooksDir, installPrePushHook } = await import('./git-hooks.mjs');
+        // Resolved through git, not `../.git/hooks`: in a worktree `.git` is a file.
+        // null when this directory is not the top of a git checkout.
+        const gitHooksDir = resolveGitHooksDir(join(import.meta.dirname, '..'));
+        if (gitHooksDir) {
            mkdirSync(gitHooksDir, { recursive: true });
            const hook = `#!/bin/bash
 # Auto-installed by postinstall — prevents CI format failures
@@ -379,9 +382,18 @@ fi
            const hookPath = join(gitHooksDir, 'pre-commit');
            writeFileSync(hookPath, hook, { mode: 0o755 });
            console.log(colors.green('✓ Git pre-commit hook installed (prettier check)'));
+
+            // Unlike the pre-commit hook above, this one is marker-owned: a pre-push
+            // hook the developer wrote themselves is left alone.
+            const action = installPrePushHook(gitHooksDir);
+            if (action === 'write') {
+                console.log(colors.green('✓ Git pre-push hook installed') + colors.dim(' (static CI checks, ~15s)'));
+            } else if (action === 'skip-foreign') {
+                console.log(colors.dim('  Existing pre-push hook left untouched (not Codeman-managed)'));
+            }
        }
    } catch {
-        // Non-critical — git hook is a convenience
+        // Non-critical — git hooks are a convenience
    }
 }