From 1d85909a0667d6d1c0ac9102403deaeadc96c2e0 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Sat, 26 Sep 2026 18:45:17 -0400 Subject: [PATCH] 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. --- .github/CONTRIBUTING.md | 7 +- .github/workflows/ci.yml | 7 + CLAUDE.md | 4 +- docs/wiki/Contributing.md | 5 + package.json | 1 + scripts/check-browser-test-excludes.mjs | 149 ++++++++++++ scripts/git-hooks.mjs | 172 ++++++++++++++ scripts/postinstall.js | 20 +- test/check-browser-test-excludes.test.ts | 97 ++++++++ test/git-hooks.test.ts | 283 +++++++++++++++++++++++ 10 files changed, 738 insertions(+), 7 deletions(-) create mode 100644 scripts/check-browser-test-excludes.mjs create mode 100644 scripts/git-hooks.mjs create mode 100644 test/check-browser-test-excludes.test.ts create mode 100644 test/git-hooks.test.ts diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 71221862..1468cff6 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -28,12 +28,15 @@ The frontend is plain JS served from `src/web/public/` with no bundler in dev: e CI runs all of these, so save yourself a round trip: ```bash -npm run typecheck # tsc --noEmit, strict mode +npm run typecheck # tsc --noEmit, strict mode npm run lint npm run format:check -npm run check:frontend-syntax # syntax-checks the plain-JS frontend modules +npm run check:frontend-syntax # syntax-checks the plain-JS frontend modules +npm run check:browser-excludes # every browser-driven test is kept out of `npm test` ``` +`npm install` also installs a `pre-push` git hook that runs these static checks (~15s) and blocks the push if one fails. Skip it once with `CODEMAN_SKIP_PREPUSH=1 git push`; it never replaces a `pre-push` hook of your own. + ### Tests ```bash diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8eceb330..fe86f671 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,6 +34,13 @@ jobs: - name: Frontend JS syntax check run: npm run check:frontend-syntax + # Asks `vitest list` what CI would actually collect, rather than matching + # filenames: a browser-driven test missing from BROWSER_TEST_GLOBS + # (config/test-suites.ts) passes locally and dies in the test job with + # "browserType.launch: Executable doesn't exist". + - name: Browser-test exclusion check + run: npm run check:browser-excludes + - name: Format check run: npm run format:check diff --git a/CLAUDE.md b/CLAUDE.md index c4a910cc..a262f074 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,6 +112,8 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph | Gesture playground | `npm run dev` **in** `packages/gesture-control/` (standalone vite demo, fake tabs) | | Check public-asset formatting | `npm run check:public-assets` (prettier-checks `src/web/public/**` text assets; `scripts/check-public-assets.mjs`) | | Frontend JS syntax check | `npm run check:frontend-syntax` (`scripts/check-frontend-syntax.mjs`; runs in CI) | +| Browser-test exclusion check | `npm run check:browser-excludes` (`scripts/check-browser-test-excludes.mjs`; runs in CI, <1s). Fails if a test importing playwright/puppeteer is still collected by `config/vitest.ci.config.ts`; add it to `BROWSER_TEST_GLOBS` in `config/test-suites.ts` | +| Pre-push hook | Installed by `npm install` (`scripts/git-hooks.mjs`, via postinstall): runs the static CI checks (~15s) before `git push`. Skip once: `CODEMAN_SKIP_PREPUSH=1 git push`. Marker-owned, so a hand-written `pre-push` is never overwritten; hooks dir resolved via `git rev-parse --git-path hooks` (worktree-safe) | | Excluded-suite runners | `npm run test:browser` · `npm run test:mobile` · `npm run test:perf` · `npm run test:all` (everything, environmental failures included) — see Testing | | Production start | `npm run start` | | Production logs | `journalctl --user -u codeman-web -f` | @@ -120,7 +122,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph | Dependency doctor | `codeman doctor` (alias `check-deps`; `--json`, `--category core\|office\|other`). Probes Node/Claude CLI/tmux/LibreOffice/MS Office against `config/dependency-registry.ts`; engine is pure given an injectable `ProbeHost` | | Multi-user accounts | `codeman users add ` / `passwd ` / `list` / `rm ` (writes `~/.codeman/users.json`, mode 0600; see Multi-user mode) | -**CI**: `.github/workflows/ci.yml` (push to master/main + PRs, Node 22) runs two jobs: **(1)** `check:lockfile`, `typecheck`, `lint`, `check:frontend-syntax`, `format:check`, then a **server boot smoke test** (`tsx src/index.ts web --port 3151` must answer `/api/status` within 30s); **(2)** the **unit/integration test suite** via `npm run test:ci` (`config/vitest.ci.config.ts` — excludes the browser-driven `test/mobile/**` suite, `perf-*` benchmarks, and 9 Playwright tests; globs live in `config/test-suites.ts`), followed by the **`packages/xterm-zerolag-input` package tests** (a bare `npx vitest run` in that directory; its vitest is hoisted by the root `npm ci`, so no separate install, and `npm test` at the root does NOT run them). `npm test` runs this same config, so local green == CI green. Tests are tmux-safe in CI: `TmuxManager` no-ops all shell commands under `VITEST` (see Testing). A third workflow, `wiki-sync.yml`, fires only on master pushes touching `docs/wiki/**` and mirrors that directory to the GitHub wiki (browser edits to the wiki are overwritten by the next sync, so fix pages via `docs/wiki/`). +**CI**: `.github/workflows/ci.yml` (push to master/main + PRs, Node 22) runs two jobs: **(1)** `check:lockfile`, `typecheck`, `lint`, `check:frontend-syntax`, `check:browser-excludes`, `format:check`, then a **server boot smoke test** (`tsx src/index.ts web --port 3151` must answer `/api/status` within 30s); **(2)** the **unit/integration test suite** via `npm run test:ci` (`config/vitest.ci.config.ts` — excludes the browser-driven `test/mobile/**` suite, `perf-*` benchmarks, and 9 Playwright tests; globs live in `config/test-suites.ts`), followed by the **`packages/xterm-zerolag-input` package tests** (a bare `npx vitest run` in that directory; its vitest is hoisted by the root `npm ci`, so no separate install, and `npm test` at the root does NOT run them). `npm test` runs this same config, so local green == CI green. Tests are tmux-safe in CI: `TmuxManager` no-ops all shell commands under `VITEST` (see Testing). A third workflow, `wiki-sync.yml`, fires only on master pushes touching `docs/wiki/**` and mirrors that directory to the GitHub wiki (browser edits to the wiki are overwritten by the next sync, so fix pages via `docs/wiki/`). **Code style**: Prettier (`singleQuote: true`, `printWidth: 120`, `trailingComma: "es5"`) — config lives in the **`"prettier"` key of `package.json`**, not a `.prettierrc` (keeps the repo root short; editors read it natively). `.prettierignore` stays at the root because Prettier resolves it relative to cwd. ESLint flat config (`config/eslint.config.js`) allows `no-console`, warns on `@typescript-eslint/no-explicit-any`. Ignores: `app.js`, `scripts/**/*.mjs`, `src/web/public/vendor/**`, `scripts/remotion/**`. diff --git a/docs/wiki/Contributing.md b/docs/wiki/Contributing.md index 892e594f..87ce1a0a 100644 --- a/docs/wiki/Contributing.md +++ b/docs/wiki/Contributing.md @@ -42,10 +42,15 @@ npm run typecheck npm run lint npm run format:check npm run check:frontend-syntax +npm run check:browser-excludes npm test -- test/.test.ts # one file, the normal way npm run test:ci # the full CI sweep ``` +`npm install` installs a `pre-push` git hook that runs the static checks above (~15s) and +blocks a push that would fail them. Skip it once with `CODEMAN_SKIP_PREPUSH=1 git push`; a +`pre-push` hook of your own is never overwritten. + **Never run bare `npm test`.** The default configuration includes browser-driven Playwright suites that need a live server, Chromium, and environment-specific baselines; they hang or fail on a normal machine. `test:ci` is the honest "run everything". diff --git a/package.json b/package.json index c51e42cc..0a57e311 100644 --- a/package.json +++ b/package.json @@ -28,6 +28,7 @@ "pretest:mobile": "node scripts/prepare-test-vendor.mjs", "test:mobile": "vitest run --config test/mobile/vitest.config.ts", "check:frontend-syntax": "node scripts/check-frontend-syntax.mjs", + "check:browser-excludes": "node scripts/check-browser-test-excludes.mjs", "fix:node-pty": "node scripts/fix-node-pty.mjs", "typecheck": "tsc --noEmit && tsc -p config/tsconfig.scripts.json", "lint": "eslint --config config/eslint.config.js 'src/**/*.ts'", diff --git a/scripts/check-browser-test-excludes.mjs b/scripts/check-browser-test-excludes.mjs new file mode 100644 index 00000000..209160ce --- /dev/null +++ b/scripts/check-browser-test-excludes.mjs @@ -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 `/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} + */ +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} 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(); +} diff --git a/scripts/git-hooks.mjs b/scripts/git-hooks.mjs new file mode 100644 index 00000000..ba2ade8e --- /dev/null +++ b/scripts/git-hooks.mjs @@ -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 " " 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 `/.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; +} diff --git a/scripts/postinstall.js b/scripts/postinstall.js index 02d23f59..6878d9f6 100644 --- a/scripts/postinstall.js +++ b/scripts/postinstall.js @@ -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 } } diff --git a/test/check-browser-test-excludes.test.ts b/test/check-browser-test-excludes.test.ts new file mode 100644 index 00000000..7dc34ad8 --- /dev/null +++ b/test/check-browser-test-excludes.test.ts @@ -0,0 +1,97 @@ +/** + * @fileoverview scripts/check-browser-test-excludes.mjs: the detection side (which test + * files need a real browser) and the leak computation. The exclusion side is vitest's own + * `vitest list`, which `npm run check:browser-excludes` exercises for real in CI. + */ + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { + findBrowserTests, + findLeaks, + importsBrowserDriver, + parseVitestFileList, +} from '../scripts/check-browser-test-excludes.mjs'; +import { BROWSER_TEST_GLOBS } from '../config/test-suites'; + +const repoRoot = resolve(import.meta.dirname, '..'); + +// Fixture sources are assembled from the module name at runtime, so THIS file never contains +// a literal driver import and is not itself flagged by the checker it tests. +const fromImport = (mod: string) => `import { chromium, type Browser } from '${mod}';\n`; + +describe('importsBrowserDriver', () => { + it.each([ + fromImport('playwright'), + fromImport('playwright-core').replace(/'/g, '"'), + fromImport('@playwright/test'), + fromImport('puppeteer'), + `import type { Page } from '${'playwright'}';`, + `const { chromium } = require('${'playwright'}');`, + `const pw = await import('${'playwright'}');`, + ])('flags %s', (src) => { + expect(importsBrowserDriver(src)).toBe(true); + }); + + it.each([ + "import { describe } from 'vitest';", + "// needs ms-playwright's cache dir\nconst dir = '.cache/ms-playwright';", + fromImport('./playwright-helpers'), + fromImport('playwright-extra-thing'), + ])('ignores %s', (src) => { + expect(importsBrowserDriver(src)).toBe(false); + }); +}); + +describe('findBrowserTests (fixture tree)', () => { + let root: string; + beforeAll(() => { + root = mkdtempSync(join(tmpdir(), 'codeman-browser-excludes-')); + const put = (rel: string, src: string) => { + mkdirSync(join(root, rel, '..'), { recursive: true }); + writeFileSync(join(root, rel), src); + }; + put('test/unit.test.ts', "import { it } from 'vitest';\n"); + put('test/legacy-name.test.ts', fromImport('playwright')); + put('test/new.browser.test.ts', fromImport('playwright')); + put('test/nested/deep.test.ts', fromImport('puppeteer')); + put('test/helpers/browser.ts', fromImport('playwright')); // not a test file + }); + afterAll(() => rmSync(root, { recursive: true, force: true })); + + it('finds driver imports by content, recursively, as sorted repo-relative paths', () => { + expect(findBrowserTests(root)).toEqual([ + 'test/legacy-name.test.ts', + 'test/nested/deep.test.ts', + 'test/new.browser.test.ts', + ]); + }); +}); + +describe('parseVitestFileList + findLeaks', () => { + it('keeps only test paths and normalizes a leading ./', () => { + const out = '\n./test/a.test.ts\ntest/b.test.ts\nsome banner line\n test/c.test.ts \n'; + expect([...parseVitestFileList(out)].sort()).toEqual(['test/a.test.ts', 'test/b.test.ts', 'test/c.test.ts']); + }); + + it('reports exactly the browser tests the CI set still collects', () => { + const ci = new Set(['test/unit.test.ts', 'test/legacy-name.test.ts']); + expect(findLeaks(['test/legacy-name.test.ts', 'test/new.browser.test.ts'], ci)).toEqual([ + 'test/legacy-name.test.ts', + ]); + expect(findLeaks(['test/new.browser.test.ts'], ci)).toEqual([]); + }); +}); + +describe('against this repository', () => { + it('detects every file already listed in BROWSER_TEST_GLOBS', () => { + // If detection stopped recognising a known browser test, the checker would go blind to + // exactly the class of file it exists for. + const detected = new Set(findBrowserTests(repoRoot)); + const literals = BROWSER_TEST_GLOBS.filter((g) => !/[*?[{]/.test(g)); + expect(literals.length).toBeGreaterThan(0); + for (const file of literals) expect(detected, file).toContain(file); + }); +}); diff --git a/test/git-hooks.test.ts b/test/git-hooks.test.ts new file mode 100644 index 00000000..04caf9a7 --- /dev/null +++ b/test/git-hooks.test.ts @@ -0,0 +1,283 @@ +/** + * @fileoverview The pre-push hook that scripts/postinstall.js installs (scripts/git-hooks.mjs). + * + * Two properties matter more than the hook's contents, because the older pre-commit + * installer gets both wrong and this one must not copy it: + * 1. It is MARKER-OWNED: a hook the developer wrote by hand is never overwritten. + * 2. The hooks directory is resolved via `git rev-parse --git-path hooks`, since in a + * worktree `.git` is a FILE and `/.git/hooks` does not exist. + * + * ⚠️ Every filesystem/git test here runs against THROWAWAY repositories under a temp dir. + * Never point the installer at this checkout: its hooks directory is shared with every + * worktree of it, including whatever the developer is running right now. + */ + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { + chmodSync, + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + rmSync, + statSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { + PRE_PUSH_CHECKS, + PRE_PUSH_MARKER, + installPrePushHook, + planHookInstall, + renderPrePushHook, + resolveGitHooksDir, +} from '../scripts/git-hooks.mjs'; + +const repoRoot = resolve(import.meta.dirname, '..'); +const read = (rel: string) => readFileSync(resolve(repoRoot, rel), 'utf8'); + +/** git with no user/system config leaking in (a global core.hooksPath would redirect everything). */ +const GIT_ENV = { + ...process.env, + GIT_CONFIG_NOSYSTEM: '1', + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_AUTHOR_NAME: 'test', + GIT_AUTHOR_EMAIL: 'test@example.invalid', + GIT_COMMITTER_NAME: 'test', + GIT_COMMITTER_EMAIL: 'test@example.invalid', + CODEMAN_SKIP_PREPUSH: '', +}; + +function git(cwd: string, args: string[], env: NodeJS.ProcessEnv = GIT_ENV): string { + return execFileSync('git', args, { cwd, env, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim(); +} + +let scratch: string; +beforeAll(() => { + scratch = realpathSync(mkdtempSync(join(tmpdir(), 'codeman-git-hooks-'))); +}); +afterAll(() => { + rmSync(scratch, { recursive: true, force: true }); +}); + +let counter = 0; +function newRepo(): string { + const dir = join(scratch, `repo-${++counter}`); + mkdirSync(dir, { recursive: true }); + git(dir, ['init', '-q', '-b', 'main']); + git(dir, ['commit', '-q', '--allow-empty', '-m', 'init']); + return dir; +} + +describe('pre-push hook body', () => { + const hook = renderPrePushHook(); + + it('carries the ownership marker', () => { + expect(hook).toContain(PRE_PUSH_MARKER); + }); + + it('runs every configured check through npm, and nothing slow', () => { + for (const args of PRE_PUSH_CHECKS) { + expect(hook).toContain(`run_check ${args.join(' ')}`); + } + expect(hook).toContain('npm run --silent "$@"'); + // The whole point of the tier: the minutes-long suites stay out of a per-push hook. + expect(hook).not.toMatch(/\btest:(ci|browser|mobile|perf|all)\b/); + }); + + it('is POSIX sh', () => { + expect(hook.startsWith('#!/bin/sh\n')).toBe(true); + const r = spawnSync('sh', ['-n'], { input: hook }); + expect(r.status).toBe(0); + }); +}); + +describe('pre-push checks match the static CI job', () => { + const scripts = JSON.parse(read('package.json')).scripts as Record; + const ci = read('.github/workflows/ci.yml'); + + it.each(PRE_PUSH_CHECKS.map((args) => [args.join(' ')] as const))('%s is a real script that CI runs', (joined) => { + const [name] = joined.split(' '); + expect(scripts[name], `package.json has no "${name}" script`).toBeTypeOf('string'); + expect(ci).toContain(`npm run ${joined}`); + }); +}); + +describe('planHookInstall', () => { + const hook = renderPrePushHook(); + + it('writes when no hook exists', () => { + expect(planHookInstall({ existing: null, next: hook })).toBe('write'); + }); + + it('refuses to clobber a hook it does not own', () => { + expect(planHookInstall({ existing: '#!/bin/sh\nmake lint\n', next: hook })).toBe('skip-foreign'); + }); + + it('refreshes its own hook when the body changed', () => { + expect(planHookInstall({ existing: `#!/bin/sh\n${PRE_PUSH_MARKER}\necho old\n`, next: hook })).toBe('write'); + }); + + it('is idempotent when already current', () => { + expect(planHookInstall({ existing: hook, next: hook })).toBe('up-to-date'); + }); + + it('treats an empty file as absent rather than foreign', () => { + expect(planHookInstall({ existing: ' \n', next: hook })).toBe('write'); + }); +}); + +describe('resolveGitHooksDir (temp repos)', () => { + it('resolves /.git/hooks in a plain checkout', () => { + const repo = newRepo(); + expect(resolveGitHooksDir(repo)).toBe(join(repo, '.git', 'hooks')); + }); + + it('resolves the SHARED hooks dir from a worktree, where .git is a file', () => { + const repo = newRepo(); + const wt = join(scratch, `wt-${counter}`); + git(repo, ['worktree', 'add', '-q', wt, '-b', 'wt-branch']); + expect(statSync(join(wt, '.git')).isFile()).toBe(true); + expect(resolveGitHooksDir(wt)).toBe(join(repo, '.git', 'hooks')); + }); + + it('returns null outside any git checkout', () => { + const dir = join(scratch, `plain-${++counter}`); + mkdirSync(dir); + expect(resolveGitHooksDir(dir)).toBeNull(); + }); + + it("returns null for a copy nested inside someone else's repo (e.g. under node_modules)", () => { + const repo = newRepo(); + const nested = join(repo, 'node_modules', 'aicodeman'); + mkdirSync(nested, { recursive: true }); + expect(resolveGitHooksDir(nested)).toBeNull(); + }); +}); + +describe('installPrePushHook (temp repos)', () => { + it('writes an executable hook into a fresh repo', () => { + const hooks = join(newRepo(), '.git', 'hooks'); + expect(installPrePushHook(hooks)).toBe('write'); + const path = join(hooks, 'pre-push'); + expect(readFileSync(path, 'utf8')).toBe(renderPrePushHook()); + expect(statSync(path).mode & 0o111).not.toBe(0); + expect(installPrePushHook(hooks)).toBe('up-to-date'); + }); + + it('leaves a foreign pre-push hook byte-identical', () => { + const hooks = join(newRepo(), '.git', 'hooks'); + const path = join(hooks, 'pre-push'); + const mine = '#!/bin/sh\n# my own hook\nexit 0\n'; + writeFileSync(path, mine, { mode: 0o755 }); + expect(installPrePushHook(hooks)).toBe('skip-foreign'); + expect(readFileSync(path, 'utf8')).toBe(mine); + }); + + it('refreshes a stale managed hook and keeps it executable', () => { + const hooks = join(newRepo(), '.git', 'hooks'); + const path = join(hooks, 'pre-push'); + writeFileSync(path, `#!/bin/sh\n${PRE_PUSH_MARKER}\necho old\n`, { mode: 0o644 }); + expect(installPrePushHook(hooks)).toBe('write'); + expect(readFileSync(path, 'utf8')).toBe(renderPrePushHook()); + expect(statSync(path).mode & 0o111).not.toBe(0); + }); +}); + +/** + * Drive the rendered hook through a real `git push` to a local bare remote. The repo gets a + * stub package.json whose check scripts only record that they ran, so this exercises the + * hook's control flow (ref parsing, skips, blocking) without running the real checks. + */ +describe('the installed hook on a real push (temp repos)', () => { + function setup(opts: { failing?: string; nodeModules?: boolean } = {}) { + const repo = newRepo(); + const remote = join(scratch, `remote-${counter}.git`); + git(scratch, ['init', '-q', '--bare', remote]); + git(repo, ['remote', 'add', 'origin', remote]); + const log = join(repo, 'ran.log'); + const scripts: Record = {}; + for (const [name] of PRE_PUSH_CHECKS) { + scripts[name] = + name === opts.failing ? `echo ${name} >> ran.log && echo boom-${name} && exit 1` : `echo ${name} >> ran.log`; + } + writeFileSync(join(repo, 'package.json'), JSON.stringify({ name: 'hook-fixture', private: true, scripts })); + writeFileSync(join(repo, '.gitignore'), 'node_modules/\nran.log\n'); + git(repo, ['add', 'package.json', '.gitignore']); + git(repo, ['commit', '-q', '-m', 'fixture']); + if (opts.nodeModules !== false) mkdirSync(join(repo, 'node_modules')); + installPrePushHook(join(repo, '.git', 'hooks')); + chmodSync(join(repo, '.git', 'hooks', 'pre-push'), 0o755); + const ran = () => { + try { + return readFileSync(log, 'utf8').trim().split('\n').filter(Boolean); + } catch { + return []; + } + }; + const push = (args: string[], env: NodeJS.ProcessEnv = {}) => + spawnSync('git', ['push', ...args], { cwd: repo, env: { ...GIT_ENV, ...env }, encoding: 'utf8' }); + return { repo, remote, ran, push }; + } + + /** What the stubs record: npm appends the args after `--` to the script, so they prove forwarding. */ + const expectedRuns = PRE_PUSH_CHECKS.map((args) => args.filter((a) => a !== '--').join(' ')); + + it('runs every check before a push, in order', () => { + const { ran, push } = setup(); + const r = push(['-q', 'origin', 'main']); + expect(r.status, r.stderr + r.stdout).toBe(0); + expect(ran()).toEqual(expectedRuns); + }); + + it('blocks the push when a check fails, but still runs the rest', () => { + const { ran, push, remote } = setup({ failing: 'lint' }); + const r = push(['origin', 'main']); + expect(r.status).not.toBe(0); + expect(r.stdout + r.stderr).toContain('pre-push: FAILED npm run lint'); + expect(r.stdout + r.stderr).toContain('boom-lint'); + expect(ran()).toEqual(expectedRuns); + expect(spawnSync('git', ['rev-parse', '--verify', '-q', 'refs/heads/main'], { cwd: remote }).status).not.toBe(0); + }); + + it('CODEMAN_SKIP_PREPUSH=1 skips every check', () => { + const { ran, push } = setup({ failing: 'lint' }); + const r = push(['-q', 'origin', 'main'], { CODEMAN_SKIP_PREPUSH: '1' }); + expect(r.status, r.stderr).toBe(0); + expect(ran()).toEqual([]); + }); + + it('a delete-only push skips the checks', () => { + const { ran, push, repo } = setup({ failing: 'lint' }); + expect(push(['-q', 'origin', 'main'], { CODEMAN_SKIP_PREPUSH: '1' }).status).toBe(0); + git(repo, ['branch', 'doomed']); + expect(push(['-q', 'origin', 'doomed'], { CODEMAN_SKIP_PREPUSH: '1' }).status).toBe(0); + const r = push(['-q', 'origin', '--delete', 'doomed']); + expect(r.status, r.stderr).toBe(0); + expect(ran()).toEqual([]); + }); + + it('skips (never blocks) when node_modules is absent', () => { + const { ran, push } = setup({ failing: 'lint', nodeModules: false }); + const r = push(['origin', 'main']); + expect(r.status, r.stderr).toBe(0); + expect(r.stdout + r.stderr).toContain('node_modules missing'); + expect(ran()).toEqual([]); + }); +}); + +describe('postinstall wiring', () => { + const postinstall = read('scripts/postinstall.js'); + + it('installs the pre-push hook through the shared module', () => { + expect(postinstall).toContain("import('./git-hooks.mjs')"); + expect(postinstall).toContain('installPrePushHook(gitHooksDir)'); + }); + + it('resolves the hooks dir through git, so worktrees work', () => { + expect(postinstall).toContain('resolveGitHooksDir('); + expect(postinstall).not.toContain("join(import.meta.dirname, '..', '.git', 'hooks')"); + }); +});