From dfd3df82891426b6a5ac3c42caf4cc4245c81e33 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Mon, 28 Sep 2026 16:28:20 +0200 Subject: [PATCH] fix(build): merge-time fixes for #500 - pre-push hook: skip with a notice when npm is not on PATH (GUI git clients and IDEs often run hooks with a minimal PATH), instead of blocking every push on "npm: not found"; real-push test with a stripped PATH - test/git-hooks.test.ts: pin GIT_CONFIG_NOSYSTEM=1 and GIT_CONFIG_GLOBAL=/dev/null around the resolveGitHooksDir tests, so an exported global or a system core.hooksPath no longer fails them - watch tsconfig.json, .prettierignore and .editorconfig too: typecheck and format:check read them - check:browser-excludes: fail loudly when the vitest list output and the walked test/**/*.test.ts tree share no path (format drift would otherwise pass vacuously) - Reword the PRE_PUSH_MARKER comment: bumping its version would make every installed v1 hook read as foreign and never refresh again. Co-Authored-By: Claude Opus 5.5 (1M context) --- scripts/check-browser-test-excludes.mjs | 45 +++++++++++++-- scripts/git-hooks.mjs | 19 ++++++- test/check-browser-test-excludes.test.ts | 24 ++++++++ test/git-hooks.test.ts | 70 ++++++++++++++++++++---- 4 files changed, 137 insertions(+), 21 deletions(-) diff --git a/scripts/check-browser-test-excludes.mjs b/scripts/check-browser-test-excludes.mjs index 3d3b6233..44617f53 100644 --- a/scripts/check-browser-test-excludes.mjs +++ b/scripts/check-browser-test-excludes.mjs @@ -63,17 +63,26 @@ function walk(dir) { } /** - * Every `*.test.ts` under `/test` that imports a browser driver, as sorted - * repo-relative POSIX paths (the form `vitest list` prints). + * Every `*.test.ts` under `/test`, as sorted repo-relative POSIX paths (the form + * `vitest list` prints). + * + * @param {string} root + * @returns {string[]} + */ +export function findTestFiles(root) { + return walk(join(root, 'test')) + .map((file) => relative(root, file).split(sep).join('/')) + .sort(); +} + +/** + * The subset of {@link findTestFiles} that imports a browser driver. * * @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(); + return findTestFiles(root).filter((file) => importsBrowserDriver(readFileSync(join(root, file), 'utf8'))); } /** @@ -93,6 +102,19 @@ export function parseVitestFileList(output) { ); } +/** + * Whether the `vitest list` paths and the walked tree name at least one file in common. + * False means the two sides are not speaking the same path format (absolute paths, backslashes + * or a new prefix after a vitest upgrade), and then {@link findLeaks} would find nothing + * against a perfectly non-empty listing. + * + * @param {Set} ciFiles + * @param {string[]} testFiles + */ +export function listingMatchesTree(ciFiles, testFiles) { + return testFiles.some((file) => ciFiles.has(file)); +} + /** * @param {string[]} browserTests * @param {Set} ciFiles @@ -103,6 +125,7 @@ export function findLeaks(browserTests, ciFiles) { } function main() { + const testFiles = findTestFiles(ROOT); const browserTests = findBrowserTests(ROOT); let collected; @@ -124,6 +147,16 @@ function main() { console.error('✗ `vitest list` reported no test files; refusing to pass on an empty CI set.'); process.exit(1); } + // Same vacuous pass, one step removed: a listing whose paths never match the tree. This guard, + // not `vitest list --json`, is the answer to format drift: the JSON form prints absolute paths + // that would need canonicalizing against ROOT (symlinked checkouts), and its shape can drift too. + if (!listingMatchesTree(ciFiles, testFiles)) { + const sample = [...ciFiles].slice(0, 3).join(', '); + console.error( + `✗ none of the ${ciFiles.size} paths \`vitest list\` reported (e.g. ${sample}) is one of the ${testFiles.length} test/**/*.test.ts files; its output format has probably changed.` + ); + process.exit(1); + } const leaked = findLeaks(browserTests, ciFiles); diff --git a/scripts/git-hooks.mjs b/scripts/git-hooks.mjs index a0b5770d..4727a5fe 100644 --- a/scripts/git-hooks.mjs +++ b/scripts/git-hooks.mjs @@ -28,7 +28,11 @@ 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. */ +/** + * Ownership marker. ⚠️ Never bump the version suffix: ownership is matched on this exact + * string, so a `v2` would read every installed `v1` hook as foreign and never refresh it. + * A changed body still reaches installed hooks, because the refresh compares the whole file. + */ export const PRE_PUSH_MARKER = '# codeman-managed-hook: pre-push v1'; /** @@ -52,8 +56,10 @@ export const PRE_PUSH_CHECKS = [ * 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). + * package.json + package-lock.json (check:lockfile), install.sh (generate:cli-catalog + * --check diffs its generated block), tsconfig.json (typecheck, and + * config/tsconfig.scripts.json extends it) and .prettierignore + .editorconfig + * (format:check; the Prettier CLI honours .editorconfig by default). */ export const PRE_PUSH_WATCHED_PATHS = [ 'src', @@ -63,6 +69,9 @@ export const PRE_PUSH_WATCHED_PATHS = [ 'package.json', 'package-lock.json', 'install.sh', + 'tsconfig.json', + '.prettierignore', + '.editorconfig', ]; /** @@ -95,6 +104,10 @@ if [ ! -d node_modules ]; then exit 0 fi +# GUI git clients and IDEs often run hooks with a minimal PATH that lacks an nvm or +# Homebrew Node. Every check would then fail with "npm: not found", so skip instead. +command -v npm >/dev/null 2>&1 || { echo "pre-push: npm not on PATH, skipping checks."; exit 0; } + # 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. # The checks below read the working tree, so they only say something about a pushed commit diff --git a/test/check-browser-test-excludes.test.ts b/test/check-browser-test-excludes.test.ts index 7dc34ad8..de522068 100644 --- a/test/check-browser-test-excludes.test.ts +++ b/test/check-browser-test-excludes.test.ts @@ -11,7 +11,9 @@ import { join, resolve } from 'node:path'; import { findBrowserTests, findLeaks, + findTestFiles, importsBrowserDriver, + listingMatchesTree, parseVitestFileList, } from '../scripts/check-browser-test-excludes.mjs'; import { BROWSER_TEST_GLOBS } from '../config/test-suites'; @@ -68,6 +70,15 @@ describe('findBrowserTests (fixture tree)', () => { 'test/new.browser.test.ts', ]); }); + + it('lists every test file, browser-driven or not, in the same form', () => { + expect(findTestFiles(root)).toEqual([ + 'test/legacy-name.test.ts', + 'test/nested/deep.test.ts', + 'test/new.browser.test.ts', + 'test/unit.test.ts', + ]); + }); }); describe('parseVitestFileList + findLeaks', () => { @@ -83,6 +94,19 @@ describe('parseVitestFileList + findLeaks', () => { ]); expect(findLeaks(['test/new.browser.test.ts'], ci)).toEqual([]); }); + + it('flags a non-empty listing whose paths never match the tree instead of passing vacuously', () => { + const tree = ['test/legacy-name.test.ts', 'test/unit.test.ts']; + // e.g. a vitest upgrade that starts printing absolute paths: nothing leaks, but only + // because nothing matches, so the checker must refuse rather than report success. + const drifted = parseVitestFileList('/repo/test/legacy-name.test.ts\n/repo/test/unit.test.ts\n'); + expect(drifted.size).toBe(2); + expect(findLeaks(['test/legacy-name.test.ts'], drifted)).toEqual([]); + expect(listingMatchesTree(drifted, tree)).toBe(false); + + const healthy = parseVitestFileList('test/legacy-name.test.ts\ntest/unit.test.ts\n'); + expect(listingMatchesTree(healthy, tree)).toBe(true); + }); }); describe('against this repository', () => { diff --git a/test/git-hooks.test.ts b/test/git-hooks.test.ts index 8ba70e87..3e00bc1e 100644 --- a/test/git-hooks.test.ts +++ b/test/git-hooks.test.ts @@ -24,6 +24,7 @@ import { realpathSync, rmSync, statSync, + symlinkSync, writeFileSync, } from 'node:fs'; import { tmpdir } from 'node:os'; @@ -133,6 +134,24 @@ describe('planHookInstall', () => { }); describe('resolveGitHooksDir (temp repos)', () => { + // resolveGitHooksDir runs git with process.env, so an exported GIT_CONFIG_GLOBAL or a system + // gitconfig carrying core.hooksPath would otherwise redirect every expectation below. + // test/setup.ts swaps HOME, which only covers ~/.gitconfig. + const ambient = { + GIT_CONFIG_GLOBAL: process.env.GIT_CONFIG_GLOBAL, + GIT_CONFIG_NOSYSTEM: process.env.GIT_CONFIG_NOSYSTEM, + }; + beforeAll(() => { + process.env.GIT_CONFIG_NOSYSTEM = '1'; + process.env.GIT_CONFIG_GLOBAL = '/dev/null'; + }); + afterAll(() => { + for (const [k, v] of Object.entries(ambient)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + }); + it('resolves /.git/hooks in a plain checkout', () => { const repo = newRepo(); expect(resolveGitHooksDir(repo)).toBe(join(repo, '.git', 'hooks')); @@ -352,18 +371,24 @@ describe('the installed hook on a real push (temp repos)', () => { expect(ran()).toEqual(expectedRuns); }); - it.each(['src/wip.ts', 'config/wip.json', 'scripts/wip.mjs', 'test/wip.test.ts', 'install.sh'])( - 'skips when %s is untracked (another session may own it)', - (rel) => { - const { ran, push, repo } = setup({ failing: 'lint' }); - mkdirSync(join(repo, rel, '..'), { recursive: true }); - writeFileSync(join(repo, rel), 'wip\n'); - const r = push(['origin', 'main']); - expect(r.status, r.stderr).toBe(0); - expect(r.stdout + r.stderr).toContain('pre-push: skipping static checks: uncommitted changes under'); - expect(ran()).toEqual([]); - } - ); + it.each([ + 'src/wip.ts', + 'config/wip.json', + 'scripts/wip.mjs', + 'test/wip.test.ts', + 'install.sh', + 'tsconfig.json', + '.prettierignore', + '.editorconfig', + ])('skips when %s is untracked (another session may own it)', (rel) => { + const { ran, push, repo } = setup({ failing: 'lint' }); + mkdirSync(join(repo, rel, '..'), { recursive: true }); + writeFileSync(join(repo, rel), 'wip\n'); + const r = push(['origin', 'main']); + expect(r.status, r.stderr).toBe(0); + expect(r.stdout + r.stderr).toContain('pre-push: skipping static checks: uncommitted changes under'); + expect(ran()).toEqual([]); + }); it('skips when a tracked package.json has an unstaged edit', () => { const { ran, push, repo } = setup({ failing: 'lint' }); @@ -395,6 +420,9 @@ describe('the installed hook on a real push (temp repos)', () => { 'package.json', 'package-lock.json', 'install.sh', + 'tsconfig.json', + '.prettierignore', + '.editorconfig', ]); }); @@ -405,6 +433,24 @@ describe('the installed hook on a real push (temp repos)', () => { expect(r.stdout + r.stderr).toContain('node_modules missing'); expect(ran()).toEqual([]); }); + + it('skips (never blocks) when npm is not on PATH, as under a GUI git client', () => { + const { ran, push } = setup({ failing: 'lint' }); + // A PATH holding only what git and the hook need, and no npm/node. Symlinks rather than + // the real directories, since /usr/bin usually holds npm right next to git. + const bin = join(scratch, `bin-${counter}`); + mkdirSync(bin); + for (const tool of ['git', 'sh', 'mktemp', 'tail', 'rm', 'cat']) { + const found = spawnSync('sh', ['-c', `command -v ${tool}`], { encoding: 'utf8' }).stdout.trim(); + expect(found, `${tool} not found on the test PATH`).toMatch(/^\//); + symlinkSync(found, join(bin, tool)); + } + expect(spawnSync('sh', ['-c', 'command -v npm'], { env: { PATH: bin } }).status).not.toBe(0); + const r = push(['origin', 'main'], { PATH: bin }); + expect(r.status, r.stderr + r.stdout).toBe(0); + expect(r.stdout + r.stderr).toContain('pre-push: npm not on PATH, skipping checks.'); + expect(ran()).toEqual([]); + }); }); describe('postinstall wiring', () => {