Merge pull request #500 from aakhter/pr/prepush-browser-excludes

build: add a browser-test exclusion check and a pre-push static-check hook
2026-09-30 12:39:42 +02:00 · 2026-09-28 16:21:09 +02:00
parent b4954c14cd e60b5a8a2c
commit fec0409315
10 changed files with 951 additions and 7 deletions
@@ -0,0 +1,152 @@
+#!/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.
+ *    ⚠️ Only a DIRECT import is seen: a test that reaches playwright through a
+ *    helper module (e.g. `test/mobile/helpers/browser.ts`) is not detected, so
+ *    such a test still has to be added to `BROWSER_TEST_GLOBS` by hand.
+ *
+ * 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,240 @@
+/**
+ * @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 ~10-40s instead (12s on a fast
+ * workstation, ~35s measured elsewhere; typecheck, format:check and lint dominate).
+ *
+ * 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.
+ *
+ * ⚠️ The checks read the WORKING TREE, not the commits being pushed. So the hook skips
+ * (with a one-line notice) whenever the two can differ: when HEAD is not the commit being
+ * pushed, and when `git status` shows uncommitted or untracked changes in a path a check
+ * reads ({@link PRE_PUSH_WATCHED_PATHS}). In a checkout shared by several agent sessions
+ * the second case is usually another session's WIP, which must not block this push.
+ *
+ * ⚠️ 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 { basename, dirname, join, resolve } 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'],
+];
+
+/**
+ * Paths whose uncommitted state would leak into a check, so a dirty one makes the hook skip.
+ * Derived from what each check reads: src/ (format:check, lint, typecheck,
+ * check:frontend-syntax), config/ (eslint + vitest configs, test-suites.ts, the CLI
+ * catalogue), scripts/ (every check is a script there, and typecheck's second pass compiles
+ * one), test/ (check:browser-excludes scans it and runs `vitest list` over it),
+ * package.json + package-lock.json (check:lockfile) and install.sh (generate:cli-catalog
+ * --check diffs its generated block).
+ */
+export const PRE_PUSH_WATCHED_PATHS = [
+  'src',
+  'config',
+  'scripts',
+  'test',
+  'package.json',
+  'package-lock.json',
+  'install.sh',
+];
+
+/**
+ * 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');
+  const watched = PRE_PUSH_WATCHED_PATHS.join(' ');
+
+  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.
+# The checks below read the working tree, so they only say something about a pushed commit
+# that IS the checked-out HEAD (tags are peeled to their commit first).
+head=$(git rev-parse -q --verify HEAD 2>/dev/null)
+has_content=0
+not_head=''
+while read -r localref localsha _remoteref _remotesha; do
+  [ -z "$localsha" ] && continue
+  case "$localsha" in
+    0000000000000000000000000000000000000000) ;;
+    *)
+      has_content=1
+      commit=$(git rev-parse -q --verify "$localsha^{commit}" 2>/dev/null)
+      [ -n "$head" ] && [ "$commit" = "$head" ] || not_head="$localref"
+      ;;
+  esac
+done
+[ "$has_content" = "0" ] && exit 0
+
+if [ -n "$not_head" ]; then
+  echo "pre-push: skipping static checks: $not_head is not the checked-out HEAD, and the checks read the working tree."
+  exit 0
+fi
+
+# Uncommitted or untracked changes in a path a check reads would be judged instead of the
+# pushed commit. In a checkout shared by several sessions that is usually someone else's WIP.
+if [ -n "$(git --no-optional-locks status --porcelain -- ${watched} 2>/dev/null)" ]; then
+  echo "pre-push: skipping static checks: uncommitted changes under ${watched} would be checked instead of the pushed commit."
+  exit 0
+fi
+
+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 (~10-40s)..."
+${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();
+}
+
+/**
+ * realpath() that tolerates a missing leaf: a fresh `.git` may have no `hooks/` yet, so
+ * canonicalize the parent and re-append the name. Throws if the parent is missing too.
+ *
+ * @param {string} path
+ */
+function canonicalPath(path) {
+  return existsSync(path) ? realpathSync(path) : join(realpathSync(dirname(path)), basename(path));
+}
+
+/**
+ * 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, so the hooks live under
+ * `--git-common-dir`.
+ *
+ * ⚠️ Returns a directory ONLY when it is this repository's own `<git-common-dir>/hooks`.
+ * `--git-path hooks` also reports `core.hooksPath`, and that setting is often GLOBAL (a
+ * shared hooks directory used by every repo on the machine); installing there would
+ * overwrite the user's own hooks and run Codeman's checks on unrelated repos. A
+ * `core.hooksPath` that points back at the repo's own hooks dir still resolves, because
+ * the comparison is on canonical paths rather than on whether the setting exists.
+ *
+ * Also 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;
+    // Both are printed relative to the cwd (repoRoot) unless already absolute.
+    const hooks = git(repoRoot, ['rev-parse', '--git-path', 'hooks']);
+    const common = git(repoRoot, ['rev-parse', '--git-common-dir']);
+    if (!hooks || !common) return null;
+    const own = join(realpathSync(resolve(repoRoot, common)), 'hooks');
+    return canonicalPath(resolve(repoRoot, hooks)) === own ? own : null;
+  } 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, ~10-40s)'));
+            } 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
    }
 }