Skip to content

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

Merged
Ark0N merged 2 commits into
Ark0N:masterfrom
aakhter:pr/prepush-browser-excludes
Sep 28, 2026
Merged

Ark0N merged 2 commits into
Ark0N:masterfrom
aakhter:pr/prepush-browser-excludes

Conversation

@aakhter

@aakhter aakhter commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

What

Two small pieces of developer tooling. Neither changes runtime behaviour.

1. npm run check:browser-excludes (new CI step in the static job)

npm test / test:ci must never collect a test that drives a real browser: a runner with no chromium dies with browserType.launch: Executable doesn't exist. The exclusions live in BROWSER_TEST_GLOBS (config/test-suites.ts), which is maintained by hand, so a new browser test only gets excluded if someone remembers. When they don't, the test passes on every machine that has run npx playwright install and fails only on a clean runner.

The checker closes that gap:

  • Detection is by content, not filename. It flags any test importing playwright, @playwright/test, playwright-core, puppeteer or puppeteer-core. Several existing browser tests predate the *.browser.test.ts convention, so a filename match would miss them.
  • Exclusion is answered by vitest itself (vitest list --config config/vitest.ci.config.ts --filesOnly), not by re-implementing glob matching, so it cannot disagree with what CI actually collects. An empty collection is treated as a failure rather than a vacuous pass.

On current master it passes: 23 browser-driven files, all excluded, 435 files collected.

2. A pre-push hook installed by npm install

postinstall.js now also installs a pre-push hook that runs the static CI checks before a push: check:lockfile, generate:cli-catalog --check, check:browser-excludes, check:frontend-syntax, format:check, lint, typecheck. That takes about 15s, and failures surface locally instead of after a CI round-trip. It deliberately skips the test suites, which take minutes.

  • Marker-owned: a pre-push hook without the # codeman-managed-hook marker is left alone, so a contributor's own hook survives npm install. (The existing pre-commit installer overwrites; that is unchanged.)
  • The hooks dir is resolved with git rev-parse --git-path hooks, so it works in a worktree, where .git is a file. The pre-commit installer now uses the same resolution.
  • It does nothing when the package is not the top of a git checkout (a registry install, or a copy nested under another project).
  • The hook skips rather than blocks when node_modules is missing or the push is a branch deletion. CODEMAN_SKIP_PREPUSH=1 bypasses it.
  • An install failure never fails npm install.

Docs: CLAUDE.md (commands table + CI note), .github/CONTRIBUTING.md, docs/wiki/Contributing.md.

Testing

  • test/check-browser-test-excludes.test.ts (15) and test/git-hooks.test.ts (29). The installer tests use throwaway git init repos, never the checkout they run in. Both files are collected by the CI config.
  • Mutation-checked:
    • Removing a glob from BROWSER_TEST_GLOBS makes the checker fail and name that file.
    • Removing the marker check, the deletion skip, the nested-repo guard or the CODEMAN_SKIP_PREPUSH bypass each fails the hook tests.
  • End-to-end in a throwaway clone:
    • A real push ran the checks (~15s) and passed.
    • A badly formatted file was blocked by format:check.
    • CODEMAN_SKIP_PREPUSH=1 and a deletion push both skipped.
  • typecheck (including config/tsconfig.scripts.json), lint, format:check, check:frontend-syntax and check:lockfile are clean. This branch's own push went through the installed hook.

… 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.
@Ark0N

Ark0N commented Sep 26, 2026

Copy link
Copy Markdown
Owner

Thanks @aakhter, this is a nice piece of tooling: a CI check that asks vitest list whether any playwright/puppeteer test is still collected by npm test, plus a pre-push hook that runs the static CI checks locally. The checker is solid (I mutation-tested it by dropping a glob and it named the file), and the marker-owned install and the worktree-aware resolution are the right ideas. There is one thing that has to change before this can merge, and one design point I would like handled.

1. The hooks dir can resolve outside the repo (must fix)

scripts/git-hooks.mjs:146: git rev-parse --git-path hooks returns core.hooksPath when it is set, including a global one. On such a machine postinstall.js:368-383 overwrites the user's global pre-commit and postinstall.js:388 installs the pre-push hook globally. I reproduced it: a global pre-commit was replaced, and git push in an unrelated repo with a package.json and node_modules was blocked by all seven checks. Before this PR the pre-commit went to .git/hooks, which git ignores under core.hooksPath, so it was harmless. It also reaches end users, because install.sh and scripts/self-update.sh run npm install in the ~/.codeman/app clone.

Please only return a hooks dir that is the repo's own: realpath(--git-path hooks) must equal realpath(--git-common-dir) + /hooks, otherwise return null. Please do not just skip whenever core.hooksPath is set: my checkout sets core.hooksPath locally to its own absolute .git/hooks, and the path comparison keeps that case working. A test with a repo-local git config core.hooksPath <outside dir> (expect null) and one pointing at the repo's own .git/hooks (expect it resolved) would pin both.

2. The hook checks the working tree, not the pushed commit

scripts/git-hooks.mjs:61-97: format:check, lint and typecheck run over whatever is on disk, untracked files included. My checkout is shared by several agent sessions at once (see Session Safety in CLAUDE.md), so a push would regularly be blocked by another session's work in progress that the pusher never touched. Could the hook skip with a one-line notice when HEAD is not the sha being pushed or git status --porcelain shows changes under src/, config/, scripts/ or package*.json? Please also add a line to the Session Safety section of CLAUDE.md saying that a pre-push failure in a file you did not touch is another session's WIP: push with CODEMAN_SKIP_PREPUSH=1 and leave it alone.

Small things (I can do these at merge time if you prefer)

  • The "~15s" figure: here it came to about 35s (typecheck 13s, format:check 10s, lint 8s). It appears in scripts/git-hooks.mjs:96, postinstall.js:390, CLAUDE.md:116, .github/CONTRIBUTING.md:38 and docs/wiki/Contributing.md:50.
  • CLAUDE.md:125 still says "9 Playwright tests"; BROWSER_TEST_GLOBS lists 14.
  • A sentence in the checker's @fileoverview noting that a test reaching playwright through a helper (for example test/mobile/helpers/browser.ts) is not detected.

Once 1 and 2 are in, I will merge. If you would rather land the checker first, splitting it into its own PR works too and I can merge that part straight away.

resolveGitHooksDir now returns a directory only when it is the repo's own
<git-common-dir>/hooks (compared on canonical paths), so a core.hooksPath
elsewhere, global or repo-local, is never written to by postinstall, while a
core.hooksPath pointing back at the repo's own .git/hooks still resolves.

The pre-push hook skips with a one-line notice when a pushed ref is not the
checked-out HEAD (tags peeled) or when git status shows uncommitted or
untracked changes under a path the checks read (src, config, scripts, test,
package.json, package-lock.json, install.sh), since the checks read the
working tree rather than the pushed commit.

Also: honest timing (~10-40s instead of ~15s), CLAUDE.md Session Safety note
on CODEMAN_SKIP_PREPUSH for another session's WIP, 14 (not 9) Playwright
tests, and a note that the browser-excludes check only sees direct imports.
@aakhter

aakhter commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review and the repro.

  1. Fixed. resolveGitHooksDir now returns a dir only if realpath(--git-path hooks) equals realpath(--git-common-dir)/hooks, else null (a not-yet-created hooks/ is compared via its parent). Both the pre-commit and pre-push installs go through it. Tests cover a repo-local hooksPath outside the repo (null), a hooksPath to a missing dir (null), your setup with hooksPath at the repo's own absolute .git/hooks (resolves), a global hooksPath via GIT_CONFIG_GLOBAL (null, from both a checkout and a worktree), and plain worktrees.
  2. Done. The hook skips with a one-line notice when a pushed ref isn't HEAD, or when git status --porcelain shows changes under src/, config/, scripts/, test/, package*.json or install.sh. I added test/ because check:browser-excludes scans it, and install.sh because the catalog check diffs it. Tested with real pushes to a temp bare remote; taking out either skip makes those tests fail. The Session Safety line is in CLAUDE.md.
  3. The timing now reads ~10-40s (I measured 12s here, you measured 35s), the Playwright count is 14, and the checker's fileoverview notes that only direct imports are detected.

I kept it as one PR since both asks were concrete, but happy to split the checker out if you'd still prefer that.

@Ark0N
Ark0N merged commit fec0409 into Ark0N:master Sep 28, 2026
2 checks passed
Ark0N pushed a commit that referenced this pull request Sep 28, 2026
- 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) <noreply@anthropic.com>
@Ark0N

Ark0N commented Sep 28, 2026

Copy link
Copy Markdown
Owner

Merged, and it ships in 1.33.2. Thanks @aakhter.

Both review points came back exactly as asked, with real-push tests behind them, and asking vitest itself what CI collects (instead of re-implementing the globs) is a nice design. Applied on the way in:

  • The hook skips with a notice when npm is not on PATH instead of blocking every push, since GUI git clients often run hooks with a minimal PATH. There's a real-push test with npm stripped.
  • The resolveGitHooksDir tests set GIT_CONFIG_NOSYSTEM=1 and GIT_CONFIG_GLOBAL=/dev/null for their block, so a machine-wide core.hooksPath can't break them.
  • tsconfig.json, .prettierignore and .editorconfig joined the watched paths.
  • check:browser-excludes fails loudly if none of the listed paths match the test tree (for example if vitest ever prints absolute paths), instead of passing vacuously.
  • The PRE_PUSH_MARKER comment said to bump the version when the body changes. Since ownership is matched on the exact string, a v2 would have made every installed v1 hook look foreign and never refresh again, so the comment now says never to bump it (the whole-file comparison already delivers a changed body).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants