mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
build: address review on the pre-push hook and hooks-dir resolution
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.
This commit is contained in:
@@ -19,6 +19,9 @@
|
||||
* (`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
|
||||
|
||||
+79
-11
@@ -5,12 +5,19 @@
|
||||
* 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.
|
||||
* 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 and measured ~15s total.
|
||||
* 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
|
||||
@@ -19,7 +26,7 @@
|
||||
|
||||
import { execFileSync } from 'node:child_process';
|
||||
import { chmodSync, existsSync, mkdirSync, readFileSync, realpathSync, writeFileSync } from 'node:fs';
|
||||
import { isAbsolute, join } from 'node:path';
|
||||
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';
|
||||
@@ -39,6 +46,25 @@ export const PRE_PUSH_CHECKS = [
|
||||
['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.
|
||||
*
|
||||
@@ -46,6 +72,7 @@ export const PRE_PUSH_CHECKS = [
|
||||
*/
|
||||
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}
|
||||
@@ -70,16 +97,36 @@ 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
|
||||
while read -r _localref localsha _remoteref _remotesha; do
|
||||
not_head=''
|
||||
while read -r localref localsha _remoteref _remotesha; do
|
||||
[ -z "$localsha" ] && continue
|
||||
case "$localsha" in
|
||||
0000000000000000000000000000000000000000) ;;
|
||||
*) has_content=1 ;;
|
||||
*)
|
||||
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
|
||||
|
||||
@@ -93,7 +140,7 @@ run_check() {
|
||||
fi
|
||||
}
|
||||
|
||||
echo "pre-push: running static checks (~15s)..."
|
||||
echo "pre-push: running static checks (~10-40s)..."
|
||||
${runs}
|
||||
|
||||
if [ -n "$failed" ]; then
|
||||
@@ -125,15 +172,33 @@ 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, and `core.hooksPath` can move it anywhere.
|
||||
* `.git` is a FILE pointing at the parent repo, so the hooks live under
|
||||
* `--git-common-dir`.
|
||||
*
|
||||
* 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
|
||||
* ⚠️ 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
|
||||
@@ -143,9 +208,12 @@ 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']);
|
||||
if (!hooks) return null;
|
||||
return isAbsolute(hooks) ? hooks : join(repoRoot, 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;
|
||||
}
|
||||
|
||||
@@ -387,7 +387,7 @@ fi
|
||||
// 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)'));
|
||||
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)'));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user