Repository navigation
Run make quality before every commit with a git pre-commit hook - #4207
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4207 +/- ##
==========================================
- Coverage 92.15% 92.11% -0.05%
==========================================
Files 296 296
Lines 16696 16696
Branches 2722 2722
==========================================
- Hits 15386 15379 -7
- Misses 794 801 +7
Partials 516 516
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
anth-volk
added a commit
that referenced
this pull request
Nov 5, 2026
…s (failing) Review round 1 of #4207 found that make quality checks the working tree while the commit takes the index, so fixes from an earlier run, partial staging and untracked files could make the result differ from what is committed; that merges committed without the hook; and that make install could change an enclosing repository's settings, replace another core.hooksPath, or stop a clone's own hooks in .git/hooks. The tests now run with a PATH of only git, make, sh, find, dirname, env and a stand-in uv that records its calls, so a missing uv or make is tested without skipping. New and changed tests: unstaged changes (one name outside ASCII, shown unquoted), untracked files and fixes left by an earlier failed run stop the commit; git commit -a runs make quality; a merge without conflicts runs the hook; MAKEFLAGS from the environment can't hide a failure; UV_PROJECT_ENVIRONMENT names the dev environment; each worktree runs its own checkout's hook; make install works in a linked worktree, installs outside a repository (checked through uv's calls), leaves an enclosing repository alone, keeps another core.hooksPath, and keeps a clone's own hooks running; both hooks are executable in git. The proof adds an unstaged change and a missing uv, and the accepted fixture handles both. 15 fail, several because the current hook needs mktemp, which the PATH leaves out; the missing-tool tests, the proof and three make install tests pass already. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk
added a commit
that referenced
this pull request
Nov 5, 2026
…er installs (failing) Review round 2 of #4207 found: - test_locked_test_environments.py asserted make install's recipe was exactly `uv sync --frozen --extra dev`, which the hooks' line breaks; it now checks the recipe syncs the locked environment and that its other commands install nothing; - a Python file hidden by .git/info/exclude or the global excludes file is still read by mypy and Import Linter; - MAKE or MAKE_COMMAND from the environment replaces $(MAKE), so quality-check could be skipped; - skip-worktree and assume-unchanged files aren't compared by git diff; - turn-on.sh missed symlinked hooks and a symlinked .git/hooks, and a locked .git/config made make install fail before uv sync; - pre-merge-commit needed dirname; - the tests read $XDG_CONFIG_HOME/git/ignore. New tests cover each; the tests' PATH no longer has dirname, and XDG_CONFIG_HOME points into the test's directory. The proof adds an untracked file and a missing dev environment, and the accepted fixture stops both. 9 fail: the merge (dirname), a hidden Python file, MAKE and MAKE_COMMAND, skip-worktree and assume-unchanged, two symlinked-hooks cases and a locked config. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk
added a commit
that referenced
this pull request
Nov 5, 2026
…(failing) Review round 3 of #4207 found that bash 3.2, macOS's /bin/sh, matches uppercase letters with [a-z] in UTF-8 locales, so the hook refused every commit there; that hidden tool settings (ruff.toml, mypy.ini, a nested pyproject.toml) and tracked files only local excludes match change what the checks see; that a hidden virtual environment stopped every commit, and slowly; that a case-only rename on a case-insensitive file system went through; and that the advice for git commit <paths> didn't help. New tests: a commit under LANG and LC_ALL en_US.UTF-8; ruff.toml, mypy.ini and lib/pyproject.toml hidden by .git/info/exclude; a hidden directory of Python files; a hidden virtual environment, which must not stop the commit; a tracked file .git/info/exclude matches; a case-only rename; and the advice for a commit of some paths. 8 fail on macOS; the hidden-directory test passes already. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tests/unit/test_pre_commit_hook.py runs the hook in temporary repositories whose Makefile's quality target stands in for the checks, with a stand-in uv and no global git settings. It checks that a commit runs make quality; that a failing check, or ruff's fixes to a staged file (named in the message), stop the commit; that unstaged changes make quality leaves alone don't; that a missing uv or .venv stops the commit with a message naming make install; that a worktree of the clone runs the hook; that make install sets core.hooksPath to .githooks, and still works outside a repository; and that the hook is executable in git. The proof reads two hooks under rules/fixtures/api.quality.pre-commit-hook. 10 fail: .githooks/pre-commit doesn't exist and make install sets nothing. The proof and the install-outside-a-repository test pass already. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- .githooks/pre-commit (executable) runs make quality from the root of the working tree. It stops the commit when make quality fails, or when ruff's fixes changed a file, which it names: it compares a hash of each file with unstaged changes before and after. It also stops the commit, saying what to install, when make, uv or the worktree's .venv is missing. - make install sets core.hooksPath to .githooks first, when it runs inside a git repository, then installs the dev environment. The setting belongs to the clone, so every worktree runs the hook from its own checkout. - New rule api.quality.pre-commit-hook, with its proof. - AGENTS.md, the GitHub PRs skill's Commit Hygiene and README.md say to run make install once per clone (and in each new worktree for its .venv) and not to skip the hook. The tests that failed now pass; make quality passes without changing a file, and shellcheck reports nothing on the hook. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s (failing) Review round 1 of #4207 found that make quality checks the working tree while the commit takes the index, so fixes from an earlier run, partial staging and untracked files could make the result differ from what is committed; that merges committed without the hook; and that make install could change an enclosing repository's settings, replace another core.hooksPath, or stop a clone's own hooks in .git/hooks. The tests now run with a PATH of only git, make, sh, find, dirname, env and a stand-in uv that records its calls, so a missing uv or make is tested without skipping. New and changed tests: unstaged changes (one name outside ASCII, shown unquoted), untracked files and fixes left by an earlier failed run stop the commit; git commit -a runs make quality; a merge without conflicts runs the hook; MAKEFLAGS from the environment can't hide a failure; UV_PROJECT_ENVIRONMENT names the dev environment; each worktree runs its own checkout's hook; make install works in a linked worktree, installs outside a repository (checked through uv's calls), leaves an enclosing repository alone, keeps another core.hooksPath, and keeps a clone's own hooks running; both hooks are executable in git. The proof adds an unstaged change and a missing uv, and the accepted fixture handles both. 15 fail, several because the current hook needs mktemp, which the PATH leaves out; the missing-tool tests, the proof and three make install tests pass already. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- .githooks/pre-commit first requires the working tree to hold exactly what is committed, since make quality checks the working tree: it stops the commit, naming the files, while tracked files have unstaged changes (including fixes an earlier run made) or untracked files exist outside .gitignore. After make quality, any changed file is a fix to stage. Names are shown as they are (core.quotePath=false). It clears MAKEFLAGS, MFLAGS, GNUMAKEFLAGS, MAKEFILES and MAKELEVEL before running make, and looks for the dev environment in UV_PROJECT_ENVIRONMENT when it is set. It no longer needs mktemp, grep or sed. - .githooks/pre-merge-commit runs pre-commit, since git runs it, not pre-commit, for a merge that commits without conflicts. - make install runs .githooks/turn-on.sh, which sets core.hooksPath only at the root of a git working tree, and leaves it alone, saying why, when it would change an enclosing repository, replace another core.hooksPath, or stop the clone's own hooks in .git/hooks. It never fails, so make install still installs. - The rule statement, AGENTS.md, the GitHub PRs and testing skills, README and the changelog say the hooks run before git commit and merge commits, not every commit (cherry-pick, revert and rebase run none), that the working tree must match the commit, and what make install leaves alone. The tests that failed now pass; make quality passes without changing a file, and shellcheck reports nothing on the three scripts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er installs (failing) Review round 2 of #4207 found: - test_locked_test_environments.py asserted make install's recipe was exactly `uv sync --frozen --extra dev`, which the hooks' line breaks; it now checks the recipe syncs the locked environment and that its other commands install nothing; - a Python file hidden by .git/info/exclude or the global excludes file is still read by mypy and Import Linter; - MAKE or MAKE_COMMAND from the environment replaces $(MAKE), so quality-check could be skipped; - skip-worktree and assume-unchanged files aren't compared by git diff; - turn-on.sh missed symlinked hooks and a symlinked .git/hooks, and a locked .git/config made make install fail before uv sync; - pre-merge-commit needed dirname; - the tests read $XDG_CONFIG_HOME/git/ignore. New tests cover each; the tests' PATH no longer has dirname, and XDG_CONFIG_HOME points into the test's directory. The proof adds an untracked file and a missing dev environment, and the accepted fixture stops both. 9 fail: the merge (dirname), a hidden Python file, MAKE and MAKE_COMMAND, skip-worktree and assume-unchanged, two symlinked-hooks cases and a locked config. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- .githooks/pre-commit:
- also stops the commit for untracked Python files outside the
repository's .gitignore files, since mypy and Import Linter read them
even when .git/info/exclude or a global excludes file hides them, and
no longer suggests .git/info/exclude;
- stops it for files marked skip-worktree or assume-unchanged, which git
diff doesn't compare, saying how to clear the marks;
- clears MAKE, MAKE_COMMAND and MAKEOVERRIDES as well as make's flags,
since $(MAKE) from the environment could skip quality-check;
- stops the commit when git diff or git ls-files itself fails.
It needs only git, make, uv and the shell (no sort, mktemp or dirname),
and avoids a case pattern inside a command substitution, which bash 3.2,
macOS's /bin/sh, can't parse.
- .githooks/pre-merge-commit finds pre-commit with ${0%/*}, not dirname.
- .githooks/turn-on.sh checks the root with git rev-parse --show-prefix,
which a differently cased path can't confuse; finds the clone's own hooks
with find -L, so symbolic links and a linked .git/hooks count; and exits
0 with a message when git can't write core.hooksPath, so make install
still installs.
- The rule's scope adds pre-merge-commit and turn-on.sh, and its statement
and the GitHub PRs skill describe the new refusals; the skill says git's
cherry-pick, revert, rebase and am can make commits without hooks.
The tests that failed now pass; make quality passes without changing a file,
and shellcheck (sh and dash) reports nothing.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…(failing) Review round 3 of #4207 found that bash 3.2, macOS's /bin/sh, matches uppercase letters with [a-z] in UTF-8 locales, so the hook refused every commit there; that hidden tool settings (ruff.toml, mypy.ini, a nested pyproject.toml) and tracked files only local excludes match change what the checks see; that a hidden virtual environment stopped every commit, and slowly; that a case-only rename on a case-insensitive file system went through; and that the advice for git commit <paths> didn't help. New tests: a commit under LANG and LC_ALL en_US.UTF-8; ruff.toml, mypy.ini and lib/pyproject.toml hidden by .git/info/exclude; a hidden directory of Python files; a hidden virtual environment, which must not stop the commit; a tracked file .git/info/exclude matches; a case-only rename; and the advice for a commit of some paths. 8 fail on macOS; the hidden-directory test passes already. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- .githooks/pre-commit:
- matches git's skip-worktree and assume-unchanged tags with
[[:lower:]S]: bash 3.2, macOS's /bin/sh, matched uppercase letters
with [a-z] in UTF-8 locales, so every commit there was refused;
- runs its git listings with core.ignorecase=false, so a case-only
rename on a file system that ignores case shows as an untracked file;
- stops the commit for Python files and tool settings (pyproject.toml,
setup.cfg, mypy.ini, ruff.toml, .importlinter) that only local or
global excludes hide, listing a wholly untracked directory once,
skipping virtual environments (pyvenv.cfg) and directories with
neither, so a hidden venv no longer stops commits or takes minutes;
- stops the commit for tracked Python files local or global excludes
match, which ruff skips but CI checks;
- suggests .git/info/exclude again for other untracked files, and, for
unstaged changes, staging what the commit needs and committing without
paths.
- The rule lists all 35 of its tests as enforcer nodes; its statement says
turn-on.sh, not make install, always exits 0, and describes the new
refusals. The GitHub PRs skill and README follow.
The tests that failed now pass, also with the test run itself under
en_US.UTF-8; make quality passes without changing a file, and shellcheck
(sh and dash) reports nothing.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk
force-pushed
the
quality/pre-commit-hook
branch
from
November 6, 2026 13:57
606860b to
fc36ee4
Compare
anth-volk
marked this pull request as ready for review
November 6, 2026 14:34
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4206
Based on
master. It was stacked on #4203, which addsmake qualityand has merged. The branch was then rebased ontomasteras a plain series of its own eight commits, with no merge commits, so GitHub can rebase-merge it. The commit references below are the rebased ones.Summary
.githooks/pre-commitrunsmake qualityfrom the root of the working tree beforegit commit.make qualitychecks the working tree, so the hook first requires it to hold exactly what is being committed. It stops the commit, naming the files and saying what to do:git stash push --keep-index"). This includes fixesmake qualitymade in an earlier, failed run,git commit <paths>while other files are changed, and a case-only rename on a file system that ignores case. The hook lists files withcore.ignorecase=falseto catch the last one;.git/info/exclude");pyproject.toml,setup.cfg,mypy.ini,ruff.toml,.importlinter) are hidden only by.git/info/excludeor a global excludes file, since ruff, mypy and Import Linter can read them anyway. The hook lists a wholly untracked directory once (--directory), and leaves out virtual environments (a directory withpyvenv.cfg) and directories with neither kind of file;git diffdoesn't compare ("rungit update-index --no-skip-worktree --no-assume-unchanged"). It matches git's tags with[[:lower:]S]. bash 3.2, macOS's/bin/sh, matches uppercase letters with[a-z]in UTF-8 locales, so[a-z]would refuse every commit there;make qualityfails;makeoruvisn't installed, or the dev environment (.venv, orUV_PROJECT_ENVIRONMENT) is missing, instead of passing without running the checks.Names outside ASCII are shown as they are. The hook clears
MAKEFLAGS,MFLAGS,GNUMAKEFLAGS,MAKEFILES,MAKELEVEL,MAKE,MAKE_COMMANDandMAKEOVERRIDESbefore running make, so they can't make it ignore a failing check or replace$(MAKE). It stops the commit ifgit difforgit ls-filesitself fails. It needs only git, make, uv and the shell..githooks/pre-merge-commitruns the same hook, without needingdirname. git runs it, notpre-commit, for a merge that commits without conflicts.make installruns.githooks/turn-on.sh, thenuv sync --frozen --extra devas before. The script setscore.hooksPathto.githooksat the root of a git working tree, including a linked worktree. It changes nothing, and says why, when:git rev-parse --show-prefix, so a differently cased path doesn't confuse it;core.hooksPathalready names another directory;.git/hooks, including symbolic links and a linked directory, whichcore.hooksPathwould stop git running;.git/config's lock.It never fails, so
make installstill installs, and outside a repository it does nothing.core.hooksPathis a setting of the clone, so every worktree runs the hooks. git resolves the relative path in each working tree, so each worktree runs its own checkout's copy, and a worktree on a branch from before the hooks runs none.New enforced rule
api.quality.pre-commit-hook.tests/unit/test_pre_commit_hook.pyruns the hooks in temporary repositories whose Makefile'squalitytarget stands in for the checks.PATHholding only git, make, sh, find, env and a stand-inuvthat records its calls. They use no global or system git settings, and a scratchXDG_CONFIG_HOME.Docs:
AGENTS.md, the GitHub PRs skill's "Commit Hygiene" (the rule's rationale), the testing skill andREADME.mdsay:make installonce per clone, and again in each new worktree for its.venv;Design choices for review
make quality, and no Claude Code hook. It covers every agent and person in clones wheremake installran.git commit --no-verifyskips it, and git's cherry-pick, revert, rebase and am can make commits without hooks (some steps, such as a rebase'sreword, do run them). CI'sQuality checksjob still runs the same checks and blocks the merge.make qualityreads files on disk. Without the rule, fixes from an earlier run, partial staging, or an untracked file could make the result differ from what is committed. The one way to commit part of the changes isgit stash push --keep-indexfirst.make installbefore its first commit. Without its.venv, the hook stops the commit and says so, rather than letuv runbuild the environment in the middle of the commit.make qualitytakes about 3 seconds with warm caches and about 10 seconds cold (mypy 6.7 s, the guards 2.0 s), measured on Run every quality check through make quality, locally and in CI #4203's branch. A review run through this hook with cold caches and no network took about 13 to 14 seconds..gitignorethat hides itself can hide files from the hook..gitattributescan make the committed blob differ from the checked file.PYTHONPATHorMYPYPATHin the environment can add code the checks read.core.ignoreStat=true, marks files so that every commit is refused..claude/or.codex/directories, commits are refused until those are listed in.git/info/excludeor ignored.make installin CI jobs now also setscore.hooksPathin the job's checkout. No job that runsmake installmakes commits.make installin my own clone, because the setting would apply to every worktree of it, including other sessions' worktrees. The tests use temporary repositories.CI wiring and environment variables
No workflow changes. The tests run in the existing
Testshards (make test).PATH, to findmakeanduv, andUV_PROJECT_ENVIRONMENTwhen it is set. They also read the variables git sets for hooks, such asGIT_INDEX_FILE. They read no secret..githooks/turn-on.shreads only git's settings.PATH,HOME,GIT_CONFIG_GLOBAL=/dev/null,GIT_CONFIG_NOSYSTEM=1, the git author and committer, and paths for their own markers, for their temporary repositories. They removeGIT_*,UV_*and make's variables inherited from the test run.Testing
Red, at
312b58bef(tests and fixtures only): 10 of 12 tests failed. There was no hook, andmake installset nothing.Green, at
be86f1117: all 12 passed.Review round 1, red at
f252e7806: 15 of 23 tests failed. Several of those failed because the first hook neededmktemp, which the tests'PATHleaves out. The rest covered:commit -a;UV_PROJECT_ENVIRONMENT;make installbeside an enclosing repository, anothercore.hooksPath, or the clone's own hooks;Green at
c764aa1c7: all 23 pass, by node ID.Review round 2, red at
26ee1ab90: 9 of 33 test cases failed. They covered:dirname;.git/info/exclude;MAKEandMAKE_COMMAND;.git/hooks;.git/config.The same commit also does two other things:
tests/unit/test_locked_test_environments.py::test_make_install_syncs_the_locked_development_environment. That test asserted thatmake install's recipe was exactlyuv sync --frozen --extra dev, and would have failed in CI. It now checks that the recipe syncs the locked environment and that its other commands install nothing.$XDG_CONFIG_HOME/git/ignore.Green at
ac4e18a2b: all 33 cases pass.Review round 3, red at
dc4ecd28c: 8 test cases failed on macOS. They covered:en_US.UTF-8;ruff.toml,mypy.iniandlib/pyproject.tomlhidden by.git/info/exclude;.git/info/excludematches;git commit <paths>.Green at
fc36ee404: all 42 hook cases pass, also with the test run itself underen_US.UTF-8.Also run locally at
fc36ee404:make quality: passes, and changes no file;python -m scripts.rules guard --base-ref origin/quality/make-qualityandvalidate: pass;Per this workstream's rules, only the tests this work added or changed were run locally; CI runs the rest.
🤖 Generated with Claude Code